Robust exception handling is one of the tenets of best practice for development, no matter what the coding language. This blog post explores the curious circumstances in which a developer trying to do the right thing – but without appreciating the full effects – could lead to data integrity issues in a Salesforce Organization. As we’ll explore, the precise impact will vary according to what’s being done to which data, but the potential for consequences detrimental to security is clear.

The Salesforce platform tries to ensure data integrity by having an automatic rollback mechanism, delimited by the common database concept of a ‘transaction’. However, as is so often the case, the devil is in the detail. On the basis of recent code reviews, it is apparently under-appreciated how the addition of exception handling in Apex (the Salesforce development language) can affect the rollback mechanism, which in turn can affect data integrity. There have been a couple of notable articles on this subject in the past [1] as well as discussions on forum sites, but the treatment has been relatively light. After checking various permutations on a test Organization, this article qualifies and expands on existing material in this space, highlighting the potential consequences for security on top of more functional side effects. In other words, while the condition is not new, its impact as a security vulnerability is explored. It’s important to understand that this is not a vulnerability in the Salesforce platform itself but a bug that could arise during custom development whose impact may extend to security.

Background

The Data Manipulation Language (DML) is essentially the subset of Apex that allows access to the database, including write operations that SOQL (the Salesforce Object Query Language) doesn’t support. There are two ways to use DML: executing DML ‘statements’ or calling Database class methods. By way of example, the following two lines are equivalent:

1.  insert newAccounts;
2.  Database.insert(newAccounts);

Both ways accept a single sObject or, as the above examples imply, a List of sObjects. So what’s the difference? For one thing, using a Database class method (line 2) allows a finer degree of control for bulk operations: an optional allOrNone Boolean parameter governs whether processing should stop if an error is encountered [2]. The default value of this parameter is true, which means that a database error stops the processing of further sObjects in the List before an exception is thrown. This mode mirrors how a DML statement handles an error when working on a List. If allOrNone is explicitly set to false, partial processing is allowed: if an error occurs, the remaining work is still given a chance to complete. In addition, instead of throwing exceptions, a result object (or an array of them if a List is passed) is returned containing the status of each operation and any errors encountered. This allows the caller to inspect the results and handle any failures appropriately [3].

As mentioned in the opening remarks, Salesforce has the concept of a ‘transaction’ – a collection of database operations through a code path that has a definite start and end point. A classic example would be a call to an Aura endpoint by a client-side Lightning Component, where the start point would be the entry into the @AuraEnabled method and the end point would be its exit through the final return. In between, any number of other methods from any number of other classes could be called, and the collection of DML operations along the way would constitute this particular transaction. Salesforce documentation explains that:

…all changes are committed to the database only after all operations in the transaction finish executing and don’t cause any errors. [4]

While true in one sense, it doesn’t capture the full range of outcomes and how conditions can arise that may cause data integrity issues [5].

Risks to Data Integrity

Setting the allOrNone parameter to false in a call to a Database method is not accidental: it can be assumed that the caller wants partial processing. But a less obvious risk to data integrity can emerge when an exception is thrown within a try block after one or more DML operations have occurred, whether in the same try block or anywhere earlier in the transaction stack. The crucial point about automatic rollback is that it occurs after unhandled exceptions. But if the exception is caught [6], it is assumed the catcher knows what they are doing! If the catch block doesn’t explicitly handle any previous DML operations, those database changes will be committed – unless, of course, a new exception is thrown that either remains unhandled or is caught further up the stack where the database state is restored.

Reflecting on this, it’s relatively easy for a developer to write code that unintentionally makes a partial set of database changes under certain error conditions. This is like calling a Database method and setting allOrNone to false accidentally! The impact is obviously context-dependent, but it’s conceivable that the resulting state could have security implications.

Published examples in this area tend to have a DML operation as the cause of the exception, with a prior database operation being the problem to clean up. But an important point to highlight is that the exception need not relate to DML. Consider the following ‘proof-of-concept’ code:

// set up chgAccounts, a List of Accounts to update
try {
   update chgAccounts;   // need not be in this (or any) try block to be affected
   Account acc = [SELECT Name FROM Account WHERE Name = 'Acme'];
   // do something with acc, and maybe some other risky things
}
catch (Exception e) {}

