Open Bug 1508123 Opened 4 years ago Updated 4 years ago

Support 'src' allowList for FeaturePolicy in sandboxed iframes

Categories

(Core :: DOM: Security, enhancement)

enhancement
Not set
normal

Tracking

()

ASSIGNED
Tracking Status
firefox65 --- affected

People

(Reporter: baku, Assigned: baku)

Details

(Keywords: dev-doc-needed, Whiteboard: [domsecurity-backlog1] [domsecurity-active])

Attachments

(4 files, 1 obsolete file)

This is a possible solution to issue #230:
https://github.com/WICG/feature-policy/issues/230
Attached patch part 4 - WPTSplinter Review
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-backlog1] [domsecurity-active]
This is what we discussed. The idea is:

1. store the FeaturePolicy directives in the outer window. The iframe's document can inherit the directives from it.
2. if the iframe's document's principal is a NullPrincipal, we remove the 'src' entries in the FeaturePolicy's allowLists after the first inherit.

All of this works fine. The remaining open question is: should HTMLIFrameElement.policy use an opaque origin in case of sandboxes? So far, I haven't touched HTMLIFrameElement::GetFeaturePolicyDefaultOrigin, but probably we should.

Do you mind to review these patches? Thanks
Flags: needinfo?(bzbarsky)
Comment on attachment 9025927 [details] [diff] [review]
part 1 - outer window

Propagating this through the loadstate via loadURI is a bit odd.  Especially given that we can create documents (via CreateAboutBlankContentViewer) which don't go through that path at all.  Speaking of which, what happens with feature policy on those?

Anyway, this seems like something we should just store on the docshell (or outer window) and maintain it from the frameloader, like we handle the "name" attribute already...
Comment on attachment 9025929 [details] [diff] [review]
part 3 - strip 'src' allowList entries

I don't quite follow this.  If I load a page in a sandboxed iframe that does _not_ come from the src load, it would still do the stripping thing in nsIDocument::InitFeaturePolicy.  Is that desirable?

And Feature::AllowListContains would test true for any src policy in that doc, even though it's not from an src load.
Flags: needinfo?(bzbarsky) → needinfo?(amarchesini)
> I don't quite follow this.  If I load a page in a sandboxed iframe that does
> _not_ come from the src load, it would still do the stripping thing in
> nsIDocument::InitFeaturePolicy.  Is that desirable?

For instance a srcdoc without src? I think yes. 2 reasons:

1. that iframe's document can navigate and the following pages should not see 'src' allowList entries.

2. 'src' in allowLists is not directly connected with 'src' loads. The spec says that we have to replace 'src' with the declared-origin as described here: the https://wicg.github.io/feature-policy/#declared-origin

> And Feature::AllowListContains would test true for any src policy in that
> doc, even though it's not from an src load.

This is what already happens, right?
Flags: needinfo?(amarchesini) → needinfo?(bzbarsky)
OK, so I think I understand the model this patch is implementing now.  

As far as I can tell, that model is not correct.  Here's an example.  Say I have:

  <iframe src="http://example.com" allow="fullscreen">

This will create a policy which has, for "fullscreen", mPrincipal == "http://example.com" and mIsSrc == true, right?

We will do the load, the document's principal is not a nullprincipal, we don't do any stripping.

Then a data: URL gets loaded in the iframe (due to a link in our document targeting the iframe, or a link in http://example.com, whatever).  This load has a nullprincipal, so it will end up matching the policy, because mIsSrc is true.  It'll then strip the mIsSrc thing out of the policy so no other loads match.  And then if you click another link that loads another "http://example.com" page in the iframe it will _not_ match the 'src' policies, which doesn't look right.

Basically, this is implementing "the first nullprincipal load that happens after we set up the inherited policies gets to match all the 'src' policies, and nothing after that matches them", which is not the behavior we want.  We only want nullprincipal loads that come from processing iframe attributes to match.  You can get access to that information via the isFromProcessingFrameAttributes attribute on the loadinfo now that bug 1487964 has landed.

If we're guaranteed that the thing on the window has been changed/updated any time we're doing such a load, then maybe just guarding on that isFromProcessingFrameAttributes thing for both matching and stripping would be good enough.  I'm somewhat worried about the assumptions here in terms of timing that I am having a hard time modeling in my head, though...  The action at a distance whereby mIsSrc makes all nullprincipals match is really worrying to me.  What happens if the nullprincipal subframe has another nullprincipal child frame?  What does the inherited policy for that look like?  Is the mIsSrc boolean inherited?
Flags: needinfo?(bzbarsky)
One other thing worth thinking about.  Say a page has:

  <iframe src="http://foo.com" allow="fullscreen" sandbox>

and http://foo.com redirects to http://bar.com.  If this were not sandboxed, the resulting page would not be allowed to do fullscreen stuff.  I would think that should still be true even with it sandboxed, right?
Flags: needinfo?(bzbarsky)
Comment on attachment 9027909 [details] [diff] [review]
part 3 - grant 'src' directives at the first frame load of a opaque-origin

> Bug 1508123 - Consider 'src' in allowList as the current origin in FeaturePolicy for opaque-origin at the first frame load, r?bz

This isn't quite what the patch is doing.  Needs to be resummarized?

>+++ b/dom/base/nsDocument.cpp
>+    bool allowSrcAllowList =

This doesn't seem to be about "allowing" src.  It's about forcing 'src' to match, right?

This should be called 'forceSrcToMatch' or something like that, and similar throughout this patch.

>+++ b/dom/base/nsGlobalWindowInner.cpp
>+#include "mozilla/dom/FeaturePolicy.h"

Given there are no other changes to this file in this changeset, should this be in some other changeset?

>+++ b/dom/security/featurepolicy/FeaturePolicy.h
>+  InheritPolicy(FeaturePolicy* aParentFeaturePolicy,
>+                bool aAllowSrcInAllowList);

Please document the boolean arg, here and in all other places in the headers.

There should probably be a comment somewhere, either where we force-allow by checking mIsSrc or at the point where the force-allow boolean is computed, explaining why we want to do the force-allow thing and why it's safe to do the force-allow.

r=me
Flags: needinfo?(bzbarsky)
Attachment #9027909 - Flags: review+
You need to log in before you can comment on or make changes to this bug.