Closed Bug 1444547 Opened 2 years ago Closed 2 years ago

Abort with InvalidStateError when allowCredentials is empty but the user touches a token anyway

Categories

(Core :: DOM: Device Interfaces, defect, P2, minor)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: jcj, Assigned: ttaubert)

Details

(Whiteboard: [webauthn] [webauthn-interop])

Attachments

(1 file)

Per step 17.4 of 5.1.4.1. PublicKeyCredential’s [[DiscoverFromExternalSource]](origin, options, sameOriginWithAncestors) method [1], when a website requests an attestation without supplying an "allowCredentials" list, we should let the authenticators try something.

U2F authenticators can't try anything, so right now rust_u2f_mgr_sign bails out [2], prompting a NS_ERROR_DOM_UNKNOWN_ERR [3].

Even though U2F can't do anything here, the spec says we should run to timeout, so we should do that.




[1] https://w3c.github.io/webauthn/#discover-from-external-source
[2] https://github.com/jcjones/u2f-hid-rs/blob/f1f0bce6818c8c321587168b4e66dbe585a77ff3/src/capi.rs#L198-L200
[3] https://searchfox.org/mozilla-central/source/dom/webauthn/U2FHIDTokenManager.cpp#195
Isn't that the same as bug 1435527?
And its follow-up bug 1437487.
Per spec, allowCredentials can be empty to allow authenticators, in the future, to pick a credential that matches the given RP ID. We currently don't support that and just keep polling.

If the user touches a token anyway we'll abort with a NS_ERROR_DOM_UNKNOWN_ERR because our Rust backend can't forward actual error codes yet. Need to implement exactly that.
Summary: Web Authentication's get method should run to timeout if no key handles are supplied in the allowed list → Abort with NotAllowedError when allowCredentials is empty but the user touches a token anyway
I think we should probably return InvalidStateError, per spec that case isn't really defined because we shouldn't do anything with authenticators that don't identify with anything in the allowCredentials list.

That would match the registration case where we report InvalidState when the user touches a token that's in the exclusion list.

Only if the user ends up doing nothing we would report NotAllowed.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Summary: Abort with NotAllowedError when allowCredentials is empty but the user touches a token anyway → Abort with InvalidStateError when allowCredentials is empty but the user touches a token anyway
Comment on attachment 8958449 [details]
Bug 1444547 - Propagate proper error codes from u2f-hid-rs to WebAuthn r=jcj

J.C. Jones [:jcj] has approved the revision.

https://phabricator.services.mozilla.com/D717
Attachment #8958449 - Flags: review+
There were a few ASan failures which I hopefully fixed by creating the UniquePtr before the early returns in the static u2f-hid-rs callbacks.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6b13c81bdd4786a4341575efed6e3165a2e17a7
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/343ea197c56e
Propagate proper error codes from u2f-hid-rs to WebAuthn r=jcj
https://hg.mozilla.org/mozilla-central/rev/343ea197c56e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Do we need this in 60 or is it ok to ride the trains?
Flags: needinfo?(ttaubert)
IMO this would be nice to uplift, but it's a big patch. It's had some good testing in Nightly though. I'd say we should go for it...
Comment on attachment 8958449 [details]
Bug 1444547 - Propagate proper error codes from u2f-hid-rs to WebAuthn r=jcj

Approval Request Comment
[Feature/Bug causing the regression]: Not a regression.
[User impact if declined]: 
[Is this code covered by automated tests?]: Only partially.
[Has the fix been verified in Nightly?]: We verified it manually and it doesn't seem to break things.
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: It's somewhat big, but not very risky.
[Why is the change risky/not risky?]: It changes error code propagation, not logic.
[String changes made/needed]: None.

It would be nice to have this in 60 so that our WebAuthn API implementation behaves as stated by the spec. Especially reporting the correct error codes will help early adopters, I assume.
Flags: needinfo?(ttaubert)
Attachment #8958449 - Flags: approval-mozilla-beta?
Comment on attachment 8958449 [details]
Bug 1444547 - Propagate proper error codes from u2f-hid-rs to WebAuthn r=jcj

error handling changes for webauthn, approved for 60.0b10
Attachment #8958449 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Is this something that would benefit from manual testing?
Flags: needinfo?(ttaubert)
Sure! Here are the steps:

1) Open https://webauthn.bin.coffee/
2) Click "Create Credential" and touch YubiKey "A"
3) Click "Get Assertion" and touch any other YubiKey [not the one used in (2)]
4) The text area should turn red and contain "InvalidStateError"

Previously in (4) it would contain "UnknownError".

Thanks!
Flags: needinfo?(ttaubert)
Thanks, Tim!
Flags: qe-verify+
(In reply to Tim Taubert [:ttaubert] from comment #17)
> Sure! Here are the steps:
> 
> 1) Open https://webauthn.bin.coffee/
> 2) Click "Create Credential" and touch YubiKey "A"
> 3) Click "Get Assertion" and touch any other YubiKey [not the one used in
> (2)]
> 4) The text area should turn red and contain "InvalidStateError"
> 
> Previously in (4) it would contain "UnknownError".
> 
> Thanks!

Unfortunately we don't have two YubiKeys (only one), so I'll drop qe-verify+ for now since I'm unable to follow the steps you provided.

Do you have other steps that involves using only one YubiKey, or something different? If so, I'll reflag it as qe-verify+ and verify it.
Flags: qe-verify+ → needinfo?(ttaubert)
I'm afraid there's no way to reproduce without a second key.
Flags: needinfo?(ttaubert)
You need to log in before you can comment on or make changes to this bug.