Imagine one day that the Acme Account doesn’t exist anymore. Following the update, a System.QueryException is thrown because there is nothing to assign to the variable acc. Because the exception is caught (although ignored) the Account updates will be committed to the database. Note, as per the comment, that the update could be anywhere in the previous transaction path. This example also shows how the general bad practice of having empty catch blocks can have a specific consequence unique to Salesforce. However, even a valiant attempt at exception handling can still lead to data integrity problems if this specific aspect hasn’t been fully considered by the developer.

Whether the Account updates in the above case will have security implications, or indeed any kind of impact, will depend on the context. Let’s briefly consider a different scenario to illustrate a security risk. Imagine that custom code creates a new User during registration but, later in the transaction, an exception is raised and caught because a business logic check fails. Without adequate handling, the new user will still be created, and therefore the registrant could log in, contrary to business rules.

Finally, a particular use of the Database class is also worth calling out, whether it’s in a try block or not. Consider this single DML operation:

Database.update(newAccount, false);

Although the allOrNone parameter indicates that partial record processing is allowed, this method of database access will never throw exceptions and, in this example, the return values are effectively discarded. Therefore the caller has no idea of the success/failure status, whether a single sObject has been passed or a List. This may be acceptable in some cases, but it should be verified as such.

Recommendations

Any code path that includes a DML operation should be evaluated in full because a data integrity vulnerability could arise from an exception being caught at any point. Where custom exception handling is implemented, consider if any database changes earlier in the transaction need to be reverted manually. It is important to remember that catching an exception of any kind (not just one related to DML) could lead to a vulnerable condition. Clearly, using DML operations to reverse database changes should be avoided, since these too could raise exceptions, and round we go again. Instead, Salesforce supports a convenient ‘savepoint’ and ‘rollback’ mechanism [7].

If partial record processing is used explicitly, it is imperative that the return values from the particular Database method are captured, inspected and handled appropriately [8], using a format such as:

Database.SaveResult[] results = Database.insert(mySObjects, false);
for (Database.SaveResult result : results) {
   // check result.isSuccess() etc.
}

While potentially vulnerable conditions can be relatively simple to spot in a single method or class, remember that a transaction spans the life of a particular execution path. Therefore, establishing whether a vulnerability exists, and its resulting impact, can involve traversing back up that path. Even an exception thrown outside a try block could still lead to a data integrity issue if it’s caught further up the stack. For these reasons, an exhaustive search for this kind of vulnerability is likely beyond the remit of most manual code reviews because of time constraints and the complexity of analysis [9].

In Summary

This article aims to raise awareness of a particular kind of Apex vulnerability. In truth, while the necessary conditions may be common, perhaps more common than previously realised, instances of an exploitable vulnerability with a tangible benefit to an attacker will be rarer. Functional side effects, or simply an ‘untidy’ database, are a more likely result, but which are nevertheless unwanted and best avoided. It all depends on the context, though, and exploitable attack opportunities leveraging this condition may well exist out there. Through articles such as this, hopefully developers and code reviewers alike will be better able to find them.

Jerome Smith @exploresecurity

(thanks to Viktor Gazdag @wucpi for proof-reading)

Notes

[1] For example, https://medium.com/elevate-salesforce/apex-transaction-control-a-dml-case-study-c4b535825205 and https://www.crmscience.com/single-post/2020/05/20/how-salesforce-developers-handle-errors-with-try-catch-rollback-best-practices.
[2] https://developer.salesforce.com/docs/atlas.en-us.apexref.meta/apexref/apex_methods_system_database.htm
[3] https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/langCon_apex_dml_database.htm
[4] https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/langCon_apex_dml_transactions.htm
[5] This is why I have so far avoided the term ‘atomic’ – either everything completes or no changes are made – when talking about a transaction. In contrast, a single DML statement, or a Database method call with allOrNone missing or true, will process a List of sObjects in a truly atomic fashion: one failure will cause all changes made to the preceding records in the List to be reverted before an exception is thrown.
[6] Occasionally, system exceptions cannot be caught. One example is System.LimitException, which is raised when ‘governor limits’ are exceeded. If this exception is thrown, even within a try block, automatic rollback will follow because an unhandled error has been thrown. More information at https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/langCon_apex_dml_examples.htm and https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/apex_exception_statements.htm.
[7] https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/langCon_apex_transaction_control.htm
[8] https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/langCon_apex_dml_database_error.htm
[9] This is potentially something that static analysis tools could do, however. The current capability of Apex analysers has not been investigated during the research for this article.