9

I'm trying to use this method in a dynamic SOQL query. Follow the sample code:

public static String getRowById(String sobjName, id id) {
    String query = 'SELECT ';            
    Map<String, Schema.SObjectField> objectFields =
        Schema.getGlobalDescribe().get(sobjName).getDescribe().fields.getMap();
    for (String f : objectFields.keySet()) {
        query += f;
        query += ',';
    }

    query = query.substring(0, query.length() - 1);
    query += ' FROM '+ sobjName;
    query += ' WHERE id = \'' + String.escapeSingleQuotes(id) + '\'';
    query += ' LIMIT 1';

    return query;
}

The problem is that this is still considered as a SOQL Injection vulnerability.

Am I missing something here? I also tried to use it as a parameter like this:

getRowById('SFT_Brand__c',
    String.escapeSingleQuotes(ApexPages.currentPage().getParameters().get('id')));
  • 3
    I'd consider the sobjName argument to be the bigger injection risk if it is coming from the query string parameters. Also, I'd be surprised in the Id type could contain single quote characters. Look at validating if sobjName is a valid sObject? – Daniel Ballinger Feb 14 '13 at 19:19
  • 2
    @DanielBallinger: but doesn't the use of sObjName in the Schema.getGlobalDescribe() call verify it's valid? It would throw an exception there if it wasn't. If OP is always hard-coding the object name in his calls, I can't see an injection vulnerability. – tomlogic Feb 14 '13 at 20:43
  • @tomlogic: Yes, I think you are correct. Schema.getGlobalDescribe().get(sobjName) returns null for an unknown type. – Daniel Ballinger Feb 14 '13 at 21:28
  • When SQL injection succeeds, can you give us the value of query variable using System.debug? – Shumon Saha Feb 15 '13 at 5:10
  • Value of query: select id,sft_brand_component_type__c,name,isdeleted,sft_brand_tire_type__c,createddate,sft_brand_vehicle_type__c,systemmodstamp,ownerid,lastmodifiedbyid,createdbyid,lastmodifieddate from SFT_Brand__c where id = 'a04U0000006id9NIAQ' limit 1 – vanderlindo Feb 15 '13 at 11:11
7

You are using it correctly. Here's a simplified version of your script, using the new String.join() method and all of the string concatenations in one statement instead of spread out over multiple statements.

public static String getRowById(String sobjName, Id id) {
    Map<String, Schema.SObjectField> objectFields =
        Schema.getGlobalDescribe().get(sObjName).getDescribe().fields.getMap();

    return 'SELECT ' + String.join( objectFields.keySet(), ', ')
           + ' FROM ' + sObjName
           + ' WHERE Id = \'' + String.escapeSingleQuotes(Id) + '\''
           + ' LIMIT 1';
}

You are correctly escaping the id parameter, but that might not even be necessary, since it's an Id and not a String.

This looks to me like a false positive by the security scan. If someone disagrees, they should provide a sample exploit that:

  • Doesn't throw an exception when sObjName is used to get the field list.
  • Accepts an invalid Id (15 or 18-character alphanumeric) as the second parameter.
  • 2
    I'd also suggest adding with sharing to the class definition so that the user doesn't get sObject access beyond their permissions. Maybe check if Schema.getGlobalDescribe().get(sObjName) returns null, indicating an unknown sObjName? – Daniel Ballinger Feb 14 '13 at 21:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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