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

Iframe sandboxing options for gUM #268

Closed
martinthomson opened this issue Oct 29, 2015 · 80 comments
Closed

Iframe sandboxing options for gUM #268

martinthomson opened this issue Oct 29, 2015 · 80 comments

Comments

@martinthomson
Copy link
Member

The fullscreen API has an option for sites to allow iframes to request fullscreen. We should discuss having that sort of option and what the default would be (allow or deny).

n.b., fullscreen prevents use of the API in an iframe by default

@martinthomson
Copy link
Member Author

A simple option here would be to add an "allow-usermedia" tag to the iframe sandbox attribute. That would make this available by default (less breakage), but make it possible to restrict what is possible in the sandbox.

@stefhak
Copy link
Contributor

stefhak commented Oct 29, 2015

I like that proposal, I think the ability for the site to disallow iframes to access camera and microphone is important.

@alvestrand
Copy link
Contributor

Would this mean that "allow-usermedia=no" would disallow iframes' access, while specifying "allow-usermedia=yes" or leaving it off would both allow it?

@adam-be
Copy link
Member

adam-be commented Nov 19, 2015

@alvestrand, from what I've seen it's only the presence of the token, no corresponding value, that activates the feature. But shouldn't it be "disallow-usermedia" to get the behaviour with default on?

@stefhak
Copy link
Contributor

stefhak commented Nov 25, 2015

@martinthomson any comment to the above?

@martinthomson
Copy link
Member Author

I don't know enough about how people use this to know whether to default to on or off. I think that @adam-be is right about the label form, but the fact that there are no other labels of the form "disallow-*", we would need to be very careful about adding that.

@adam-be
Copy link
Member

adam-be commented Dec 2, 2015

I think the main question here is what the default should be. Then we can figure out how to enforce that; "allow-usermedia" or something else.

As @martinthomson wrote above, default on means less breakage. But it seems that most (read as all) iframe sandbox features are default off and the script opts in.

@dontcallmedom
Copy link
Member

there are two types of sandboxing:

The first one has for effect that by default, you get an iframe that has many features cut-off, and get specific features re-enabled via the keywords allow-foo and allow-bar in my example. The currently recognized features are: allow-forms, allow-modals, allow-pointer-lock, allow-popups, allow-popups-to-escape-sandbox, allow-same-origin, allow-scripts, and allow-top-navigation.

The second one has only been defined for fullscreen at the moment; in that model, fullscreen is disabled by default in any iframe, and can only be enabled specifically by adding that attribute.

While I think it would be useful to think about both getUserMedia and WebRTC impact on the sandbox attribute, I think in this particular case, we're really thinking about the second model, and whether to take the lenient backwards-compatible appraoch (which would require a disallowusermedia attribute) or to take the more stringent likely not bw-compatible approach, with an allowusermedia attribute.

I personally think the latter is cleaner, but we would need visibility on the deployment reality to determine if that's still an option.

@alvestrand
Copy link
Contributor

Den 02. des. 2015 16:10, skrev Dominique Hazael-Massieux:

there are two types of sandboxing:

The first one has for effect that by default, you get an iframe that has
many features cut-off, and get specific features re-enabled via the
keywords |allow-foo| and |allow-bar| in my example. The currently
recognized features are: |allow-forms, allow-modals, allow-pointer-lock,
allow-popups, allow-popups-to-escape-sandbox, allow-same-origin,
allow-scripts, and allow-top-navigation|.

So if you don't have a sandbox attribute, modals (for instance) are
allowed, but if you have a sandbox attribute with an empty value, modals
are disallowed?

The second one has only been defined for fullscreen at the moment; in
that model, fullscreen is disabled by default in any iframe, and can
only be enabled specifically by adding that attribute.

While I think it would be useful to think about both getUserMedia and
WebRTC impact on the sandbox attribute, I think in this particular case,
we're really thinking about the second model, and whether to take the
lenient backwards-compatible appraoch (which would require a
|disallowusermedia| attribute) or to take the more stringent likely not
bw-compatible approach, with an |allowusermedia| attribute.

I personally think the latter is cleaner, but we would need visibility
on the deployment reality to determine if that's still an option.

Seems that people who use the sandbox attribute would care about
restricting the capabilities of the iframe, so would be happy (?) to see
usermedia starting out as default off, while people who don't use it
would perhaps want the default (with no sandbox attribute) to be the
status quo - that it's allowed on.

Would that be the best of all possible worlds?

@dontcallmedom
Copy link
Member

So if you don't have a sandbox attribute, modals (for instance) are allowed, but if you have a sandbox attribute with an empty value, modals are disallowed?

Yes.

Seems that people who use the sandbox attribute would care about restricting the capabilities of the iframe, so would be happy (?) to see usermedia starting out as default off, while people who don't use it would perhaps want the default (with no sandbox attribute) to be the status quo - that it's allowed on. Would that be the best of all possible worlds?

I think making getUserMedia (and likely, WebRTC) disabled by default and reenabled by opt-in in a sandboxed iframe would make sense. But I don't think it's sufficient or a "best of all possible" solution.

The problem with <iframe sandbox> is that it forces you into a very constrained iframe, and the list of constraints on this type of iframes can grow over time, so you have to keep adding "allow-foo" keywords if you were only interested in disabling a specific feature.

My understanding of the case we're trying to address is to make getUserMedia an independent toggle; i.e. an iframe could have or not have access to getUserMedia independently of whether that iframe is sandboxed.

Looking at at the usual suspects for iframes, ads, I think most developers would NOT want ads to have access to getUserMedia, and most legitimates ads should NOT need it, but I don't think most developers would want or would be allowed to serve ads from <iframe sandbox>.

There seemed to have been a long discussion in webappsec on the topic back in February; that's probably exploring as well (or maybe more simply, get some input from webappsec on this?)

@adam-be
Copy link
Member

adam-be commented Dec 3, 2015

Seems that people who use the sandbox attribute would care about restricting the capabilities of the iframe, so would be happy (?) to see usermedia starting out as default off, while people who don't use it would perhaps want the default (with no sandbox attribute) to be the status quo - that it's allowed on. Would that be the best of all possible worlds?

I think making getUserMedia (and likely, WebRTC) disabled by default and reenabled by opt-in in a sandboxed iframe would make sense. But I don't think it's sufficient or a "best of all possible" solution.

From @alvestrand's description above about people that use the sandbox attribute and those who don't, is seems that the following would fit.

  • no sandbox attribute: enabled (works as before)
  • sandbox attribute without "allow-usermedia": disabled
  • sandbox attribute with "allow-usermedia": enabled

@dontcallmedom
Copy link
Member

It would fit the toggle/untoggle usermedia, but it would not fit the atomic aspect of it: the only way you can disable getUserMedia in iframe then requires to use the sandboxed model of iframes, which is way more restrictive than e.g. most ads providers are likely to accept.

@adam-be
Copy link
Member

adam-be commented Dec 3, 2015

Taking the rest of @dontcallmedom's input into consideration; especially (correct me if I've got this wrong Dom):

  • Add creators don't want to have their ads in sandboxed iframes (a problem for the site owner since the advertisers won't pay for premium in this case [1])
  • User media default on means that ads (in iframes) on existing pages, that aren't updated with restrictions, can start using it in the future

I think we're best of with <iframe allowusermedia> (= default off)

[1] https://lists.w3.org/Archives/Public/public-webappsec/2015Feb/thread.html#msg225

@alvestrand
Copy link
Contributor

We should take this to the list .... I can't find an example of an ad that's presented in an iframe. It's been touted as one of the main reasons for sandboxing, but I can't find a live example.

@adam-be
Copy link
Member

adam-be commented Dec 3, 2015

Let's continue on the list to make this discussion more visible.

https://lists.w3.org/Archives/Public/public-media-capture/2015Dec/0008.html

@stefhak
Copy link
Contributor

stefhak commented Dec 3, 2015

@alvestrand found a few iframe ads at http://www.idg.se/

@alvestrand
Copy link
Contributor

Adam is working on a PR for this - based on an "allowusermedia" IDL and content attribute.

@adam-be
Copy link
Member

adam-be commented Feb 4, 2016

Proposed fix #313

