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

Status: RESOLVED FIXED

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

Description

[J.C. Jones [:jcj] (he/him)]

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

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Isn't that the same as bug 1435527?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

And its follow-up bug 1437487.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 5

[Phabricator Automation]

Attachment: Bug 1444547 - Propagate proper error codes from u2f-hid-rs to WebAuthn r=jcj

Comment 6

[Phabricator Automation]

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

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7128f574de5d9c753a8145f3815e880317e1547c

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/343ea197c56e
Propagate proper error codes from u2f-hid-rs to WebAuthn r=jcj

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/343ea197c56e

Comment 11

[Julien Cristau [:jcristau]]

Do we need this in 60 or is it ok to ride the trains?

Comment 12

[J.C. Jones [:jcj] (he/him)]

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 13

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 14

[Julien Cristau [:jcristau]]

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

Comment 15

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6e38c6caaba9

Comment 16

[Andrei Vaida [:avaida]]

Is this something that would benefit from manual testing?

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

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!

Comment 18

[Andrei Vaida [:avaida]]

Thanks, Tim!

Comment 19

[Bogdan Maris, Desktop QA]

(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.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

I'm afraid there's no way to reproduce without a second key.