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

Padding out chapter 4 (access control) #2033

Closed
EnigmaRosa opened this issue Aug 29, 2024 · 21 comments
Closed

Padding out chapter 4 (access control) #2033

EnigmaRosa opened this issue Aug 29, 2024 · 21 comments
Assignees
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 4) proposal for review Issue contains clear proposal for add/change something V4 Temporary label for grouping authorization related issues _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@EnigmaRosa
Copy link
Contributor

At the moment, chapter 4 is very bare-bones. @jmanico and I went over NIST's ABAC standard and pulled verification requirements from there that we think are necessary, a total of 13 new verification requirements.

V4.1 General Access Control Design

# Description L1 L2 L3 CWE
4.1.6 Verify documentation and definition of all user, resource, action, and environmental attributes and access control policies. 284
4.1.7 Verify the adherence of the access control mechanism to the design documentation via regular review and audit. 284
4.1.8 Verify access control policies enforce separation of duties as a means to prevent conflict of interest cases and fraud. 654
4.1.9 Verify that access control policy attributes are transmitted and stored securely to maintain their integrity and confidentiality. 922
4.1.10 Verify that the access control system makes real-time access control decisions based on current attribute values.

V4.2 Operation Level Access Control

# Description L1 L2 L3 CWE
4.2.3 Verify that multi-tenant applications use cross-tenant controls to prevent privileges from crossing over between tenants. 284
4.2.4 Verify that access to an object is based on the originating sybject's (e.g. user's) permissions, not on the permissions of any intermediary or service acting on their behalf. 441
4.2.5 Verify that access decisions are only made once the subject has been authenticated. 306

V4.3 Other Access Control Considerations

# Description L1 L2 L3 CWE
4.3.4 Verify that sensitive information cannot be inferred from user, resource, action, or environment attributes or their aggregation. 1230
4.3.5 Verify that environmental and contextual attributes such as time of day and location are incorporated into access control decisions. 1220
4.3.6 Verify that the application has appropriate user and/or administrator interfaces to securely manage access control attributes and policies. 282
4.3.7 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
4.3.8 Verify that access control decisions are securely logged. 778
@elarlang elarlang added the V4 Temporary label for grouping authorization related issues label Aug 30, 2024
@elarlang
Copy link
Collaborator

elarlang commented Aug 30, 2024

Thank you, @EnigmaRosa !

Many requirements need more explanation (at least for me) - please provide information, why those (proposals) exist and what precise security problem it solve. We do it for every requirement issue-by-issue to make changes traceable for later analysis and questions. If we don't have this information to provide, we need to re-invent everything again in the future.

4.1.6 - It has been missing a requirement so far, as it is a documentation requirement. According to current logic, it belongs to section 1.4.

4.1.7 - any regular review seems some process that is out of scope or I just can not understand the requirement

4.1.8 - needs explanation

4.1.9 - personally I don't think we need a requirement for "check authorization for authorization information", we have had similar discussions over 4.1.2 (#1524)

4.1.10 - is it for checking that if you make changes to the authorization setup/user privileges etc, then it has immediate effect? In a way related to 3.8.3 and 3.8.5


4.2.3 - needs explanation, how it is different from or what it gives extra for 4.1.3 or 4.2.1?

4.2.4 - needs explanation

4.2.5 - A non-authenticated user has also permissions (at least seeing a login page and making authentication requests) and needs to make authorization decisions. For me, it just duplicates 4.1.3 + 4.2.1


4.3.4 - from an abstract point of view, it is still accessing the data that you should not access? Although there is logic, that if you collect the data piece-by-piece from other objects to achieve the same goal. Not sure, should it belong to cheat sheet content?

4.3.5 - it must rely on business decisions and can not be asked from every application as a general requirement

4.3.6 - I don't think that is in the scope, it is about the internal process. "securely manage" is covered by general access control requirements, again 4.1.3+4.2.1

4.3.7 - needs explanation

4.3.8 - duplicate of 7.2.2

