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

setPointerCapture should be disabled in sandboxed iframes by default #16

Closed
RByers opened this issue Jun 22, 2015 · 19 comments
Closed

setPointerCapture should be disabled in sandboxed iframes by default #16

RByers opened this issue Jun 22, 2015 · 19 comments
Assignees
Labels
question security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response. v3

Comments

@RByers
Copy link
Contributor

RByers commented Jun 22, 2015

The algorithm for setPointerCapture says nothing about different iframes.

If, for example, code running in some iframe happens to call setPointerCapture for a pointer ID that would otherwise have its events sent to another frame, then setPointerCapture should presumably fail somehow (eg. to prevent input stealing). How exactly should this fail?

What about more subtle scenarios? Eg. if I mouse down over one frame, can it capture events so that when I drag out of that frame events consider to go to it?

Also, Is there any reason to prevent a child frame from explicitly directing capture to ancestor frame somehow? I've seen sites do this today by delegating all events inside a child into their parent via postMessage (eg. to allow dragging on a window resize handle within the frame to resize the frame).

@RByers
Copy link
Contributor Author

RByers commented Jun 22, 2015

This is related to this discussion about the correct default behavior for Mouse Events

@jacobrossi
Copy link
Member

I suppose the reason the spec doesn't say this is because there isn't any specialized limitations that I'm aware of. This happens to work because:

  1. the API requires it be called while the pointer is down.
  2. the API is called off an element, which means you have to have access to that element to call it. This means the same-origin policy applies.
  3. the target property is fixed for the events once you set capture. This means it can never be a reference to an element, say, from another document than the capture target node. This also means that the event properties always refer to the same document (the target node's doc).

So:

  • Yes, you can capture events while you drag out of one document onto another. But that's OK!
  • Yes, you can delegate the capture to an ancestor frame provided you have access to the API (see # 2).
  • Yes, you can steal input from another frame. This is variant of the above point. Again, SOP applies given # 2.

I think, perhaps, we could clarify that when capture is set then document-relative properties (e.g. pageX, pageY) continue to refer to the owner document of the capture target node. Technically, these properties already say they refer to event.target's doc, but it could be made explicit for clarity.

Do you agree with this behavior? Is there more clarification (normative or non-normative) you think we should add?

@RByers
Copy link
Contributor Author

RByers commented Jun 25, 2015

Thanks, this mostly sounds good to me (I love the lack of special cases here). But I'm worried it's not quite enough to maintain security properties I thought were important (so figured there was probably some special case in implementations here). In particular, if the assignment of pointer IDs is predictable, then this allows an iframe to steal input events for touches that are nowhere near it. So, for example, an ad sandboxed in an iframe could cause any tap/click on the page to open a pop-up of it's choosing. In some cases, the pattern of mouse/touch events themselves may be sensitive - eg. for a site that has a pin-pad for entering your password - an iframe shouldn't have access to those input events, right?

@RByers
Copy link
Contributor Author

RByers commented Apr 25, 2016

The IDs for touch are often small and monotonically increasing in Edge (and it's of course a constant for mouse), so I think there is a legitimate attack possible here (eg. could probably even try calling setPointerCapture on thousands of possible IDs on every RAF tick). @NavidZ do you want to see if you can write a proof-of-concept for this and try it on Edge? Eg. even having some cross-origin iframe attempt to setPointerCapture the mouse ID every RAF. I expect there will be some mitigation in the implementation that isn't captured by the spec.

Here's a couple possible mitigations:

  1. Restrict setPointerCapture to occurring only within the scope of a pointer event handler. Since you can only synchronously communicate between frames when they're the same origin, this should be adequate to eliminate any attack. But this might break some existing legitimate use cases where the decision to capture is made asynchronously.
  2. Make setPointerCapture fail when the last target for the event belongs to a document that is not the same or a descendant of the document being captured to. This would still prevent some potentially useful scenarios, so we could augment this with:
  3. Also allow setPointerCapture into a particular document when the pointer is already captured to an HTMLIframeElement which is the frameElement corresponding to the targeting Element's ownerDocument.

@NavidZ
Copy link
Member

NavidZ commented Apr 25, 2016

In the Edge implementation (I tested with IE11) another frame can indeed steal the input from the outer frame. Particularly in this sample you can see that another iframe is stealing the mouse input (knowing the id is always 1 for mouse or I guess it could have randomly tests thousands ids as you suggested) from the outer frame in IE 11.

http://output.jsbin.com/nadoxi

All you need to do is clicking outside of the iframe. Beside the stealing there are other issues as well like although the click is released the pointer is captured but they are not related to the issue of stealing the input.

@RByers
Copy link
Contributor Author

RByers commented Apr 26, 2016

We discussed this today on the call.

It sounds like there's agreement that at a minimum sandboxed iframes shouldn't permit capture from outside their frame in order to prevent user annoyance, in the same way that they don't permit pointer lock by default.

However it's not clear whether there is real privacy/security risk here in practice. There are a number of reasons this is hard to usefully exploit. Discussion of this is moving to this private issue to permit concrete discussion of potential attack vectors.

@RByers
Copy link
Contributor Author

RByers commented Jun 22, 2016

Summary: it sounds like everyone agrees that the security risk is pretty low but that sandboxed iframes should have some mitigation to prevent user annoyance due to malicious (eg. attention grabbing) iframes. So the outstanding question is, is there a minimal mitigation we can apply that doesn't break any reasonable use case that we should just apply universally rather than need to special case sandboxed iframes. @teddink says he'll talk to more MS folks and follow up with details on the security bug.

@RByers
Copy link
Contributor Author

RByers commented Jul 6, 2016

I'm now convinced that, although we should probably do something here, it's probably not urgent. Likely worst case is user annoyance, and so this only really becomes urgent if frames start abusing this. Removing the v2-blocking label.

@RByers
Copy link
Contributor Author

RByers commented Jan 11, 2017

I think the main thing we need to do here is have setPointerCapture fail by default inside a sandboxed iframe, and have a new allow-pointer-capture option to re-enable it if needed. Filed whatwg/html#2259 for HTML. Then we can add some text (in Level 2 or extension.html for Level 3) similar to that in requestPointerLock.

/cc @dtapuska. chromium implementation bug.

@RByers RByers changed the title setPointerCapture should say something about iframes setPointerCapture should be disabled in sandboxed iframes by default Jan 11, 2017
@scheib
Copy link

scheib commented Jan 11, 2017

I would suggest that element.setPointerCapture(id) should only be possible in response to an event dispatched to that element from pointer#id.

This solves the iframe and even same document issues ensuring that capture is only occurring for elements that are being interacted with. In the rare cases where a developer may need to capture events from an area distant from the control they are conceptually related to then the event handler must be placed at some parent node, e.g. the document.body.

If that suggestion is followed, then I do not think a sandbox attribute for allow-pointer-capture is necessary because the iframe contents will only be able to enter capture when they are interacted with and capture will be released when user interaction ceases (pointer exits the active buttons state).

@RByers
Copy link
Contributor Author

RByers commented Jan 11, 2017

@schieb: thanks for the suggestion. We discussed a much more permissive (but more hacky) form of that restriction in the chromium bug and Microsoft folks were concerned that it would break some legitimate use cases (eg. where one frame postMessage's to another to tell it to capture a pointer). For example, there's a scenario in Google Feedback where you can drag on a resize handle inside the feedback frame itself which delegates to the containing frame to actually resize the iframe container in response to that dragging. That's done today by just postMessaging the pointer position, but would be simpler and more efficient if the inner frame could just tell it's containing frame that it's time to capture the pointer over it's iframe element. But I'm not really convinced that these use cases are valuable enough to justify the complexity of needing a solution for sandboxed iframes.

One idea is to add a metric to Chrome to track what fraction of setPointerCapture calls today would violate a restriction like that. If it's essentially zero, then there shouldn't be much harm in having such a restriction for now, and exploring lifting it only if we get reports of concrete use cases people want to enable but can't.

Thoughts?

@scheib
Copy link

scheib commented Jan 11, 2017

I do think the cross frame capturing will be a concern (certainly spammy advertisements would love to be obnoxious to containing frames, there's ample evidence of them doing anything possible). So I support closing that.

Using the sandbox tag is a solution, but seems heavier weight than necessary, and won't solve non-sandboxed iframes. It passes complexity to the web app developers and in some cases would be easily overlooked (it's opt in).

I continue to think the "only setPointerCapture from an event to that element" is much cleaner, deals with the issues without needing web developer involvement. I think the rare cases capture is needed to work around that are still possible, e.g. by the methods I and you just cited. A search for "resize iframe interactively" found a few hits and demos (both simple html5 and jquery), so I don't think your last example is a reason to complicate capture.

However, if folks did want to pursue the sandbox with allow-pointer-capture, I don't see other reasons not to.

@patrickhlauke
Copy link
Member

@RByers (mainly because you were so active on this, I know it's not directly your area anymore), @smaug---- @NavidZ any thoughts on this? any change required at the PE spec end?

@RByers
Copy link
Contributor Author

RByers commented Feb 15, 2018

I think others convinced me it wasn't as serious of an issue as I feared. I'd be OK calling this WontFix unless/until we see someone actually exploiting it...

@NavidZ
Copy link
Member

NavidZ commented Feb 15, 2018

I agree.

@RByers
Copy link
Contributor Author

RByers commented Feb 15, 2018

We could also really easily define a feature policy for this now - to at least allow embedders to turn this off. /cc @clelland

@patrickhlauke
Copy link
Member

we'd need to decide if anything needs adding/mentioning (even in a non-normative note) in the spec, or if we want to defer this to future-v3

@plehegar plehegar added the security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response. label Mar 28, 2018
@NavidZ NavidZ self-assigned this Mar 28, 2018
@NavidZ NavidZ added the v3 label Mar 29, 2018
@NavidZ
Copy link
Member

NavidZ commented Mar 29, 2018

I talked again with the folks here and as I said in the call they don't see a security risk here (also mentioned in that private chromium bug with MS people and others). There might be an abuse of the API that may cause some user annoyance but we suggest to keep things as is and if that becomes a thing then we add more restrictions to the API as this is the pattern in the web platform for potential user annoyance from an API. I added future-v3 for now to keep and eye on this issue. I'm happy to discuss this further with other security folks @plehegar.

@patrickhlauke
Copy link
Member

patrickhlauke commented Sep 16, 2020

Discussed in PEWG call today. https://lists.w3.org/Archives/Public/public-pointer-events/2020JulSep/0062.html Closing this, with understanding that as it's in the security tracker, we'll get this flagged again if it's a concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response. v3
Projects
None yet
Development

No branches or pull requests

9 participants