Where is my dynamic SQOL query is vulnerable?

Question

I'd like to better understand which parts of dynamic SOQL can be vulnerable. I have a query:

Database.query(
    ' SELECT ' + queryFieldNameFromUserInput +
    ' FROM '   + objectNameFromUserInput     +
    ' WHERE '  + whereFieldNameFromUserInput + 
            ' LIKE \'%' + whereFieldValueFromUserInput + '%\''
);`

Which of those variables should be checked for the vulnerability and why?:

• queryFieldNameFromUserInput
• objectNameFromUserInput
• whereFieldNameFromUserInput
• whereFieldValueFromUserInput

Answer 1

What ever your are passing in dynamic query and have that input from user is vulnerable. So while passing you should use String.escapeSingleQuotes(yourparamater). for example in your query you should use

Database.query(
        ' SELECT ' + String.escapeSingleQuotes(queryFieldNameFromUserInput) +
        ' FROM ' + String.escapeSingleQuotes(objectNameFromUserInput) +
        ' WHERE ' + String.escapeSingleQuotes(whereFieldNameFromUserInput) + 
                ' LIKE \'%' + String.escapeSingleQuotes(whereFieldValueFromUserInput) + '%\'');

For more information on it please have a reference to salesforce docs such as SOQL Injection.

Answer 2

The vulnerable parts of your query are: queryFieldNameFromUserInput, objectNameFromUserInput, whereFieldNameFromUserInput, and whereFieldValueFromUserInput.

Depending on your controller's "sharing" keyword, a comprehensive check would include something like the following:

String
queryFieldNameFromUserInput, objectNameFromUserInput, whereFieldNameFromUserInput, whereFieldValueFromUserInput;

Boolean isObjectAccessible = false, isFieldAccessible = String.isNotBlank(queryFieldNameFromUserInput),
    isWhereAccessible;
SObjectType baseType;
DescribeSObjectResult baseDescribe;
Map<String, SObjectField> fields;
// Check object accessibility
try {
    isObjectAccessible =
        (baseDescribe = 
        (baseType = ((SObject)Type.forName(objectNameFromUserInput).newInstance())
        .getSObjectType())
        .getDescribe())
        .isAccessible();
} catch(Exception e) {

}
if(!isObjectAccessible) {
    return; // display an error or something here
}
fields = baseDescribe.fields.getMap();
// Check field accessibility
try {
    for(String field: queryFieldNameFromUserInput.replaceAll('\\s+','').split(',')) {
        isFieldAccessible &= fields.get(field).getDescribe().isAccessible();
    }
} catch(Exception e) {
    isFieldAccessible = false;
}
if(!isFieldAccessible) {
    return; // display an error or something here
}
// Check more field accessibility
try {
    isWhereAccessible = fields.get(whereFieldNameFromUserInput).getDescribe().isAccessible();
} catch(Exception e) {
    isWhereAccessible = false;
}
if(!isWhereAccessible) {
    return; // display an error or something here
}
// Do query
SObject[] results = Database.query(
    'SELECT '+queryFieldNameFromUserInput+
    ' FROM '+objectNameFromUserInput+
    ' WHERE '+whereFieldNameFromUserInput+
    ' LIKE %'+String.escapeSingleQuotes(whereFieldValueFromUserInput)+'%'
);

Note that I still consider this incomplete, because you'd also need to check the data type for the whereFieldNameFromUserInput, because you might need it to be a number, date, date/time, Boolean, etc, or the query itself can still crash.

And, as I mentioned in the second sentence of this answer, you also need to make sure that your class is declared as public with sharing class XYZ, to make sure the query cannot return records the user cannot see.

Answer 3

+1 for Rishi's answer.

But be aware that where it is only data values that you want to vary in the where clause using a simple (no dots) bind expression (see e.g. Dynamic SOQL) is usually a cleaner way to go and avoids the SOQL injection risk:

String abc = '...';
String xyz = '...';
String xyzLike = '%' + xyz + '%';

Database.query('select ... where Abc__c = :abc and Xyz__c like :xyzLike');