Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4.1.4 - how should we relate to "deny by default"? #1085

Closed
jmanico opened this issue Oct 21, 2021 · 21 comments
Closed

4.1.4 - how should we relate to "deny by default"? #1085

jmanico opened this issue Oct 21, 2021 · 21 comments
Assignees
Labels
4b Major-rework These issues need to be part of a full chapter rework V4 Temporary label for grouping authorization related issues _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@jmanico
Copy link
Member

jmanico commented Oct 21, 2021

4.1.4 looks a little sloppy and could use some clarification.

@jmanico
Copy link
Member Author

jmanico commented Oct 22, 2021

Hey @cmlh thanks for posting the text. Do you agree this needs a little cleanup? Any suggestions?

@cmlh
Copy link
Contributor

cmlh commented Oct 23, 2021

@jmanico wrote

Hey @cmlh thanks for posting the text. Do you agree this needs a little cleanup? Any suggestions?

I'd recommend 4.1.4 be applicable to Level 3 (as defined under the 4.x TOV specification).

We should split and into two separate requirements i.e. "...minimal or no permissions and users/roles...".

Also a certifier could interpret "users/roles do not receive access to new features until access is explicitly assigned." as requiring to add the existing group to a newly created "new features" group even when the existing users are exactly the same and therefore cause debt within IAM?

@elarlang
Copy link
Collaborator

elarlang commented Oct 23, 2021

Never used this requirement in pen-tests, what it gives extra which is not covered by 4.1.3 for example?

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.

4.1.4 basically says, that user should not have permissions when just created? And should not have access till it's not given? For me it's just common sense to use 4.1.3.

Probably reason for different requirements comes from covering CWE's:

So, the question is - do we need to have separate requirement for "incorrect default permissions"

@elarlang elarlang added the 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet label Oct 23, 2021
@jmanico
Copy link
Member Author

jmanico commented Oct 23, 2021

I typically start news users with default permissions. I am ok with deleting this requirement. Are you ok with a delete here too @cmlh ? Is 4.1.3 emough?

@cmlh
Copy link
Contributor

cmlh commented Oct 23, 2021

Are you ok with a delete here too @cmlh ? Is 4.1.3 enough?

I am willing to commit to deletion 4.1.4 @jmanico provided we have addressed CWE-285 and CWE-276 in their entirety as raised by @elarlang, perhaps aligning the requirement text to reflect that of CWE-276 is the solution?

@elarlang
Copy link
Collaborator

@cmlh , I made quick stats for you, based on ASVS v4.0.2, we have:

  • requirements: 286
  • requirements with cwe set: 276
  • unique cwe values: 129

The point - not all CWE's are covered.

... the point and question here is, what problem or vector this requirement could solve which is not covered by other requirements? In this case, 4.1.3.

@jmanico
Copy link
Member Author

jmanico commented Oct 24, 2021

Did a lot of searching and found deny by default is mostly a network security concept and in the interest of so many new requirements entering ASVS I'm politely deleting this requirement

@jmanico jmanico closed this as completed Oct 24, 2021
elarlang pushed a commit to elarlang/ASVS that referenced this issue Oct 25, 2021
@tghosth
Copy link
Collaborator

tghosth commented Jan 15, 2023

I am reopening this as I think that it bears further discussion.

I actually think that 4.1.4 "deny by default" is a lot more actionable/testable than the "principle of least privilege" which to me is a lot more conceptual.

  • "deny by default" - says that a newly created user should not receive permissions until explicitly assigned them. Can verify this by looking at the user creation process.
  • "principle of least privilege" - says that at any point in time, a user should only have the permissions that they need. Verifying this is very subjective.

I would actually be inclined to drop 4.1.3 and restore 4.1.4 but I am open to suggestions.

See also #1352

@tghosth tghosth reopened this Jan 15, 2023
@elarlang elarlang added the V4 Temporary label for grouping authorization related issues label Jan 17, 2023
@tghosth
Copy link
Collaborator

tghosth commented Jul 10, 2023

I am not going to revert this but I think we do need to find a way of reflecting the deny by default concept in the updated version.