@jmanico
Copy link
Member

jmanico commented Aug 30, 2024 via email

@EnigmaRosa
Copy link
Contributor Author

Thanks for the feedback, @elarlang. Please see my reasoning below:

4.1.6 - There should be documentation for the access control system. I agree that as it stands, it belongs in the documentation section; is that currently being re-worked? I figured I could get ahead of that by adding here, but I can add it to section 1.4 instead for the meantime.

4.1.7 - This is essentially making sure that the access control system in place matches the one outlined in the documentation. We can remove the bit about regular review and audit if you believe that is out of scope. I do believe it is important though to have a requirement ensuring that the design documentation matches the implementation, especially for access control attributes and policies.

4.1.8 - Where necessary (and only L2 and L3 cases this should be an issue), sensitive actions or access to sensitive data should be defined by an access control policy. I believe this was something @jmanico had proposed in our work together.

4.1.9 - This is ensuring that the attributes that define a subject such as group or permission information are protected in transit and at rest (i.e. sent over HTTPS). I do not believe this is tied to the issue you linked.

4.1.10 - If changes are made to the access control attributes or policies, they should take effect immediately. I do not believe this is linked to 3.8.3 or 3.8.5, as those are linked to session termination.

4.2.3 - Ensuring that there is not only access control within a given organization, but also across tenants. Yes, this is addressed to some extent with IDOR, but I believe that it is vital to specifically call out multi-tenant applications.

4.2.4 - This is specific to application infrastructures where a component may be the subject trying to access an object, but it is doing so on behalf of some other subject (like a user) which has its own permissions. An example is where an application makes an API call to execute some function. When access is checked for whatever function, it is not the user's permissions that are checked, but rather those of the service. This is proposed as an L3 requirement because for most applications it is sufficient for authorization checks to occur somewhere, but this could act as defense in depth for the most sensitive applications.

4.2.5 - I understand your view about access decisions being made to allow non-authenticated users to view some data and complete some actions (such as logging in), but this is directly called for in the ABAC standard.

4.3.4 - This is combination of both aggregation security concerns. but also privacy. Comes from ABAC standard.

4.3.5 - I add additional attributes only for L3 - the access decisions must be extra sensitive and consider beyond basic business purposes.

4.3.6 - Applications should have some way of assigning permissions - I don't think this is an "internal process" thing.

4.3.7 - This addresses two needs: there should not be any objects that don't have their access undefined, but if there is, deny by default. The first part in particular originated from the ABAC standard.

4.3.8 - Agreed

@elarlang
Copy link
Collaborator

elarlang commented Sep 1, 2024

If somewhere else is written some security requirement, it is a good starting point to investigate, why it is there and what security problem it solves. Just the fact that something is written in some other standard is not enough to just copy it to ASVS without being able to explain, why the security requirement exists. Additionally, is it more a recommendation or is it an actual fail/pass requirement?

For every proposed requirement it should be clearly answered, what it gives extra to existing ones (e.g. 4.1.3 and 4.2.1) - what security problem we have in the application, if we don't have this requirement in ASVS.

As I wrote in my previous comment - we need this information to be clearly written somewhere, otherwise we have requirement by requirement new issues opened as "this is duplicate to this one" and "why this exists".

I would like to get feedback from @tghosth as well.

@EnigmaRosa
Copy link
Contributor Author

Sorry Elar, hopefully this is closer to what you were expecting.


4.1.6 - This should really be 1.4.7 - the existing access control architecture requirements to not address the documentation of the attributes and access control policies that define them. Especially given that access control is integral to maintaining all part of the CIA triad, keeping track of the complex system defining it should be a given.

4.1.7 - This is essentially making sure that the access control system in place matches the one outlined in the documentation. We can remove the bit about regular review and audit if you believe that is out of scope. I do believe it is important though to have a requirement ensuring that the design documentation matches the implementation, especially for access control attributes and policies. This can become 1.4.8 instead of being added to chapter 4.

