Skip to main content
P
pbk-nFjYI4
Participant
July 23, 2009
Answered

Preventing SQL injection - can't use cfqueryparam in this case

  • July 23, 2009
  • 4 replies
  • 1700 views

Hello. I have a form with a checkbox next to each row.  If the user checks some boxes, then clicks the "Delete" button, I want to execute the following query, but I want to protect it from sql injection attacks:

    <cfquery datasource="#application.mainDS#">
        delete userMessages
        where messageID in (#form.messageID#)
    </cfquery>

As written above, it works fine.  But if I try to protect this code with <cfqueryparam value="#form.messageID#" cfsqltype="cf_sql_varchar">, I get this error: "Conversion failed when converting the varchar value '7,21' to data type int" (7 and 21 are the messageID's to be deleted).  Obviously the comma prevents conversion to an integer.

If I use cfsqltype="cf_sql_integer", then the string gets converted to a single integer (in this case 40015, which is nonsense).

I tried passing form.messageID to a stored procedure, but I seemed to have the same problem there.  I could run the query in a loop where I just delete one row at a time, but I'd like to run just one query if I can do it safely.  Any ideas?

Thanks.

PK

This topic has been closed for replies.
Correct answer -__cfSearching__-

You just need to add the "list" attribute to cfqueryparam to indicate the "value" contains multiple messageID's.

<cfqueryparam value="#form.messageID#" cfsqltype="cf_sql_integer" list="true">

4 replies

T
TLC-IT
Inspiring
July 24, 2009

I agree that you should not do an SQL "DELETE" from a web page.  Instead, use "soft deletes," where you contrive for there to be a deleted_flag (boolean), and maybe deleted_by (varchar) and deleted_timestamp.  Then create an SQL "VIEW" which automagically omits the "deleted" records.

It is also a very good idea to refer to the records using a nonsensical, made-up "moniker" instead of actual record-IDs.  You see, "if I am a nasty person and I know that there is a record #123456, then I'll bet I know the record-IDs of 123,455 other records, too."  But if you refer to the record as "QZB0E9S" and the next record-id in the list is "4Q_9RJPEM2" then it won't take me long to realize that I can't get too far, not even by brute-force.  (And if I see that the record-IDs seem to have verification tags, like "QZB0E9S:4E396", then I know that I am really scroo'd in my hacking-attempt because even if I did somehow million-monkeys my way into a valid record-ID, I've got no earthly idea how to come up with the tag.

It pays to code defensively, like this.  And it doesn't really take more time.  Without question, always use <cfqueryparam> !!

-
-__cfSearching__-
Inspiring
July 24, 2009

Those are all good points about deleting information.

While I am a fan of the soft delete method myself,  obviously you should also consider the type of information being "deleted".  Some types of data, like preferences, can be safely discarded without disturbing the functionality of an application.  However, as others have mentioned, if you think you may need to recover the information in the future (or if the deletions make break other queries due to relationships), soft deletes are a safer bet.

A
Anonymous
July 24, 2009

Personally I think it's a bad idea to use a delete query from any data sent from a web site. It may be too late at this point but if you use a bit or an integer column you can switch the value to active (1) or deleted (0) without actually deleting data. Then if you really want to delete the records query those

with a different SQL user off the site with delete capabilities.

A
Anonymous
July 24, 2009

to expand on CFMXPrGrmR's answer;

what would happen if someone were to craft their own post? yourwebsite.com/action.cfm?messageID=1,2,3,4,5,6,7,8,9,10,11,12....and so-on ?

you need more checking to be 'resaonably' sure that the request can delete messages, i.e. in your sql, create a join on the user's table, delete only messages belonging to that user. do some conditional session checking before the SQL is executed, do some validation on the messageID variable to be sure it contains ONLY a list of integers, some sanitization to strip out things like select, drop, create, etc.etc.etc.  and possibly some logging on failure/injection attempts so you have an audit trail and saom idea of what was attempted.

basically make sure you check the sql using non-client/form supplied data [session vars, credentials etc.]

-sean

Dileep_NR
Dileep_NR
Inspiring
July 24, 2009

Hi,

try this

  <cfquery datasource="#application.mainDS#">
        delete userMessages

        where messageID in (<cfqueryparam cfsqltype="cf_sql_integer" list="true" value="#form.messageID#" separator="," />)
    </cfquery>

-
-__cfSearching__-
Inspiring
July 24, 2009

catchme_dileep wrote:

where messageID in (<cfqueryparam cfsqltype="cf_sql_integer" list="true" value="#form.messageID#" separator="," />)

Umm.. was that not what was already suggested ;-) ?   BTW, "separator" is not be needed here as the default is already ","

-
-__cfSearching__-Correct answer
Inspiring
July 23, 2009

You just need to add the "list" attribute to cfqueryparam to indicate the "value" contains multiple messageID's.

<cfqueryparam value="#form.messageID#" cfsqltype="cf_sql_integer" list="true">

Terms & ConditionsAccessibility statement
Using the community Terms of use Privacy Cookie preferences Do not sell or share my personal information ad choicesAdChoices