webauthn
webauthn copied to clipboard
w3c
Move step 16 of Registration to between 21 and 22
Step 16 of registration ( https://w3c.github.io/webauthn/#sctn-registering-a-new-credential ) is:
Verify that the "alg" parameter in the credential public key in authData matches the alg attribute of one of the items in options.pubKeyCredParams.
However, if an RP implementor is following and implementing the specification to the letter, with the steps in order (which they should be! The order of these steps is vital!), then the value of 'credential public key' is not available at this point. credential public key is first made available by the parsing of the attested credential data, and it's subsequent verification during attestation in steps such as: https://w3c.github.io/webauthn/#sctn-packed-attestation verification procedure step 2.
It may then make more sense to move step 16 to between step 21 and 22, so that the data is attested and validated first, then then credential public key can have it's alg checked.
hm... Though, step 12 is:
- Perform CBOR decoding on the attestationObject field of the AuthenticatorAttestationResponse structure to obtain the attestation statement format fmt, the authenticator data authData, and the attestation statement attStmt.
If we want/need to be pedantically clear, perhaps we ought to add at the end of the above something like ", from which the [=credential public key=] is obtained."
...?
I don't think any change is necessary. The combination of step 12:
Perform CBOR decoding on the attestationObject field [...] to obtain [...] the authenticator data authData
and step 16:
the "alg" parameter in the credential public key in authData
seems pretty unambiguous to me. We also don't have detailed steps for in which order the RP should parse all the parts of the attestation object, and I don't think we should. Although I will agree that the name "attested credential data" for the sub-datastructure is a bit unfortunate, since it doesn't really have much to do with attestation.
I don't think any change is necessary [ ... ]
Fine by me, especially given where we are in the process for completing L2. Like I said above, we could clarify it "...if we want/need to be pedantically clear...", implying I don't feel its fully necessary.
...the name "attested credential data" for the sub-datastructure is a bit unfortunate, since it doesn't really have much to do with attestation.
FWIW, I disagree. That "sub-datastructure" (i.e., attested credential data) is data signed by the attestation private key, and hence is "attested" by said private key. See also figure 6 "attestation object". We also use the phrase "attested credential public key" in a couple places.
hm... Though, step 12 is:
- Perform CBOR decoding on the attestationObject field of the AuthenticatorAttestationResponse structure to obtain the attestation statement format fmt, the authenticator data authData, and the attestation statement attStmt.
If we want/need to be pedantically clear, perhaps we ought to add at the end of the above something like ", from which the [=credential public key=] is obtained."
...?
Perhaps this is the issue. In my implementation, we do not extract the credential public key until the verification of the attStmt is completed. However, I believe this is the correct order of operations because you should validate the attestation before trusting the content of the credential public key contained within that attestation.
This is why I suggest that the step be moved since I think that this progression seems more robust as the credential public key being contained within the attestation, does "hint" that we should validate that attestation first before we trust it's internals.
I agree with the premise of #1064 - verify cryptographic assertions first, then validate our remaining parameters are what we expect, and kind of what this ticket is about too.
for L2, if we do anything, I suggest we only make the editorial change described above in https://github.com/w3c/webauthn/issues/1555#issuecomment-772682205, and in any case, punt dealing with the full scope of this to L3.
Great thanks. Is there anything actionable you need from me at this point for this topic?
Sorry for the delay. But coming back to this, I no longer feel convinced that any change is necessary.
The primary argument in favour of a change is that you shouldn't trust the contents of the attestation object before you've verified and decided to trust (or decided to ignore) the attestation signature. But if the contents are wrong - be it because "alg" has an unallowed value, or the challenge is incorrect, or whatever - then you're going to reject the registration anyway, so whether you trust the attestation is irrelevant. The registration has to pass BOTH content validation AND signature verification to be accepted, so it doesn't matter for security in which order you perform those checks. If anything it's a tiny bit better for performance and energy savings to validate the contents before verifying the signature, as the signature verification is likely to take the most work to perform.
So I now think we should close this issue with no further action.