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)

60 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 62+ fixed
firefox60 --- wontfix
firefox61 + wontfix
firefox62 + fixed

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)

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'.
Group: firefox-core-security → dom-core-security
Component: Untriaged → DOM: Security
Product: Firefox → Core
Could you take a look at this, J.C.? Thanks.
Flags: needinfo?(jjones)
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)
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....
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
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]
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.
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
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
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)
Whiteboard: [webauthn] [webauthn-interop] → [webauthn] [webauthn-interop][domsecurity-active]
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)
Attached patch patchSplinter Review
Attachment #8986301 - Flags: review?(kyle)
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.
Attachment #8986301 - Flags: review?(kyle) → review+
(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
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.
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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?
Flags: qe-verify-
Whiteboard: [webauthn] [webauthn-interop][domsecurity-active] → [webauthn] [webauthn-interop][domsecurity-active][post-critsmash-triage]
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-
Attached patch patch for esr60Splinter Review
[Approval Request Comment] see comment 16
Flags: needinfo?(dkeeler)
Attachment #8992089 - Flags: review+
Attachment #8992089 - Flags: approval-mozilla-esr60?
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+
Group: core-security-release
Whiteboard: [webauthn] [webauthn-interop][domsecurity-active][post-critsmash-triage] → [webauthn] [webauthn-interop][domsecurity-active][post-critsmash-triage][adv-main62-]
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.

Attachment

General

Created:
Updated:
Size: