Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Packed and U2F Attestation Statements' verifications don't differentiate between Basic and Privacy CA Attestation Types #656

Closed
jcjones opened this issue Oct 18, 2017 · 7 comments

Comments

@jcjones
Copy link
Contributor

jcjones commented Oct 18, 2017

The Packed Attestation Statement Format is valid for all Attestation Types.

webauthn/index.bs

Lines 2313 to 2317 in b8c6027

: Attestation statement format identifier
:: packed
: Attestation types supported
:: All

However, in its verification procedure it assumes that if x5c is present, that attestations are type Basic:

webauthn/index.bs

Lines 2387 to 2393 in b8c6027

3. If |x5c| is present, this indicates that the attestation type is not [=ECDAA=]. In this case:
- Verify that |sig| is a valid signature over the concatenation of |authenticatorData| and |clientDataHash| using the
attestation public key in |x5c| with the algorithm specified in |alg|.
- Verify that |x5c| meets the requirements in [[#packed-attestation-cert-requirements]].
- If |x5c| contains an extension with OID `1 3 6 1 4 1 45724 1 1 4` (id-fido-gen-ce-aaguid) verify that the value of this
extension matches the <code>[=aaguid=]</code> in |authenticatorData|.
- If successful, return attestation type Basic and trust path |x5c|.

However, that's what the Privacy CA attestation will look like, too.

Similarly, it's technically feasible for a browser to use the Privacy CA option for U2F, and we might want to do so for - say - private browsing mode. Yet U2F Attestation Format suffers the same issue -- in addition, it excludes Privacy CA which seems wrong, as it'd be useful:

webauthn/index.bs

Lines 2686 to 2690 in b8c6027

: Attestation statement format identifier
:: fido-u2f
: Attestation types supported
:: Basic, [=self attestation=]

@emlun
Copy link
Member

emlun commented Oct 19, 2017

I'm not sure having Privacy CA as a separate attestation type is very meaningful, as like you say (and @balfanz notes in #628 (comment)), to the RP it looks and act the same as Basic Attestation. I would suggest merging the two concepts, but I'm not sure how that would affect the TPM attestation statement format which seems to be intimately connected to the Privacy CA model.

@Kieun
Copy link
Member

Kieun commented Oct 19, 2017

In the view point of RP, RP cannot differentiate between Basic and Privacy CA from the attestation data. Since both attestation data have same structure having x5c. So, if x5c is present, it would be Basic or Privacy CA attestation type. Thus, the verification procedures for both are same.
The thing is that there is no way for RP to know that received attestation is Basic or Privacy type. Sometime, RP may want to get attestation types for evaluate security and privacy risk.

@nadalin nadalin modified the milestones: CR, PR Nov 9, 2017
@equalsJeffH
Copy link
Contributor

equalsJeffH commented Nov 9, 2017

@jcjones notes in webauthn f2f tpac 2017-11-09 that we can push this to PR, we can address it with non-nomative editorial language. the present text is misleading.

@jcjones
Copy link
Contributor Author

jcjones commented Nov 9, 2017

The verification procedures still need to be updated, but I fixed the omission of Privacy CA for U2F in 5f4f3e6

@equalsJeffH
Copy link
Contributor

fyi: "privacy CA" is now "attestation CA", I believe.

@selfissued
Copy link
Contributor

Per the 28-Feb-18 call, @jcjones will verify that this has been addressed.

@jcjones
Copy link
Contributor Author

jcjones commented Feb 28, 2018

I confirm this has been addressed. Closing this issue.

@jcjones jcjones closed this as completed Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants