Fix ssi sqlis

[C3realGuy]

Many limits got concatenate in SSI.php which is not sql injection safe. If you once miss to parse the limit argument as an int, you can (maybe) perform an sql injection. Better fix em.

[Nao]

I'm not getting it.. By definition, if you can call a SSI function, it means you have admin-type rights, so you wouldn't need to 'inject' anything via SQL anyway, e.g. you could simply make your own wesql::query call...

As for your code, I noticed at least three breaking errors, so I couldn't do a merge even if I wanted to. ^^

[C3realGuy]

I'm not an SMF expert, but those SSI stuff also often got used for additional pages or smaller scripts which should extend SMF. You could require SSI.php und could build you own page with the style of the forum. In the SSI.php there were also some helper functions, and as those I see them. Of course you could always cast the limit as an integer and you would be safe for sql injections. But as soon as you don't do it, there's a possibility of an sql injection.
For example in template_home_topics you never check if it's an true integer, if the value is from the databse (this topics:13 in homepage custom content settings). For sure you need admin access, but still this is something which should get fixed. It's wrong in my understanding to not parameterize any value like this.
Also because this are helper functions, especially developement beginners will use them. Dynamic numbers of posts by getting it via an $_GET parameter is very thinkable. Just not force to cast it to an integer and you have some bad things going on.
I don't think that this will change anything on the performance on those requests.
So this is nothing which is really critical, I don't think that you can exploit it on a normal wedge installation. But maybe a plugin which uses one of those functions and doesn't handle the input for any limits correctly. Still i think wedge is quite safe because of the anti hacking system, which blocked nearly all my attempts to do something bad.

What breaking errors did you notice?

[Nao]

Breaking errors I noticed: - you added a "LIMIT {int:something} {int:something}" without a comma, I don't know if it'd work, and at least it's not how it's done in this codebase. - you added a variable but didn't add it to the array below it (instead, you added it to an array below it, which isn't related to the sql query at all.) - you replaced a count($var) with $var itself. Oops? ;) 2017-01-15 12:16 GMT+01:00 C3realGuy <notifications@github.com>:

…

I'm not an SMF expert, but those SSI stuff also often got used for additional pages or smaller scripts which should extend SMF. You could require SSI.php und could build you own page with the style of the forum. In the SSI.php there were also some helper functions, and as those I see them. Of course you could always cast the limit as an integer and you would be safe for sql injections. But as soon as you don't do it, there's a possibility of an sql injection. For example in template_home_topics you never check if it's an true integer, if the value is from the databse (this topics:13 in homepage custom content settings). For sure you need admin access, but still this is something which should get fixed. It's wrong in my understanding to not parameterize any value like this. Also because this are helper functions, especially developement beginners will use them. Dynamic numbers of posts by getting it via an $_GET parameter is very thinkable. Just not force to cast it to an integer and you have some bad things going on. I don't think that this will change anything on the performance on those requests. What breaking errors did you notice? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#44 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABxWqDpyYPH-tfuS2yeetMbuDf5zPlcuks5rSgAdgaJpZM4Ljn-D> .

[C3realGuy]

Now I see 🗡️
Changed and rebased.