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

Is 13.1.4 a duplicate with Access Control? #1312

Closed
tghosth opened this issue Jul 3, 2022 · 16 comments
Closed

Is 13.1.4 a duplicate with Access Control? #1312

tghosth opened this issue Jul 3, 2022 · 16 comments
Assignees
Labels
4a) Waiting for another This issue is waiting for another issue to be resolved 4b Major-rework These issues need to be part of a full chapter rework Community wanted We would like feedback from the community to guide our decision otherwise we will progress josh/elar V4 Temporary label for grouping authorization related issues V13 _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@tghosth
Copy link
Collaborator

tghosth commented Jul 3, 2022

# Description
13.1.4 Verify that authorization decisions are made at both the URI, enforced by programmatic or declarative security at the controller or router, and at the resource level, enforced by model-based permissions.

This requirement feels like it is a duplication of what we talk about in the access control chapter and the relevant architecture section.

@tghosth tghosth changed the title Is 13.1.4 a duplicate with Access Control Is 13.1.4 a duplicate with Access Control? Jul 3, 2022
@tghosth tghosth added help wanted _5.0 - prep This needs to be addressed to prepare 5.0 Community wanted We would like feedback from the community to guide our decision otherwise we will progress labels Jul 3, 2022
@Sjord
Copy link
Contributor

Sjord commented Aug 13, 2022

Well, 4.1.3 basically says "users should only be able to access what they are allowed to access". That's true, but too wide. I think it would have value to split this up into function-level and object-level access control. 4.2.1 says you have to protect against insecure direct object references, which implies object-level access control, but is worded really in the sense of an attack instead of how to implement authorization.

However, all this is not specific to APIs, so chapter 13 is not the correct place for this.

@TobiasAhnoff
Copy link

I would argue that 13.1.4 is not a duplicate. The language "enforce access control rules at a trusted service layer" from 4.1.1 is too wide from an API at L2 perspective. 13.1.4 adds additional information that authorization is required "at the controller or router, and the resource level."

We often hear the argument that access control only should be put in front of an API (in a gateway, sidecar, declarative routing level using web framework policies, etc.) This pattern is problematic due to security misconfiguration and is susceptible to IDOR.

I find that 13.1.4 captures a strong pattern and is a favorite part of ASVS.

@tghosth
Copy link
Collaborator Author

tghosth commented Sep 13, 2022

Interesting, @TobiasAhnoff do you think other parts of ASVS could be clarified to avoid the appearance of duplication?

@TobiasAhnoff
Copy link

TobiasAhnoff commented Sep 18, 2022

Hard question to answer and to keep it short…hope this will be good input!

In general, I think ASVS is well organized and a great resource when building secure applications, but for authorization there might be a risk that readers expect V4 to contain all verifications. The second time I wanted to read 13.1.4 I started looking in V4…so there might be some room for improvement...

I think V1.4, V4, 13.4.2 and 13.3.4 all capture important principles and are not overlapping.

To me, 13.4.2 and 13.3.4 captures parts of the Defense in depth and Zero trust principles, which the other verifications does not include. But they are a little hidden in the more specific sections for Web Service and GraphQL.

Maybe by making these more general and adding a new verification in V4.3 (at L2 or maybe L3), would make it clearer? I am thinking something like this:

4.3.x Verify that authorization is correct using a mandatory pattern at the business logic layer, not assuming other layers or services have done or will do authorization (apply the principle of Zero trust for authorization at the application level.)

In example, if a gateway/firewall is removed/misconfigured, or the application layer is refactored (to a new web framework), or the data layer changes from SQL (with support for tenant authorization) to NoSQL (with no tenant authorization support), then authorization should still be correct. The business layer should always perform correct/complete authorization, Zero Trust in both application security and infrastructure security.

To also address Defense in depth we could start by making 13.4.2 and 13.3.4 more general, and modify them like this:

13.3.4 Verify that authorization decisions are made at both the application layer, enforced by programmatic or declarative security supported by the application framework, and at the business logic layer, enforced by model-based permissions (apply the Defense in depth principle for authorization at the application level.)

13.4.2 Verify that data layer authorization logic is implemented at the business logic layer as well.

And move them to 4.3 (close to 4.3.x and 4.3.3 or maybe 4.1.1), but then I think 13.4.2 would be a duplicate of the new 4.3.x and could be removed (or be kept as is in GraphQL, even if it is an duplicate, if it is especially important for GraphQL).

The "new 13.3.4”, as well as the current one, are not overlapped by 4.3.x, since 13.3.4 address Defense in depth.

In this way (add 4.3.X, remove 13.4.2, make a more general 13.3.4 and move it to 4.3) all authorization related verifications are found in V1.4 and V4, and from my perspective it is clear that each are important (not duplicates).

And, in addition to Least Privilege, V4 would also include Zero trust and Defense in depth for authorization at the application level, which I feel is a little bit hidden (or missing) in ASVS 4 and I think something like this would add value.

But I fully understands that authorization can be implemented in many ways and this could of course be expressed and organized in other ways, and I also want to point out that I like ASVS and 13.3.4 as is, but I hope this is good input for a discussion on possible improvements!

@elarlang
Copy link
Collaborator

Thank you Tobias, quite a lot of input here.

As I'm working with Authorization topic, I have opened separate "parent" issue for giving an overview #1352

Please take a look at it. I feel that solving problems listed in the parent issue is a bit pre-condition to work on with this issue - as some of the problems may get solved already.

@TobiasAhnoff
Copy link

Yes, authorization is a challenging and interesting topic!

I will look at #1352 more during this week, thanks for reading!

@tghosth tghosth added josh/elar 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet and removed help wanted labels Dec 7, 2022
@tghosth
Copy link
Collaborator Author

tghosth commented Jan 24, 2023

Requirement history

# Description
13.1.4 Verify that authorization decisions are made at both the URI, enforced by programmatic or declarative security at the controller or router, and at the resource level, enforced by model-based permissions.
18.9 Verify that authorization decisions are made at both the URI and resource level, not just at the resource level.
18.12 Verify that authorization decisions should be at both the URI and resource level, not just at the resource level.

This requirement was basically created in e9b0f94 based on a request from @ossie-git in #285 and then refined in #586.

@ossie-git, any comments you have to add would be great :)

Hi @TobiasAhnoff,

Revisiting this issue. I can see you are a fan of 13.1.4 which is great 😀 so I am trying to understand better how it is interpreted and then we can consider what to do with it.

When I read this, it appears to be mandating the need for both vertical access control (i.e. what functions can the user access) and horizontal access control (i.e. what objects/entities is the user allowed to perform functions on). I would not consider that to be defence in depth but rather a key aspect of AuthZ.

In addition to that, I would be a little nervous to mandating multiple locations to verify authorization unless we are clear what is being checked in each one.

What do you think?

@TobiasAhnoff
Copy link

TobiasAhnoff commented Jan 24, 2023

I believe maybe it can be both defence in depth and a key aspect of authorization.
An example of how 13.1.4 can be interpreted to code can be found here https://github.com/Omegapoint/defence-in-depth

Where authorization at the routing level would be implemented using e g .NET Authorize attributes or Java Spring Security PreAuthorize tags.
Note that the authorization at this point typically is only be based on e g JWT scope claims.
The important thing is that this layer provides us with a fast way (without consuming system resources or do complex domain logic) to deny requests that do not have access to the operation (route).
https://github.com/Omegapoint/defence-in-depth/blob/main/rest-api/9-complete-with-all-defence-layers/Controllers/ProductsController.cs#L22

Then, to have an application core or business domain layer that always will enforce proper authorization, the ProductService in this example should not assume that other layers has don authorization like access to the operation. And it must also verify access to the data (horizontal access control).
https://github.com/Omegapoint/defence-in-depth/blob/main/rest-api/9-complete-with-all-defence-layers/Domain/Services/ProductService.cs#L31

In this way we have a "defence in depth" solution, where we always verify access to the operation and access to the data at the resource level, even if our service is consumed from e g a service bus handler (not a API controller where the authz attributes are). And at the same time we will also deny unauthorized API requests early, and make sure it takes two developer mistakes to miss verifying access to the operation.

We could also do basic authorization based on token claims in a gateway or sidecar component to make sure that we always do token validation and basic parts of authorization even if developers makes mistakes in the API or service implementation.

And in my opinion 13.1.4 could be interpreted to captures most parts of all this, and help limit the risk of exposing services without proper authorization. That´s why I´m happy to have 13.1.4 in ASVS, but it could of course be expressed in other ways or organized differently. To me the important thing is the AND.

When it comes to performance, that might depend on context, but in my experience validating a JWT twice and make authorization decisions based on simple token claims like scopes twice has not been an issue. And the same reasoning applies to input validation, where you might have a WAF providing basic input validation, but your service/domain layer should not assume this and always do proper and deeper validation (according to domain rules a WAF probably should not be aware of.)
But it is of course important to consider performance and availability aspects as well.

My colleagues and I have also written an article about it here https://securityblog.omegapoint.se/en/secure-apis-by-design

Hope this is good input!

@elarlang elarlang added the V4 Temporary label for grouping authorization related issues label Jan 25, 2023
@elarlang
Copy link
Collaborator

My interpretation/takeaways here: double-check (routing/WAF + application/API level) gives you:

  • better performance as clear incorrect requests are not reaching to the API/application and does not waste resources
  • 2nd (extra) layer of defense in case one of the authorization check does not work correctly

But. I would say it is not required to have this extra layer, and you have implemented authorization correctly and only in one place.

What do you think about that, @TobiasAhnoff ?

@TobiasAhnoff
Copy link

I agree, and I think it is important to emphasize that the "correctly and one place" should be in the API/application, not only in the gateway (WAF).

Maybe having an extra layer (WAF) could be a separate L2 requirement?

@tghosth tghosth added the 4b Major-rework These issues need to be part of a full chapter rework label May 23, 2023
@tghosth tghosth removed the 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet label Jun 1, 2023
@elarlang
Copy link
Collaborator

Re-read it all. My current understanding is, that even the requirement 13.1.4 gives some extra layer, it's hard to require it - if the access control is done correctly, it can be recommended, but not required.

In big picture we need to recheck the issue after V4 reorg is sorted out: #1352

@elarlang elarlang added the 4a) Waiting for another This issue is waiting for another issue to be resolved label Oct 31, 2023
@tghosth tghosth added the V13 label Mar 15, 2024
@tghosth
Copy link
Collaborator Author

tghosth commented Mar 15, 2024

In the same way that 13.4.2 provides a GraphQL specific implementation requirement for authorisation, I think we could structure 13.1.4 so that it provides an API specific implementation requirement for authorization.

I would suggest something like:

# Description L1 L2 L3 CWE
13.1.4 [MODIFIED] Verify that APIs enforce authorization controls at both the URI level, (using programmatic or declarative security at the controller or router) but also at the resource level, enforced by model-based permissions within the API's code. 285

Opinions @elarlang @TobiasAhnoff

@jmanico
Copy link
Member

jmanico commented Mar 15, 2024

My take on this is that different frameworks treat URIs differently. In some framework a single URL can actually actually map to dozens of different features based on the payload of that request.

So to some degree, I think this kind of access control requirement is a little dated. The only needed access controls in my world is at the feature level and then at the data specific level. I also prefer to see user specific things, like a user profile, verify who the current user is from the session as opposed to a parameter and for me those three things cover everything.

@tghosth
Copy link
Collaborator Author

tghosth commented Mar 17, 2024

I am not convinced I would describe it as dated as it is something I am seeing a lot of however I think that given that it is specific to a particular way of designing an API, I do question whether it is a little too specific...

I think maybe this is another one that I would be happy to see removed, assuming we do mention the different types of authorization such as horizontal and vertical somewhere else in the standard.

@tghosth tghosth mentioned this issue Mar 20, 2024
@tghosth
Copy link
Collaborator Author

tghosth commented Apr 18, 2024

I am going to remove it here as I think it is covered in 4.2.1 but I have opened #1934 to make sure that it is clear enough.

@tghosth
Copy link
Collaborator Author

tghosth commented Apr 21, 2024

Closed by #1935

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4a) Waiting for another This issue is waiting for another issue to be resolved 4b Major-rework These issues need to be part of a full chapter rework Community wanted We would like feedback from the community to guide our decision otherwise we will progress josh/elar V4 Temporary label for grouping authorization related issues V13 _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

No branches or pull requests

5 participants