SQL Injection in dependencies

[altf4]

It's all over. Here is just one example:

https://github.com/OculusVR/RakNet/blob/master/DependentExtensions/Autopatcher/AutopatcherMySQLRepository/AutopatcherMySQLRepository.cpp#L170

(which is funny because it buffer overflows AND SQL injects on the same line)

I'm not sure what the current state of those dependencies are, and if there's newer patched versions. Definitely not safe to use in the meantime.

I just did a quick grep for:

grep -rn "SELECT " .

Try doing the same and you'll see loads of hits.

[wreiske]

You fail to see the line before that, and also the %s references an escaped object.
Look at the line directly before that

RakNet::RakString escapedApplicationName = GetEscapedString(applicationName);

It's escaped.

[altf4]

True, bad example. Though in general, I dislike trying to manually escape each string as they come in. It is so easy to accidentally miss one and screw everything up. Also, if you try perturbing the escaped string at all (such as with truncation) you open yourself back up to SQLi.

This one is clearly not escaped:

https://github.com/OculusVR/RakNet/blob/master/DependentExtensions/Autopatcher/AutopatcherMySQLRepository/AutopatcherMySQLRepository.cpp#L620

Neither is this:

https://github.com/OculusVR/RakNet/blob/master/DependentExtensions/Lobby2/PGSQL/Lobby2Message_PGSQL.cpp#L291

(though it looks like there aren't any interesting calls made to it)

And neither are either of these two:

https://github.com/OculusVR/RakNet/blob/master/DependentExtensions/SQLite3Plugin/Logger/ServerOnly/SQLiteServerLoggerPlugin.cpp#L675
https://github.com/OculusVR/RakNet/blob/master/DependentExtensions/SQLite3Plugin/Logger/ServerOnly/SQLiteServerLoggerPlugin.cpp#L690

(though, I don't know enough about the inputs there to say if there's a good attack path to reach that code)

Whereas if you just make parametrized queries, everything is safe and stable. You can throw whatever unvalidated input you want at it. There's even a QueryVariadic() function that does this sometimes in the RakNet code. I'd suggest moving towards that. Because some queries which are technically safe right now, are one very simple change away from being exploitable. Such as this:

https://github.com/OculusVR/RakNet/blob/master/DependentExtensions/Autopatcher/AutopatcherMySQLRepository/AutopatcherMySQLRepository.cpp#L704

applicationID is not validated, which is okay since it's just an int type. But it would be all too easy for a new developer to change the type to a string and not realize the consequences here.