4.1.8 - This can be removed, I believe it may be a duplicate of 1.11.4:

# Description L1 L2 L3 CWE
1.11.4 [ADDED] Verify that expectations for business logic limits and validations are clearly documented including both per-user and also globally across the application.

4.1.9 - This is ensuring that the attributes that define a subject such as group or permission information are protected in transit and at rest (i.e. sent over HTTPS). I do not believe this is tied to the issue you linked. In my opinion (and would appreciate others, as I don't 100% know how much of a concern this is) - the security issue presented here could be when user role (such as "Admin") is included in an unencrypted JWT.

4.1.10 - If changes are made to the access control attributes or policies, they should take effect immediately. I do not believe this is linked to 3.8.3 or 3.8.5, as those are linked to session termination, in particular users having the ability to view and terminate sessions. This addresses a scenario where a user's access permissions are modified while that user has an active session let's say (i.e. admin revokes access to edit files) - if the system does not check the user's permissions in real time (i.e. instead relying on cached access information), the user would be able to edit a file, which they no longer should be able to do.

4.2.3 - Ensuring that there is not only access control within a given organization, but also across tenants. Yes, this is addressed to some extent with IDOR, but I believe that it is vital to specifically call out multi-tenant applications. This is very easily testable, and I do not believe multi-tenant access control is addressed elsewhere in the standard.

4.2.4 - This is specific to application infrastructures where a component may be the subject trying to access an object, but it is doing so on behalf of some other subject (like a user) which has its own permissions. An example is where an application makes an API call to execute some function. When access is checked for whatever function, it is not the user's permissions that are checked, but rather those of the service. This is proposed as an L3 requirement because for most applications it is sufficient for authorization checks to occur somewhere, but this could act as defense in depth for the most sensitive applications. This is different from 1.4.6, which calls for least necessary privileges for components - here we are requiring that it is not the component privileges that define access, but the originating user's.

4.2.5 - I understand your view about access decisions being made to allow non-authenticated users to view some data and complete some actions (such as logging in), but this becomes an issue when we get to specific privileged access (i.e. something that not all users, including semi-anonymous users, can do). To some extent, subject authorization attributes are conveyed in the authentication credentials, or at least insofar as they identify the subject.

4.3.4 - This is combination of both aggregation security concerns. but also privacy. Comes from ABAC standard (3.2.16 Attribute Privacy Considerations), specifically that it may

increase the risk of privacy violation of personally identifiable information (PII) to to inadvertent exposure of attribute data

Especially in an age where applications are tracking everything about their users (and then selling the data), we would be remiss in not including this. If there are concerns about it being a true security issue, we can discuss including this in the cheat sheet.

4.3.5 - I add additional attributes only for L3 - the access decisions must be extra sensitive and consider beyond basic business purposes. For example, a bank employee accessing a web application that allows them to transfer customer funds is normal from 9 am - 5 pm on a work day. But unless they have an unusual shift, that same employee accessing the app in the middle of the night should not be allowed.

4.3.6 - Applications should have some way of assigning permissions - I don't think this is an "internal process" thing.

4.3.7 - This addresses two needs: there should not be any objects that don't have their access undefined, but if there is, deny by default. The first part in particular originated from the ABAC standard, but essentially, access control doesn't work if objects don't have access rules. And IF there is a case where those access rules don't exist, access should not be granted to the item, as that threatens the confidentiality and integrity of the object.

4.3.8 - Agreed - this is redundant to security logging and can be removed.

@elarlang
Copy link
Collaborator

elarlang commented Sep 2, 2024

Thank you for explanation. I make second feedback round, need to admit it did not change too much from the 1st round.

4.1.6 - waiting feedback from others


4.1.7 - we (will) have (now) documentation requirement and the rest of the requirements in 4.* are implementation requirements. I don't think we need another requirement, just to say that we need to check implementation against documentation. This is what ASVS is about.

At the moment, I don't see any new aspect here.


4.1.8 - covered by 1.11.4


4.1.9 - the linked issue is to point out, that it is like "world in the world" where authorization functionality is proposed to have separate and duplicate set of requirements for just that piece of software, although it is abstractly covered elsewhere. For example, we don't need to say separately, that authorization information need to be transferred over HTTPS, as we have requirement that says "everything need to go over HTTPS". The same is when including sensitive data in the JWT (and in the related issue you personally said, it is common sense: #1919).

At the moment, I don't see any new aspect here.


4.1.10 Verify that the access control system makes real-time access control decisions based on current attribute values.

First, from feedback "If changes are made to the access control attributes or policies, they should take effect immediately.". I think "current attribute values" are a bit different thing than "policy".

Pointing to 3.8.3 is probably mistake/typo from my side, it should be 3.8.5 and 3.8.6 for related to session termination.

Linked session handling issues are related as for example, when the application uses tokens, then the usage of tokens loses its point for being "stateless", if it needs to read the up-to-date information from somewhere. If the permissions for the users are changed, the update must be done via token revocation. But this all is covered by current:

3.5.7 Verify that all active stateless tokens, which are being relied upon for access control decisions, are revoked when admins change the entitlements or roles of the user.

If to read the requirement clearly, it is limited to "decisions based on current attribute values", then attribute values most likely are not part of authorization ruleset.

At the moment I think it needs more clarification and at least wording improvement, what is the precise event ("access control decisions based on current attribute values" (data vs policy)) that is referenced in the requirement.


4.2.3 - if would like to see some example situation. Waiting feedback from others.


4.2.4 - at the end, if user can see the data or access functionality he/she has no permission, it is authorization problem on the application side. The description is just for verifying a potential cause for the problem.

For me it seems testing-guide or cheat sheet material. From pen-testing perspective it does not give anything extra for 4.1.3+4.2.1.

Waiting feedback from others.


4.2.5 - I really don't understand the point for the requirement. If there is special privileged access or some application that requires user to be authenticated, then it is business logic / authorization ruleset for that application and for that functionality. I don't think it is material for general requirement for every application and for every situation.

It may cause more confusion than it solves - it may give message, that (just) checking "is user authenticated" is already good enough. Based on pen-test practice, it is a widespread mistake.

At the moment, I don't see any new aspect or clear problem to solve here.


4.3.4 - tracking user data is more collecting sensitive data issue (V8), selling it is also sensitive data issue, but clearly out of ASVS scope.

It is not clear for me what problem it should solve and can it be in practice testable.


4.3.5 - I stand for my previous comment, we can not require it as general requirement. It is application specific. Also, if we look towards Zero Trust framework, it is going against it (checking Location). It can be material for recommendation.


4.3.6 - I stand for my previous comment. It is also vague - what means "appropriate", what means "securely manage"? Not testable.


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.


Those were just my opinions, ping @tghosth .

@tghosth
Copy link
Collaborator

tghosth commented Sep 2, 2024

First. thanks to @EnigmaRosa and @jmanico for the hard work on the new additions and thanks to @elarlang for the comprehensive reviews.

A few general comments:

  • With respect to NIST and the good work that they do, I find that their documentation contains a lot of jargon to an extent which makes their documentation very hard to understand. I think we can be inspired by NIST but we have to ensure that a developer with a reasonable level of security knowledge can understand our requirements.
  • Whilst I think this is certainly an area where more requirements are needed, in general, we only want to add more requirements where we are convinced of their impact.

More detailed comments:

  • 4.1.6/1.4.7:
    • I agree that this should be documentation requirement in 1.4.
    • I also think this is an important specific requirement because of the importance and complexity of access control.
    • However, I think the wording needs to be simpler/clearer.
  • 4.1.7:
    • I think that this belongs in V4 and should be simplified to something like "verify that access control is implemented as per the documentation".
    • From my perspective, we need to have implementation requirements if we have documentation requirements.
    • I agree that a review process is out of scope.
  • 4.1.8:
  • 4.1.9:
    • I must admit that my mind also jumped to Don't see the point of 4.1.2 #1524
    • The reason is that it sounds like we have a specific requirement which duplicates a more general requirement.
    • We already say that all data should be encrypted in transit, why would we need to say so again here?
    • We already define in 1.8.2 that data should be stored based on its sensitivity, why do we need to say so again here?
  • 4.1.10:
    • We definitely should have this requirement in some form as it is important.
    • I agree that this is separate to 3.8.5 and 3.8.6...
    • ...but it definitely needs to be deduplicated against 3.5.7.
    • Would the phrase "current permission values" be clearer?
  • 4.2.3:
    • I agree that multi-tenanted access control should be called out explicitly.
    • What do you think about: "Verify that multi-tenant applications use cross-tenant controls to ensure user operations will never affect tenants which they do not have permissions to interact with."
  • 4.2.4:
    • I agree that this is not really pentestable and I think that it therefore makes sense as an L3 control.
    • Anecdotally, this is certainly an issue in real apps and ignoring this consideration in design makes it very hard to correctly enforce authorization.
  • 4.2.5:
    • I understand the concept and agree with this is a design principle.
    • However, I am not convinced it has sufficient impact on top of other requirements in order to merit its own actual requirement.
  • 4.3.4:
    • I understand the concept although it does feel like a data protection topic.
    • I also feel like I don't know how I would test this or help someone implement this... The complexity makes me think that maybe it is more suited to a cheat sheet.
    • Even after the two points above are resolved, I agree that it would be L3 in ASVS but I am not convinced it is a good fit for ASVS.
  • 4.3.5:
    • So I actually think this is an important requirement for L3.
    • I think there would be a question as to whether this is an AuthN or an AuthZ question but I think maybe it fits better here.
  • 4.3.6:
    • I kinda get the concept but I am not sure about the practicality as a requirement.
    • What are examples of negative cases where the app is not complying with this requirement, but still operates as a functional application?
  • 4.3.7:
    • 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.
  • 4.3.8:
    • Agree this is redundant.

@tghosth tghosth added 4) proposal for review Issue contains clear proposal for add/change something _5.0 - prep This needs to be addressed to prepare 5.0 labels Sep 2, 2024
@elarlang
Copy link
Collaborator

elarlang commented Sep 3, 2024

My attempt to summarize the situation and prepare for the next step.

  • Clear moving forward: 4.1.6
  • Skip recommendation from Elar and Josh: 4.1.8, 4.1.9, 4.2.4, 4.2.5, 4.3.4, 4.3.6, 4.3.8
  • Needs further discussion: 4.1.7 (x), 4.1.10, 4.2.3, 4.3.5 (x), 4.3.7
    • x - would like to get more feedback from Josh, as I find those requirements not reasonable

For moving forward from here:
@EnigmaRosa @jmanico - do you agree with the feedback and point-of-views or do you have clear arguments and views, as to why some of those requirements for sure must be presented in the ASVS?


Questions to @tghosth

4.1.7 - feedback from Josh:

I think that this belongs in V4 and should be simplified to something like "verify that access control is implemented as per the documentation".

If we have this proposed requirement, we can delete all other requirements. What this proposal should give extra to existing requirements?

4.3.5 - feedback from Josh:

So I actually think this is an important requirement for L3.

As this is clearly business/application specific, we can not require it as a fail/pass requirement always and from everyone. It can be pointed out as an in-depth defense technique via chapter text, recommendation, or documentation requirements.

@elarlang elarlang added the 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet label Sep 3, 2024
@tghosth
Copy link
Collaborator

tghosth commented Sep 3, 2024

4.1.7:

I think that this belongs in V4 and should be simplified to something like "verify that access control is implemented as per the documentation".

If we have this proposed requirement, we can delete all other requirements. What this proposal should give extra to existing requirements?

Ok I take your point @elarlang , do we need to update other requirements to match them to the documentation requirement?

4.3.5:

So I actually think this is an important requirement for L3.

As this is clearly business/application specific, we can not require it as a fail/pass requirement always and from everyone. It can be pointed out as an in-depth defense technique via chapter text, recommendation, or documentation requirements.

I guess this might be less applicable for an application that is only ever accessible within a closed network or something but otherwise I think it would be very widely applicable. Can you think of other examples where it would not be applicable @elarlang ?

@elarlang
Copy link
Collaborator

elarlang commented Sep 3, 2024

Ok I take your point @elarlang , do we need to update other requirements to match them to the documentation requirement?

In general those need to match and work as a unit. I don't have any proposal for changes at the moment.

Can you think of other examples where it would not be applicable @elarlang ?

Any email web service. Should it be limited with time-window? The proposal is usable for back-office applications, and that is why I say, that is application and business-need specific and can not be applied to every application.

Additionally, also from NIST: https://www.nist.gov/publications/zero-trust-architecture about the location limitation:

Zero trust (ZT) is the term for an evolving set of cybersecurity paradigms that move defenses from static, network- based perimeters to focus on users, assets, and resources. A zero trust architecture (ZTA) uses zero trust principles to plan industrial and enterprise infrastructure and workflows. Zero trust assumes there is no implicit trust granted to assets or user accounts based solely on their physical or network location (i.e., local area networks versus the internet) or based on asset ownership (enterprise or personally owned). Authentication and authorization (both subject and device) are discrete functions performed before a session to an enterprise resource is established. Zero trust is a response to enterprise network trends that include remote users, bring your own device (BYOD), and cloud- based assets that are not located within an enterprise-owned network boundary.

@tghosth
Copy link
Collaborator

tghosth commented Sep 3, 2024

Any email web service.

Would this be L3?

If it was, if I usually login from a Windows PC in Israel and then suddenly I log in from Arch Linux in Malaysia, I'd like it to flag something up, no?

@jmanico
Copy link
Member

jmanico commented Sep 3, 2024 via email

@elarlang
Copy link
Collaborator

elarlang commented Sep 3, 2024

If it was, if I usually login from a Windows PC in Israel and then suddenly I log in from Arch Linux in Malaysia, I'd like it to flag something up, no?

I would put it to the threat-model-based-monitoring section, not the application authorization decision.

@tghosth
Copy link
Collaborator

tghosth commented Sep 3, 2024

Do we have a threat-model-based-monitoring section?

@elarlang
Copy link
Collaborator

elarlang commented Sep 3, 2024

7.2.5 [MODIFIED, MOVED FROM 11.1.8] Verify that the application has configurable alerting when unusual or malicious activity is detected.

@tghosth
Copy link
Collaborator

tghosth commented Sep 3, 2024

OK so that is alerting rather than taking active action. Maybe we can expand and improve this requirement to also talk about taking action and be clearer about the environmental aspects?

@elarlang
Copy link
Collaborator

elarlang commented Sep 3, 2024

In this issue I would like to fix the requirement list that needs further discussion (#2033 (comment)) and open separate issue for each one of them. Then we can continue the discussion for each one of them separately, here it's getting too messy really fast.

@jmanico
Copy link
Member

jmanico commented Sep 4, 2024

I agree with @elarlang that having all of these contested issues broke into separate issues is best. Maybe tag them all as access control. @EnigmaRosa do you want to take this on?

@EnigmaRosa
Copy link
Contributor Author

EnigmaRosa commented Sep 4, 2024 via email

@EnigmaRosa
Copy link
Contributor Author

I have created issues #2058 #2059 #2060 #2061 #2062 #2063 and #2065 from this

@elarlang
Copy link
Collaborator

elarlang commented Sep 5, 2024

Seems that this issue has now served its goal of collecting initial feedback and filtering.

All proposed requirements have their own issues now and this issue can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 4) proposal for review Issue contains clear proposal for add/change something 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

4 participants