Opened 12 years ago

Closed 11 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 dmorissette)

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)

bug3974.patch (1.8 KB ) - added by assefa 12 years ago.
patch against trunk

Download all attachments as: .zip

Change History (7)

comment:1 by dmorissette, 12 years ago

Cc: dmorissette assefa sdlime aboudreault added
Description: modified (diff)
Owner: changed from aboudreault to assefa

by assefa, 12 years ago

Attachment: bug3974.patch added

patch against trunk

comment:2 by assefa, 12 years ago

Cc: tomkralidis 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 rouault, 12 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:4 by assefa, 12 years ago

Correct. Updated. Thanks.

comment:5 by sdlime, 11 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 assefa, 11 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.