Potential unsafe code in SOS code
|Reported by:||rouault||Owned by:||assefa|
|Cc:||dmorissette, assefa, sdlime, aboudreault, tomkralidis|
Description (last modified by )
Extract of private discussion with Daniel on IRC :
[21:41] <EvenR> hum I didn't look at the sos code before, but line 2086 of mapogcsos.c looks suspicious [21:43] <dmorissette> checking... [21:44] <EvenR> it depends if the tokens[j] can come from user input or just from some metadata in the mapfile [21:47] <dmorissette> :) [21:47] <dmorissette> I mean :( [21:47] <dmorissette> PROCEDURE is a request parameter [21:47] <EvenR> comes from sosparams->pszProcedure at line 1945 apparently [21:47] <dmorissette> see lime 2951 [21:47] <EvenR> arg [21:48] <EvenR> well, I'm not sure how popular SOS is... [21:49] <dmorissette> oh... there is hope [21:49] <dmorissette> line 2041 [21:49] <dmorissette> if layer not found => no matching procedure [21:49] <dmorissette> an exception would be produced if procedure is an invalid value, before the token is used in line 2086 [21:51] <EvenR> hum I'm wondering what happens if the test at line 2005 is evaluated to false [22:01] <dmorissette> sounds like an invalid procedure value may end up being passed to the SQL query [22:01] <dmorissette> are you going to create a ticket? [22:03] <EvenR> still looking if it is a real issue or not. but I'm just discovering this code [22:05] <EvenR> looks like the test around line 1956 will check that the passed procedure will match one of the value provided by sos_procedure metadata [22:05] <EvenR> so indeed it cannot be arbitrary value [22:06] <dmorissette> phewww [22:07] <EvenR> it isn't obvious though. it might be safer at some point to just use the escaping procedure to avoid wondering later [22:08] <dmorissette> should we file another ticket to include in this round... while we're at it? [22:08] <EvenR> by the way, I saw in autotest sos.map that SOS can also use FILTER like WFS, so unpatched version are likely vulnerable [22:09] <dmorissette> yeah, that's what they seemed to have confirmed as well in #mapserver [22:10] <EvenR> a ticket could be usefull, but it doesn't strictly need to fixed right now (unless we have missed something). There are likely enough changes for that round.
line 2080: pszBuffer = msStringConcatenate(pszBuffer, (char *)pszProcedureItem);
line 2086: pszBuffer = msStringConcatenate(pszBuffer, tokens[j]);
from mapogcsos.c are potential candidate for SQL injection, but it *looks* like both values are tested against the sos_procedure and sos_procedure_item metadata item from the mapfile. The eye from someone familiar with this piece of code would be needed to confirm that.
It might be safer for the future to apply proper escaping, just in case, and to make future security reviews easier for non-expert eyes.