Salesforce Stack Exchange is a question and answer site for Salesforce administrators, implementation experts, developers and anybody in-between. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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)
share|improve this question
    
why are you using database.executequery insted of square bracket query feature? – AtulRajguru9 Jan 7 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 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 – AtulRajguru9 Jan 7 at 5:05
up vote 6 down vote accepted

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?
}
share|improve this answer
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. – Robert Sussland Jan 7 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... – BritishBoyinDC Jan 7 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 at 17:11

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

share|improve this answer
    
agreed. I would suggest you to use square bracket query which will speedup security review process. – AtulRajguru9 Jan 7 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 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? – BritishBoyinDC Jan 7 at 17:09
    
No. You will have to escape quotes or html etc if you want to use a dynamic query – Eric Jan 7 at 17:31

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.

share|improve this answer
    
Yeah, that's a good point - the code actually uses an encrypted id so that should prevent that happening... – BritishBoyinDC Jan 7 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. – IllusiveBrian Jan 7 at 17:49

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

share|improve this answer

protected by Community Jan 28 at 14:30

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.