Apex Code Development You need to sign in to do that
Don't have an account?
New Security Review Rule for enforcing FLS and CRUD Checks in Custom Controllers
Last week, it was brought to my attention that security reviews and audits are now asking ISVs to check for Field Level Security (FLS) and Object CRUD permissions prior to any DML outside of standard controllers.
This is a fairly radical change that impacts a lot of managed package applications across the board. I and others have several concerns about this change in the security review process:
This is a fairly radical change that impacts a lot of managed package applications across the board. I and others have several concerns about this change in the security review process:
-
Why the sudden change in security review policy?
- None of the ISVs I have talked to were informed of the change prior to being either audited or submitting any apps for security review. The only official documentation I can find on the change is a blurb in the Security Scanner help page here: http://security.force.com/security/tools/forcecom/scannerhelp.
- As ISVs, it would be greatly appreciated to have an official Security Review Guide, that is updated well in advance to when policy changes like this take place. Some ISVs have several packages, others lots of customizations, and we all need time to prepare for the security review.
-
Has the security scanner been updated to enforce this rule yet?
- I have not run an app through the Security Review process recently, so I am curious if this rule has been implemented already in the security scanner. (http://security.force.com/security/tools/forcecom/scanner)
-
Why force FLS and CRUD checks on every DML transaction?
- I am a fan of using standard controllers when necessary, but several apps, including one my company develops rely heavily on Custom Controllers. In most cases where FLS comes into play, typically it's a simple fix of a profile permission or permission set that resolves the issue. Instead, now, we have to implement FLS and CRUD checking prior to each DML transaction. This adds complexity on top of existing DML calls.
- If need be, can the FLS and CRUD checking be done on a one-time run in an install script?
- Does Salesforce have any plans of either adding FLS checking for custom controllers or adding methods to Apex for checking credentials similar to the Force.com EASPI library? (https://code.google.com/p/force-dot-com-esapi/)
First, I’m sorry to hear that this came as a surprise to you. Hopefully I can help with some resources that should make it easier and more predictable in the future.
1. Why the sudden change in security review policy?
The changing nature of security dictates that requirements will evolve over time as platform features change and as the security/threat landscape changes. In this case we’ve had this listed as a requirement since October 2009 on the requirements checklist. (See the link below) It’s important to keep up with security best practices and requirements to ensure that our mutual customers and their data are protected. We have a site dedicated to developer resources that should help. Please see the links below:
Secure Cloud Development site:
http://developer.force.com/security
Secure coding Guidelines (includes Crud/FLS Guidance):
http://wiki.developerforce.com/page/Secure_Coding_Guideline
Requirements checklist: http://wiki.developerforce.com/index.php?title=Requirements_Checklist
Between this documentation and the guidance on OWASP that is cross-linked you should have a comprehensive set of documentation to help you to prepare for the security review.
2. Has the security scanner been updated to enforce this rule yet?
We have a beta rule that can find some instances of CRUD/FLS violations but it is prone to false positives and negatives. This is a really difficult issue to find using static analysis because it’s more of a program logic issue than a data flow issue like XSS.
Unfortunately tools cannot find every issue or even reliably find all classes of issues. otherwise security would be a solved problem. Secure coding standards and manual reviews play a huge part in securing your offering.
3. Why force FLS and CRUD checks on every DML transaction?
The spirit of the requirement is to honor the access control configuration choices that org admins make in ISV offerings. If an admin explicitly restricts access control for sharing/CRUD/FLS then ISV offerings should respect that. The dynamic nature of access control makes a one time check an ineffective way to enforce the policy. I agree that it is a bit tedious but our mutual customers expect it.
I personally think that easier CRUD/FLS enforcement would be a fantastic feature and I’d encourage you to add that to our Ideas site. https://success.salesforce.com/ideaSearch The teams really do look at ideas and some of our best features have come from our users.
I hope this helps. Again we don’t want to put undue burden on our partners but our mutual customers have high expectations for the security of our platform and our ISV offerings. Security is part of your success and ours.
All Answers
I want to pretty much echo everything you just said, but want to add a few additional things. We too just got an awful surprise when our package (4 years old) was rejected during re-review du to FLS/CRUD.
CRUD/FLS should be enforced by the plaform -- not subject to a novice developer, or a quickly implemented feature by a seasoned Apex developer that bypasses it. Having to explicitly code each CRUD/FLS check is a recipe for an inadvertant security leak and is the reason platforms provide core service layers -- to prevent everyone from having to code the same tedious thing a zillion times over.
Salesforce sells security as one of the features of the platform. Customers expect it to be enforced.
Salesforce's security review process is inconsistent, slow, and doesn't require that every package version be tested -- just a periodic re-review -- so this leaves Salesforce open to embarassment when a customer discovers that the Profiles and Permission Sets they set are not honored in all circumstances.
As for the new security review requirement: the security review is a complete mystery to pretty much anyone except those within the security team. Each package we've had reviewed is a white knuckle affair because the automated security scanner doesn't identify the same things that the human doing the review identifies and with a large package (ours has over 100k lines of Apex), having any reasonable conversation about each individual issue turns into chaos. No one can say for sure when the CRUD/FLS checks became mandatory because they were always "policy" -- they apparently just got 'overlooked'? in the past? (Where "in the past" is as recent as June -- that's when some of our other packages with definite CRUD/FLS problems were happily approved)
I'm most disappointed in the fact that this new policy didn't spawn someone on the Platform team to speak up and say "yes, we can do this in a comprehensive way as part of the plaform" rather than shed the responsiblity onto the thousands of ISVs that chose the platform because of 'security as a service'. This leaves a bitter taste in our mouths.
What I'd like to hear is that the Platform team has heard us loud and clear, is going to implement native support for CRUD/FLS within one or two Salesforce releases, and that ISVs currently getting rejected will be granted provisional passing of their review with the expectation that they enable support for the new platform enforcement as soon as it's available.
I, too, look forward to feedback from Salesforce.
-Dave
First, I’m sorry to hear that this came as a surprise to you. Hopefully I can help with some resources that should make it easier and more predictable in the future.
1. Why the sudden change in security review policy?
The changing nature of security dictates that requirements will evolve over time as platform features change and as the security/threat landscape changes. In this case we’ve had this listed as a requirement since October 2009 on the requirements checklist. (See the link below) It’s important to keep up with security best practices and requirements to ensure that our mutual customers and their data are protected. We have a site dedicated to developer resources that should help. Please see the links below:
Secure Cloud Development site:
http://developer.force.com/security
Secure coding Guidelines (includes Crud/FLS Guidance):
http://wiki.developerforce.com/page/Secure_Coding_Guideline
Requirements checklist: http://wiki.developerforce.com/index.php?title=Requirements_Checklist
Between this documentation and the guidance on OWASP that is cross-linked you should have a comprehensive set of documentation to help you to prepare for the security review.
2. Has the security scanner been updated to enforce this rule yet?
We have a beta rule that can find some instances of CRUD/FLS violations but it is prone to false positives and negatives. This is a really difficult issue to find using static analysis because it’s more of a program logic issue than a data flow issue like XSS.
Unfortunately tools cannot find every issue or even reliably find all classes of issues. otherwise security would be a solved problem. Secure coding standards and manual reviews play a huge part in securing your offering.
3. Why force FLS and CRUD checks on every DML transaction?
The spirit of the requirement is to honor the access control configuration choices that org admins make in ISV offerings. If an admin explicitly restricts access control for sharing/CRUD/FLS then ISV offerings should respect that. The dynamic nature of access control makes a one time check an ineffective way to enforce the policy. I agree that it is a bit tedious but our mutual customers expect it.
I personally think that easier CRUD/FLS enforcement would be a fantastic feature and I’d encourage you to add that to our Ideas site. https://success.salesforce.com/ideaSearch The teams really do look at ideas and some of our best features have come from our users.
I hope this helps. Again we don’t want to put undue burden on our partners but our mutual customers have high expectations for the security of our platform and our ISV offerings. Security is part of your success and ours.
To clarify your point partners are required in the partner agreement to enforce a level of security consistent with our requirements and security best practices. There are manual touch points such as the periodic re-review and automated touch points where every package version is scanned using the code scanner. We rely on the hard work and diligence of our partner community to keep offerings on the AppExchange secure.
I ask because depending on the codebase and how dyanmic it is CRUD and FLS checks can consume a rather substantial amount of limits use, and in many cases can actually limit the configuration possible on an application since apex can involve use cases other and just read/write (like reading only to derive other values in a controller and not exposing the field's value to the user).
Can anyone tell me what is the mistake I have done in the below code, because checkmarx throwing it as FLS Vulnerabilitie.
Savepoint sp = Database.setSavepoint();
User u = new User();
u=[SELECT Contact.email, Contact.firstName, Contact.lastName FROM User WHERE id=:userId];
if(Schema.sObjectType.Contact.fields.email.isupdateable())
{
u.Contact.email = data.email;
}
if(Schema.sObjectType.Contact.fields.firstName.isupdateable())
{
u.Contact.firstName = data.firstName;
}
if(Schema.sObjectType.Contact.fields.lastName.isupdateable())
{
u.Contact.lastName = data.lastName;
}
if(Schema.sObjectType.User.fields.firstName.isupdateable())
{
u.firstName = data.firstName;
}
if(Schema.sObjectType.User.fields.LastName.isupdateable())
{
u.lastName = data.lastName;
}
if(Schema.sObjectType.User.fields.email.isupdateable())
{
u.email = data.email;
}
try {
if(User.sObjectType.getDescribe().isupdateable())
{
update u;
}
if(Contact.sObjectType.getDescribe().isupdateable())
{
update u.Contact;
}
} catch (Exception e) {
Database.rollback(sp);
}
Appricate for the fast response.
Thanks,
SaleemBaba Mohammed
We just finished retrofitting our large code base (100k lines of Apex) with a utility that we are open sourcing. We corresponded with a member of the security review team and it was given initial blessing and we finally received approval for our package today. As the README indicates, this is really the low-risk band-aid to retrofit existing code. It adds some additional cost with respect to governor limits (max SOQL queries, and max rows retrieved) so if you have methods already at the brink of exploding, you might have some refactoring in your future.
https://github.com/patronmanager/apex-dml-manager
So that I give credit where credit is due, the core concept was that of Eric Whipple and with the help of Tom Fuda, we crossed all the I's and dotted the T's .. or something like that
Please vote up a couple ideas that would've made this a little less hacky:
https://success.salesforce.com/ideaView?id=08730000000GzMwAAK
https://success.salesforce.com/ideaView?id=08730000000l5vbAAA
We're glad to accept contributions -- just create a Pull Request
I also want to call attention to Dan Appleman's excellent and timely post about sharing rules in general -- the last paragraph is really the best piece of advice you can take. Paraphrasing: As long as you can demonstrate that you thought about security when writing your Apex code, the security review team is quite reasonable to accept justifications for cases where system access is necessary
https://developer.salesforce.com/page/Without_Sharing
As an experiment goes it works quite well with zero changes to the existing sample app demonstrating the patterns. However sadly (for now at least) shares the pain (https://success.salesforce.com/ideaView?id=08730000000l5vbAAA) of determiing the fields that have been modified with David's solution. i'll be writing up a blog post with more details on the pros and cons (as well as less controversial improvements to the patterns).
While I'm commited to helping the community with this challange in the here and now of the current Security Review requirements. I am even more dedicated to encouraging Salesforce to add platform support for this asap. The bottom line is, it's unacceptable to seed even a moderate code base with these kind of checks. Ideally we need seamless support for this kind of checking, as per 'with sharing' with something like 'with user permissions' keyword perhaps? Or at the least in the meantime, some improved support to Dynamic Apex as per the idea here (https://success.salesforce.com/ideaView?id=08730000000l5vbAAA).
"Each package we've had reviewed is a white knuckle affair because the automated security scanner doesn't identify the same things that the human doing the review identifies and with a large package (ours has over 100k lines of Apex), having any reasonable conversation about each individual issue turns into chaos." Still an issue 2 years later.
I have scenarios that fall underneath the WITHOUT SHARING article. https://developer.salesforce.com/page/Without_Sharing
As a OEM building custom software on the platform it is extremely difficult to work with SFDC Security Engineers on any of those items, even though it is published.
There has been no improvements on visibility into this process either.
Furthermore there is no predictability. I have walked 2 App Extensions through Security Review Successfully. Then when I use the same justifications for design decisions that were accepted by 1 security reviewer and then they are denied by a 2nd reviewer after waiting months in the review queue.
The first (https://success.salesforce.com/Ev_Sessions?eventId=a1Q30000000DHQlEAO#/session/a2q30000001FmIhAAK)was a fireside with Mike Tetlow from Bracket Labs and he brought to everyone's attention that CRUD/FLS checks are now also being enforced on READ (i.e. SOQL queries) as well as on insert/update by the Security Review team. This further complicates development in Apex. We read the Salesforce Developer blog and are subscribed to all of the appropriate groups in the Partner Community; none of this was ever announced or discussed. It just arrives in your inbox as a surprise new policy when you fail security review.
The second (https://success.salesforce.com/Ev_Sessions?eventId=a1Q30000000DHQlEAO#/session/a2q300000019kL6AAI) was with Ryan Flood from Salesforce security team. I brought up the core concerns outlined in this thread and Ryan was taken aback, almost as if he had never heard from an ISV about how onerous of a process it is to outfit your entire code base with CRUD/FLS checks. Additionally, I pointed out that CRUD/FLS is one of the dirtiest secrets since not a shred of sample code/sample apps (except that one Testing CRUD and FLS page) illustrates secure coding practices in Apex. I followed up with Ryan after Dreamforce but never received a reply.
Lastly, Lightning Components don't address CRUD/FLS at all, even the standard ones (https://developer.salesforce.com/docs/atlas.en-us.lightning.meta/lightning/apex_crud_fls.htm). So, in this regard, the platform is moving backwards -- code is insecure by default, rather than the other way around.
Sad.
https://success.salesforce.com/ideaView?id=08730000000Lj8GAAS
Ryan Flood did reply to me and pointed out that there wasn't a formal idea on the Ideas site yet .. so I created one.
A contrivied but hopefully familliar example. Consider I want to pay Andy £100. So I call a service or controller saying "Pay Andy £100". The system checks that I have the money and am allowed to make the payment to Andy before updating our bank balances and recording the transaction. (All atomically of course).
I should not be able to access Andy's balance, although I can read my balance. Maybe Sharing would fix that. But at no point should I be able to write my bank balance. The system can do so on my behalf, but if I was given FLS Write access to that field then I could become very wealthy indeed.
So here is a situation where I'd expect to not see an FLS check for a DML operation and I'd expect nobody to have write access to the affected field. (I've seen a financial system in a previous job maintain a cryptographic signature on records to stop even the database admin changing data!)
A lower level example perhaps, code that maintains integrity constraints. We use a hash in a field on some tables with composite identities, in other words a situation where a combination of multiple values has to be unique. We can also use the hash to help query on unique combinations. The hash field is set to database unique and is maintained by trigger in the BEFORE stage. As it is enforced by trigger in the AFTER stage then granting FLS Write would be harmless, but why should it be needed? The system maintains that field, not the user.
try{
if(FancyFLSHelper.isUpdatable(Contact.SObjectType, List<String>{'LastName'})){
update c;
}
}catcht(CRUDFLSException e){
Page.addError(e.getMessage());
}
instead of
try{
update c;
}catcht(DMLException e){
Page.addError(e.getMessage());
}
Shouldn't SFDC always ensure that the current user is changing data that he has permissions?