1

We received a note from the security review team highlighting a CRUD/FLS vulnerability in our package and in the note it says there is a "Instances of SELECT vulnerability found across the application".

An example provided is shown below in a "with sharing" class:

myAttachments = [SELECT name, id, parentid, CreatedDate,CreatedById FROM Attachment WHERE parentid=:myAccount.id];

If the user does not have access to the object or the field, the result will be null. So the FLS must be enforced.

The documentation here does not specify the issue. How can we resolve this issue?

Improve this question
3
  • I would reply back asking what specific classes/lines have the vulnerability, then you'll at least know where you're wrong. It sounds like they are saying is your app makes a query for a field but you haven't given them access to the field thus the user would not have access to it and it would fail.
    – Dan Wooding
    Jul 14, 2016 at 17:41
  • I can't see the context, but perhaps the vulnerability is that (since APEX can run in god mode) you're not properly ensuring that the user has access to the account in question?
    – Sebastian Kessel
    Jul 14, 2016 at 17:55
  • What is god mode? The code is in a class with "with sharing" so how can it have access to an account if the user doesn't have access?
    – Jorjani
    Aug 19, 2016 at 18:45

2 Answers 2

Reset to default
2

That specific query would only be flagged running in a class either marked as "without sharing" or without any sharing specified. Basically, you're potentially leaking attachments that the user should not be able to see or interact with.

It should be noted that the scanner isn't "perfect." It believes you may be violating security based on heuristics. You'll want to actually review each instance that was flagged to determine if you're violating security best practices or not. For example, if myAccount comes from a class where sharing is enforced, then the user would be able to see the attachments, and this would be a false positive. I'm not sure if the scanner can detect situations like that correctly or not.

I actually just found this document that explains that Salesforce knows of both false positives and false negatives. From what I can tell from the wording, false negatives may be more frequent than false positives, but of course, there's no way to tell since false negatives are obviously not reported. A cursory glance over your files might not be a bad idea, but definitely check the files that were flagged just to be sure.

Improve this answer
4
  • It is in a with sharing class.
    – Jorjani
    Jul 14, 2016 at 18:00
  • @DavidJ You'll just want to examine each instance that was flagged. The scanner isn't perfect, so this example very likely is a false positive. However, you may have actual issues with some of the others, so it's vital to check them all.
    – sfdcfox
    Jul 14, 2016 at 18:03
  • Thanks. Another question is that is the list they provided exhaustive? We have hundreds of classes and only two are specified. They state that there might be more but seems like a waste of time to hunt for potential false positives . . .
    – Jorjani
    Jul 14, 2016 at 18:13
  • @DavidJ Found a document that I've linked to. Salesforce admits that the tool isn't perfect. I'd say that if the two that were flagged check out as false positives, the rest of your code is most likely okay.
    – sfdcfox
    Jul 14, 2016 at 18:21
2

What they're saying is you're not checking for permissions before you allow users to access or update objects using code like below. There are lots of examples on the page you referenced.

(!Schema.sObjectType.Account.fields.Name.isAccessible())

In the example you've cited, you're attempting to access Attachment on Account. Both of the objects need to be checked as above. Depending on what you do with them, you may need to check other CRUD permissions as well as explained in the document.

Improve this answer
3
  • I know for certain that if the user does not have access to the fields or the objects, the query returns null. So the check above seems redundant.
    – Jorjani
    Jul 14, 2016 at 18:02
  • They may also be concerned about SOQL injection if you're using text input. Since I've not seen your visualforce, I can't say. Remember that in your DE org, you're in the same namespace. Once installed, you won't be in the same namespace as the users. So permissions may not be quite as obvious. This is more than anything about best practice to prevent security breaches.
    – crmprogdev
    Jul 14, 2016 at 18:06
  • That makes sense. We do check for injections. In the example I shared above, we are using a standard object and id that comes from another query so it cannot be injection. That's why I'm puzzled.
    – Jorjani
    Jul 14, 2016 at 18:09

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .