1

Ran a security scan against an URL and received the report below:

The vulnerability affects

 /rolecall.cfm , bbb_id

This is the rolecall.cfm code:

<cfscript>
if (isDefined("url") and isDefined("url.bbb_id")) {
    if (url.dept_id eq -1)
        _include("sql", "getB");
    else
        _include("sql", "getBNow");
}

/*...*/
_include("sql", "getDPlaces");

/*Set up the model and go*/
model = {
    add    = 1,
    edit   = 0,
    remove = 0,
    places   = getDPlaces
};
</cfscript>
FunkoRobo
  • 11
  • 2
  • 1
    I am not understanding what `_include` is. Please add more information on how the query looks and if possible add an example query constructed with the above code. – rrk Sep 06 '18 at 14:18
  • I've updated with the _include file for more clarity hopefully. – FunkoRobo Sep 06 '18 at 14:44
  • 3
    The query should be using [cfquerparam](https://helpx.adobe.com/coldfusion/cfml-reference/coldfusion-tags/tags-p-q/cfqueryparam.html) on all client supplied parameters (URL, FORM, etc..) - in this query `#url.dept_id#`. Also, what is the source of `#db.root#`, `#db.dept#`, etc..? If they're user supplied they're also a sql injection risk. However, you can't use cfqueryparam on table names. – SOS Sep 06 '18 at 15:04
  • 1
    Not related to sql injection, but the JOIN's can be simplified to eliminate a lot of the nested (SELECT * FROM...)'s. – SOS Sep 06 '18 at 15:08
  • Anything inside of `#url...#`, shouldn't be anywhere near a connection to a database. Definitely sanitize those values then put them in a `cfqueryparam` to further mediate injection issues. And you'll probably also get flagged for using dynamic table names. You should definitely, at the very least, validate those entries against a whitelist of acceptable tables. – Shawn Sep 06 '18 at 17:08
  • And just out of curiosity, which security scanner are you using? Some can false flag and throw up pains that cause a work-around that is sometimes worse than the actual security issue it initially identified. – Shawn Sep 06 '18 at 17:15
  • If you're worried about security, you may want to consider using a CDN with WAF rules. They can stop attacks before they even reach your hosting facility. – Jules Sep 07 '18 at 02:19
  • @FunkoRobo - S.O. discourages large edits that signifigantly alter the content of the question, because it may invalidate existing answers and comments :) If you wish to add more detail, please *append* it to the question. To remove sensitive information, try genericizing the variable and table names in such a way that it still preserves the original meaning (or submit a request to redact it). If you have a different question, please open a separate thread. – SOS Sep 07 '18 at 13:46

2 Answers2

4

If you're using IIS, you should read this article to see how to add SQL Injection protection directly to the web server. This will keep attack requests from ever reaching ColdFusion.

Be cautious of the strings they suggest you deny:

<denyStrings> 
   <add string="--" /> 
   <add string=";" /> 
   <add string="/*" /> 
   <add string="@" /> 

Make sure you never pass an email address as the value of a query string parameter, otherwise you'll reject a legitimate request. You can allow the @ symbol if needed.

I would also highly suggest you take a look at HackMyCF, which will show you many other security concerns if they exist.

Adrian J. Moreno
  • 14,350
  • 1
  • 37
  • 44
  • While I do agree that every little bit helps, I'm still more of a fan of whitelisting than blacklisting. Apparently, IIS Request Filtering does have an option to sort of canonicalize the request, which would be necessary to effectively blacklist strings. It's kinda sad that some form of injection has essentially been the top issue of OWASP Top 10 for most of the last 15 years. :-( https://www.owasp.org/index.php/Reviewing_Code_for_SQL_Injection – Shawn Sep 06 '18 at 19:45
3

SQL Injection exploits databases by stuffing malicious sql commands into a query where they're not expected. Tricking the query into do something different than what it was designed to do, like performing a DROP or DELETE instead of a SELECT.

  1. Queries that use raw client parameters like this, are vulnerable:

    WHERE policy_funct_id = #url.dept_id#
    

    Instead, always wrap client supplied parameters in cfqueryparam. It prevents them from being executed as a command. I don't know your column data types, so modify the cfsqltype as needed.

    WHERE policy_funct_id = <cfqueryparam value="#url.dept_id#" cfsqltype="cf_sql_integer">
    
  2. All of the dynamic table names are another (potential) vulnerability, like:

    -- potential sql-injection risk
    SELECT * FROM #db.root#
    

    If #db.root# is user supplied, it's a sql-i risk. Unfortunately, cfqueryparam cannot be used on table names. Those must be manually (and carefully) validated.


Few other suggestions, unrelated to sql injection:

  • All the nested (select * from...) statements decrease readability. Instead, use a single level JOIN.

  • When using JOIN's, best to specify the source table (or table alias) for all columns. That avoids ambiguity and increases readability for yourself and anyone else reviewing the code. No need to guess which columns comes from which table.

Example

-- psuedo example
 SELECT  root.ColumnA
    , root.ColumnB
    , dept.ColumnC
    , subcat.ColumnC
    , etc... 
 FROM #db.root# root 
       INNER JOIN #db.content# content ON root.policy_root_id = content.content_id
       INNER JOIN #db.dept# AS dept ON ON content.dept_id = dept.policy_funct_id
       INNER JOIN #db.subcat# subcat ON subcat.dept_id = dept.policy_funct_id
 WHERE dept.policy_funct_id = <cfqueryparam value="#url.dept_id#" cfsqltype="cf_sql_integer">
 AND   content.is_newest = 1
SOS
  • 6,430
  • 2
  • 11
  • 29
  • 2
    Thanks for your help. Our previous programmer sort of left us in a hole. I am just trying to fix where I can. – FunkoRobo Sep 06 '18 at 16:48
  • Okay. Even if you leave the nested JOIN's for now, be sure to do 1 and 2, to plug any sql-injection holes. – SOS Sep 06 '18 at 17:03
  • @Ageax Once again I got delayed in responding, and you've beaten me to the answer. And once again, you are creeping around in my brain. Good to see though that you and I agree on how to unwind that query. The only change I'd suggest would be to put `content.is_newest=1` and `dept.policy_funct_id=.....` into the `JOIN` conditions rather than the `WHERE`. It won't make a difference on `INNER JOIN` but can change `OUTER JOIN`s. The query optimizer will deal with it how it wants, but I like to be consistent in the habit of putting `JOIN` conditions in the `JOIN` and filters in the `WHERE`. – Shawn Sep 06 '18 at 17:46
  • @Shawn - Finally, a difference! ;-) I agree about putting filters in the WHERE clause, rather than cluttering up the ON condition, but personally I view `is_newest` and `policy_funct_id` as filters. I prefer to only place conditions inside the ON condition if absolutely required, helping distinguish between OUTER and INNER JOIN's. – SOS Sep 06 '18 at 18:49
  • 1
    @Ageax And that, my friend, is an _extremely_ philosophical debate (about on the same level as "spaces vs tabs"). That sort of discussion is probably not really suited for SO. Or maybe even Reddit. :-) – Shawn Sep 06 '18 at 18:58
  • @Shawn - Heh... now I am VERY curious about spaces vs tabs (of course there is only 1 correct answer ;-), but .. alas not the place for it. – SOS Sep 06 '18 at 19:09
  • 1
    @Ageax I'll stick with the only IT answer guaranteed to be 100% correct: "It depends." – Shawn Sep 06 '18 at 19:13
  • 1
    @Shawn - When I first got started, I used to *hate* that answer, but .. quickly discovered it is soo true. – SOS Sep 06 '18 at 20:19