Salesforce Security

S4 Security Scan Results

Salesforce Security
3 MAY 2022
blog

The use of static code analysis has significant benefit for a development team, most importantly it establishes best practices patterns of reuse. All dev teams run based on what is common within the existing code base and the use of copy and paste to support development outcomes is universal!

Static code analysis and their recommendations are, however, not enough to ensure that anti patterns are not introduced and propagated. Additional quality processes must be used to support this, the simplest being pull requests.

What do I mean by an anti-pattern? Here’s a common scenario that is flagged by all static code analysis engines and the recommendation to correct.

S4 Warning

Background

CRUD violations occur when the Apex code doesn't enforce CRUD checks and runs in system context. This leads to authorization by-pass. Authorization, or access control, is a way of mediating access to resources and application functionality based on the identity of a user. Anytime a user can gain access to a resource that is denied to their role, they have performed authorization bypass. There are countless ways authorization bypass can occur. In this case, the user is able to execute a Read, Update, or Delete (CRUD) function in Apex code without their permission being validated correctly. Authorization Bypass occurs when user permissions are not validated prior to executing a Create, Read, Update, or Delete (CRUD) function in Apex code.

Remediation Guideline

User permissions should always be validated before a CRUD function is executed in Apex code. This way, if the user does not have the correct permissions for what they are trying to do, they will now be allowed to proceed.

Implement access control checks such as isAccessible() prior to Select. isAccessible() checks to see if the user who is trying to select something has permissions to access that information.

Implement access control checks such as isCreateable() prior to insert. isCreateable() checks to see if the user who is trying to insert something is allowed to insert that type of information/object.

Implement access control checks such as isUpdateable() prior to update. isUpdateable() checks to see if the user who is trying to update something is allowed to do so.

Implement access control checks such as isDeleteable() prior to delete. isDeleteable() checks to see if the user who is trying to delete something is allowed to do so.

Code Snippet:

 if(Schema.sObjectType.Case.isCreateable()) {insert SecurityScanCase;}

WITH SECURITY_ENFORCED in SOQL Select statements can also be used instead of isAccessible().

Code Scan reports similar

Field Level Security Vulnerabilities

Vulnerability
Major
owasp-a3, owasp-a5, salesforce
Available Since Feb 22, 2019
Apex (APEX)
Constant/issue: 30min

This rule makes sure that the code checks for access permissions before running a SOQL, SOSL, or DML operation. This prevents access to protected resources by users without the correct privileges.

The recommendations made are syntactically correct and rerunning a scan will remove the flagged failure if you implement one of the suggestions, however the introduction of this test has now potentially introduced a significant defect.

Case caseRec = new Case();  
 
if(Case.SObjectType.getDescribe().isAccessible() &&  
 
Schema.SObjectType.Case.fields.Id.isAccessible() && Schema.SObjectType.Case.fields.ContactId.isAccessible() && Schema.SObjectType.Case.fields.ContactEmail.isAccessible()) {  
 
caseRec = [Select ContactId, ContactEmail From Case where Id =: caseRecord FOR UPDATE];  
 
}  
 
String targetObjectId = caseRec.ContactId;

Logically a test for something must assert both a positive and a negative response, this is the point that is missing from the recommendation. Without an alternate path, the code will simply fail the data access and the assignment will fail.

String targetObjectId = caseRec.ContactId;

While the simplest way to test is to generate an exception.

If (caseRec == null) throw SomeException(‘Access to case is not allowed’); 

I also like the use of assert to provide some context and provides more readability, I've also removed the redundant isAccessible() test for the Id field.

Case caseRec = new Case();  
 
System.assert(Case.SObjectType.getDescribe().isAccessible(), ‘The Case object is not accessible’);  
 
System.assert(Schema.SObjectType.Case.fields.ContactId.isAccessible() ‘The Contact field on the Case object is not accessible’);  
 
System.assert(Schema.SObjectType.Case.fields.ContactEmail.isAccessible() ‘The Contact Email field on the Case object is not accessible’);  
 
caseRec = [Select ContactId, ContactEmail From Case where Id =: caseRecord FOR UPDATE];  
 
String targetObjectId = caseRec.ContactId;

This anti-pattern is compounded through the use of some of Salesforce security controls in particular the use of stripInaccessible

public static List<ContentDocumentLink> getContentDocumentLinkFromLinkedEntityId(Set<Id> linkedEntityIdSet){ 
 
List<ContentDocumentLink> returnList = new List<ContentDocumentLink>();  
 
SObjectAccessDecision securityDecision = Security.stripInaccessible(AccessType.READABLE, [SELECT Id, LinkedEntityId FROM ContentDocumentLink WHERE LinkedEntityId IN :linkedEntityIdSet]);  
 
returnList = (List<ContentDocumentLink>) securityDecision.getRecords();  
 
return returnList;  
 
}

The result in the condition where Access is not granted is spectacularly bad as there may be records returned that do not contain the full set of required fields (LinkedEntityId) dependent on the permissionSet, permissionSetGroup or Profile applied.
Again assertion should be made in this case using the object isSet() function

SObjectAccessDecision securityDecision = Security.stripInaccessible(AccessType.READABLE, sourceRecords);  
 
Contact c = securityDecision.getRecords()[0];  
 
System.assert(c.isSet('social_security_number__c')); 

In summary, static code analysis is incredibly useful but it does not stop you from introducing defects, to do this you need a robust DevSecOps model – preferably with separation of QA, Release & Dev resources. At AppGenie we are happy to assist in designing your perfect delivery pipline!