alvestrand added a commit that referenced this issue Feb 18, 2016
Extend iframe with a new allowusermedia attribute (issue: #268)
@stefhak
Copy link
Contributor

stefhak commented Feb 18, 2016

Closed, #313 addressed this one.

@foolip
Copy link
Member

foolip commented Jul 5, 2016

This came up in whatwg/html#1492.

Per some ad-hoc testing in whatwg/html#1492 (comment) it looks like getUserMedia() is currently allowed in iframes, and it's quite possible that it's too late to change that. Does anyone have any data on the prevalence of getUserMedia() requests in iframes, to get an idea about the risk?

@annevk
Copy link
Member

annevk commented Aug 17, 2016

I suspect something like feature-policy is the way to go, but that will take some time to reach consensus across browsers and community, settle on syntax, figure out edge cases, etc. (And that too will need to be integrated with HTML, which I guess brings us back to my earlier comment. 😊)

@alvestrand
Copy link
Contributor

We seem to have a flag in the WHATWG version of HTML (allowusermedia). The policy feature work will also set or unset the flag. We have specified the handle they're going to manipulate.
Nothing more to be done here.

@annevk
Copy link
Member

annevk commented Nov 16, 2016

@alvestrand I'm not sure what you mean by "seem to have a flag". I don't think that's true. There's an algorithm if you want to use an attribute. And if you want to use that then you need to upstream that attribute.

I guess you wouldn't use the permission delegation then as discussed upthread.

@raymeskhoury
Copy link

I think @alvestrand is referring to: https://html.spec.whatwg.org/multipage/embedded-content.html#the-iframe-element:attr-iframe-allowusermedia

@alvestrand just a heads-up that we're currently working on adding allowusermedia as a Feature Policy flag as well.

@alvestrand
Copy link
Contributor

@raymeskhoury Yes, that's the one I was thinking of. I certainly hope you end up with a consistent set of feature identifiers in all those places.

@annevk
Copy link
Member

annevk commented Nov 21, 2016

That is all fine and well, but that does not work without https://w3c.github.io/mediacapture-main/ actually invoking the "allowed to use" algorithm defined there.

@alvestrand
Copy link
Contributor

@annevk can you file a bug on getusermedia telling us what to replace the text "If the current [HTML51] Document's user media enabled flag is unset, return a promise rejected with a DOMException object whose name attribute has the value SecurityError." with?

@annevk
Copy link
Member

annevk commented Nov 21, 2016

Isn't this that bug?

@alvestrand
Copy link
Contributor

@annevk I thought we had solved it, and you're telling me the solution is wrong. Thought it would be simpler to open a new bug, but since you ask...

Reopened. Can you suggest how we should phrase it?

@alvestrand alvestrand reopened this Nov 21, 2016
@annevk
Copy link
Member

annevk commented Nov 21, 2016

I think you need to make the following changes.

  1. Remove "All Documents have a user media enabled flag. This specification does not detail how or when this flag is set."
  2. You need to rephrase "If the current [HTML51] Document's user media enabled flag is unset" somehow. I don't understand what "current Document" means. But once you have a handle on a document, you want to say something like "If document is allowed to use the feature indicated by attribute name allowusermedia, then ..."

See https://fullscreen.spec.whatwg.org/#fullscreen-element-ready-check for an example.

@stefhak
Copy link
Contributor

stefhak commented Jan 11, 2017

@annevk I've started to draft a PR to do what you propose in #268 (comment), and have one question. In the fullscreen element ready check there is a check whether the allowfullscreen attribute is set, but should there not be some kind of chain here? I mean if an iframe without allowfullscreen creates an iframe with allowfullscreen, should the later really be allowed to use full screen?

@annevk
Copy link
Member

annevk commented Jan 11, 2017

The allowed to use algorithm does the recursion, no?

Now, there are some changes underway to rather than have a dynamic check, make a snapshot when the browsing context is created. @zcorpan is leading that in whatwg/html#2195 and whatwg/html#2217.

If allowusermedia is not implemented yet, it should be snapshotted like we will do for payments and not be dynamic like we do for fullscreen. @zcorpan can update the PRs to account for that if indeed it's not implemented yet.

Hope this helps.

@stefhak
Copy link
Contributor

stefhak commented Jan 11, 2017

Thanks. allowed to use algorithm does the recursion indeed.
I personally agree allowusermedia should be snapshotted.

@stefhak
Copy link
Contributor

stefhak commented Jan 19, 2017

Can we close this one now that #427 is merged?

@alvestrand
Copy link
Contributor

I think so. Closing.

@annevk
Copy link
Member

annevk commented Jan 19, 2017

Yeah, seems pretty confusing to reference conflicting HTML standards, but if that's how you want to make progress, so be it I guess.

@alvestrand
Copy link
Contributor

Yeah, moving all the pointers to point to WHATWG is not an option, so conflicting pointers it is.

@stefhak
Copy link
Contributor

stefhak commented Jan 20, 2017

@annevk regarding #268 (comment): of course any advice is welcome. https://w3c.github.io/browser-payment-api/#dependencies says html51 defines "allowed to use" and "allowpaymentrequest"; however I can't find them being defined in https://www.w3.org/TR/html51/ (though I must admit I find it hard search in that document) and trying to look into the latest editors draft takes me to html5.2 (and it is equally hard to search in that doc).

@annevk
Copy link
Member

annevk commented Jan 20, 2017

My advice is not reference that document as it repeatedly causes confusion for those implementing (who need to ignore it). See https://annevankesteren.nl/2016/01/film-at-11 for more detailed rationale.

@stefhak
Copy link
Contributor

stefhak commented Jan 23, 2017

Thanks (though I'm pretty sure we're supposed to be referencing W3C specs when we can).

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