ASVS icon indicating copy to clipboard operation
ASVS copied to clipboard

4.3.5 - Coverage by access control policies and deny by default otherwise

Open EnigmaRosa opened this issue 1 year ago • 16 comments

Note: This is referenced as 4.3.7 in #2033 but has updated numbering

This requirement addresses two parts: there should not be any objects that don't have their access undefined, but if there is, deny by default. Because this cannot exactly be penetration tested, it is L2 and L3.

# Description L1 L2 L3 CWE
4.3.5 [ADDED] Verify that every object is addressed by at least one access control policy, and when an object does not have an access control policy all access to that object is rejected. 280

EnigmaRosa avatar Sep 04 '24 22:09 EnigmaRosa

Comment from Elar (https://github.com/OWASP/ASVS/issues/2033#issuecomment-2323964821):

4.3.7 - not sure about this one. From implementation perspective it maybe makes sense, from pen-testing perspective is just finding a technical reason for 4.1.3 or 4.2.1 requirement.

Comment from Josh (https://github.com/OWASP/ASVS/issues/2033#issuecomment-2324157184):

  • I think this is a little too specific, I also think that practically speaking some data objects will be publicly accessible.
  • I would rather see this requirement be a little less specific and lean more into the deny by default concept, which we discussed in 4.1.4 - how should we relate to "deny by default"? #1085.

elarlang avatar Sep 05 '24 05:09 elarlang

I think this is an excellent "secure by default" requirement. Access control is very very hard to test comprehensively and I'd like to have some leeway here to steer developers in the right direction since it's essentially business logic.

jmanico avatar Sep 05 '24 11:09 jmanico

An object being publicly accessible doesn't mean that it lacks an access control policy - in fact, the access control policy should be "allow access independent of attribute" if we want to force deny by default.

EnigmaRosa avatar Sep 09 '24 00:09 EnigmaRosa

@tghosth @elarlang following up on this

EnigmaRosa avatar Sep 29 '24 21:09 EnigmaRosa

Thanks for the ping @EnigmaRosa,

I think it would be good to reword the proposed requirement around deny by default.

Do you think it could be made into a merge of 4.1.3 and the original 4.1.4 as it seems to be related.

tghosth avatar Oct 08 '24 06:10 tghosth

How about: "Verify that every object is addressed by at least one access control policy, and when an object lacks an access control policy, access to it is automatically denied by default."

Do you think it could be made into a merge of 4.1.3 and the original 4.1.4 as it seems related

I don't know if I would want to combine them - it is probably in best interest to have deny by default and principle of least privilege as unique verification requirements

EnigmaRosa avatar Oct 14 '24 23:10 EnigmaRosa

I tend to agree with @EnigmaRosa. Since access control is purely business logic, very different in every app, and is the top vulnerability category statistically (per the OWASP Top Ten stats call and more), I'd like to give more leeway to this category. It's really hard to come up with good requirements that actually help developers here.

jmanico avatar Oct 15 '24 12:10 jmanico

I'd probably also want to move the sub-section this is under

EnigmaRosa avatar Oct 19 '24 12:10 EnigmaRosa

Minor tweak but otherwise sounds good :)

"Verify that every object is addressed by at least one access control policy. When an object lacks an access control policy, access to it should be automatically denied by default."

Where do you want to put this requirement @EnigmaRosa ?

tghosth avatar Oct 22 '24 07:10 tghosth

From implementation perspective it maybe makes sense, from pen-testing perspective is just finding a technical reason for 4.1.3 or 4.2.1 requirement.

So how this proposed requirement is not duplicate of 4.1.3 and 4.2.1?

elarlang avatar Oct 22 '24 08:10 elarlang

So I think 4.1.3 is looking at how permissions are assigned to users and the proposed requirement is looking at how access controls are assigned to resources. I think these are both separate and valid concerns from an implementation perspective, even if from the testing perspective they seem the same.

I wonder whether 4.2.1 should be merged into 4.1.3 though?

@EnigmaRosa @jmanico what do you think?

tghosth avatar Oct 22 '24 10:10 tghosth

I wonder whether 4.2.1 should be merged into 4.1.3 though?

I prefer not, since IDOR is a really specific issue that I'd like to see directly addressed (4.2.1) and 4.1.3 is more of a general principle of overall access control design.

jmanico avatar Oct 23 '24 09:10 jmanico

I wonder whether 4.2.1 should be merged into 4.1.3 though?

I prefer not, since IDOR is a really specific issue that I'd like to see directly addressed (4.2.1) and 4.1.3 is more of a general principle of overall access control design.

Reproducing 4.1.3 here:

# Description L1 L2 L3 CWE
4.1.3 Verify that the principle of least privilege exists - users should only be able to access functions, data files, URLs, controllers, services, and other resources, for which they possess specific authorization. This implies protection against spoofing and elevation of privilege. 285

I am not raising a separate issue for this as I think it is very relevant to the originally proposed requirement.

So I get that 4.1.3 it is a principle, I just don't think that it is currently very implementable or testable. On the other hand, I think the requirement above is implementable and testable and so is the IDOR requirement 4.2.1.

@jmanico if you think that we need 4.1.3 on top of the new requirement and 4.2.1, can you think how to make it more actionable?

tghosth avatar Oct 23 '24 10:10 tghosth

The way that we test for function level access control (does a user have access to a certain feature) and the way we test for IDOR (does a user have access to certain pieces of data) is rather different and uses different tool types. I really want to keep these separate for both testers and developers.

So I suggest that 4.1.3 should go away and instead break it down into the right pieces.

jmanico avatar Oct 23 '24 10:10 jmanico

Probably worth a separate issue, but as the discussion is here at the moment...

edit: I made separate issue for that #2196

For me the 4.1.3 is source for confusions for so many reasons. I think the reason is, that the first part of the requirement is abstract principle and other really-really-widespread test-case that covers basically everything.

So first move should be maybe to split it up and move the 2nd part away to V4.2.

Then we can come back to the question - what those principles like "least privileges" and "deny by default" gives us - how is it possible to test them etc.

elarlang avatar Oct 23 '24 10:10 elarlang

I agree with you Elar on the POLP principle, it covers everything but it's not so actionable.

jmanico avatar Oct 23 '24 10:10 jmanico

If we want to keep it, do we have a proposal for alternative wording?

tghosth avatar Oct 24 '24 19:10 tghosth

Alternatively, and I know the idea is to minimize the amount of text we have at the beginning of each chapter, should we perhaps include a brief explanation of the principles at the start of V4 and omit 4.1.3?

EnigmaRosa avatar Oct 28 '24 00:10 EnigmaRosa

Ok, I think the 4.1.3 discussion has now moved to #2196

In the meantime, I think the most recent proposal for his requirement is here: https://github.com/OWASP/ASVS/issues/2063#issuecomment-2428538405

I think this proposal is not just a principle but actually actionable from an implementation perspective and should also be testable as well although it may be slightly more tricky.

I think that this is different enough to 4.1.4 to consider it a separate requirement.

@elarlang do you think we can open a PR or do you want to wait for #2196 to be resolved?

tghosth avatar Oct 28 '24 09:10 tghosth

Alternatively, and I know the idea is to minimize the amount of text we have at the beginning of each chapter, should we perhaps include a brief explanation of the principles at the start of V4 and omit 4.1.3?

So you propose #2195 + #2196 ? :)

@elarlang do you think we can open a PR or do you want to wait for https://github.com/OWASP/ASVS/issues/2196 to be resolved?

~~I can not see PR material here. I think this requirement comes from a "problem" "we have too few requirements in V4".~~

Let's wait the clarification from previously mentioned issues. As it is too slow ping-pong in issue comments, I hope to solve it during the summit.

elarlang avatar Oct 28 '24 10:10 elarlang

This is already covered by 4.1.3 and the upcoming 4.1.7 and 4.1.8 so I'm politely closing this out to reduce the v4 noise.

jmanico avatar Nov 06 '24 10:11 jmanico