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

Feature Policy shouldn't be overridable #357

Closed
shhnjk opened this issue Dec 6, 2019 · 9 comments · Fixed by #378
Closed

Feature Policy shouldn't be overridable #357

shhnjk opened this issue Dec 6, 2019 · 9 comments · Fixed by #378
Milestone

Comments

@shhnjk
Copy link
Member

shhnjk commented Dec 6, 2019

In current spec, setting feature-policy: geolocation 'self' in top-frame wouldn't restrict cross-origin iframe to request access to Geo location (e.g. <iframe allow="geolocation" src="https://cross-origin.tld"></iframe>). This has to change, in order to provide some mechanism to avoid leaking permission to cross-origin iframe (especially in the browser that supports Permission Delegation).

@clelland
Copy link
Collaborator

clelland commented Dec 6, 2019

'self' is essentially the default in that case, and in most cases; currently a header policy of 'self' has no actual effect -- I believe that every algorithm will run the same way whether or not that header is present, if the default allowlist for a feature is 'self'.

Is the suggestion here that an explicit header policy of 'self' should be interpreted as "this origin and no others, regardless of any iframe attributes"?

Presumably that logic would have to extend to any header-set policy, that if a header is present at all, then any origins not mentioned in the header can not have the feature delegated to them.

@shhnjk
Copy link
Member Author

shhnjk commented Dec 6, 2019

Is the suggestion here that an explicit header policy of 'self' should be interpreted as "this origin and no others, regardless of any iframe attributes"?

Right :)
You can also check https://bugs.chromium.org/p/chromium/issues/detail?id=937131 😊

@clelland
Copy link
Collaborator

clelland commented Dec 6, 2019

One approach we might take with this would be to change the way that the header and allow attributes work -- to say that in cross-origin cases, both header and allow attribute have to agree in order for any feature to be delegated.

For example, if we could choose to make the header default '*' for a feature, then the allow attribute could be used to delegate to any domain, but it wouldn't happen automatically. (This would be the same behaviour as exists today, if the header is not present). A site could choose to restrict that via header to just self, or even to none.

That would mean that you can't just use the header to set the defaults, and expect automatic delegation without the attribute, so I'd want to investigate how compatible that is with existing usage.

I suspect that it is compatible, that current usage falls into one of three categories, that are unchanged by this:

  • No header, no attributes
  • No header, attributes used to target specific frames
  • Header set to 'none', which would deny in the current frame, and therefore to all subframes anyway.

Cases which would see changes:

  • Header set to 'self', allow attributes used to target specific frames (those cross-origin frames would no longer be granted the feature)
  • Header set to specific origins, no allow attributes used (the origins mentioned would no longer be granted the feature)

There may be other cases I'm not thinking of; changing that behaviour for existing sites may be trickier than I'm thinking :)

sync-xhr would also be an issue, as the current default is *, and allow attributes are generally not used to delegate it, so that would require careful thought.

@bershanskiy
Copy link
Member

Is the suggestion here that an explicit header policy of 'self' should be interpreted as "this origin and no others, regardless of any iframe attributes"?

Right :)

One approach we might take with this would be to change the way that the header and allow attributes work -- to say that in cross-origin cases, both header and allow attribute have to agree in order for any feature to be delegated.

An outsider here (aka someone not involved with Mozilla/Google/Microsoft/W3C). This proposed behavior is more intuitive (more similar to CSP's 'self') and makes development of secure applications easier. If 'self' is overidable with <iframe> attribute, then (obviously) HTML injection can lead to unintentional permission delegation. Also, HTTP headers seem to be more trusted than HTML tags. E.g., <meta http-equiv="set-cookie" ...> was removed in favor of using only HTTP headers and JS.

@jpchase
Copy link
Contributor

jpchase commented Jan 15, 2020

Another consideration for backwards compatibility is that sites often include third-party scripts in the top-level document (e.g. ads, social widgets, comment frameworks, etc.), for additional functionality. Sites may or may not have the expectation that the third-parties can add/modify frames in the document. Even if the sites expect frames to modified, the sites might not know the origins of such frames in advance. The end result seems hard to distinguish from malicious markup injection.

In theory, this doesn't seem to introduce any new problems, since CSP already has frame-src. Sites using CSP in these scenarios would already have to know the domain(s) to set frame-src appropriately. However, I'm wondering if there's a practical difference in deployment. Namely, even when CSP is used, then frame-src is not used, or is less restricted, to handle unknown origins in frames. Also, CSP allows wildcards, while Feature Policy only supports full origins.

@clelland clelland added this to the vNext milestone Jan 24, 2020
@annevk
Copy link
Member

annevk commented Apr 30, 2020

FWIW, I think this change makes a lot of sense. (Another interesting feature might be blocking everything unless it's safelisted.)

@clelland
Copy link
Collaborator

That sounds like the long-standing #208, which hopefully we can resolve now, with most of the potentially problematic features moving to Document Policy.

@clelland
Copy link
Collaborator

I meant #189 actually, as it's the more flexible version of the same concept.

@annevk
Copy link
Member

annevk commented Apr 30, 2020

Yeah, you'd only disable them for yourself as third parties couldn't get them anyway so that should work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants