boughtonp / qpscanner

MOVED TO https://codeberg.org/boughtonp/qpscanner
https://www.sorcerersisle.com/software/qpscanner
GNU General Public License v3.0
22 stars 5 forks source link

application.table name is flagged as a risk #8

Closed mrbusche closed 11 years ago

mrbusche commented 11 years ago

A query such as

SELECT column FROM #application.tablename# WHERE id = cfqueryparam cf_sql_type="cf_sql_integer" value="4"

is flagged as invalid. I've been trying to figure out a workaround (aside from excluding from the query output), but I haven't figured out a good solution yet.

(Sorry not quite sure how to format my cfqueryparam code style.)

boughtonp commented 11 years ago

What do you mean "flagged as an error" ? It doesn't report errors, it reports queries where there is a potential for SQL injection (which more-or-less means any unparameterised variable), and thus it is correct for that query to be flagged.

There is no way for it to know whether application.tablename always only contains "bob" or whether you have code looking like:

<cfif StructKeyExists(Url,'SwitchTable')>
    <cfset Application.Table = Url.SwitchTable />
</cfif>

So all it can do is flag the query to allow the developer to decide what to do.

Beyond post-processing the reported potential risks, there isn't currently a way to specify known-safe variables. (Would be easy enough to do a basic filter, but solving the problem properly is more complex and haven't worked out a suitable solution yet.)

(p.s. code formatting works using standard Markdown indenting and backticks - same way as StackOverflow)

mrbusche commented 11 years ago

Okay, that makes sense. I was trying to do some post-processing to ignore anything in the FROM statement and assume we aren't allowing any URL variables to change the table names. Now, I can see how this wouldn't be a viable option for everyone.

boughtonp commented 11 years ago

One way to solve this would be to include a SHA hash of the whole query code in the output, then have a KnownSafe field in the config which accepted a comma-delimited list of those hashes, and then skipped matching queries when scanning.

That would of course be a per-query thing, so less flexible than variable-based matching, but also far easier to implement (since it doesn't need any form of CFML/SQL parsing).

Would that help in your situation?

mrbusche commented 11 years ago

In my situation I want to exclude any FROM line from being considered a vulnerability. I need to dive more into the code and figure out where that should be added. On May 29, 2013 5:00 PM, "Peter Boughton" notifications@github.com wrote:

One way to solve this would be to include a SHA hash of the query code in the output, then have a KnownSafe field in the config which accepted a comma-delimited list of those hashes, and then skipped those queries when scanning.

That would of course be a per-query thing, so less flexible than variable-based matching, but also far easier to implement (since it doesn't need any form of CFML/SQL parsing).

Would that help in your situation?

— Reply to this email directly or view it on GitHubhttps://github.com/boughtonp/qpscanner/issues/8#issuecomment-18649172 .

boughtonp commented 11 years ago

Ah, ok. Excluding FROM is potentially complicated because of the possibility for stuff like:

SELECT [ from ] AS user_from , [ where ] AS location
FROM emails
WHERE [ from ] is not null
AND subject = 'message from bob'

Which is contrived but defeats a simple regex - ideally needs a proper SQL parser. (Would be nice to provide separate variables containing select/from/where/etc in the output.)

If you're not bothered by that, you can do an imperfect version by copying how the ORDER BY stuff does it...

Something like this added after line 43 of cfcs/qpscanner.cfc

    , killFrom      = new cfregex( '(?si)\n\s*FROM\b.*?\s+WHERE\b' )

Then this after line 171:

<cfif NOT This.scanFrom>
    <cfset rekCode = Variables.Regexes['killFrom'].replace( rekCode , '' )/>
</cfif>

And then update arguments and view accordingly.

mrbusche commented 11 years ago

Thanks a lot for your help, Peter. I finally got around to working on this again and got this working. If anyone else is interested in this the <cfif NOT This.scanFrom> would go after line 220 in v0.8-rc.