SQL injection vulnerability in FLTGetSQLExpression on FILTER_NODE_TYPE_FEATUREID request

[rouault]

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

[rouault]

(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.

[rouault]

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

[assefa]

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.

[sdlime]

Worth pinging Paul R.? Steve

[assefa]

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

[dmorissette]

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

[rouault]

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

[pramsey]

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.

[assefa]

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.

[dmorissette]

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.

[dmorissette]

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.

[dmorissette]

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

[dmorissette]

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

[sdlime]

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

Steve