When will cfqueryparam NOT protect me?
Posted by
Brad Wood
Jul 22, 2008 20:31:00 UTC
JR asked a good question on my queryparam Scanner post. He noticed that I had stopped short of saying cfqueryparam would ALWAYS stop ALL SQL injection. He said, "Can you give an example of a SQL Injection attack which is not caught by cfqueryparam ?" I'm glad you asked JR.SQL injection can occur any time cautions are not taken to completely separate your SQL code from its parameters and the parameters being passed in are allowed to flow into the SQL statement itself. SQL injection can also occur when building the SQL statement's table and column names dynamically.
Here is an example in which every input into the cfquery is parameterized, but SQL injection can still occur:
[code]<cfquery name="qry_hack_me" datasource="foo"> DECLARE @var AS varchar(50) DECLARE @sql_string AS varchar(200) SET @var = <cfqueryparam value="#url.id#"> SET @sql_string = 'SELECT column ' SET @sql_string = @sql_string + 'FROM table t ' SET @sql_string = @sql_string + 'WHERE column2 = ' + chr(39) + @var + chr(39) EXEC @sql_string </cfquery> [/code]AS you can see, the only way the parameter gets into the query is through the cfqueryparam. The SQL statement, however, takes that parameter and unscrupulously mixes it in with SQL code in a manner that would allow injection to occur. The EXEC and EXECUTE statements not differentiate between SQL and parameters since it is just all one big string as far as it is concerned. If the @var variable contains a single tick, it will "break out" into the rest of the statement. The example above is obviously simplified to prove the point. There would be no reason to do exactly what I did, but people often find reasons to do stuff like that. There is a work around though. MSSQL also gives us the system proc sp_executesql. Here's what it would look like:
[code]<cfquery name="qry_unhackable" datasource="foo"> DECLARE @var AS varchar(50) DECLARE @sql_string AS nvarchar(200) DECLARE @param_def AS nvarchar(200) SET @var = <cfqueryparam value="#url.id#"> SET @sql_string = N'SELECT column ' SET @sql_string = @sql_string + N'FROM table t ' SET @sql_string = @sql_string + N'WHERE column2 = @use_this_id' SET @param_def = N'@use_this_id varchar(50)' EXECUTE sp_executesql @sql_String, @parm_def, @use_this_id = @Var </cfquery> [/code]sp_executesql allows you to concoct your dynamic SQL statement, but still keep your parameters separate from the statement itself until execution. If you are on MSSQL and you MUST dynamically build a statement on the Database side, please consider sp_executesql. The other common way cfqueryparam won't help you is if you have a stored procedure with dynamic SQL in which the table or column names themselves are dynamic. Additionally the order by clause is commonly made dynamic. While code like that can be VERY flexible, I would suggest refactoring your application to not require it. If your app unavoidably requires a SQL statement build with random tables, NEVER accept input directly in. Check systables or INFORMATION_SCHEMA with a parameterized query first to make sure that is a valid table or column name. Alternatively, instead of passing variables directly into the statement, use the variable in an if statement of predefined choices so in the worst case scenario your default case will be used, but the incoming variable itself never makes it into the statement. Consider the following stored procedure which may very well have been called with the uber-safe cfstoredproc and cfprocparam tags:
[code]CREATE PROCEDURE sp_test @order_by varchar(4) = 'ASC' -- populated from form input as ASC or DESC AS BEGIN DECLARE @SQL_STRING AS varchar(200) SET @sql_string = 'SELECT column FROM table ORDER by column ' + @order_by EXEC @sql_string END [/code]This procedure is indeed vulnerable. A parameterized query won't solve it this time because the ASC or DESC part of the ORDER BY clause isn't a parameter. (Note, that declaring @order_by as a varchar(4) will definitely mitigate the impact though) In this case, it would be more code, but safer to do the following:
[code]CREATE PROCEDURE sp_test @ORDER_BY varchar(4) = 'ASC' -- populated from form input as ASC or DESC AS BEGIN DECLARE @SQL_STRING AS varchar(200) SET @sql_string = 'SELECT column FROM table ORDER by column ' IF @order_by = 'ASC' BEGIN SET @sql_string = @sql_string + 'ASC' END ELSE BEGIN SET @sql_string = @sql_string + 'DESC' END EXEC @sql_string END [/code]In this way, the parameters to the proc are used to control the SQL statement, but they aren't allowed to enter the statement. There can be endless combinations to this-- many of them exist inside stored procedures-- all of them equally as vulnerable. You can feel safe about using cfqueryparam and cfstoredproc/param, but please don't do it just because I tell you to or someone else tells you to. Do it because you understand WHY SQL injection works and HOW cfqueryparam can (and can't) stop it.
Dan Lancelot
Some great points there Brad
Just one more thing to add (and I know this isn't the point you were trying to make!)
Another good reason for avoiding dynamically generated SQL (apart from the security risks outlined here) - is that in most circumstances a new QEP will be generated for every distinct execution of the SQL - resulting in a significant performance hit if the statement is executed multiple times.
ike
Speaking of sp_executeSQL and Query Execution Plans (QEP)... I know in theory the server is supposed to cache the execution plans for statements for performance reasons because it takes a certain amount of processing cycles to analyze the query and then analyze the tables and indexes to determine the fastest way to execute it... And so I'd always been under the impression that like Dan mentioned, if you generated the query dynamically for execution using EXEC or EXECUTE that you were basically eliminating the performance gains from cached execution plans.
Someone recently told me that this is not the case if you use sp_executeSQL, that it will still cache and reuse the execution plans from one call to the next. At least on the surface that sounds unlikely to me. How would the server know that @string_x and @string_y contain equivalent queries (and can use the same cached plan)?
So I was wondering if you (or any of the other readers) had any information about that, if you could confirm either way about sp_executeSQL either caching or not caching the plans. Does it work if you use separate parameters but not if you mash everything into the string?
Thanks
Brad Wood
@Ike: Good questions.
Read this blog post that explains how QEP's work and see if that helps.
http://www.codersrevolution.com/index.cfm/2008/7/26/cfqueryparam-its-not-just-for-security-also-when-NOT-to-use-it
Feel free to post additional questions if you have them.
Keep in mind that using a cfquery WITHOUT cfqueryparams is the equivalent of building a string and passing it into EXEC.
Using a cfquery WITH cfqueryparam is the equivalent of using sp_executeSQL with parameters.
The caching rules apply the same whether the query is coming from ColdFusion or from another piece of SQL (like a stored proc)
ike
Aha! Thanks Brad, that did indeed clear up my confusion. :)
When I was working for DealerPeak out in Portland, CFChris (Philips) had mentioned that a DBA had recommended that they stop parameterizing their client-id, because they use a single database with all their clients in it (I want to call that "single tenant", but I'm having difficulty finding clear definitions of what "single tenant" and "multi-tenant" mean with regard to Software as a Service (SaaS)). Anyway the DBA they hired to performance tune their application recommended un-parametering the client-id because there were so many queries using that variable out of the session and because with a large number of clients any given client only used a small part of the database. I got the gist of the reason why it was important for them to un-parameter it, however, that other article you posted really clarifies exactly how that works and why it's important. So in their case the db would have cached an execution plan based on whichever client was the first to hit the application that day and then likely all other clients would be getting queries optimized for that one client.
My solution would have been individual databases, which if I'm not mistaken from reading, would also mean that each client gets their own set of cached execution plans instead of a single set for all clients, effectively multiplying the available QEP caching by the number of clients on their system.
ike
Or more likely -- optimized for all clients even though only one client would be relevant for a given query.
Brad Wood
Glad this helped.
It is true that all execution plans are not equal. SQL Server's Query Optimizer does a good job though and the same index can have the possibility of being useful for everyone.
I would probably not recommend un-Parameterizing queries on nothing more than the suspicion of non-optimal cached plans unless you have proof. If the DBA was just guessing, I would have asked him to prove it.
If he had actually witnessed slow performance due to cached plans that did not apply very well, then it might solve your performance problem.
Keep in mind, QEP's are most likely to differ when the cardinality of your data is lower.
Mikey
I'm not sure that your simplified version gives CFQUERYPARAM full justice. In your post, you opted to omit the "cfsqltype" attribute of the tag. Using this in conjunction with a CFTRY block can stop invalid parameter types getting to the database. You could also use this in conjunction with the val() function:
<cfqueryparam value="#val(url.id)#" cfsqltype="cf_sql_integer" />
I dunno, I'm not as experienced as you guys, only been programming a few years so maybe I'm just being naive. Nice blog though!!
:)
ike
Hey Mikey, no worries re: "getting the hang of things". I've been at this programming thing for a decade and you can see from the comments that I was still a bit confused about sp_executeSQL. I don't think anyone in this industry is ever really done learning. :)
In most cases you're right that using the cfsqltype attribute can help prevent invalid data types from reaching the database, and of course you can use val() in conjunction with it if that's helpful for your use case. But this article really wasn't about whether or not cfquerparam is a good tag or you should use it. It is and you should. There's no question there. This article was merely intended to show that even though cfqueryparam is an excellent tool and should be used, it is still possible to leave yourself open to SQL injection attacks even when you're using cfqueryparam if you write your queries in this way. The structure of Brad's examples is admittedly a little unusual but certainly still valid with SQL Server and there are bound to be similar ways to achieve the same results with Oracle or MySQL.
So the idea here is simply to show that you still need to exercise a little bit of caution to prevent SQL injection (because cfqueryparam doesn't automatically mean you're protected).
In this case the cfqueryparam tag ensures that the input is a string, which is perfectly valid, so it doesn't throw any errors. And then that string is used to build a separate SQL statement which again is perfectly valid and no errors are thrown. Finally the new SQL statement string is executed via sp_executeSQL and again, perfectly valid, no errors are thrown. The problem is that because the input values are strings and they're just concatenated, it basically creates an "end-run" around the cfqueryparam tag where it's the same thing as not having used cfqueryparam at all.
Most of the time you don't need to worry about this sort of thing because you won't be building your queries this way. But it's good to keep in mind just to be sure you don't fall prey to a false sense of security if you do end up needing to write a query like this.