Public Forums 71,145 posts

[doorknob Friend doorknobJoin date: December 2013Posts: 36Downloads:0Uploads:1 Thanks: 2 Thanked: 20 times in 1 posts]

#193818

I experienced a series of SQL errors using the extension iCagenda. I traced the root of the problem to the T3 plugin (v2.0.2).
in the function getTemplate(), in t3/includes/core/t3.php line 333 is as follows:

$id = $input->getCmd('styleid', $input->getCmd('id'));
This retrieves the value of ‘styleid’ but, if no value is present, uses ‘id’ as a default.

Almost every extension uses ‘id’ in a url at some point and so some kind of problem is inevitable. The effect that this will have on T3 depends on the data type of the value retrieved. If the value is integer then the sql will not generate an error and most of the time no record will be retrieved (which doesn’t cause any problem). Occasionally, when the value coincides with a valid template style, that style will be selected with unpredictable results. If the value is not integer then an sql error is generated and the page fails.
To fix this, I changed the line to:

$id = (int)$input->getCmd('styleid');
and now my site works fine.

Regards
Phil

[doorknob Friend doorknobJoin date: December 2013Posts: 36Downloads:0Uploads:1 Thanks: 2 Thanked: 20 times in 1 posts]

#518963

It also occurs to me that, because the value extracted from the url is not typed for integer or validated in any way, it also represents a security flaw by providing an injection opportunity

[Wall Crasher Developer Wall CrasherJoin date: December 2011Posts: 1113Downloads:0Uploads:15 Thanks: 66 Thanked: 361 times in 300 posts]

#519035

Hi Phil,
Thank for bringing the case to our attention. We will apply the fix.

[doorknob Friend doorknobJoin date: December 2013Posts: 36Downloads:0Uploads:1 Thanks: 2 Thanked: 20 times in 1 posts]

#519105

With security in mind, I looked at some more sql queries and t3/includes/admin/layout.php lines 476-482 might be more secure if changed from:

if ($state != '') {
$query->where('enabled = ' . $db->quote($state));
}

if ($template != '') {
$query->where('element = ' . $db->quote($template));
}
to:

if ($state != '') {
if (version_compare(JVERSION, '3.0', 'lt')) {
$state = $db->getEscaped($state, true);
} else {
$state = $db->escape($state, true);
}
$query->where('enabled = ' . $db->quote($state));
}

if ($template != '') {
if (version_compare(JVERSION, '3.0', 'lt')) {
$template = $db->getEscaped($template, true);
} else {
$template = $db->escape($template, true);
}
$query->where('element = ' . $db->quote($template));
}
The values may have been screened before the function is called but all functions should look after themselves.

Same thing in t3/includes/admin/megamenu.php at line 110 and again at line 167

Similarly, t3/includes/renderer/t3ajax.php line 90 would be more secure if changed from

$mid = $input->getCmd('m');

$mid = (int)$input->getCmd('m');

Finally, in t3/t3.php at around line 250, I changed

$query
->select('*')
->from('#__template_styles')
->where('template=' . $db->quote($data->template));

to

$template = $data->template;
if (version_compare(JVERSION, '3.0', 'lt')) {
$template = $db->getEscaped($template, true);
} else {
$template = $db->escape($template, true);
}
$query
->select('*')
->from('#__template_styles')
->where('template=' . $db->quote($template));


1 user says Thank You to doorknob for this useful post

1. JA Developer

[Wall Crasher Developer Wall CrasherJoin date: December 2011Posts: 1113Downloads:0Uploads:15 Thanks: 66 Thanked: 361 times in 300 posts]

#519170

Hi Phil,

Thanks for sharing your tweak. Though getEscaped has been deprecated and quote also includes escape call by default.

[doorknob Friend doorknobJoin date: December 2013Posts: 36Downloads:0Uploads:1 Thanks: 2 Thanked: 20 times in 1 posts]

#519212

<blockquote>getEscaped has been deprecated</blockquote>That part of the code was for J2.5 only.
<blockquote>quote also includes escape call by default.</blockquote>Thanks, I didn’t know that.

There’s still a module id in t3/includes/renderer/t3ajax.php at line 90 that’s not typed as integer

[doorknob Friend doorknobJoin date: December 2013Posts: 36Downloads:0Uploads:1 Thanks: 2 Thanked: 20 times in 1 posts]

#519692

The fix applied in v2.1.0 prevents sql errors but still uses the value of id as a default for styleid. This will be the source of bizarre random errors forever.

[Wall Crasher Developer Wall CrasherJoin date: December 2011Posts: 1113Downloads:0Uploads:15 Thanks: 66 Thanked: 361 times in 300 posts]

#519781

Hi Phil,

Thanks for the information. Please let me know steps to produce the issue, I will help to debug.

[Author]

Posts