Security hole in payment API when a constructor from a no longer active document is invoked

[bzbarsky]

Step 3.3 of the payment request constructor now says:

If any ancestor browsing context of context has an active document with an origin that is not the same as origin and context's browsing context container's node document is not allowed to use the feature indicated by attribute name allowpaymentrequest, then throw a SecurityError.

This seems wrong in the case when the constructor doesn't come from the active document. In particular it seems to me that this allows the following attack:

1. Site A opens a new window and navigates it to a page on site B that has an iframe.
2. Site A navigates that iframe to a URL from site A.
3. Site A grabs the PaymentRequest constructor from the document currently in the iframe.
4. Site A navigates the iframe again, to a URL from site B.
5. Site A invokes the constructor it grabbed.

[bzbarsky]

Or alternately B could window.open another window from B, then grab a reference to the constructor, from that window or one of its subframes, then navigate to A, then call the constructor.

What you may want to do is restrict this API to fully active documents only. But even then, walking up the creator document chain (but only until you get to a document in a toplevel browsing context) makes more sense than walking up the ancestor browsing context chain.

[rsolomakhin]

Can you submit a pull request with your suggestion, please? Also would be nice to have a test case for implementers.

[bzbarsky]

@rsolomakhin No, I cannot. I have way too many other things on my plate to start editing this spec. Even just taking the time to look for issues in it was a significant problem time-wise. I expect the editors to actually do their job and address issues that are raised, instead of asking the issue filers to also take on fixing them.

[zcorpan]

@rsolomakhin I can volunteer to work on test cases and pull request for this and related issues.

@bzbarsky would it not fail at step 5 for WebIDL security checks that we have in place in general? At least Gecko and Chromium throw for alert and WebSocket:

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4719
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4720

https://heycam.github.io/webidl/#dfn-perform-a-security-check
https://html.spec.whatwg.org/#integration-with-idl step 2

However I can't find where in WebIDL it says to "perform a security check" for constructors, it seems it only applies to operations and attributes. Is each constructor algorithm expected to do a security check, or is this a bug in WebIDL (or am I missing something)?

[bzbarsky]

The WebSocket testcase isn't quite right. You want this one: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4721

Note that that one does not throw in Chrome. It does throw in Gecko, but a non-spec exception, because per websocket spec this situation should not throw for websocket. Maybe that's a flaw in the websocket spec; hard to tell. I would think there is, because nothing obviously shuts down that websocket connection if it gets established! I filed whatwg/websockets#22 on this.

But note that websocket doesn't seem to rely in any way on the active document of the browsing context of its global or anything like that, and there's no real spoofing risk there either. So at least it's not a security issue.

However I can't find where in WebIDL it says to "perform a security check" for constructors

Perform a security check against what object, exactly?

Is each constructor algorithm expected to do a security check

I don't think it's a matter of security checks at all. It's a matter of active document checks. And yes, constructors should perform those themselves, because some constructors may be perfectly fine to call in inactive documents, if we think it's ok to do anything at all in inactive documents.

[zcorpan]

Active document check:
whatwg/html#2160

Always invoking "allowed to use":
#383

Tests:
web-platform-tests/wpt#4309

[zcorpan]

What you may want to do is restrict this API to fully active documents only.

This should be done with my proposed changes.

But even then, walking up the creator document chain (but only until you get to a document in a toplevel browsing context) makes more sense than walking up the ancestor browsing context chain.

Are you referring here to the model of snapshotting the attribute at document creation time? If not, I do not follow.

[bzbarsky]

Are you referring here to the model of snapshotting the attribute at document creation time?

No, I'm referring to walking up through the node document of the iframe element you're nested through, as opposed to walking up to the parent browsing context and then trying to recover a document. This part was addressed by switching to the HTML "allowed to use" concept, which already does this properly.

[zcorpan]

Ah, I see.