This topic was marked solved by its starter, on January 17th, 12:53 AM
[LOW-SQLi] Possible SQL injection on ssi_recentTopics

CerealGuy

  • Posts: 343
[LOW-SQLi] Possible SQL injection on ssi_recentTopics
«  »Last edited
ssi_recentTopics() is not filtering the $num_recent argument correctly.
Code: [Select]
// Find all the posts in distinct topics. Newer ones will have higher IDs.
$request = wesql::query('
SELECT
t.id_topic, b.id_board, b.name AS board_name, b.url
FROM {db_prefix}topics AS t
INNER JOIN {db_prefix}messages AS ml ON (ml.id_msg = t.id_last_msg)
LEFT JOIN {db_prefix}boards AS b ON (b.id_board = t.id_board)
WHERE {query_see_topic}
AND t.id_last_msg >= {int:min_message_id}' . (empty($exclude_boards) ? '' : '
AND b.id_board NOT IN ({array_int:exclude_boards})') . '' . (empty($include_boards) ? '' : '
AND b.id_board IN ({array_int:include_boards})') . '
AND {query_wanna_see_board}' . (empty(we::$user['can_skip_approval']) ? '
AND ml.approved = {int:is_approved}' : '') . '
ORDER BY t.id_last_msg DESC
LIMIT ' . $num_recent,
array(
'include_boards' => empty($include_boards) ? '' : $include_boards,
'exclude_boards' => empty($exclude_boards) ? '' : $exclude_boards,
'min_message_id' => $settings['maxMsgID'] - 35 * $num_recent,
'is_approved' => 1,
)
);

The dangerous part: 'LIMIT ' . $num_recent,'
You can exploit it through custom homepage contents over acp. (Adding something like 'topics:10 UNION SELECT...'). But you need permissions to acp. And even if you have them, the anti hacking protection of wedge looks quite nice. No multiple statemants, it detects weird behaviour couldn't really exploit it besides an more or less useless blind sqli which just worked once :lol:.
But still, better fix it.

How to fix:
Code: [Select]

// Find all the posts in distinct topics. Newer ones will have higher IDs.
$request = wesql::query('
SELECT
t.id_topic, b.id_board, b.name AS board_name, b.url
FROM {db_prefix}topics AS t
INNER JOIN {db_prefix}messages AS ml ON (ml.id_msg = t.id_last_msg)
LEFT JOIN {db_prefix}boards AS b ON (b.id_board = t.id_board)
WHERE {query_see_topic}
AND t.id_last_msg >= {int:min_message_id}' . (empty($exclude_boards) ? '' : '
AND b.id_board NOT IN ({array_int:exclude_boards})') . '' . (empty($include_boards) ? '' : '
AND b.id_board IN ({array_int:include_boards})') . '
AND {query_wanna_see_board}' . (empty(we::$user['can_skip_approval']) ? '
AND ml.approved = {int:is_approved}' : '') . '
ORDER BY t.id_last_msg DESC
LIMIT {int:num_recent}',
array(
'num_recent' => $num_recent,
'include_boards' => empty($include_boards) ? '' : $include_boards,
'exclude_boards' => empty($exclude_boards) ? '' : $exclude_boards,
'min_message_id' => $settings['maxMsgID'] - 35 * $num_recent,
'is_approved' => 1,
)
);

PR: https://github.com/Wedge/wedge/pull/43

Many limit arguments don't get passed parameterized in SSI.php. We should change that.

EDIT1: WTF. This is nearly the same in the SMF codebase. Do I miss something or is this just really bad practice? I mean, i don't know if they have a hacking protection like wedge, but if they don't...
Besides that it looks like they fixed it sometimes and sometimes not :whistle:
https://github.com/SimpleMachines/SMF2.1/blob/release-2.1/SSI.php#L518

EDIT2: Fixed other limits too. https://github.com/Wedge/wedge/pull/44