Closed Bug 1329764 Opened 7 years ago Closed 7 years ago

Support relaxing the same origin policy for WebAuthn

Categories

(Core :: DOM: Device Interfaces, enhancement, P1)

Product:
Core
Component:
DOM: Device Interfaces
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: jcj, Assigned: jcj)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(2 files)

Bug 1329764 - WebAuthn's RP IDs must be domain strings
59 bytes, text/x-review-board-request
keeler
: review+
Details
Bug 1329764 - Call IsRegistrableDomainSuffixOfOrEqualTo for WebAuthn
59 bytes, text/x-review-board-request
keeler
: review+
Details 
Web Authentication's specification says that during processing, the procedure used by HTML5 when setting the "document.domain" attribute should be used for a WebAuthn purpose. This is somewhat ill-defined at present, though I have two outstanding spec pull requests that define it in different ways [1][2]. As of present time, though, it's not resolved.

This bug is to implement whatever the spec decides on. I had one possibility [3] in an earlier bug, which I pulled out when the ambiguities became more clear.

[1] https://github.com/w3c/webauthn/pull/314
[2] https://github.com/w3c/webauthn/pull/319
[3] https://reviewboard.mozilla.org/r/98942/diff/#index_header
(In reply to J.C. Jones [:jcj] from comment #0)
> Web Authentication's specification says that during processing, the
> procedure used by HTML5 when setting the "document.domain" attribute should
> be used for a WebAuthn purpose.

Do we have to? We would like sites to stop using document.domain. The Secure Contexts spec tried to provide a push factor to push sites away from using it by saying that secure contexts and document.domain use were mutually exclusive[1]. Unfortunately that requirement had to be dropped since it was essentially incompatible with the Web. However, in the case of WebAuthn there is no existing use of the web, so it seems like it would be fine to exclude the use of the API if document.domain is touched.

CC'ing Bobby and Anne for their input.

[1] https://github.com/w3c/webappsec-secure-contexts/issues/10
Flags: needinfo?(bobbyholley)
We should absolutely not be building any support for document.domain (or any analogous machinery) into new specs. It mostly breaks the security model of the web, and vendors have gone to great lengths to reduce document.domain support to the bare minimum required for web-compat.
Flags: needinfo?(bobbyholley)
Jonathan, Bobby: Could you comment on the WebAuthn spec? Issue #256 [1] (and its associated PRs [2]) is the driver of this particular bug.

More specifically though, it appears that we could adjust the extracted algorithm in [2] to fail/throw if document.domain has been set to resolve the concern? That would simplify some things, certainly. Still, the "relaxing the same origin policy" algorithm at deciding for the purposes of WebAuthn whether a given URI (rpId) is able to share a scoped credential with another URI is I think still part of the spec. If we think that should be removed entirely (in favor of postMessage, I guess?), that deserves its own issue at WebAuthn.

Thanks!

[1] https://github.com/w3c/webauthn/issues/256
[2] https://github.com/w3c/webauthn/pull/314
Flags: needinfo?(jwatt)
I'm going to punt this to Bobby since he's been more involved in the effort to deprecate document.domain and can probably expand on the arguments better than I can. :)
Flags: needinfo?(jwatt) → needinfo?(bobbyholley)
And I will punt to Anne, because I don't have cycles to get involved in a spec discussion here. :-)
Flags: needinfo?(bobbyholley) → needinfo?(annevk)
Having looked at this again, the usage of document.domain here appears to be a distraction since it's not actually what they need and will be refactored per comment 0. However, it's not clear what they actually do need since the return value of the document.domain abuse is used in unclear ways. I filed https://github.com/w3c/webauthn/issues/320 on that.

J.C. Jones, you describing rpId as a URL is not helping, since document.domain only deals in hosts...
Flags: needinfo?(annevk)
Anne: Sorry for my sloppy use of language! I'm going to be opening an issue at the HTML spec to request refactoring relevant parts of the document.domain setter into an algorithm WebAuthn would be able to call. I'll refer to the PR https://github.com/w3c/webauthn/pull/314 as a suggestion of where we were going with it.

Bobby, Jonathan, Anne: Thanks for the review. :)
(In reply to J.C. Jones [:jcj] from comment #7)
> Bobby, Jonathan, Anne: Thanks for the review. :)

No problem - thanks for steering this stuff to a better place!
Depends on: 1342258
Comment on attachment 8884113 [details]
Bug 1329764 - WebAuthn's RP IDs must be domain strings

https://reviewboard.mozilla.org/r/155036/#review160366

Looks good in general - I have some clarifying questions, though.

::: dom/webauthn/WebAuthnManager.cpp:144
(Diff revision 1)
>  
>    return NS_OK;
>  }
>  
>  nsresult
> +OriginToAsciiHost(nsPIDOMWindowInner* aParent,

Maybe we should just update GetOrigin to return both the origin and host?

::: dom/webauthn/WebAuthnManager.cpp:151
(Diff revision 1)
> +                  /* out */ nsACString& aHost)
> +{
> +  MOZ_ASSERT(aParent);
> +  nsCOMPtr<nsIDocument> doc = aParent->GetDoc();
> +  MOZ_ASSERT(doc);
> +  nsIPrincipal* principal = doc->NodePrincipal();

nsCOMPtr<nsIPrincipal> ?

::: dom/webauthn/WebAuthnManager.cpp:161
(Diff revision 1)
> +  }
> +  nsCOMPtr<nsIURI> uri;
> +  if (NS_FAILED(originUri->Clone(getter_AddRefs(uri)))) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  if (NS_FAILED(uri->SetSpec(NS_ConvertUTF16toUTF8(aInput)))) {

Why is setting the spec necessary?

::: dom/webauthn/WebAuthnManager.cpp:165
(Diff revision 1)
> +  }
> +  if (NS_FAILED(uri->SetSpec(NS_ConvertUTF16toUTF8(aInput)))) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  nsAutoCString originHost;
> +  if (NS_FAILED(uri->GetAsciiHost(originHost))) {

Seems like we could just pass in aHost here.

::: dom/webauthn/tests/test_webauthn_loopback.html:93
(Diff revision 1)
>      let signatureValue = aAssertion.response.signature.slice(5);
>  
>      let rpIdHash = aAssertion.response.authenticatorData.slice(0,32);
>  
>      // Assemble the signed data and verify the signature
> -    return deriveAppAndChallengeParam(clientData.origin, aAssertion.response.clientDataJSON)
> +    return deriveAppAndChallengeParam(document.domain, aAssertion.response.clientDataJSON)

To be consistent with line 80, shouldn't this be window.location.host?
Attachment #8884113 - Flags: review?(dkeeler)
Comment on attachment 8884114 [details]
Bug 1329764 - Call IsRegistrableDomainSuffixOfOrEqualTo for WebAuthn

https://reviewboard.mozilla.org/r/155038/#review160368

LGTM.

::: dom/webauthn/WebAuthnManager.cpp:181
(Diff revision 1)
>                  /* out */ nsACString& aRelaxedRpId)
>  {
>    MOZ_ASSERT(aParent);
> +  nsCOMPtr<nsIDocument> doc = aParent->GetDoc();
> +  MOZ_ASSERT(doc);
> +  nsIPrincipal* principal = doc->NodePrincipal();

I'm seeing a mix of bare pointer and nsCOMPtr<nsIPrincipal> for similar situations, so I'm not actually sure what's correct here.
Attachment #8884114 - Flags: review?(dkeeler) → review+
Comment on attachment 8884113 [details]
Bug 1329764 - WebAuthn's RP IDs must be domain strings

https://reviewboard.mozilla.org/r/155036/#review160366

Thanks for the excellent questions and suggestions!

> nsCOMPtr<nsIPrincipal> ?

I don't know why this is normally a raw pointer, but I keep finding instances of it done this way around the code. Probably to avoid the refcounting cost for something that is owned by the refcounted `doc`. Anyway, this is not performance-critical code, so I've changed it to use the nsCOMPtr.

> Why is setting the spec necessary?

In this version of the code, it is the way to get the arbietrary input into the URL. However, per your later comment I refactored this to always come from the origin (which is all we need), so all fixed!

> Seems like we could just pass in aHost here.

Great idea. That makes this much cleaner, thank you.

> To be consistent with line 80, shouldn't this be window.location.host?

Yes, you're right certainly. Thanks!
Comment on attachment 8884114 [details]
Bug 1329764 - Call IsRegistrableDomainSuffixOfOrEqualTo for WebAuthn

https://reviewboard.mozilla.org/r/155038/#review160368

> I'm seeing a mix of bare pointer and nsCOMPtr<nsIPrincipal> for similar situations, so I'm not actually sure what's correct here.

I changed it here, too - I think (per other review) that it's a performance thing not relevant to us.
Comment on attachment 8884113 [details]
Bug 1329764 - WebAuthn's RP IDs must be domain strings

https://reviewboard.mozilla.org/r/155036/#review160382

Cool - lgtm.
Attachment #8884113 - Flags: review?(dkeeler) → review+
Thanks again for the reviews, Keeler! Try run looks OK [1]. Marking checkin-needed.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=88b81655e8be
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e173fd86d931
WebAuthn's RP IDs must be domain strings r=keeler
https://hg.mozilla.org/integration/autoland/rev/1379551e5fae
Call IsRegistrableDomainSuffixOfOrEqualTo for WebAuthn r=keeler
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e173fd86d931
https://hg.mozilla.org/mozilla-central/rev/1379551e5fae
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: