J.C. Jones [:jcj] (he/him) Assignee
	Description • 9 years ago

The U2F API should be protected by the WebIDL annotation [SecureContext] from Bug 1177957.

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 1 • 8 years ago

Use the [SecureContext] webidl notation to hide the powerful "window.u2f"
feature when not loaded in a secure context.

Review commit: https://reviewboard.mozilla.org/r/55052/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55052/

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 2 • 8 years ago

The trivial patch for this is insufficient, as setting the [SecureContext] WebIDL feature breaks the standard mechanism for executing secure Mochitests. The core method, isSecureContext(), marks as insecure any page loaded from a context that originated from an insecure parent. 

Marking blocked on Bug 1274315.

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 3 • 8 years ago

It looks like this is doable by setting the pref `"dom.securecontext.whitelist"` to cover the mochi.test:8888 host rather than waiting on Bug 1274315.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 4 • 7 years ago

It looks like this has tests running under https now, except for the unsecured appid facet test. Is it worth adding SecureContext on this yet, or are we just gonna deprecate this anyways since WebAuthn shipped?

	Tim Taubert [:ttaubert] (inactive)
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8756254 [details]
Bug 1247124 - Limit FIDO U2F to Secure Contexts

https://reviewboard.mozilla.org/r/55052/#review213440

Cool.

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 7 • 7 years ago

You're right, we have it basically ready now Kyle with the test refactoring, and with a minor tweak it works fine. 

Smaug - There's no WebIDL for this, but it's a powerful feature.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8756254 [details]
Bug 1247124 - Limit FIDO U2F to Secure Contexts

https://reviewboard.mozilla.org/r/55052/#review213520

::: dom/webidl/U2F.webidl:14
(Diff revision 2)
>   * https://www.fidoalliance.org/specs/fido-u2f-v1.1-id-20160915/fido-u2f-javascript-api-v1.1-id-20160915.html
>   */
>  
>  [NoInterfaceObject]
>  interface GlobalU2F {
> -  [Throws, Pref="security.webauth.u2f"]
> +  [SecureContext, Throws, Pref="security.webauth.u2f"]

Why is this enough?..

::: dom/webidl/U2F.webidl:71
(Diff revision 2)
>  
>  callback U2FRegisterCallback = void(RegisterResponse response);
>  callback U2FSignCallback = void(SignResponse response);
>  
>  [Pref="security.webauth.u2f"]
>  interface U2F {

...shouldn't also this and possibly some other related interfaces, if there are such, be limited to SecureContext

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 7 years ago

I guess there should be a test that '"U2F" in window' is false in non-SecureContext. 

(I'm not sure which spec to look at here)

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 10 • 7 years ago
mozreview-review-reply

Comment on attachment 8756254 [details]
Bug 1247124 - Limit FIDO U2F to Secure Contexts

https://reviewboard.mozilla.org/r/55052/#review213520

Re: the test, `dom/u2f/tests/test_appid_facet_insecure.html` does check that accessing `window.u2f` from insecure context throws, so I think that's covered.

Thanks, Olli!

> Why is this enough?..

Tim and I took a look at WebCrypto's SecureContext patch-in-progress and it looks like we should also do this for all Interfaces, which is just the one. Added; thanks!

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 7 years ago

(In reply to J.C. Jones [:jcj] from comment #10)
> Comment on attachment 8756254 [details]
> Bug 1247124 - Limit FIDO U2F to Secure Contexts
> 
> https://reviewboard.mozilla.org/r/55052/#review213520
> 
> Re: the test, `dom/u2f/tests/test_appid_facet_insecure.html` does check that
> accessing `window.u2f` from insecure context throws, so I think that's
> covered.
Testing that window.u2f throws (why should it throw?) is different from testing whether there is
interface object for U2F in the scope.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8756254 [details]
Bug 1247124 - Limit FIDO U2F to Secure Contexts

https://reviewboard.mozilla.org/r/55052/#review213574

Of course I wish there was a spec for this stuff.

::: dom/u2f/tests/frame_appid_facet_insecure.html:20
(Diff revision 3)
>    var version = "U2F_V2";
>    var challenge = new Uint8Array(16);
>  
>    local_is(window.location.origin, "http://test2.example.com", "Is loaded correctly");
>  
> -  await promiseU2FRegister(null, [{
> +  local_is('u2f' in window, false, "window.u2f must be undefined when accessed from an insecure origin");

FWIW, also U2F should be false.
u2f refers to the getter in window object, U2F to the interface object.

	Pulsebot
	Comment 15 • 7 years ago

Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c3feee4dfd2
Limit FIDO U2F to Secure Contexts r=ttaubert,smaug

	Andrei Ciure[:aciure]
	Comment 16 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4c3feee4dfd2