Requiring validation patterns for runtime subs and cgi-based attribute queries.

[sdlime]

Version 4.10+ of MapServer allows users to define validation patterns to ensure the data to be substituted at runtime is of a valid type. This is an optional check. I'm proposing that it be mandatory. That is, no runtime substitutions can be done without passing a validation regex.

Note this should also apply to validation on qstring values for cgi-based attribute queries. This was just added (r6766) but is also optional.

Steve

[sdlime]

Note, the change is simple. I just have to move a brace closing an if-then check...

Steve

[dmorissette]

Not sure what to think. It would definitely be safer to make the validation pattern mandatory, but I wonder how many users will know how to setup a robust validation pattern. You'll probably need a mini-howto explaining how variable substitution and validation patterns work.

Also, in the absence of validation pattern we should produce a fatal error and not just silently skip this variable so that people who forget the validation pattern don't end up spending hours trying to figure out why the substitution doesn't work. Given the way the code is setup now I think that would complicate things.

Um... perhaps we are better leave the validation pattern optional for now and just create a mini-howto that makes it clear what the risks are and strongly advise users to use validation patterns... I didn't even know that this feature existed myself until I read this ticket.

[sdlime]

Sorry you're seeing this so late. I had the wrong friggin' name in the cc.

I agree on the fatal errors if the patterns are mandatory although that may not be possible with the runtimes cause you don't know what substitutions to expect. It's certainly possible with the attribute queries. To me that's (attribute queries) the biggest security hole and really does need some additional investigation. For example, if Oracle, SDE or PostGIS allow the stringing together of SQL queries via the where clause (e.g. via the FILTER)then that would be *really* bad.

I was planning on a general security how-to where this would figure prominently.

Steve

[dmorissette]

Okay, since qstring is not disabled by default, that means that if the validation pattern is not made mandatory any mapserver install may be vulnerable to SQL injection.

If that's right then I agree with making the qstring_validation_pattern mandatory and produce a fatal error if qstring is used and the validation pattern is missing.

Are you planning to do that before the RC, i.e. in the next hour or two?

[sdlime]

I just updated the code in mapserv.c and *think* it is correct but I'd like to test and have a meeting until 2pm (central). Can you wait to release until I confirm the fix?

Steve

[dmorissette]

I'll wait to hear back from you.

[sdlime]

Ok, I tested briefly and it seems to work. Of course users have to write a decent validation pattern. I'll try and do some SQL injection testing just to try and understand potential issues better. May want to simply use the OWS filter encoding specification but that may not be immune either depending on how it's used.

I think the runtime substitutions are pretty safe given they usually are used to modify a portion of an expression or filter. Validation filters are much easier to write for those.

Steve

[dmorissette]

I think an entry should be added for this in MIGRATION_GUIDE.TXT before rc1 since the validation pattern is a new requirement for qstring to work.

[sdlime]

I'll do that right now. We may up needing to create multiple patterns depending on the column being queried but again that can wait.

Steve

[dmorissette]

FYI I had already started putting something together, I just committed it to SVN in r6816. Feel free to edit or flush it completely since I wrote it quickly and I'm sure you can come up with something better.

[sdlime]

I tweaked just a bit. The runtime validation is in 4.10, the attribute query stuff is new and will be the migration issue. Otherwise you're language is fine.

Steve

[tomkralidis]

Can this one be closed or is there more work/testing to be done?