Closed
Bug 1468909
Opened 7 years ago
Closed 7 years ago
Web Authentication - Enforce URL rules in RP ID
Categories
(Core :: DOM: Security, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla62
People
(Reporter: antonio.sanso, Assigned: keeler)
Details
(Keywords: sec-low, Whiteboard: [webauthn] [webauthn-interop][domsecurity-active][post-critsmash-triage][adv-main62-][adv-esr60.2-])
Attachments
(2 files)
6.61 KB,
patch
|
qdot
:
review+
RyanVM
:
approval-mozilla-esr60-
|
Details | Diff | Splinter Review |
6.47 KB,
patch
|
keeler
:
review+
RyanVM
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180605171542
Steps to reproduce:
Firefox 60 support WebAuthn/CTAP1 .
The keys are supposed to be bound to specific domains but Firefox has a small glitch on the logic and ignores the port
Actual results:
You can find a POC in https://www.intothesymmetry.com/
As you can see the Security Key works as a charm even if a different port is used
navigator.credentials.create({
publicKey: {
challenge: new TextEncoder().encode(challenge),
pubKeyCredParams: [{
type: 'public-key',
alg: cose_alg_ECDSA_w_SHA256
}],
rp: {
id: 'www.intothesymmetry.com:5001',
name: 'Nov Sample'
},
user: user
}
}).then(registered);
};
See view-source:https://www.intothesymmetry.com/u2f.js
According to Firefox policy www.intothesymmetry.com:5001 is technically a different domain than www.intothesymmetry.com and should fail
Expected results:
If you try https://www.intothesymmetry.com/
in Chrome there is an Exception
Uncaught (in promise) DOMException: The relying party ID 'www.intothesymmetry.com:5001' is not a registrable domain suffix of, nor equal to 'https://www.intothesymmetry.com'.
Updated•7 years ago
|
Group: firefox-core-security → dom-core-security
Component: Untriaged → DOM: Security
Product: Firefox → Core
Comment 2•7 years ago
|
||
Hmmm - That's correct behavior per the spec's definition of "RP ID" [1]. See the note on the same-origin relaxations and restrictions, quoted below:
Note: A Public key credential's scope is for a Relying Party's origin, with the following restrictions
and relaxations:
* The scheme is always https (i.e., a restriction), and,
* the host may be equal to the Relying Party's origin's effective domain, or it may be equal
to a registrable domain suffix of the Relying Party's origin's effective domain (i.e., an available
relaxation), and,
* all (TCP) ports on that host (i.e., a relaxation).
This is done in order to match the behavior of pervasively deployed ambient credentials (e.g., cookies,
[RFC6265]). Please note that this is a greater relaxation of "same-origin" restrictions than what
document.domain's setter provides.
As-is we explicitly allow this, and while I know we did have tests to verify it, I'm having trouble finding said tests right now. ;) It's in the web platform tests, I'm pretty sure, though. (Not that we have those enabled yet!)
[1] https://w3c.github.io/webauthn/#rp-id
Flags: needinfo?(jjones)
Reporter | ||
Comment 3•7 years ago
|
||
Thanks J.C.
this makes sense.
In the case of Firefox though I think there is a bit too much relaxation e.g. even www.intothesymmetry.com:5001@whatever.com
navigator.credentials.create({
publicKey: {
challenge: new TextEncoder().encode(challenge),
pubKeyCredParams: [{
type: 'public-key',
alg: cose_alg_ECDSA_w_SHA256
}],
rp: {
id: 'www.intothesymmetry.com:5001@whatever.com',
name: 'Nov Sample'
},
user: user
}
}).then(registered);
};
is allowed....
Comment 4•7 years ago
|
||
Antonio: Aha, you're right. Excellent catch!
If you go to https://webauthn.bin.coffee/, look at the bottom of the page for "Advanced" and enter an RP ID of the form:
webauthn.bin.coffee:[zero or more characters of any sort]
or (due to the relaxation to the eTLD+1)
bin.coffee:[zero or more characters of any sort]
and hit "Create Credential".
You'll see in the output box:
"rp": {
"name": "Acme",
"id": "webauthn.bin.coffee:anyway you want it, that's the way you need it, anyway you want it"
},
and a success.
Since this is never displayed to users, I do not believe it is a confusable suitable for phishing. Also, the first part (before the port delimiter ':') is still validated to be either the origin, or a registrable parent domain of the origin, despite what madness happens after the colon, so I don't believe this is a security issue either. It is definitely a correctness issue though!
We'll want to track this for 61. I will attempt to get a patch together for next week before I go on parental leave.
Thanks, Antonio!
Status: UNCONFIRMED → NEW
status-firefox60:
--- → wontfix
status-firefox61:
--- → affected
status-firefox62:
--- → affected
tracking-firefox61:
--- → ?
Ever confirmed: true
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
Summary: Issue in the Same domain policy protection of WebAuthn → Web Authentication - Enforce URL rules in RP ID
Whiteboard: [webauthn] [webauthn-interop]
Comment 5•7 years ago
|
||
The first Fx61 RC is being built today. It's pretty unlikely that we'll be able to take a patch for this in 61.0 at this point, but I'll still leave the door open for a dot release ride-along should the opportunity present itself.
status-firefox-esr52:
--- → unaffected
status-firefox-esr60:
--- → affected
tracking-firefox62:
--- → +
tracking-firefox-esr60:
--- → ?
Comment 6•7 years ago
|
||
I'm now on parental leave, as of last weekend. David, I'm going to assign this to you. I don't know that it needs to get super high priority. Feel free to re-delegate. Thanks!
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Comment 7•7 years ago
|
||
The fix is going to be in WebAuthnManager.cpp's RelaxSameOrigin method, which should clean up aInputRpId before assigning it to aRelaxedRpId. Probably for now just add a Parse call and then re-generate the "host:port" (e.g., a "domain string" [1][2]), making sure that port is a valid port.
[1] https://w3c.github.io/webauthn/#rp-id
[2] https://url.spec.whatwg.org/#host-writing
Comment 8•7 years ago
|
||
Is JC saying that chrome rejecting a different port (expected results in comment 0) is a spec-compliance bug on their part?
Flags: needinfo?(dkeeler)
Updated•7 years ago
|
Whiteboard: [webauthn] [webauthn-interop] → [webauthn] [webauthn-interop][domsecurity-active]
![]() |
Assignee | |
Comment 9•7 years ago
|
||
From my reading of the spec ([0]), an rp id can only be a domain string, so the presence of the port signifier (':') and anything following it should cause these examples to be rejected (i.e. I think Chrome is correct here).
[0] https://w3c.github.io/webauthn/#relying-party-identifier
Flags: needinfo?(dkeeler)
![]() |
Assignee | |
Comment 10•7 years ago
|
||
Attachment #8986301 -
Flags: review?(kyle)
Reporter | ||
Comment 11•7 years ago
|
||
Sharing the Chrome's answer
Thanks for the report, Antonio!
I agree that the part you quoted from the spec is rather unclear and should be rephrased.
The intention of the spec (and the behavior of Chrome) is that the API can be called with the RPID "example.com" from any the following origins:
https://example.com
https://example.com:5001
https://login.example.com
However, the format of the RPID is always a domain name, that is, "example.com", so it cannot have a scheme or a port.
Updated•7 years ago
|
Attachment #8986301 -
Flags: review?(kyle) → review+
![]() |
Assignee | |
Comment 12•7 years ago
|
||
(In reply to Antonio Sanso from comment #11)
> Sharing the Chrome's answer
>
> Thanks for the report, Antonio!
>
> I agree that the part you quoted from the spec is rather unclear and should
> be rephrased.
>
> The intention of the spec (and the behavior of Chrome) is that the API can
> be called with the RPID "example.com" from any the following origins:
>
> https://example.com
> https://example.com:5001
> https://login.example.com
>
> However, the format of the RPID is always a domain name, that is,
> "example.com", so it cannot have a scheme or a port.
Thanks!
I can't find any explicit tests for these examples - might be good to add them.
Keywords: sec-low
![]() |
Assignee | |
Comment 13•7 years ago
|
||
Comment 14•7 years ago
|
||
Given that we're already building Fx61 RC builds and this is rated sec-low, I'm calling this wontfix for 61. If you want to nominate this for ESR60 approval to go into the 60.2 release shipping alongside Fx62 in September, be my guest though.
![]() |
||
Comment 15•7 years ago
|
||
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
![]() |
Assignee | |
Comment 16•7 years ago
|
||
Comment on attachment 8986301 [details] [diff] [review]
patch
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: I think this is more of a spec correctness bug than a security issue, but in any case we've got high hopes for webauthn so we should ship as solid of an implementation as we can.
User impact if declined: probably honestly nothing
Fix Landed on Version: 62
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8986301 -
Flags: approval-mozilla-esr60?
Updated•7 years ago
|
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [webauthn] [webauthn-interop][domsecurity-active] → [webauthn] [webauthn-interop][domsecurity-active][post-critsmash-triage]
Comment 17•7 years ago
|
||
Comment on attachment 8986301 [details] [diff] [review]
patch
This'll need a rebased patch if it's going to be uplifted to ESR60.
Flags: needinfo?(dkeeler)
Attachment #8986301 -
Flags: approval-mozilla-esr60? → approval-mozilla-esr60-
![]() |
Assignee | |
Comment 18•7 years ago
|
||
[Approval Request Comment]
see comment 16
Flags: needinfo?(dkeeler)
Attachment #8992089 -
Flags: review+
Attachment #8992089 -
Flags: approval-mozilla-esr60?
Comment 19•7 years ago
|
||
Comment on attachment 8992089 [details] [diff] [review]
patch for esr60
Improves our WebAuthn spec compliance. Approved for ESR 60.2.
Attachment #8992089 -
Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Comment 20•7 years ago
|
||
uplift |
Flags: in-testsuite+
Updated•6 years ago
|
Group: core-security-release
Updated•6 years ago
|
Whiteboard: [webauthn] [webauthn-interop][domsecurity-active][post-critsmash-triage] → [webauthn] [webauthn-interop][domsecurity-active][post-critsmash-triage][adv-main62-]
Updated•6 years ago
|
Whiteboard: [webauthn] [webauthn-interop][domsecurity-active][post-critsmash-triage][adv-main62-] → [webauthn] [webauthn-interop][domsecurity-active][post-critsmash-triage][adv-main62-][adv-esr60.2-]
You need to log in
before you can comment on or make changes to this bug.
Description
•