ASVS icon indicating copy to clipboard operation
ASVS copied to clipboard

Consider Adding Feature-Policy Header Verification to ASVS

Open ImanSharaf opened this issue 2 years ago • 14 comments

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."

ImanSharaf avatar Oct 13 '23 23:10 ImanSharaf

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 avatar Oct 14 '23 07:10 elarlang

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/

elarlang avatar Oct 14 '23 09:10 elarlang

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 avatar Oct 23 '23 15:10 tghosth

https://github.com/OWASP/ASVS/issues/1755#issuecomment-1762698351

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 avatar Oct 23 '23 17:10 elarlang

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.

elarlang avatar Oct 23 '23 18:10 elarlang

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 ?

tghosth avatar Oct 25 '23 06:10 tghosth

See my comment here (https://github.com/OWASP/ASVS/issues/1755#issuecomment-1762778518) - 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 avatar Oct 25 '23 06:10 elarlang

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: https://github.com/w3c/webappsec-permissions-policy/issues/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.

tghosth avatar Oct 31 '23 14:10 tghosth

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: https://github.com/w3c/webappsec-permissions-policy/issues/189

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

elarlang avatar Oct 31 '23 14:10 elarlang

@ImanSharaf any further thoughts?

tghosth avatar Oct 31 '23 15:10 tghosth

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.

ImanSharaf avatar Oct 31 '23 16:10 ImanSharaf

@elarlang correctly noted that #1201 is related to this

tghosth avatar Feb 04 '24 16:02 tghosth

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?

meghanjacquot avatar Sep 05 '24 22:09 meghanjacquot

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.

elarlang avatar Sep 06 '24 05:09 elarlang