@tghosth tghosth added 4b Major-rework These issues need to be part of a full chapter rework _5.0 - prep This needs to be addressed to prepare 5.0 and removed 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet labels Jul 10, 2023
@tghosth tghosth changed the title 4.1.4 4.1.4 - how should we relate to "deny by default"? Jul 10, 2023
@elarlang elarlang assigned tghosth and unassigned jmanico and elarlang Jan 16, 2024
@elarlang elarlang added the 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos label Jan 16, 2024
@tghosth tghosth removed the 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos label Jan 24, 2024
@EnigmaRosa
Copy link
Contributor

@tghosth Looking over this, I'm inclined to agree with the deletion decision. That being said, because deny by default principle is a factor in access control, I think we would be remiss if we don't mention it. What if we did something along the lines of "Verify that all users are explicitly assigned permissions instead of being added to a generally-permissioned group by default".

@elarlang
Copy link
Collaborator

@EnigmaRosa - can you please clarify, what is the attack vector / problem / vulnerability the proposal addresses?

@EnigmaRosa
Copy link
Contributor

@elarlang - the only one I can think of is for highly sensitive (L3) applications, where users need to be explicitly permissioned, such as for medical records.

@elarlang
Copy link
Collaborator

Is it actionable? And is it different by concept from the requirements we have (4.1.3, 4.2.1 - related discussion #1934)?

@EnigmaRosa
Copy link
Contributor

Actionable? Yes - we state that users for L3 applications have to be explicitly granted permissions and not put in a default group. Is it worthwhile? Unsure. Different in concept? No

@elarlang
Copy link
Collaborator

With "actionable" - my question is, how do you verify that? What are the preconditions to do that?

@jmanico
Copy link
Member Author

jmanico commented May 28, 2024

This can all be verified by auditing the flow of access control assignment to users and objects. I like this direction.

Access control is one of the top priorities (per the OT10) and being business logic, it's not easy to flesh out this section. But since it's so important to security, I like these kinds of additional design-centric requirements for access control.

@tghosth
Copy link
Collaborator

tghosth commented Jun 2, 2024

I actually think that 4.1.4 "deny by default" is a lot more actionable/testable than the "principle of least privilege" which to me is a lot more conceptual.

  • "deny by default" - says that a newly created user should not receive permissions until explicitly assigned them. Can verify this by looking at the user creation process.
  • "principle of least privilege" - says that at any point in time, a user should only have the permissions that they need. Verifying this is very subjective.

I would actually be inclined to drop 4.1.3 and restore 4.1.4 but I am open to suggestions.

See also #1352

So having returned to this issue 1.5 years later, my thoughts are pretty much the same as my previous comment which I have quoted here.

Practically speaking, I am ok with some form of merge between 4.1.3 and 4.1.4 as I also noted here.

This then brings us to Elar's very valid questions:

what is the attack vector / problem / vulnerability the proposal addresses?

I would argue that this is wider than just medical applications but rather the concept here is that a user should not start off with more permissions than they actually need.

With "actionable" - my question is, how do you verify that? What are the preconditions to do that?

This is clearly a verifiable requirement although it would require either creating a new user and seeing what permissions they received or understanding the logic from the application developers, i.e. probably not just a pen test :)

I also agree with the direction of Jim's comment below:

Access control is one of the top priorities (per the OT10) and being business logic, it's not easy to flesh out this section. But since it's so important to security, I like these kinds of additional design-centric requirements for access control.

@jmanico
Copy link
Member Author

jmanico commented Jun 2, 2024 via email

@tghosth
Copy link
Collaborator

tghosth commented Jun 13, 2024

@EnigmaRosa should hopefully handle this as part of the V4 rework

@EnigmaRosa
Copy link
Contributor

Now that we're addressing some re-structuring, I'd like to return to this - I believe #2063 covers deny by default, and would like to propose closing this issue out in favor of continuing there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4b Major-rework These issues need to be part of a full chapter rework V4 Temporary label for grouping authorization related issues _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

No branches or pull requests

5 participants