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

Consider Adding Feature-Policy Header Verification to ASVS #1755

Open
ImanSharaf opened this issue Oct 13, 2023 · 14 comments
Open

Consider Adding Feature-Policy Header Verification to ASVS #1755

ImanSharaf opened this issue Oct 13, 2023 · 14 comments
Assignees
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet Community wanted We would like feedback from the community to guide our decision otherwise we will progress V50 Group issues related to Web Frontend _5.0 - draft This should be discussed once a 5.0 draft has been prepared.

Comments

@ImanSharaf
Copy link
Collaborator

I've noticed that the current version of ASVS does not have an item covering the implementation of the Feature-Policy header (also known as Permissions-Policy in its latest iteration). This header provides a method to control which browser features and APIs can be invoked, thereby playing a crucial role in minimizing the potential for abuse by malicious actors.

Proposed ASVS Item:

"Verify that a Feature-Policy (or Permissions-Policy) header is implemented to specify which browser features can be used by the application, minimizing the potential for abuse."

@elarlang
Copy link
Collaborator

elarlang commented Oct 14, 2023

Verify that a Feature-Policy (or Permissions-Policy) header is implemented to specify which browser features can be used by the application, minimizing the potential for abuse.

We have requirement for that topic.

# Description L1 L2 L3 CWE
8.3.11 [MODIFIED, MOVED FROM 10.2.2, LEVEL L2 > L1] Verify that the application does not ask for unnecessary or excessive permissions to privacy related features or sensors, such as cameras, microphones, or location. 272

The question is - should the 8.3.11 more clearly point to Permission-Policy header and is it in correct category (8.3 Sensitive Private Data vs V14.4 HTTP Security Headers)

Or in other words - the question - we have requirement for that application by it's logic does not ask for unnecessary permissions, but we maybe should have separate or modified requirement to send the message: in case of "XSS" or some other attack, attacker can not ask those permissions from the victim.

@elarlang elarlang added the 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet label Oct 14, 2023
@elarlang
Copy link
Collaborator

Permission-Policy replaces Feature-Policy, but based on caniuse.com currently Feature-Policy has 95.64% global support vs Permission-Policy 68.84%.

So at the moment we probably need to cover both headers, like in 14.4.7 we ask to use X-Frame-Options although it is/will be deprecated and replaced by Content-Security-Policy: frame-ancestors.

Additionally we need to keep eye on Document-Policy but at the moment it does not seems to be in the state to be added to standard as a requirement https://wicg.github.io/document-policy/

@tghosth
Copy link
Collaborator

tghosth commented Oct 23, 2023

Ok so this raises an interesting question.

I see two threat scenarios here:

  1. An application deliberately requires more sensitive permissions than it needs.
  2. An attacker/malicious developer/misguided developer introduces code into the application which uses unexpected sensitive permissions.
  1. An application deliberately requires more sensitive permissions than it needs.

I think that this is the threat which 8.3.11 is coming to try and address and the reason it got moved to V8. Ultimately it is more of a policy question than a malicious attacker question.

  1. An attacker/malicious developer/misguided developer introduces code into the application which uses unexpected sensitive permissions.

This seems like the reason for the permission policy header.

As such, it sounds like we need two separate requirements.

What do you think?

@tghosth tghosth added _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 Oct 23, 2023
@elarlang
Copy link
Collaborator

#1755 (comment)

but we maybe should have separate or modified requirement to send the message: in case of "XSS" or some other attack, attacker can not ask those permissions from the victim.

Now thinking more about it, it think it makes sense to have this separate requirement.

@elarlang
Copy link
Collaborator

From the original proposal:

"Verify that a Feature-Policy (or Permissions-Policy) header is implemented to specify which browser features can be used by the application, minimizing the potential for abuse."

I think the requirement message should be: all features and accesses to device sensors must be disallowed or blocked by default and only allowed for those URL's and those sensors which application needs.

@tghosth
Copy link
Collaborator

tghosth commented Oct 25, 2023

OK so how about:

# Description L1 L2 L3 CWE
14.4.10 [ADDED] Verify that a Feature-Policy (or Permissions-Policy) header is used to only allow browser features which are specifically required by the application. 266

CWE-266 seems like the closest match.

L2 because not everything can be a L1 and it is a defense in depth requirement. I'd also consider L3.

Comments, @ImanSharaf @elarlang ?

@elarlang
Copy link
Collaborator

See my comment here (#1755 (comment)) - I think for some time we need to ask both headers.

As by everything is allowed by default, current wording gives feeling that "you need to allow them only when you use them". In practice - you always need to block everything.

As it is not tiny change, I don't try to create new wording myself :)

I don't think CWE-266 "Incorrect Privilege Assignment" is suitable - the application do not assign any privileges, it just do not block potential malicious privilege assignment by default. By title it is possible to find matching CWE's, but if to read descriptions as well, then those seems to be all file-permissions specific.

@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 Oct 30, 2023
@tghosth
Copy link
Collaborator

tghosth commented Oct 31, 2023

Ok so I did some more reading on this and I have identified a few problems.

  1. As Elar says, the number of supporting browsers for the newer header is very small with only really Chromium support at the moment.
  2. There is no effective deny-all mechanism at this point in time, there is an open issue about this here: Proposal: define default for all w3c/webappsec-permissions-policy#189 It basically means that every single permission you want to block needs to be mentioned on every single response and you need to add new permissions to the header each time a new permission is added to the browser. This seems very cumbersome and sub-optimal.
  3. From the number of open issues, I am worried that this whole concept is not mature enough to include in ASVS yet.

I am tagging @kevinkiklee to see if he has any other insights on it. (Hi @kevinkiklee, we were thinking of adding the Feature-Policy/Permissions-Policy headers as a requirement to ASVS but as discussed in this comment I don't think we can can at this stage)

Otherwise, at this point I would recommend that we tag this as something to revisit when we reach the ASVS 5.0 "Draft" stage. IF at this stage, the situation has improved then maybe we can add it simply. If not, I think we skip it.

@elarlang
Copy link
Collaborator

elarlang commented Oct 31, 2023

I agree the maturity is questionable and to declare it's permission separately is nightmare - so maybe we set precondition here that if it is actually doable (without causing extra kilobytes to the traffic) and browers support it: w3c/webappsec-permissions-policy#189

edit: ... or we try to develop level 3 requirement and we make it level 2 when it matches the previous lines.

@tghosth
Copy link
Collaborator

tghosth commented Oct 31, 2023

@ImanSharaf any further thoughts?

@ImanSharaf
Copy link
Collaborator Author

at this point I would recommend that we tag this as something to revisit when we reach the ASVS 5.0 "Draft" stage. IF at this stage, the situation has improved then maybe we can add it simply.

I believe this works.

@tghosth tghosth added _5.0 - draft This should be discussed once a 5.0 draft has been prepared. and removed 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos _5.0 - prep This needs to be addressed to prepare 5.0 labels Oct 31, 2023
@elarlang elarlang added the V14 label Nov 27, 2023
@tghosth
Copy link
Collaborator

tghosth commented Feb 4, 2024

@elarlang correctly noted that #1201 is related to this

@meghanjacquot
Copy link
Collaborator

Following up here, it seems like this issue is closed for V14 and that it does not need to be currently considered for the current draft that we are working on as it is lacking maturity. Is that accurate @tghosth?

@elarlang elarlang added V50 Group issues related to Web Frontend and removed V14 labels Sep 6, 2024
@elarlang
Copy link
Collaborator

elarlang commented Sep 6, 2024

I answer myself - there are no big changes, so we leave it out at the moment.

I updated the category to V50. V14 it was previously and it is outdated information.

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 Community wanted We would like feedback from the community to guide our decision otherwise we will progress V50 Group issues related to Web Frontend _5.0 - draft This should be discussed once a 5.0 draft has been prepared.
Projects
None yet
Development

No branches or pull requests

4 participants