1380529 - WebAuthn: Use WD-05 U2F Attestation Format

Status: RESOLVED FIXED

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

Description

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

WebAuthn's U2F attestation format is based on CBOR objects, whereas the current implementation naively dumps the U2F payload into the AuthenticatorAttestationResponse.AttestationObject struct. This must follow the spec.

Comment 1

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

There's another point: We need to use JWK-naming for the signature algorithm, so instead of "p-256" we have to only accept "es256". As that's also an attestation format change, I'll include it as a separate commit here.

Comment 2

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

Attachment: Bug 1380529 - Add a CBOR library for WebAuthn (1/3)

Web Authentication's WD-05 specification moves to using (CBOR) Concise Binary
Object Representation to transmit the binary data... most of it. This lands
the Apache 2-licensed "CBOR C++" serialization library [1] into webauthn's path.

It does not add any code to use this code; see patch 2/3.

[1] https://github.com/naphaso/cbor-cpp/

Review commit: https://reviewboard.mozilla.org/r/157450/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157450/

Comment 3

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

ttaubert: This is just an early patch for the CBOR library, as I'm not wholly sure what to do with it. 

1) As the license is fine for NSS, I'm assuming it's fine for Gecko. Is there someone I should loop in to check?
2) I stuck it in WebAuthn, as we're only going to use it there. I assume if someone needs this elsewhere they can move it around.
3) I pulled in the whole lib, even though we're only using the serialization parts -- and that's all I'm building in the moz.build. Is that OK?
4) I'm not making any efforts to run the upstream tests... as they are. Should I write some mochitests for it?

Comment 4

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

(In reply to J.C. Jones [:jcj] from comment #3)
> 3) I pulled in the whole lib, even though we're only using the serialization
> parts -- and that's all I'm building in the moz.build. Is that OK?

Adding to this, there are apparently unfixed memory issues with the decoder [1]. Maybe I should carve this down to only files we need, and toss a README-MOZILLA in there explaining why?

[1] https://github.com/naphaso/cbor-cpp/issues/8

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to J.C. Jones [:jcj] from comment #3)
> 1) As the license is fine for NSS, I'm assuming it's fine for Gecko. Is
> there someone I should loop in to check?

I assume it's fine. Not sure who exactly we could ask here...

> 2) I stuck it in WebAuthn, as we're only going to use it there. I assume if
> someone needs this elsewhere they can move it around.

Agreed.

> 3) I pulled in the whole lib, even though we're only using the serialization
> parts -- and that's all I'm building in the moz.build. Is that OK?
> 4) I'm not making any efforts to run the upstream tests... as they are.
> Should I write some mochitests for it?

