Opened 13 years ago
Closed 12 years ago
#3945 closed defect (fixed)
Potential unsafe code in SOS code
Reported by: | rouault | Owned by: | assefa |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | Security/Vulnerability (Private) | Version: | unspecified |
Severity: | normal | Keywords: | |
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.
Summary :
line 2080: pszBuffer = msStringConcatenate(pszBuffer, (char *)pszProcedureItem);
and
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.
Attachments (1)
Change History (7)
comment:1 by , 13 years ago
Cc: | added |
---|---|
Description: | modified (diff) |
Owner: | changed from | to
by , 13 years ago
Attachment: | bug3974.patch added |
---|
comment:2 by , 13 years ago
Cc: | added |
---|
I believe line 2080 to be safe. I am not 100% sure about line 2086. I have patched the code to do the escaping. sos test was run on msautotest. I have attached the patch against trunk. Tom if you have a minute and have an sos setup on a postgis, could you patch and test to make sure things still work? Thanks.
comment:3 by , 13 years ago
Assefa,
shouldn't you use EscapeSQLParam() instead of EscapePropertyName() to escape tokens[j] ? And let the single-quotes to surround the escaped value.
comment:5 by , 12 years ago
Can this be closed? This would have been fixed in the 6.0.1, 5.6.7, etc... releases.
Steve
comment:6 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
patch against trunk