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

Possible duplication between 1.4.1 Access Control Architecture and 4.1.1 General Access Control Design #1031

Closed
lfservin opened this issue Jul 7, 2021 · 17 comments

Comments

@lfservin
Copy link
Contributor

lfservin commented Jul 7, 2021

1.4.1 Verify that trusted enforcement points, such as access control gateways, servers, and serverless functions, enforce access controls. Never enforce access controls on the client. is very close to
4.1.1 Verify that the application enforces access control rules on a trusted service layer, especially if client-side access control is present and could be bypassed.

in both cases I would recommend the developers to defer the authorization to something like the open policy agent (OPA).

Furthermore, 1.4.1 seems to be pretty close to 1.4.4 Verify the application uses a single and well-vetted access control mechanism for accessing protected data and resources. All requests must pass through this single mechanism to avoid copy and paste or insecure alternative paths.

I would again recommend using OPA in this case

@elarlang
Copy link
Collaborator

elarlang commented Aug 5, 2021

Status: waits "scope definition" for V1 category (requirements).

Related to #877

@elarlang elarlang added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet on hold labels Aug 5, 2021
@elarlang elarlang added 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR and removed on hold labels Oct 15, 2021
@elarlang
Copy link
Collaborator

Well, we kinda figured out what to do with V1 category (see #1063, TL;DR - if you can check it from actual implementation, move it to another category, in this case V4).

@lfservin - I would like to see proposals related to 1.4.1, 1.4.4 and 4.1.1.

@elarlang
Copy link
Collaborator

elarlang commented Nov 7, 2021

I try to move this forward.

V1.4.1 and V4.1.1 are clear duplicates for me, if something specific is not hidden in terms like "trusted enforcement points" or "access control gateways".

  • V1.4.1 Verify that trusted enforcement points, such as access control gateways, servers, and serverless functions, enforce access controls. Never enforce access controls on the client.
  • V4.1.1 Verify that the application enforces access control rules on a trusted service layer, especially if client-side access control is present and could be bypassed.

Point for both is, don't check user permissions on the client side, as it is under client control and can not be trusted (can be bypassed). Both have CWE 602.

Proposals:

  • remove requirement 1.4.1 as duplicate of 4.1.1, it is implementation check, so we don't keep architecture one
  • remove unnecessary part from 4.1.1

V4.1.1 Verify that the application enforces access control rules on a trusted service layer, especially if client-side access control is present and could be bypassed.

Furthermore, 1.4.1 seems to be pretty close to 1.4.4

I can see logic behind this, but I see those as separate problems and would like to keep separate requirements. If you make "single vetted checks" on client side, then you "pass the check" for that, but it is done in wrong place.

@elarlang elarlang added 4) proposal for review Issue contains clear proposal for add/change something and removed 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR labels Nov 7, 2021
@jmanico
Copy link
Member

jmanico commented Nov 9, 2021

How about:

V4.1.1 Verify that the application enforces access control rules on a trusted service layer. Never enforce access controls on the client.

@elarlang
Copy link
Collaborator

"Never enforce access controls on the client."

I think it's a bit too strong and may give some wrong signal - like you can not do something on the client side. The point is, you can do whatever on the client, just this is usability and you can not rely on that from security perspective.

So maybe - "Never rely on access controls on the client." <- language checks/fixes needed

@jmanico
Copy link
Member

jmanico commented Nov 10, 2021

The point is, you can do whatever on the client, just this is usability and you can not rely on that from security perspective.

That is not (at all) true. You cannot send sensitive data to the client that a user does not have access to and restrict the view of that data via client-side access control. This is a very common anti-pattern.

The principle of least privilege rules here. Only send to the client what the user has access to. You simply cannot do effective access control client-site at all.

@elarlang
Copy link
Collaborator

elarlang commented Nov 11, 2021

I agree with sensitive data part, we have one pending requirement on that topic: #934

I talk more about frameworks like Angular, where there is classical solution, that all functionality is sent to the client, but then decisions what functionality to show and what not, are made on the client.

@jmanico
Copy link
Member

jmanico commented Nov 11, 2021

I talk more about frameworks like Angular, where there is classical solution, that all functionality is sent to the client, but then decisions what to show and what not, are made on the client.

I politely agree with that statement as well. Any intellectual property-based functionality like shipping routes, stock-trading AI, or other proprietary functionality needs to stay server-side a well.

@jmanico
Copy link
Member

jmanico commented Nov 11, 2021

I still do not like the quote "Never enforce access controls on the client" it's not nuanced enough and I do get your point above @elarlang

@tghosth
Copy link
Collaborator

tghosth commented Feb 23, 2022

What about:

V4.1.1 Verify that the application enforces access control rules at a trusted service layer and doesn't rely on controls at an untrusted/client accessible layer.

@elarlang
Copy link
Collaborator

Maybe just untrusted layer? Term "client accessible" is confusing (at least for me), client can access (for reading) backend service as well.

@tghosth tghosth added 6) PR awaiting review and removed 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 labels Feb 24, 2022
@elarlang
Copy link
Collaborator

Reopen.

# Description L1 L2 L3 CWE
1.4.1 Verify that trusted enforcement points, such as access control gateways, servers, and serverless functions, enforce access controls. Never enforce access controls on the client. 602
4.1.1 [MODIFIED] Verify that the application enforces access control rules at a trusted service layer and doesn't rely on controls which an untrusted user could manipulate such as client-side JavaScript. 602

Was 1.4.1 kept because of some good reason or marking it as duplicate/removed was just accidentally left out from PR?

@elarlang elarlang reopened this Aug 26, 2022
@elarlang elarlang assigned tghosth and unassigned jmanico Aug 26, 2022
@elarlang elarlang added 2) Awaiting response Awaiting a response from the original poster and removed 6) PR awaiting review labels Aug 31, 2022
@tghosth
Copy link
Collaborator

tghosth commented Sep 11, 2022

Ugh, this whole thing is tricky. To me, 4.1.1 is the better wording but I actually think that because it is more conceptual rather than a specific control, I think it should belong in the architecture section 1.4. @elarlang do you disagree?

@elarlang
Copy link
Collaborator

For me V1 is just for documentation and decisions, if we need to/can check it from actual implementation, it should be out of V1.

@tghosth
Copy link
Collaborator

tghosth commented Sep 13, 2022

So would you prefer:

Verify that the architecture defines trusted enforcement points, such as access control gateways, servers, and serverless functions, for access controls to avoid enforcing access controls at an untrusted layer such as the end client.

@elarlang
Copy link
Collaborator

I prefer to get rid of 1.4.1, because in practice I can not see, what it gives extra or additionally to 4.1.1.

We moved "conceptual" "Verify that input validation is enforced on a trusted service layer." also from 1.5.3 to 5.1.6. For me it's the same style.

@tghosth tghosth mentioned this issue Sep 14, 2022
@tghosth
Copy link
Collaborator

tghosth commented Sep 14, 2022

Ok, I opened #1372 to bin it

@tghosth tghosth added 6) PR awaiting review and removed 2) Awaiting response Awaiting a response from the original poster labels Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants