Added TPM verification clarifications by yackermann · Pull Request #966 · w3c/webauthn

yackermann · 2018-06-22T14:38:10Z

Ref #929

Preview | Diff

yackermann · 2018-06-22T14:38:20Z

cc @akshayku

yackermann · 2018-06-22T14:38:27Z

cc @apowers313

yackermann · 2018-06-22T15:08:38Z

Ref #950

equalsJeffH

thanks for clarifying this stuff @herrjemand -- just a few rough edges to polish it seems.

equalsJeffH · 2018-06-25T17:48:14Z

    If |x5c| is present, this indicates that the attestation type is not [=ECDAA=]. In this case:
    - Verify the |sig| is a valid signature over |certInfo| using the attestation public key in |x5c| with the
        algorithm specified in |alg|.
+    - Verify certificate chain in |x5c| against attestationRootCertificates in Metadata Statement specified by AAGUID in authData.


we should have this match how we elsewhere mention the metadata service.
suggest:

- If the prior step was successful, obtain a list of acceptable trust anchors (attestation root certificates) for the [=AttCA=] attestation type and attestation statement format `tpm`, from a trusted source or from policy. For example, the FIDO Metadata Service [[FIDOMetadataService]] provides one way to obtain such information, using the <code>[=aaguid=]</code> in the <code>[=attestedCredentialData=]</code> in |authenticatorData|. - Verify certificate chain in |x5c| against the trust anchors obtained in the prior step.

scratch the above, see below.

equalsJeffH · 2018-06-25T18:36:39Z

        whose `name` field contains a valid Name for |pubArea|,