(In reply to J.C. Jones [:jcj] from comment #4)
> (In reply to J.C. Jones [:jcj] from comment #3)
> > 3) I pulled in the whole lib, even though we're only using the serialization
> > parts -- and that's all I'm building in the moz.build. Is that OK?
> 
> Adding to this, there are apparently unfixed memory issues with the decoder
> [1]. Maybe I should carve this down to only files we need, and toss a
> README-MOZILLA in there explaining why?

This seems such a minimalist library, and we're only using the encoder, that likely it would at most take a day to translate it to Rust, including tests. That would "fix" points 1 + 3 + 4, and if we ever feel like we need a decoder we will have to write it in Rust.

WDYT?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8886663 [details]
Bug 1380529 -  Add a CBOR library for WebAuthn (1/3)

https://reviewboard.mozilla.org/r/157450/#review162824

Comment 7

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

(In reply to Tim Taubert [:ttaubert] from comment #5)
> This seems such a minimalist library, and we're only using the encoder, that
> likely it would at most take a day to translate it to Rust, including tests.
> That would "fix" points 1 + 3 + 4, and if we ever feel like we need a
> decoder we will have to write it in Rust.
> 
> WDYT?

I think this is a reasonable idea, but I think we should land it with this C++ lib first and file a follow-on to switch to a thin Rust impl. 

Doing it in Rust makes even more sense than you think, actually: When we support CTAP, we'll do that code in Rust (like we have U2F), and CTAP is pure CBOR, so we'll have to have a Rust lib.

That said, we don't need to write it. There are 3 Rust implementations mentioned on http://cbor.io/impls.html, so we'll just have to hook one up.

Let me know if this seems reasonable to you as an approach. If so, I'll file the follow-on to swap.

Comment 8

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

I opened Bug 1381578 to switch to a Rust-based CBOR library.

Comment 9

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

Attachment: Bug 1380529 - Use CBOR for the Create Credential WebAuthn call (2/3)

The WebAuthn Create Credential method should encode its results using CBOR;
this patch changes to that format.

The CBOR formats for the U2F data are specified in [1][2]

The attestation data format is in [3]

The high-level layout is in [4]

[1] https://w3c.github.io/webauthn/#generating-an-attestation-object
[2] https://w3c.github.io/webauthn/#fido-u2f-attestation
[3] https://w3c.github.io/webauthn/#sec-attestation-data
[4] https://w3c.github.io/webauthn/#sctn-attestation

Review commit: https://reviewboard.mozilla.org/r/157924/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157924/

Comment 10

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

Attachment: Bug 1380529 - Only permit "ES256" as pubkey type for WebAuthn (3/3)

Web Authentication uses JWK algorithm names (ES256) instead of WebCrypto names
(such as P-256). There are other JWK algorithm names, but our current U2F-backed
implementation only can support ES256 anyway, as that's all that FIDO U2F
devices understand. This patch limits us to the name ES256 for the "alg"
parameter.

Review commit: https://reviewboard.mozilla.org/r/157926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157926/

Comment 11

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

Comment on attachment 8886663 [details]
Bug 1380529 -  Add a CBOR library for WebAuthn (1/3)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157450/diff/1-2/

Comment 12

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

Comment on attachment 8886663 [details]
Bug 1380529 -  Add a CBOR library for WebAuthn (1/3)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157450/diff/2-3/

Comment 13

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

Comment on attachment 8887190 [details]
Bug 1380529 -  Use CBOR for the Create Credential WebAuthn call (2/3)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157924/diff/1-2/

Comment 14

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

Comment on attachment 8887191 [details]
Bug 1380529 - Only permit "ES256" as pubkey type for WebAuthn (3/3)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157926/diff/1-2/

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8887191 [details]
Bug 1380529 - Only permit "ES256" as pubkey type for WebAuthn (3/3)

https://reviewboard.mozilla.org/r/157926/#review163630

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8886663 [details]
Bug 1380529 -  Add a CBOR library for WebAuthn (1/3)

https://reviewboard.mozilla.org/r/157450/#review163624

::: dom/webauthn/cbor-cpp/src/output_dynamic.cpp:25
(Diff revision 3)
> +void output_dynamic::init(unsigned int initalCapacity) {
> +    this->_capacity = initalCapacity;
> +    this->_buffer = new unsigned char[initalCapacity];
> +    this->_offset = 0;
> +}

Is it worth not using `output_dynamic` because we can't have Rust allocate memory later anyway? Not sure what the Rust CBOR libraries' APIs look like. We can definitely do that when switching to the Rust impl though, just wanted to throw this out there for consideration.

::: dom/webauthn/moz.build:18
(Diff revision 3)
> +    'cbor-cpp/src/cbor.h',
> +    'cbor-cpp/src/encoder.h',
> +    'cbor-cpp/src/output_dynamic.h',

We're only including the first one, do we need the others here too? Actually, do we need any of them exported? (I might be confusing Gecko vs. NSS build systems here though.)

Comment 17

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

Comment on attachment 8886663 [details]
Bug 1380529 -  Add a CBOR library for WebAuthn (1/3)

https://reviewboard.mozilla.org/r/157450/#review163624

Thanks for the reviews!

> Is it worth not using `output_dynamic` because we can't have Rust allocate memory later anyway? Not sure what the Rust CBOR libraries' APIs look like. We can definitely do that when switching to the Rust impl though, just wanted to throw this out there for consideration.

I kept the variable-length output simply because I'm not sure/too lazy to figure of the maximum size of the resulting object. I agree that we should be cleaner when it refactors to use a Rust lib -- besides, it'll almost have to.

> We're only including the first one, do we need the others here too? Actually, do we need any of them exported? (I might be confusing Gecko vs. NSS build systems here though.)

You're right, we really don't need to export them. Monkey-see/do syndrome!

Comment 18

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

Comment on attachment 8886663 [details]
Bug 1380529 -  Add a CBOR library for WebAuthn (1/3)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157450/diff/3-4/

Comment 19

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

Comment on attachment 8887190 [details]
Bug 1380529 -  Use CBOR for the Create Credential WebAuthn call (2/3)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157924/diff/2-3/

Comment 20

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

Comment on attachment 8887191 [details]
Bug 1380529 - Only permit "ES256" as pubkey type for WebAuthn (3/3)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157926/diff/2-3/

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8887190 [details]
Bug 1380529 -  Use CBOR for the Create Credential WebAuthn call (2/3)

https://reviewboard.mozilla.org/r/157924/#review163664

::: dom/webauthn/WebAuthnManager.cpp:708
(Diff revision 2)
>      return;
>    }
>  
> +  // Construct the public key object
> +  mozilla::dom::CryptoBuffer xBuf;
> +  if (NS_WARN_IF(!xBuf.SetCapacity(32, mozilla::fallible))) {

Don't need to do this. Declaring `xBuf` is sufficient, `ReadToCryptoBuffer` will allocate, although in 1-byte increments. It might actually make sense to move the `SetCapacity` call to `ReadToCryptoBuffer` and pass it `aLen` there?

::: dom/webauthn/WebAuthnManager.cpp:713
(Diff revision 2)
> +  if (NS_WARN_IF(!xBuf.SetCapacity(32, mozilla::fallible))) {
> +    Cancel(rv);
> +    return;
> +  }
> +  mozilla::dom::CryptoBuffer yBuf;
> +  if (NS_WARN_IF(!yBuf.SetCapacity(32, mozilla::fallible))) {

See above.

::: dom/webauthn/WebAuthnManager.cpp:723
(Diff revision 2)
> +  if (NS_FAILED(U2FDecomposeECKey(pubKeyBuf, xBuf, yBuf))) {
> +    Cancel(rv);
> +    return;
> +  }
> +
> +  cbor::output_dynamic cborPubKeyOut;

Why not move this declaration into the scope, put `CryptoBuffer pubKeyObj` here, and call `pubKeyObj.Assign` before we exit the scope? Then we have a limited lifetime for `cborPubKeyOut` and it's a tad more obvious we're constructing `pubKeyObj`.

::: dom/webauthn/WebAuthnManager.cpp:735
(Diff revision 2)
> +    eccPubKey = { alg: eccAlgName, x: biguint, y: biguint }
> +    eccAlgName = "ES256" / "ES384" / "ES512"
> +    */
> +    cbor::encoder encoder(cborPubKeyOut);
> +    encoder.write_map(3);
> +    {

I understand why you added the extra scope, although it confused me for a few seconds. I thought I missed some variable going out of scope somehow.

::: dom/webauthn/WebAuthnManager.cpp:737
(Diff revision 2)
> +    */
> +    cbor::encoder encoder(cborPubKeyOut);
> +    encoder.write_map(3);
> +    {
> +      encoder.write_string("alg");
> +      encoder.write_string("ES256"); // Always ES256 for U2F

nit: `JWK_ALG_ECDSA_P_256`?

::: dom/webauthn/WebAuthnManager.cpp:774
(Diff revision 2)
> +    Cancel(NS_ERROR_OUT_OF_MEMORY);
> +    return;
> +  }
> +
> +  authDataBuf.AppendElements(rpIdHashBuf, mozilla::fallible);
> +  authDataBuf.AppendElement(0x41, mozilla::fallible);

Can we turn `0x41` into `FLAG_TUP|FLAG_AT` by declaring two consts `TUP=0x01` and `AT = 0x40` at the top?

::: dom/webauthn/WebAuthnManager.cpp:782
(Diff revision 2)
> +  authDataBuf.AppendElement((keyHandleBuf.Length() >> 8) & 0xFF, mozilla::fallible);
> +  authDataBuf.AppendElement((keyHandleBuf.Length() >> 0) & 0xFF, mozilla::fallible);

`MOZ_ASSERT(keyHandleBuf.Length() <= 0xffff)` ?

::: dom/webauthn/WebAuthnManager.cpp:787
(Diff revision 2)
> +  authDataBuf.AppendElement((keyHandleBuf.Length() >> 8) & 0xFF, mozilla::fallible);
> +  authDataBuf.AppendElement((keyHandleBuf.Length() >> 0) & 0xFF, mozilla::fallible);
> +  authDataBuf.AppendElements(keyHandleBuf, mozilla::fallible);
> +  authDataBuf.AppendElements(pubKeyObj, mozilla::fallible);
> +
> +  cbor::output_dynamic cborAttOut;

Also move into scope, as suggested above?

::: dom/webauthn/WebAuthnManager.cpp:841
(Diff revision 2)
>    // values returned from the authenticator as well as the clientDataJSON
>    // computed earlier.
>    RefPtr<AuthenticatorAttestationResponse> attestation =
>        new AuthenticatorAttestationResponse(mCurrentParent);
>    attestation->SetClientDataJSON(clientDataBuf);
> -  attestation->SetAttestationObject(regData);
> +  attestation->SetAttestationObject(attObj);

That function... has become longer. What would you think about a separate file that takes care of CBOR encoding? This might look nicer if we had something like:

```
WebAuthnCBOREncoder enc(mInfo, aRegBuffer);

CryptoBuffer attObj;
enc.MakeAttestionObject(attObj);

...
```

::: dom/webauthn/WebAuthnUtil.cpp:158
(Diff revision 2)
> +  rv = ReadToCryptoBuffer(input, aYcoord, 32);
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +
> +  MOZ_ASSERT(input.AtEnd());

Should we assert this if we don't control the input?

::: dom/webauthn/tests/u2futil.js:143
(Diff revision 2)
> -    keyHandleBytes: aRegData.slice(67, 67 + keyHandleLength),
> -    attestationBytes: aRegData.slice(67 + keyHandleLength)
> -  }
> -
> -  u2fRegObj.keyHandle = bytesToBase64UrlSafe(u2fRegObj.keyHandleBytes);
> +  }
> +
> +  let rpIdHash = attObj.authData.slice(0, 32);
> +  let flags = attObj.authData.slice(32, 33);
> +  let counter = attObj.authData.slice(33, 37);
> +  let attData = {}

nit: missing semicolon

Comment 22

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

Comment on attachment 8887190 [details]
Bug 1380529 -  Use CBOR for the Create Credential WebAuthn call (2/3)

https://reviewboard.mozilla.org/r/157924/#review163664

Thanks for all these great ideas, Tim. I've given it a shot, see what you think in the update.

> Don't need to do this. Declaring `xBuf` is sufficient, `ReadToCryptoBuffer` will allocate, although in 1-byte increments. It might actually make sense to move the `SetCapacity` call to `ReadToCryptoBuffer` and pass it `aLen` there?

Good idea; I moved the `SetCapacity` call into `ReadToCryptoBuffer`, replacing the `ClearAndRetainStorage` call. This looks clean for all uses.

> Why not move this declaration into the scope, put `CryptoBuffer pubKeyObj` here, and call `pubKeyObj.Assign` before we exit the scope? Then we have a limited lifetime for `cborPubKeyOut` and it's a tad more obvious we're constructing `pubKeyObj`.

Much nicer!

> nit: `JWK_ALG_ECDSA_P_256`?

*sheepish* Yeah...

> Can we turn `0x41` into `FLAG_TUP|FLAG_AT` by declaring two consts `TUP=0x01` and `AT = 0x40` at the top?

Definitely best practice!

> That function... has become longer. What would you think about a separate file that takes care of CBOR encoding? This might look nicer if we had something like:
> 
> ```
> WebAuthnCBOREncoder enc(mInfo, aRegBuffer);
> 
> CryptoBuffer attObj;
> enc.MakeAttestionObject(attObj);
> 
> ...
> ```

I've given it a shot, in a super-simple way. See what you think.

> Should we assert this if we don't control the input?

I think we should, yes. It's not a release assert, but it would be extremely useful to know if we are dealing with non-compliant hardware devices, at least during testing.

Comment 23

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

Comment on attachment 8887190 [details]
Bug 1380529 -  Use CBOR for the Create Credential WebAuthn call (2/3)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157924/diff/3-4/

Comment 24

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

Comment on attachment 8887191 [details]
Bug 1380529 - Only permit "ES256" as pubkey type for WebAuthn (3/3)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157926/diff/3-4/

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8887190 [details]
Bug 1380529 -  Use CBOR for the Create Credential WebAuthn call (2/3)

https://reviewboard.mozilla.org/r/157924/#review165150

This is much easier to read, thanks!

::: dom/webauthn/WebAuthnCBORUtil.cpp:19
(Diff revision 4)
> +nsresult
> +CBOREncodePublicKeyObj(const CryptoBuffer& aPubKeyBuf,
> +                       /* out */ CryptoBuffer& aPubKeyObj)
> +{
> +  mozilla::dom::CryptoBuffer xBuf, yBuf;
> +  nsresult rv = U2FDecomposeECKey(aPubKeyBuf, xBuf, yBuf);

Should we just move this function here? This seems to be the only caller. Or.. if that doesn't really fit the `CBORUtil` label then do this before calling `CBOREncodePublicKeyObj` and pass `x` and `y`?

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Oh, another thought. I'm not actually a DOM peer, or even a dom/webauthn peer. Do we need to get someone else to sign these off?

Comment 27

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

Comment on attachment 8887190 [details]
Bug 1380529 -  Use CBOR for the Create Credential WebAuthn call (2/3)

https://reviewboard.mozilla.org/r/157924/#review165150

> Should we just move this function here? This seems to be the only caller. Or.. if that doesn't really fit the `CBORUtil` label then do this before calling `CBOREncodePublicKeyObj` and pass `x` and `y`?

It's a good idea, and I thought about it too -- but it seems like CBOREncodePublicKeyObj should take a public key and encode it, and there are more pubkey types possible with WebAuthn than the decomposed EC key, so I think this (in the future) should check the ASN.1 key type and decide what method to call.

Comment 28

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

(In reply to Tim Taubert [:ttaubert] from comment #26)
> Oh, another thought. I'm not actually a DOM peer, or even a dom/webauthn
> peer. Do we need to get someone else to sign these off?

I think you're at least half of a webauthn peer by working so closely on code over the last quarter. It's good question re: getting a DOM sign-off, but we're not hitting any IDL, or affecting anything else in the DOM code, and the license is known-good, so ISTM that we're good to proceed.

Try for builds is good - the run with tests is somewhere off in history I could dig up. https://treeherder.mozilla.org/#/jobs?repo=try&revision=62ec5542c1d5

Marking checkin-needed.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45b4405c24ca
Add a CBOR library for WebAuthn (1/3) r=ttaubert
https://hg.mozilla.org/integration/autoland/rev/070367125549
Use CBOR for the Create Credential WebAuthn call (2/3) r=ttaubert
https://hg.mozilla.org/integration/autoland/rev/1f66a39c19f1
Only permit "ES256" as pubkey type for WebAuthn (3/3) r=ttaubert

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/45b4405c24ca
https://hg.mozilla.org/mozilla-central/rev/070367125549
https://hg.mozilla.org/mozilla-central/rev/1f66a39c19f1