2

I ask this question with a bit of sheepishness because I should know the answer. Could someone be kind and explain if and how injection could occur in the following code?

<cfquery>
    select * from tableName
    where fieldName = '#value#'
</cfquery>

I'm specifically curious about injection attempts and other malicious input, not about best practices or input validation for handling "normal" user input. I see folks strongly advocating use of CFQueryParam, but don't think I see the point. If user input has been validated for consistency to the database schema (e.g. so that input must be numeric for numerical database fields), is there anything else gained by using CFQueryParam? What does <cfqueryparam CFSQLType = "CF_SQL_VARCHAR"> do that '#value#' doesn't do?

Peter Boughton
  • 110,170
  • 32
  • 120
  • 176
bwhet
  • 158
  • 2
  • 12
  • 7
    `cfqueryparam` [sanitizes](http://en.wikipedia.org/wiki/Secure_input_and_output_handling) the input. [Coldfusion has great easily readable documentation that explains what it does and it's purpose, fairly well](http://livedocs.adobe.com/coldfusion/8/htmldocs/help.html?content=Tags_p-q_18.html). And yes, you should always use `cfqueryparam` for values. – mawburn Sep 13 '13 at 17:52
  • Please don't be patronizing, Zap. I've read the docs and think I understand them. I'm specifically wondering about string validation. It says in the docs "escapes string variables in single-quotation marks" but doesn't CF already "magically" do this in CF query tag when you wrap evaluated variables in single quotes? – bwhet Sep 13 '13 at 18:00
  • 3
    @bwhet the answer is it sanitizes the input. There's zero reason you should not be using `cfqueryapram` for every query. If `; DROP TABLE USERS` was passed into the query as `value` it would not drop your table. – Matt Busche Sep 13 '13 at 18:16
  • 3
    `cfqueryparam` does not only 'sanitize' the data, it binds the data type. By using `` whatever value is passed will be treated as a string, not a SQL command. – Scott Stroz Sep 13 '13 at 18:25
  • I actually conducted a series of tests with cfquery once to see I could get malicious code to execute. For sql, your query is probaly safer than the others are implying. I tested with 3 database engines (sql server, oracle, and redbick) and two datatypes (integer and varchar). The only time I could get submitted sql to execute was with integer fields on sql server. Javascript was another story. Query parameters did nothing from a security point of view. I could store the javascript in my db, but if I retrieved it an outputted to my browser, it would execute. – Dan Bracuk Sep 13 '13 at 19:31
  • @bwhet, regarding escaping string variables, in your query, if value was equal to smith, you would be fine without query parmaters. If it was o'reilly, you would get a database error without query parameters. With query parameters the query would execute successfully. – Dan Bracuk Sep 13 '13 at 19:33
  • @Matt - That doesn't actually seem to be the case, because of the magic quotes. – bwhet Sep 13 '13 at 19:49
  • @Scott - I hear you, about binding. – bwhet Sep 13 '13 at 19:53
  • @Dan - It does seem like sql is somehow prevented from being executed in at least MSSQL, surprising as that is. CFQueryParam wouldn't help with the XSS Javascript problem, right? Or would it? Also, "O'Neil" actually seems to work, again because of the magic quotes. – bwhet Sep 13 '13 at 19:53
  • The main question I'm wrestling with is whether to update lines upon millions of lines of old code that I wasn't responsible for. Poor performance I can live with, but if there is a risk of injection I will need to do something about it. – bwhet Sep 13 '13 at 19:56
  • 1
    @bwhet - It depends on your DBMS and whether the fields are quoted. With quoted values, you obviously get CF's fallback protection. (While it seems pretty durable, I do not know how it ultimately stacks up against bind variable protection. ie If there is anything that can break it). If the parameters are *not* quoted, there is definitely a vulnerability that should be patched. The [QPScanner](http://qpscanner.riaforge.org/) might help. – Leigh Sep 13 '13 at 20:10
  • 1
    *user input has been validated for consistency* Also consider how you are validating. If the application uses client side validation (only), its basically worthless. Ultimately queries are only as safe as your validation scheme. – Leigh Sep 13 '13 at 20:30

3 Answers3

9

Update:

While this answers part of your question, Peter's response is better, in that it directly addresses your question of "Why use cfqueryparam, when CF automatically adds protection by escaping single quotes?". Answer: In short, because the latter does not always work. Bind variables do.

It says in the docs "escapes string variables in single-quotation marks" but doesn't CF already "magically" do this in CF query tag when you wrap evaluated variables in single quotes?

Yes, most versions automatically escape single quotes as a protection measure for those not using cfqueryparam. However, as Scott noted above, it is better to use cfqueryparam (ie bind variables) because they ensure parameters are not executed as sql commands. Bind variables work, even in cases where the automatic escaping does not, as Peter's answer demonstrates.

That said, sql injection protection is really just a side effect of using bind variables. The primary reason to use bind variables is performance. Bind variables encourage databases to re-use query plans, instead of creating a new plan every time your #parameters# change. That cuts down on compilation time, improving performance.

Cfqueryparam also has a number of other benefits:

  • Provides data type checking (length, value, type, ...)
  • Provides attributes that simplify handling of "lists" and null values
  • Performs data type checking before any sql is sent to the database, preventing wasted database calls

While it does not really apply to string columns, IMO another big reason to use it is accuracy. When you pass a quoted string to the database, you are relying on implicit conversion. Essentially you are leaving it up to the database to figure out how to best perform the comparison, and the results are not always what you were expecting. (Date strings are a prime example). You may end with inaccurate results, or sometimes slower queries, depending on how the database decides to execute the sql. Using cfqueryparam avoids those issues by eliminating the ambiguity.

Community
  • 1
  • 1
Leigh
  • 28,765
  • 10
  • 55
  • 103
3

doesn't CF already "magically" do this in CF query tag when you wrap evaluated variables in single quotes?

Yep, it'll convert ' to '' for you.

Now guess what SQL you get from this code:

<cfset value = "\'; DROP TABLE tableName -- " />

<cfquery>
    select * from tableName
    where fieldName = '#value#'
</cfquery>


The cfqueryparam tag works; using query params solves SQL injection.

Any custom written attempts at validating, sanitizing, or escaping (all separate things, btw) are, at best, only as good as the developer's knowledge of the database system the code is running against.

If the developer is unaware of other escape methods, or if the values are modified between validation/escaping and them being rendered into SQL, or even if the codebase is ported to another database system and seems to be fine, there's a chance of custom code breaking down.

When it comes to security, you don't want chances like that. So use cfqueryparam.

Peter Boughton
  • 110,170
  • 32
  • 120
  • 176
  • Cool! Now I have the answer to my question about how it stacks up against bind variables ;-) I ran it against a SQL Server and MySQL datasource. Curiously it did nothing in SQL Server. Not sure what the difference is .. – Leigh Sep 14 '13 at 01:06
  • 1
    (Edit) Ah, [`\\` is an escape character in MySQL](http://dev.mysql.com/doc/refman/5.1/en/string-literals.html). Yipes, that is a major flaw in the escaping algorithm ... @bwhet - You have your answer. – Leigh Sep 14 '13 at 04:17
  • 1
    Yep, the point I was too tired to make last night (but have added now) is that custom written validation/escaping is limited by the developer's knowledge of the database system the code is running against (and even then might be unsafe if done once at the start of a request), whilst query params simply solve the problem. – Peter Boughton Sep 14 '13 at 11:07
0

To answer the first part of your question, setting your #value# variable to the following:

someValue'; DELETE FROM tableName WHERE '1' = '1

would result in this query being executed:

<cfquery>
    select * from tableName
    where fieldName = 'someValue'; DELETE FROM tableName WHERE '1' = '1'
</cfquery>
Avery Martell
  • 317
  • 1
  • 7
  • **Nope**. It would not. Not in any recent version. As mentioned in the comments above, CF automatically escapes embedded single quotes. – Leigh Sep 13 '13 at 19:37
  • From tests, though, it doesn't seem like that actually happens. Sorry Leigh, didn't see your comment. – bwhet Sep 13 '13 at 19:54
  • Sorry, @Leigh is correct. It may have been the case in earlier versions, but not any longer. I also agree with the assertions Leigh makes about the main benefits of using bind variables. – Avery Martell Sep 13 '13 at 20:09
  • @AveryMartell - Yeah, that was the case in early versions. They finally wised up and added in the auto escaping sometime around CF6 (I *think*). – Leigh Sep 13 '13 at 20:15