Closed
Bug 1387820
Opened 7 years ago
Closed 7 years ago
WebAuthn assertion signatureData should contain only the signature
Categories
(Core :: DOM: Device Interfaces, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: mail, Assigned: jcj)
References
(Blocks 1 open bug, )
Details
(Whiteboard: [webauthn] [webauthn-interop])
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
keeler
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
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.)
Updated•7 years ago
|
Component: Untriaged → DOM: Security
Product: Firefox → Core
Assignee | ||
Comment 1•7 years ago
|
||
Thanks, Kohei. I'll double-check this against the spec and come up with a plan in about 24 hours.
Assignee: nobody → jjones
Component: DOM: Security → DOM: Device Interfaces
Assignee | ||
Comment 2•7 years ago
|
||
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
Priority: -- → P2
Whiteboard: [webauthn] [webauthn-interop]
Reporter | ||
Comment 3•7 years ago
|
||
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
Assignee | ||
Comment 5•7 years ago
|
||
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.
Blocks: webauthn
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Unspecified → All
Priority: P2 → P1
Hardware: Unspecified → All
Summary: WebAuthn assertion signature doesn't conform WD-05 format → WebAuthn assertion signatureData should contain only the signature
Version: unspecified → 56 Branch
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Reporter | ||
Comment 7•7 years ago
|
||
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
Assignee | ||
Comment 8•7 years ago
|
||
(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.
Reporter | ||
Comment 9•7 years ago
|
||
> #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 10•7 years ago
|
||
mozreview-review |
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
Attachment #8895657 -
Flags: review?(dkeeler) → review+
Comment 11•7 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 14•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d95683eef9ba
WebAuthn WD-05 Get Assertion Data Fix r=keeler
Keywords: checkin-needed
Comment 15•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 16•7 years ago
|
||
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.
Attachment #8895657 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
status-firefox56:
--- → affected
Comment 17•7 years ago
|
||
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.
Attachment #8895657 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 18•7 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
Comment 19•7 years ago
|
||
(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.
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•