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

1.4.5 #1183

Open
jmanico opened this issue Feb 2, 2022 · 30 comments
Open

1.4.5 #1183

jmanico opened this issue Feb 2, 2022 · 30 comments
Assignees
Labels
4b Major-rework These issues need to be part of a full chapter rework owasp_class_hel V1 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 Feb 2, 2022

Need to explain the term "attribute or feature-based access control" in this section header or summary.

@elarlang
Copy link
Collaborator

elarlang commented Feb 2, 2022

Note: I plan to move this requirement to V4.*.

@tghosth
Copy link
Collaborator

tghosth commented Jun 22, 2022

This seems like a classic architecture level control where the conceptual basis is included in 1.4.x but the practical requirements are included in 4.x. Do you disagree @elarlang ?

@tghosth tghosth added 2) Awaiting response Awaiting a response from the original poster _5.0 - prep This needs to be addressed to prepare 5.0 labels Jun 22, 2022
@elarlang
Copy link
Collaborator

V1.4 Access Control Architecture

# Description L1 L2 L3 CWE
1.4.5 Verify that attribute or feature-based access control is used whereby the code checks the user's authorization for a feature/data item rather than just their role. Permissions should still be allocated using roles. (C7) 275

Current V4.1 General Access Control Design is missing requirement for checking attribute level permissions. Also related to issue #934

My point of views:

  • V1 should contain at the end only documentation or business logical decision as requirement. If you need to check something from program code, it should be in some other more specific category
  • we need to have separate requirement for checking attribute level permissions in V4.1
  • if we want to use current V1.4.5, we can use it as (more clear) architecture requirement, and also in V4.1

@tghosth
Copy link
Collaborator

tghosth commented Jun 23, 2022

I think that 1.4.5 is not something you explicitly need to check in the code but rather describing a specific AuthZ paradigm so I think it is fine.

What do you think of:

1.4.5

Verify that the access control mechanism checks a user's authorization for a specific application feature or data item based on a specific permission attribute rather than just based on the user's role.

1.4.x

Verify that the access control mechanism allocates permissions to users via roles rather than allocating the permission to the user directly.

@tghosth
Copy link
Collaborator

tghosth commented Jun 23, 2022

I agree there should also be a specific requirement in 4.x

@elarlang
Copy link
Collaborator

elarlang commented Jul 5, 2022

What are the security risks in an application if it does not meet those requirements?

@tghosth
Copy link
Collaborator

tghosth commented Jul 26, 2022

1.4.5, checking access control based on roles rather than permission attributes makes it more difficult to remove permissions from a role as the access control checking code is hardcoded to the role.

1.4.x risks having a huge mess of permission attributes added to users ad-hoc making it much harder to manage.

@pglizniewicz
Copy link

I think that 1.4.5 is not something you explicitly need to check in the code but rather describing a specific AuthZ paradigm so I think it is fine.

What do you think of:

1.4.5

Verify that the access control mechanism checks a user's authorization for a specific application feature or data item based on a specific permission attribute rather than just based on the user's role.

1.4.x

Verify that the access control mechanism allocates permissions to users via roles rather than allocating the permission to the user directly.

This is way better, than what is currently in the standard. The current wording is misleading, because it suggests, that only using ABAC or feature based access control (which is vague) will make the application compliant, when RBAC will work as well and is compliant with what you have written.

@tghosth
Copy link
Collaborator

tghosth commented Aug 7, 2022

@elarlang any further comments?

@tghosth tghosth added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet josh/elar and removed 2) Awaiting response Awaiting a response from the original poster labels Dec 7, 2022
@elarlang elarlang added the V4 Temporary label for grouping authorization related issues label Jan 17, 2023
@tghosth tghosth added V1 4b Major-rework These issues need to be part of a full chapter rework and removed 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet josh/elar labels Jul 10, 2023
@EnigmaRosa
Copy link
Contributor

Agreed on the importance of the 2 proposed requirements, especially as they clarify a bit of a mess presented by 1.4.5. Given the V1 overhaul, what is the sentiment towards bringing these to 4.1?

@elarlang
Copy link
Collaborator

@tghosth :

@elarlang any further comments?

... it has been a while ...

Verify that the access control mechanism allocates permissions to users via roles rather than allocating the permission to the user directly.

I'm not really sure about it. Is it a clear security requirement, or is it code beauty? I can understand, that there is maintainability involved, but - if the application does not have authorization vulnerability is it in the scope of ASVS to say "yes, you don't have vulnerability but what you did, is wrong?". I think this "requirement" belongs to a cheat sheet or best current practices, but by principle, it is not a security requirement.

@EnigmaRosa :

what is the sentiment towards bringing these to 4.1?

By current definitions or structure - if you check it from the documentation, it belongs to V1. If you check it from implementation, it belongs to V4.

@EnigmaRosa
Copy link
Contributor

Is it a clear security requirement, or is it code beauty? I can understand, that there is maintainability involved,

@elarlang If you assign permissions directly to a user, I believe the fear is permission creep (the user amassing permissions over time as they shift roles in a system/company). By having permissions assigned to roles, and then the role assigned to a user, you can much more clearly see if users have excessive privileges, and it makes permission management much easier.

@elarlang
Copy link
Collaborator

Yeah, I can understand that and it is pretty much what I said. For example, if you have an ugly "spaghetti code", but it does not have any security hole to report, is it in the scope and responsibility of ASVS to say "Hey, no security holes, but, it's ugly"?

@jmanico
Copy link
Member Author

jmanico commented May 28, 2024 via email

@tghosth
Copy link
Collaborator

tghosth commented Jun 2, 2024

So this is a tricky one and I think it becomes a question of how prescriptive we want to be.

On the one hand, we have previously said that we want to cut down the number of requirements and focus more specifically on things without which the application is less secure.

On the other hand, in my opinion, this point is more than just "code beauty" as it is talking about a paradigm to prevent shooting yourself in the foot through permissions creep. At the very least, it would be considered a strong recommendation but I think that in reality it would be hard to implement a correctly operating authorization mechanism without applying these principles.

@EnigmaRosa, I think you are correct that these would need to move to V4 but how crucial do you think they are?

@EnigmaRosa
Copy link
Contributor

but I think that in reality it would be hard to implement a correctly operating authorization mechanism without applying these principles.

If we intend for ASVS to be used as a guide to building secure applications, we'd be remiss in leaving it out.

@elarlang
Copy link
Collaborator

If we intend for ASVS to be used as a guide to building secure applications, we'd be remiss in leaving it out.

The scope for ASVS is to provide true-of-false security verification requirements. Guideline sounds more like a cheat sheet area.

@EnigmaRosa
Copy link
Contributor

Isn't the purpose of the ASVS both for true-or-false security verification requirements and secure application creation? I'd say these requirements both have a place.

@elarlang
Copy link
Collaborator

I have nothing to add to my already written comment: #1183 (comment)

We have more those code-beauty issues to discuss, it's a bit wider ASVS scoping question.

@elarlang elarlang added the next meeting Filter for leaders label Oct 15, 2024
@jmanico
Copy link
Member Author

jmanico commented Oct 15, 2024

I agree with @EnigmaRosa here. This is not purely true/false verification list. The ASVS documentation has stated for years that the ASVS is also for secure development. I would ask that especially for access control and business logic you give us some leeway to go beyond the true/false mindset.

@elarlang
Copy link
Collaborator

Some reading for Jim: #1434 (comment)

@jmanico
Copy link
Member Author

jmanico commented Oct 15, 2024

I still agree with my earlier 2023 comment when it comes more technical vuln's like SQL Injection. The guidance for those issues is very clear. But I think we need more leeway into design for issues like access control and business logic as stated above since it's a lot more difficult to provide guidance for the more business logic centric items.

@elarlang
Copy link
Collaborator

We just re-discussed it today with Josh, hopefully he adds his own summary and understanding.

We can watch it as a recommendation how to build things, but not as a security verification requirement.

@tghosth
Copy link
Collaborator

tghosth commented Oct 15, 2024

I had another look at this and thought more about it.

I agree that ASVS is intended for both true-or-false security verification and secure application creation.

With this in mind, I still don't think we should be including them as requirements.

I have a few reasons which combine to lead me to this decision:

  • I think we need to bias for fewer requirements wherever possible.
  • Think this is a problematic verification item because I don't think that failing this requirement automatically makes your app insecure, even if it does make it more risky.
  • This would be a non-trivial requirement to fix in an established system. (I don't think that this is a reason on it's own not to have a requirement but I think it is a factor)
  • In the last few months at least we are generally trying to make requirements more outcomes based and less prescriptive. This
    feels a little too prescriptive.
  • This would be very difficult to comprehensively verify. (I don't think that this is a reason on it's own not to have a requirement but I think it is a factor)

However, because of the value these items provide in building a secure application, I believe they should definitely be in the recommendations section.

@jmanico @EnigmaRosa any further thoughts?

@tghosth tghosth removed the next meeting Filter for leaders label Oct 22, 2024
@jmanico
Copy link
Member Author

jmanico commented Oct 23, 2024

Yup. This is a really opinionated design principle. I am ok with it going away.

@elarlang
Copy link
Collaborator

Is there something to put into the recommendations or should we just delete it?

@jmanico
Copy link
Member Author

jmanico commented Oct 25, 2024

I do like it as a recommendation. It's a good design principle that leads to better access control for complex software.

@elarlang
Copy link
Collaborator

Should we have a separate section in V4 paragraph for "Principles and recommendations" for me, many existing and proposed requirements are not security requirements or are not verifiable requirements, which makes them to be out of scope.

@jmanico
Copy link
Member Author

jmanico commented Oct 25, 2024 via email

@elarlang
Copy link
Collaborator

This requirement gets removed anyway and let's leave this issue only for that.

For V4 scope and principles I opened a separate issue #2195

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 owasp_class_hel V1 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