2

I'm not ColdFusion developer but this has something put on my head to fix within week.

Problem: our legacy app creates view by empid and use logged user id to select records from view.

<cfset empid= session.emp_id>

Then this variable is used in cfquery as

Select columns from sometext_#empid#

This has been flagged by Veracode for SQL injection and it's being done in lots of pages so manual code change in all queries is little impossible.

I changed #empid# with cfqueryparam and it's not flagged by Veracode anymore but I read it's not what cfqueryparam is meant for and I'm worried it might break

Tried to find solution but could not find anything close to my problem. Is there any way to validate SQL injection at cfset tag itself and hope Veracode understand not to flag this variable or what would be the proper way to fix this?

Miguel-F
  • 13,450
  • 6
  • 38
  • 63
Navi
  • 21
  • 2
  • 1
    Where did you read that `cfqueryparam` is not to prevent SQL injection? It is indeed one of the things that tag is used for. This has been documented several times already. Here are the top 4 from my Google - [ref 1](https://www.adobe.com/devnet/coldfusion/articles/sql_injection.html), [ref 2](https://community.hostek.com/t/how-to-prevent-coldfusion-sql-injection/418), [ref 3](https://bobby-tables.com/coldfusion), [ref 4](https://stackoverflow.com/questions/2592700/how-do-i-prevent-sql-injection-with-coldfusion) – Miguel-F Sep 09 '19 at 11:53
  • You can use queryparams in table names? – Bernhard Döbler Sep 09 '19 at 12:42
  • https://www.bennadel.com/blog/1396-ask-ben-dynamic-table-names-in-coldfusion-queries.htm – Bernhard Döbler Sep 09 '19 at 13:08
  • @BernhardDöbler I assumed that was just a bad example since he said he already changed the query to use `cfqueryparam` and got passed the Veracode check. – Miguel-F Sep 09 '19 at 14:21
  • 1
    Because Veracode no longer sees the erroneous pattern. Did the query run and return results? – Bernhard Döbler Sep 09 '19 at 14:23
  • 1
    *I changed #empid# with cfqueryparam* 1. Could you please post your cfquery? What you're describing (i.e. `select column from table`) shouldn't work - **at all** ;-). Cfqueryparam is only designed for use with query parameters - NOT table names. 2. What exactly do these sql views contain? – SOS Sep 09 '19 at 15:08
  • ... I realize you're simplifying for the sake of clarity, but this is a case where we need to see the actual code involved, or at least a "runnable" version :-) – SOS Sep 09 '19 at 15:25
  • You are trying to fix something that isn't broken. Assuming `session.emp_id` is an integer generated when the user logs in, and not available for modification, sql injection will simply not happen. The most efficient use of time and effort is to document that this particular Veracode item is not an issue. – Dan Bracuk Sep 09 '19 at 16:54
  • You shouldn't rely on input being pre-validated by an external source. Always validate at the query level. – SOS Sep 09 '19 at 17:32

1 Answers1

4

I've dealt with this a few ways. The bottom line is you need to verify the value of empid is numeric and not a string when passing it as part of the table name. These scanners look for these patterns, but will not recognize that the solution addresses it. You can get the company running the scans or your security team to verify the solution works with QA testing.

One solution for this is to wrap the query with a function (if it isn't already). The function argument verifies the value passed in can only be numeric. Numeric values can't cause SQL injection attacks. 

<cffunction name="getEmployeeViewByID" access="public" output="false" returntype="query">
    <cfargument name="employee_id" type="numeric" required="true" hint="Employee ID">
    <cfquery name="local.q">
        SELECT * FROM sometext_#arguments.employee_id#
    </cfquery>
    <cfreturn local.q>
</cffunction>

<cfdump var="#getEmployeeViewByID(int(session.emp_id)#">

Notice that the variable session.emp_id is wrapped with the int() function. That function only allows a number as an argument. Any string passed in will throw an error. We implemented that function often to verify numeric values in query string parameters and other places where SQL injection could occur.

You might be able to get away with just this: 

Select columns from sometext_#int(empid)#

but it depends on the amount of refactoring involved.

Adrian J. Moreno
  • 14,350
  • 1
  • 37
  • 44
  • 2
    *These scanners look for these patterns, but will not recognize that the solution addresses it.* Yes, that's been my experience in scans of other languages too. @Navi - I understand time may not allow refactoring, but the per-user views is a bit unusual. What is it that they do that can't be accomplished with a normal `where` clause filter? – SOS Sep 09 '19 at 20:37