Closed Bug 1247124 Opened 4 years ago Closed 2 years ago

Use [SecureContext] for FIDO U2F JS API

Categories

(Core :: DOM: Device Interfaces, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jcj, Assigned: jcj)

References

Details

Attachments

(1 file)

The U2F API should be protected by the WebIDL annotation [SecureContext] from Bug 1177957.
Depends on: 1231681, 1177957
Depends on: 1162772
Component: DOM: Security → DOM: Device Interfaces
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/
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.
Depends on: 1274315
Assignee: nobody → jjones
Status: NEW → ASSIGNED
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.
Priority: -- → P3
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?
Flags: needinfo?(jjones)
Comment on attachment 8756254 [details]
Bug 1247124 - Limit FIDO U2F to Secure Contexts

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

Cool.
Attachment #8756254 - Flags: review?(ttaubert) → review+
Attachment #8756254 - Flags: review?(bugs)
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.
Flags: needinfo?(jjones)
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
Attachment #8756254 - Flags: review?(bugs) → review-
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)
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!
(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.
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.
Attachment #8756254 - Flags: review?(bugs) → review+
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c3feee4dfd2
Limit FIDO U2F to Secure Contexts r=ttaubert,smaug
https://hg.mozilla.org/mozilla-central/rev/4c3feee4dfd2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.