Added TPM verification clarifications #966

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

Ref #929

Preview | Diff

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

cc @akshayku

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

cc @apowers313

herrjemand · 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

index.bs

    - Note that the remaining fields in the "Standard Attestation Structure" [[TPMv2-Part1]] 
        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:
    - 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

index.bs

@@ -3430,15 +3430,16 @@ engine.
    - Verify that `extraData` is set to the hash of |attToBeSigned| using the hash algorithm employed in "alg".
    - Verify that `attested` contains a `TPMS_CERTIFY_INFO` structure as specified in [[TPMv2-Part2]] section 10.12.3,
        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

index.bs

-        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

index.bs

    - Note that the remaining fields in the "Standard Attestation Structure" [[TPMv2-Part1]] 
        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:
    - 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

index.bs

@@ -3430,15 +3430,16 @@ engine.
    - Verify that `extraData` is set to the hash of |attToBeSigned| using the hash algorithm employed in "alg".
    - Verify that `attested` contains a `TPMS_CERTIFY_INFO` structure as specified in [[TPMv2-Part2]] section 10.12.3,
        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

index.bs

-        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.
    - Note that the remaining fields in the "Standard Attestation Structure" [[TPMv2-Part1]] 
        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 herrjemand 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 herrjemand, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added TPM verification clarifications #966

Added TPM verification clarifications #966

herrjemand commented Jun 22, 2018 •

edited by pr-preview bot

herrjemand commented Jun 22, 2018

herrjemand commented Jun 22, 2018

herrjemand commented Jun 22, 2018

equalsJeffH left a comment

equalsJeffH Jun 25, 2018

equalsJeffH Jun 26, 2018

equalsJeffH Jun 25, 2018 •

edited

herrjemand Jun 25, 2018

emlun Jun 26, 2018

equalsJeffH Jun 26, 2018

bradlitterell May 13, 2020

akshayku May 20, 2020

equalsJeffH Jun 25, 2018 •

edited

equalsJeffH commented Jun 25, 2018

emlun left a comment

emlun Jun 26, 2018

equalsJeffH Jun 26, 2018 •

edited

emlun Jun 26, 2018

equalsJeffH Jun 26, 2018

apowers313 commented Jun 26, 2018

equalsJeffH commented Apr 17, 2019

akshayku commented Jun 3, 2020

Added TPM verification clarifications #966

Added TPM verification clarifications #966

Conversation

herrjemand commented Jun 22, 2018 • edited by pr-preview bot

herrjemand commented Jun 22, 2018

herrjemand commented Jun 22, 2018

herrjemand commented Jun 22, 2018

equalsJeffH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

equalsJeffH Jun 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

equalsJeffH Jun 25, 2018 • edited

Choose a reason for hiding this comment

equalsJeffH commented Jun 25, 2018

emlun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

equalsJeffH Jun 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apowers313 commented Jun 26, 2018

equalsJeffH commented Apr 17, 2019

akshayku commented Jun 3, 2020

herrjemand commented Jun 22, 2018 •

edited by pr-preview bot

equalsJeffH Jun 25, 2018 •

edited

equalsJeffH Jun 25, 2018 •

edited

equalsJeffH Jun 26, 2018 •

edited