-        as computed using the algorithm in the `nameAlg` field of |pubArea| using the procedure specified in [[TPMv2-Part1]]
-        section 16.
+        as computed by concatinating algorithm identifier from the signauture algorithm(for RS256 it's sha256) specified in TPM_ALG_ID, with hash of the `pubArea` field as specified in [[TPMv2-Part1]] section 16.


I'm thinking we do not want to get into this much detail since it is already spec'd in the TPM specs. Suggest doing only this:

as computed using the hash algorithm identified by the value of the `nameAlg` field of |pubArea| and using the procedure specified in section 16 of [[TPMv2-Part1]].

@equalsJeffH Navigating TPM specs is a hell. So I think we should be slightly more specific in this example

I agree with @equalsJeffH here. Duplicating instructions risks getting them wrong in subtle ways, and I think that risk outweighs the small possible convenience gain.

I agree that the TPM specs are difficult to decipher -- i spent a ton of time yesterday trying to figure out how the nameAlg field works, and am not sure my interpretation is correct -- hence not wanting to bake errornous interp into webauthn, as @emlun notes.

Also agree - the TPM specs are difficult, so let's not add to confusion by potentially getting it different here.

@BradL-Msft Are other changes in this PR OK?

equalsJeffH · 2018-06-25T18:47:24Z

-        as computed using the algorithm in the `nameAlg` field of |pubArea| using the procedure specified in [[TPMv2-Part1]]
-        section 16.
+        as computed by concatinating algorithm identifier from the signauture algorithm(for RS256 it's sha256) specified in TPM_ALG_ID, with hash of the `pubArea` field as specified in [[TPMv2-Part1]] section 16.
+    - Verify that `pubArea` contains TPMU_PUBLIC_ID that is set to newly generate public key as specified in [[TPMv2-Part2]] section 12.2.3.2.


newly generate -> the newly generated
as specified in [[TPMv2-Part2]] section 12.2.3.2 -> as specified in sections 12.2.3.2 and 12.2.4 of [[TPMv2-Part2]].

equalsJeffH · 2018-06-25T18:58:55Z

is this intended to fix issue #929 ? I'm not sure what you mean by "ref #950" and "ref #929" ?

emlun

Thanks for writing this up, but I agree with @equalsJeffH that it's probably best to not include all of it.

emlun · 2018-06-26T14:21:49Z

    If |x5c| is present, this indicates that the attestation type is not [=ECDAA=]. In this case:
    - Verify the |sig| is a valid signature over |certInfo| using the attestation public key in |x5c| with the
        algorithm specified in |alg|.
+    - Verify certificate chain in |x5c| against attestationRootCertificates in Metadata Statement specified by AAGUID in authData.


I'd argue this is already covered by steps 15 and 16 in §7.1. Relying Party Operations: Registering a new credential, and doesn't need to be repeated here.

perhaps steps 15 and 16 in §7.1. Relying Party Operations: Registering a new credential can be appropriately referenced here -- we can add anchors to individual steps if we wish -- see step 13 in S 6.2.3.

emlun · 2018-06-26T14:31:19Z

        whose `name` field contains a valid Name for |pubArea|,
-        as computed using the algorithm in the `nameAlg` field of |pubArea| using the procedure specified in [[TPMv2-Part1]]
-        section 16.
+        as computed by concatinating algorithm identifier from the signauture algorithm(for RS256 it's sha256) specified in TPM_ALG_ID, with hash of the `pubArea` field as specified in [[TPMv2-Part1]] section 16.


I agree with @equalsJeffH here. Duplicating instructions risks getting them wrong in subtle ways, and I think that risk outweighs the small possible convenience gain.

equalsJeffH · 2018-06-26T15:53:17Z

        section 31.2, i.e., `qualifiedSigner`, `clockInfo` and `firmwareVersion` are ignored.
        These fields MAY be used as an input to risk engines.

    If |x5c| is present, this indicates that the attestation type is not [=ECDAA=]. In this case:


perhaps this ought to be rewritten as:
If |x5c| is present, the attestation type is [=AttCA=].

..?

apowers313 · 2018-06-26T17:44:56Z

For anyone else that wants to play along from home, here is some help understanding TPM. I'll probably stick these in a blog post or something in the near future:

tpm-decode.txt

equalsJeffH · 2019-04-17T16:53:53Z

Note: issue #950 is closed -- at least a portion of what this PR is trying to accomplish was addressed in PR #952.

This seems to be a "nice to have" sort of spec-polishing, I suggest we keep it open. I marked it as low priority.

akshayku · 2020-06-03T18:46:41Z

After more talking internally, it looks like we don't want to duplicate things that are already in TPM specs. Which has a risk of going out of sync. If TPM specs are hard to read, that should be solved somewhere else.

I would propose to close this.

Added TPM verification clarification

a0481de

equalsJeffH suggested changes Jun 25, 2018

View reviewed changes

emlun requested changes Jun 26, 2018

View reviewed changes

equalsJeffH reviewed Jun 26, 2018

View reviewed changes

equalsJeffH assigned yackermann Jun 26, 2018

nadalin added the type:editorial label Jun 27, 2018

nadalin added this to the REC milestone Jun 27, 2018

nadalin added the type:technical label Jun 27, 2018

nadalin modified the milestones: REC, PropRec, L2-WD-00 Jun 27, 2018

emlun mentioned this pull request Jul 4, 2018

Decoding TPM certInfo and pubArea? #984

Closed

nadalin modified the milestones: L2-WD-00, L2-WD-01 Feb 13, 2019

equalsJeffH added the priority:low label Apr 17, 2019

akshayku self-requested a review May 29, 2019 19:14

equalsJeffH assigned akshayku and unassigned akshayku May 29, 2019

nadalin assigned akshayku May 29, 2019

jafisher-microsoft self-assigned this Jun 19, 2019

nadalin modified the milestones: L2-WD-02, L2-WD-03 Aug 28, 2019

akshayku unassigned yackermann, jafisher-microsoft and akshayku Jun 3, 2020

akshayku closed this Jun 3, 2020

equalsJeffH mentioned this pull request Jun 29, 2020

TPM section is missing TPMT_PUBLIC validation steps #929

Closed

Uh oh!

Conversation

yackermann commented Jun 22, 2018 • edited by pr-preview Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yackermann commented Jun 22, 2018

Uh oh!

yackermann commented Jun 22, 2018

Uh oh!

yackermann commented Jun 22, 2018

Uh oh!

equalsJeffH left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

equalsJeffH Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

equalsJeffH Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

equalsJeffH commented Jun 25, 2018

Uh oh!

emlun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

equalsJeffH Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apowers313 commented Jun 26, 2018

Uh oh!

equalsJeffH commented Apr 17, 2019

Uh oh!

akshayku commented Jun 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

yackermann commented Jun 22, 2018 •

edited by pr-preview Bot

Loading

equalsJeffH Jun 25, 2018 •

edited

Loading

equalsJeffH Jun 25, 2018 •

edited

Loading

equalsJeffH Jun 26, 2018 •

edited

Loading