User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID: 20170804193726 Steps to reproduce: Tested on Firefox Nightly 57.0a1 (2017-08-04). Called `navigator.credentials.get()` method and retrieved signature from `AuthenticatorAssertionResponse.signature`. Actual results: binary concatenation of `presenceAndCounter`(in 5 octets) and actual signature in ASN.1 DER format was returned. Expected results: `presenceAndCounter` should not be prepended. signature should be in JWA format (https://tools.ietf.org/html/rfc7518#section-3.4). (I assume we should use JWA format instead of ASN.1 DER format because we use JWA algorithm identifier like "ES256". Maybe it should be explicitly written in Web Authentication specification.)

Thanks, Kohei. I'll double-check this against the spec and come up with a plan in about 24 hours.

Kohei, I think your concerns are legitimate, but I'm not sure whether to fix Firefox or to fix the spec. I've posted to the Web Authentication mailing list here: https://lists.w3.org/Archives/Public/public-webauthn/2017Aug/0078.html

Hi Jones, Thank you for your prompt response. You made reference to the FIDO U2F "Attestation" Statement Format in your post to the web authentication mailing list, but I'm talking about "assertion" signature, and it seems neither ASN.1 nor RFC7518 is specified as its format. So I assumed it should be RFC7518. https://www.w3.org/TR/2017/WD-webauthn-20170505/#op-get-assertion (I haven't checked the "attestation" signature that Firefox returns closely, I cannot say anything about the signature in "attestation" statement for now.)

Nojima-san wrote: > I'm talking about "assertion" signature, and it seems neither ASN.1 nor RFC7518 is specified > as its format Actually, the format of the U2F assertion signature value (termed just "signature" in [0] section 5.4 and figure 5) is specified as being encoded in ANSI X9.62 format [1]. If one does not have access to [1] (it costs money), one may equivalently (in practice) refer to [2] or [3]. This format, expressed in ASN.1 is: ECDSA-Sig-Value ::= SEQUENCE { r INTEGER, s INTEGER, a INTEGER OPTIONAL, y CHOICE { b BOOLEAN, f FieldElement } OPTIONAL } [2] and [3] omit the optional 'a' and 'y' members. HTH, =JeffH [0] https://fidoalliance.org/specs/fido-u2f-v1.2-ps-20170411/fido-u2f-raw-message-formats-v1.2-ps-20170411.html [1] Public Key Cryptography for the Financial Services Industry: The Elliptic Curve Digital Signature Algorithm (ECDSA), ANSI X9.62-2005. American National Standards Institute, November 2005, URL: http://webstore.ansi.org/RecordDetail.aspx?sku=ANSI+X9.62%3A2005 [2] SEC 1: Elliptic Curve Cryptography. Certicom Research, May 21, 2009, Version 2.0, URL: http://www.secg.org/sec1-v2.pdf [3] Elliptic Curve Cryptography Subject Public Key Information. https://tools.ietf.org/html/rfc5480

There appears agreement that the AuthenticatorAssertionResponse.Signature field should be only the bytes of the actual X9.62 signature, so we'll want to take this for Firefox 56 as that's our interop build.

Hi Jones, Your patch looks good to me as far as looking the test code(test_webauthn_loopback.html). The issue that the flags byte and the counter are prepended to the signature seems resolved. I suppose another issues (Please see [1] #2 and #3) still remain regarding the assertion signature, but it is about the web authentication spec, not about Firefox implementation. [1] https://lists.w3.org/Archives/Public/public-webauthn/2017Aug/0103.html

(In reply to Yoshikazu Nojima from comment #7) > Your patch looks good to me as far as looking the test > code(test_webauthn_loopback.html). > The issue that the flags byte and the counter are prepended to the signature > seems resolved. > > I suppose another issues (Please see [1] #2 and #3) still remain regarding > the assertion signature, > but it is about the web authentication spec, not about Firefox > implementation. Thank you for checking! I agree, issues #2 and #3 need to be resolved upstream. I think #2 will be resolved as explicitly defining the format of the key, and #3 is apparently going to be better defined by another specification, CTAP, but I'm not clear on its publish schedule.

> #3 is apparently going to be better defined by another > specification, CTAP, but I'm not clear on its publish schedule. Yes, the details of the assertion signature calculation target data is to be defined by another specification, but current web authentication spec defines the signature verification procedure in this way: > Using the credential public key looked up in step 1, verify that sig is a valid signature over the binary concatenation of aData and hash. (`aData` is an `authenticatorData`, and `hash` is a hash of `clientData`) (https://www.w3.org/TR/2017/WD-webauthn-20170505/#verifying-assertion 6.2. Step #10) This need to be changed to leave room for another specification to define it.

Comment on attachment 8895657 [details] Bug 1387820 - WebAuthn WD-05 Get Assertion Data Fix https://reviewboard.mozilla.org/r/166916/#review172476 Looks good to me. ::: dom/webauthn/WebAuthnUtil.cpp:51 (Diff revision 1) > - const CryptoBuffer& aRpIdHash, > - const CryptoBuffer& aSignatureData) > -{ > - // The AuthenticatorData for U2F devices is the concatenation of the > - // RP ID with the output of the U2F Sign operation. > - if (aRpIdHash.Length() != 32) { > + const uint8_t flags, > + const CryptoBuffer& counterBuf, > + const CryptoBuffer& attestationDataBuf, > + /* out */ CryptoBuffer& authDataBuf) > +{ > + if (NS_WARN_IF(!authDataBuf.SetCapacity(32 + 1 + 4 + attestationDataBuf.Length(), I think we keep the rpIdHashBuf length check. ::: dom/webauthn/WebAuthnUtil.cpp:63 (Diff revision 1) > + flagSet |= FLAG_AT; > + } > + > + authDataBuf.AppendElements(rpIdHashBuf, mozilla::fallible); > + authDataBuf.AppendElement(flagSet, mozilla::fallible); > + authDataBuf.AppendElements(counterBuf, mozilla::fallible); We should probably also check the counterBuf length to make sure we've actually reserved sufficient space for the data we're appending. ::: dom/webauthn/WebAuthnUtil.cpp:87 (Diff revision 1) > + mozilla::fallible))) { > + return NS_ERROR_OUT_OF_MEMORY; > + } > + > + attestationDataBuf.AppendElements(aaguidBuf, mozilla::fallible); > + attestationDataBuf.AppendElement((keyHandleBuf.Length() >> 8) & 0xFF, Might be good to check that the key handle length fits in two bytes. ::: dom/webauthn/WebAuthnUtil.cpp:98 (Diff revision 1) > + return NS_OK; > +} > + > +nsresult > +U2FDecomposeSignResponse(const CryptoBuffer& aResponse, > + /* out */ uint8_t* aFlags, Let's just make this a reference? ::: dom/webauthn/WebAuthnUtil.cpp:110 (Diff revision 1) > } > > - if (!aAuthenticatorData.AppendElements(aRpIdHash, mozilla::fallible)) { > + Span<const uint8_t> rspView = MakeSpan(aResponse); > + *aFlags = rspView[0]; > + > + if (NS_WARN_IF(!aCounterBuf.AppendElements(rspView.FromTo(1,5), nit: space after ',' ::: dom/webauthn/tests/test_webauthn_loopback.html:111 (Diff revision 1) > - return deriveAppAndChallengeParam(window.location.host, aAssertion.response.clientDataJSON) > .then(function(aParams) { > - console.log(aParams.appParam, rpIdHash, presenceAndCounter, aParams.challengeParam); > + console.log(aParams); > console.log("ClientData buffer: ", hexEncode(aAssertion.response.clientDataJSON)); > console.log("ClientDataHash: ", hexEncode(aParams.challengeParam)); > - return assembleSignedData(aParams.appParam, presenceAndCounter, aParams.challengeParam); > + return assembleSignedData(aParams.appParam, aParams.attestation.flags, aParams.attestation.counter, aParams.challengeParam); nit: this line is a bit long

Comment on attachment 8895657 [details] Bug 1387820 - WebAuthn WD-05 Get Assertion Data Fix https://reviewboard.mozilla.org/r/166916/#review172476 > I think we keep the rpIdHashBuf length check. er, there should be a "should" in there somewhere

Comment on attachment 8895657 [details] Bug 1387820 - WebAuthn WD-05 Get Assertion Data Fix https://reviewboard.mozilla.org/r/166916/#review172476 Thanks for the review, Mr. Keeler! > er, there should be a "should" in there somewhere Good catch. Also checking the counter as being length 4 while I'm at it. > We should probably also check the counterBuf length to make sure we've actually reserved sufficient space for the data we're appending. Er, yeah. What you just said.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d95683eef9ba WebAuthn WD-05 Get Assertion Data Fix r=keeler

Comment on attachment 8895657 [details] Bug 1387820 - WebAuthn WD-05 Get Assertion Data Fix Since this went out without trouble in Nightly, and this is an interop bug for W3C Interop testing in September, requesting uplift to 56. Approval Request Comment [Feature/Bug causing the regression]: This bug, Bug 1387820 [User impact if declined]: Impacts to Web Authentication WD-05 interop testing in September, which was planned to use Firefox 56 / beta. We'd need to use Nightly, which would delay landing WD-06 patches in Nightly until after interop. [Is this code covered by automated tests?]: Yes, and the tests are updated in this patch. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: Not needed; this feature is pref'd off. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No; the feature is pref'd off. [Why is the change risky/not risky?]: The feature is pref'd off. [String changes made/needed]: None.

Comment on attachment 8895657 [details] Bug 1387820 - WebAuthn WD-05 Get Assertion Data Fix Let's uplift this for testing in beta, since it's preffed off sounds like it's low risk.

(In reply to J.C. Jones [:jcj] from comment #16) > [Is this code covered by automated tests?]: Yes, and the tests are updated > in this patch. > [Has the fix been verified in Nightly?]: Yes. > [Needs manual test from QE? If yes, steps to reproduce]: Not needed; this > feature is pref'd off. Setting qe-verify- based on J.C. Jones's assessment on manual testing needs and the fact that this fix has automated coverage.