8

Trying to work out if this a valid issue with my code, or just a false positive I can note in report for Security Review.

I have provided a slightly simplified version below. The Id I am querying for is provided as a bind variable, though the field I am querying is also inserted from a setting using dynamic SOQL. I believe the bind variable should mitigate against the SOQL injection, and I would rather do that than use escape single quotes. So not sure if it is the syntax throwing off the scanner, or if there is an issue I am missing?

Thanks

//Page params is a map for a custom setting called mycustomsetting__c 

String sQueryFinal = 'SELECT Id, LastName FROM Contact ';
mycustomsetting__c conpageparam = pageparams.get('KnownContactId');
String sPersonPageId = conpageparam.kwac__Source_Id__c;

sQueryFinal+= ' WHERE '+ conpageparam.kwac__Source_Field__c + ' =  :sPersonPageId ';    

sQueryFinal += ' ORDER BY CreatedDate ASC';
database.executequery(sQueryFinal)
3
  • why are you using database.executequery insted of square bracket query feature? Jan 7, 2016 at 4:59
  • @AtulRajguru9 - What do you mean square bracket feature. This is a dynamic SOQL and thus cannot use the typical [] query language
    – Eric
    Jan 7, 2016 at 5:02
  • I got that its dynamic query butmy suggestion was to make many(possible 2-3) static queries and use if else condition to choose correct static SOQL Jan 7, 2016 at 5:05

4 Answers 4

6

If you want to be cautious, you can check that it is an actual field first. Possibly easier to sanitize input than enforce namespace.

public static List<Contact> safeQuery(MyCustomSetting__c setting, String filterValue)
{
    SObjectField field = SObjectType.Contact.fields.getMap.get(settng.Filter_Field__c);
    if (field != null && field.getDescribe().isAccessible()) return Database.query(
        'SELECT Id FROM Contact WHERE ' + String.valueOf(field) + '= : filterValue'
    );
    // "else"
    // return empty collection?
    // throw exception?
    // send email?
}
3
  • 1
    Yes, this is the only thing you can do, but also do the .isAccessible() check, since even if it's a valid field name, the logged in user may not have permission to view this field, and the query leaks info about the field even if the field itself is not returned to the user. Jan 7, 2016 at 9:41
  • Just to confirm - as written, that doesn't create an injection risk since the variable is contained in a bind variable? The field itself is setup during the installation, so isn't likely to be ever modified, by the variable being queried will change... Jan 7, 2016 at 17:06
  • You might want to do some exception handling so you don't make it possible to discover what variable names are defined in scope. I'd have to play around with it to see, but my guess is you'd catch Query Exception.
    – Adrian Larson
    Jan 7, 2016 at 17:11
5

It could be an SOQL injection issue depending on how the overall application populates the custom setting.

If the user has access to populate the custom setting then they could populate it with a string that would cause SOQL injection.

Best bet would be to just use the proper methods to prevent it.

Although, you would probably get away with an explanation that the value is being queried for and not actually entered but he user. The big difference in this case is that the database itself will not ensure its format like it would if the value was an ID field or something. I have had success with the explanation as long as the value being queried is never populated by a User Interface that could make it an issue

4
  • agreed. I would suggest you to use square bracket query which will speedup security review process. Jan 7, 2016 at 5:02
  • @AtulRajguru9 - See my comment above in the question. If you are talking about [select ... from ...] that is not possible here as it is a dynamic query. note OP stated it is a simplified version so I would assume the use of a dynamic query is necessary in the complete solution
    – Eric
    Jan 7, 2016 at 5:02
  • So yes, the id could be set by a user request, though the values passed in has been encrypted, so the user can't just pass any value in and hope it works. But if they can potentially pass in any value, is there something else can be added to avoid an injection risk aside from using a bind variable/adding string.escapequotes? Jan 7, 2016 at 17:09
  • No. You will have to escape quotes or html etc if you want to use a dynamic query
    – Eric
    Jan 7, 2016 at 17:31
2

Perhaps this is prevented in your full query, but in the one provided a person can enumerate valid contact IDs, and any which match a custom setting would appear to complete the query, allowing anyone to query the ID and last name of any contact in the setting.

I agree with your main point that this is not vulnerable to injection in the general sense, since the value passsed is not actually put in the query.

2
  • Yeah, that's a good point - the code actually uses an encrypted id so that should prevent that happening... Jan 7, 2016 at 17:04
  • 1
    @BritishBoyinDC When we had a similar situation, we switched from using the record ID to a random number, but hashing or encrypting the id will probably suffice, so long as it is prohibitively expensive to enumerate the possible hashes/ciphertexts. Jan 7, 2016 at 17:49
0

When write a dynamic soql we need to mind these points:

  1. All parameters should be checked with appropriate methods like String.escapeSingleQuotes
  2. All fields you are trying to access should be checked, like if user have access of those fields and Object.
  3. All DML operations should be validated with user's available permission in Apex code.

For your code, it seems like if you will check the page parameters and will validate the fields security, there are no reasons for a security threat.

I am sure that security scanner will not raise issue if we follow these steps and if does, then we can report as false positive.

Thanks

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .