Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CFQUERYPARAM_REQ false positives #282

Closed
TheRealAgentK opened this issue Jun 7, 2017 · 10 comments
Closed

CFQUERYPARAM_REQ false positives #282

TheRealAgentK opened this issue Jun 7, 2017 · 10 comments
Assignees
Labels

Comments

@TheRealAgentK
Copy link
Collaborator

I found a bunch of false positives when running CFQUERYPARAM_REQ, all on MS SQL Server. I know on MySQL apparently one can cfqueryparam in the SELECT statement for instance, but MS SQL Server doesn't allow that.

Is there any chance a change/improvement to the SQL parsing process could weed out some or all of those kinds of reportings?

  1. SELECT TOP #arguments.numberOfRecords# ...

  2. SELECT something FROM #application.config.LinkedServerName#.somethingelse.dbo.Comment C WITH (NOLOCK)...

  3. <cfqueryparam value="Data copied from #variables.siteDetailList[arguments.siteID]["name"]# - #dateFormat(now(),"DD/MM/YYYY")#" cfsqltype="varchar">

  4. OPEN SYMMETRIC KEY #config.symmetrickey#
    DECRYPTION BY CERTIFICATE #config.dbCertificate#
    ...
    CLOSE SYMMETRIC KEY #config.symmetricKey#

  5. SELECT '#arguments.additionalValue#' AS aID, '#arguments.additionalOption#' AS trans ...

@ryaneberly
Copy link
Contributor

#3 should not flag since it is in a cfqueryparam tag.

But all of the others are potential SQL injection vectors, unless I am missing something.

I would be comfortable excluding variables from specific scopes (like application), but #arguments.someValue# is exactly what this rule is intended to flag.

@TheRealAgentK
Copy link
Collaborator Author

Ha, yeah - those are interesting ones.

I agree - number 3 is an obvious false positive.

The issue with any of the other ones is that it seems cfqueryparam can't be used to fix them, at least not on SQL Server, because it doesn't seem to accept query params in the Select part of a query. Some people on the web claim that MySQL does allow that, for instance.

For 1, I could at least improve the situation by using val() around it - CFLint would probably still report it though. 2,4 and 5, I'm not sure how to deal with them.

So - you're kind of right, maybe the term "false positives" is wrong for those - but if they're not cases for cfqueryparam to fix, I'm not sure if the cfqueryparam rule should pick them up.

I actually think that excluding certain scopes is not a good idea, probably not even using the application or server scopes etc.

This would be less of an issue if we could get the ignoring to work, I reopened #195 for exactly this case with some test cases in the dev branches.

ryaneberly added a commit that referenced this issue Jun 16, 2017
@ryaneberly
Copy link
Contributor

I fixed the issue with #3. #195 should take care of the rest.

TheRealAgentK added a commit to TheRealAgentK/CFLint that referenced this issue Jun 19, 2017
@TheRealAgentK
Copy link
Collaborator Author

TheRealAgentK commented Jun 19, 2017

Yup, can confirm that sub-issue 3 from above is fixed and that ignoring on a query-level now is working. Opened #295 for a more fine-grain ignore handling within SQL - but I'm not sure if that's feasible.

ryaneberly pushed a commit that referenced this issue Jun 19, 2017
@KamasamaK
Copy link
Collaborator

@TheRealAgentK What version of SQL Server are you using? I've tested on SQL Server 2012 with CF9 and this is what I found.

1. works with parentheses.
SELECT TOP (<cfqueryparam cfsqltype="CF_SQL_INTEGER" value="#arguments.numberOfRecords#">) *

5. works as is.
SELECT <cfqueryparam cfsqltype="CF_SQL_VARCHAR" value="#arguments.additionalValue#"> AS aID, <cfqueryparam cfsqltype="CF_SQL_INTEGER" value="#arguments.additionalOption#"> AS trans

@TheRealAgentK
Copy link
Collaborator Author

@KamasamaK

Re 1: Ah, we didn't try with parentheses around the number, will give that a try.
Re 5: Hmmm, I'm pretty sure we tried that and it didn't work. SQL Server 2012/2014 with CF11. Need to try that one again, too.

@KamasamaK
Copy link
Collaborator

@TheRealAgentK I've found that not only does 5 work like that, but also in functions. For example, we have some queries that use things like
SELECT ISNULL(someValue, <cfqueryparam cfsqltype="CF_SQL_VARCHAR" value="#someDefaultValue#">) AS someValue

@TheRealAgentK
Copy link
Collaborator Author

@KamasamaK 1 does indeed work fine, but I can't get 5 to work. Well, it does work syntactically, how would I get it to actually to get to not return the strings refunddate or member_length?

<cfset column = "refunddate">
<cfset column2 = "member_length">

<cfquery name="paymenthistory" datasource="something">
	SELECT TOP (<cfqueryparam value="#numberOfRecs#" cfsqltype="cf_sql_integer">)
	CASE WHEN (some condition) THEN something ELSE somethingelse END AS paiddate,
	<cfqueryparam value="#column#" cfsqltype="cf_sql_varchar"> as refund,
	<cfqueryparam value="#column2#" cfsqltype="cf_sql_varchar"> as length, ...

Running this query with cfqp essentially spits out the literal strings refunddate and member_length in each row of the query in those columns. What I want though is to dynamically pass in the column names to be used - and I still think that is not possible.

@KamasamaK
Copy link
Collaborator

@TheRealAgentK Oh, I misunderstood the intent. Your example has them in single quotes, so it was misleading. cfqueryparam will only work for passing data, not database objects/object names. I'm no SQL Server expert, but I doubt you can do that without resorting to dynamic SQL. I don't even think there's a way to do this with stored procedure params and invoking directly in SQL Server (ignoring ColdFusion completely).

@TheRealAgentK
Copy link
Collaborator Author

Yeah, I know. You can do it without cfqueryparam by just putting stuff in # pounds at that's what they've been doing quite extensively.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants