Opened 12 years ago

Closed 11 years ago

#3876 closed defect (fixed)

SQL injection vulnerability in FLTGetSQLExpression on FILTER_NODE_TYPE_FEATUREID request

Reported by: rouault Owned by: aboudreault
Priority: normal Milestone: 6.0.1 release
Component: Security/Vulnerability (Private) Version: svn-trunk (development)
Severity: normal Keywords:
Cc: assefa, sdlime, dmorissette, pramsey, colivier, msmitherdc

Description

The following triggers a SQL injection :

http://127.0.0.1:8080/mapserv.cgi?map=pg.map&SERVICE=WFS&VERSION=1.1.0&REQUEST=GetFeature&TYPENAME=world&PROPERTYNAME=name&FEATUREID=world.6%27%29%29%29;DELETE%20FROM%20world;SELECT%20name%20FROM%20world%20WHERE%20%28%28%28ogc_fid%20=%20%276

Unescaped :

http://127.0.0.1:8080/mapserv.cgi?map=pg.map&SERVICE=WFS&VERSION=1.1.0&REQUEST=GetFeature&TYPENAME=world&PROPERTYNAME=name&FEATUREID=world.6')));DELETE FROM world;SELECT name FROM world WHERE (((ogc_fid = '6

Affected code is located at line 2102 in mapogcfilter.c :

                        if (bString)
                            snprintf(szTmp, sizeof(szTmp), "(%s = '%s')" , pszAttribute, tokens[i]);
                        else
                            snprintf(szTmp, sizeof(szTmp), "(%s = %s)" , pszAttribute, tokens[i]);

This is the same as the issue that has just been fixed in TinyOWS in http://tinyows.org/trac/changeset/533

Change History (16)

comment:1 by rouault, 12 years ago

(Adding comment provided to Alan by email)

I'm wondering if instead of looking all the possible places where a SQL injection could happen, it wouldn't be simpler to use PQexecParams() instead of PQexec(), as suggested by

http://www.network-theory.co.uk/docs/postgresql/vol2/MainFunctions.html

That way, only one SQL command could be run at a time.

comment:2 by dmorissette, 12 years ago

Cc: assefa sdlime dmorissette added
Milestone: 6.0.1 release

comment:3 by rouault, 12 years ago

Olivier has just adopted PQexecParams() in TinyOWS. See http://tinyows.org/trac/changeset/545

comment:4 by assefa, 12 years ago

I seems to be the simpler approach indeed. Is replacing all the PQexec with PQexecParams is no risk? It does not seem to be from what I can read (expect some note of the function being only available for protocol 3.0, which seems to be postgresql 7.4) but I still don't feel very confident my self doing this change, specially back porting it to older versions without some one more knowledgeable confirming it.

comment:5 by sdlime, 12 years ago

Worth pinging Paul R.? Steve

comment:6 by assefa, 12 years ago

I think so. Olivier is also knowledgeable on postgres/postgis.

comment:7 by dmorissette, 12 years ago

Cc: pramsey colivier added

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

comment:8 by rouault, 12 years ago

Hum, interestingly, both the if (bString) and else cases are vulnerable.

See the following exploit :

http://127.0.0.1:8080/mapserv.cgi?map=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

comment:9 by pramsey, 12 years ago

I think it's OK to move forward our version compatibility a bit to 7.4+ at this point. Well, for trunk. The issue is this is a security patch back into older versions, and we'll be pushing up the version required. When you consider that 7.4 was released in 2003, I guess it should be fine.

comment:10 by assefa, 12 years ago

Thanks Paul.

Are there any objections on applying the patches from 4.10 and up? Patches would include what is there in #3874 (escaping inputs) + changes to use PQexecParams? Note that 4.10 was released in 2006-10-04.

I can also only do the PQexecParams cahnges for the 6.0+ versions and only apply the "input escaping" to older versions.

comment:11 by dmorissette, 12 years ago

I think the PostgreSQL 7.4 (2003) requirement would be fine for all MapServer releases that we fix, but please let me double-check with the binary package maintainers and then I'll get back to you.

comment:12 by dmorissette, 12 years ago

I got confirmation that all currently supported distros of MapServer on MacOSX, Debian, Ubuntu, RHEL and OpenSUSE all run PostgreSQL 8.x, so the 7.4 dependency is not a problem at all.

comment:13 by dmorissette, 12 years ago

Public ticket #3903 has been created for this (current ticket #3876 will remain private)

comment:14 by dmorissette, 12 years ago

Cc: msmitherdc added

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

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

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