[3/3] Keydata v3 TPM platform changes by sespiros · Pull Request #271 · canonical/secboot

sespiros · 2023-11-17T21:22:59Z

Original commit: chrisccoulson@87d9aa3

I will split the commit above in 3 separate cascading PRs and add tests for it:

This PR is rebased on top of 2. It adds the changes to the TPM platform that use the new v3 cross-platform API + fixes existing tests and adds a few more.

keyDataVersion should be exported so that platform implementations can refer to it.

under the new design we change how we compute the authorization policies of the PCR policy counter: - the new role field is used to limit the scope of signed policy by including it in the policy calculation hash. - we change how we use the revocation NV index which is now going to be incremented on certain authorization policy updates (and this is reflected by the new policies around CommandNVIncrement)

Decrypt for v3 keyData objects will perform the AES-GCM decryption operation. This is not implemented for versions of keyData objects less than v3.

new v3 keys are decrypted using AES-GCM. The auth value derivation operation has been moved to the platform agnostic API. Legacy keys are handled separately in tpm2/keydata_legacy.go and RecoverKeys() will now marshal them using ASN1.

New v3 keys will use a PCR policy counter (NV index) with the TPMA_NV_POLICYREAD attribute set. This policy restricts read access to the index value only if the policy is satisfied. createPcrPolicyCounter was split into 2 functions: - ensurePcrPolicyCounter for v3 - createPcrPolicyCounterLegacy for the rest

under the new v3 design the role field is taken into account when doing auth policy validation. For legacy keydata versions ValidateData will return error if the role argument is non-empty.

pass the role field when validating data

For keydata v3, role needs to be included in data validation since it's part of the PCR policy. For legacy keydata, empty role is passed.

updatePCRProtectionPolicyImpl was renamed to updatePCRProtectionPolicyNoValidate. It also now receives an extra argument that instructs it to increment the counter or not. The new counter value is passed as the policySequence argument to UpdatePCRPolicy.

…alidate

The following new APIs were introduced: - NewTPMProtectedKey (was ProtectKeyWithTPM) - NewTPMPassphraseProtectedKey - NewExternalTPMProtectedKey (was ProtectKeyWithExternalStorageKey) The following APIs were removed: - ProtectKeyWithTPM - ProtectKeysWithTPM - ProtectKeyWithExternalStorageKey In v3 several values will be now derived from a single primary key, such as the disk unlock key and the passphrase-derived key that encrypts the payload. makeKeyData(), makeKeyDataPolicy() and makeKeyDataWithPolicy() were merged in makeSealedKeyData(). The new encrypted payload is encrypted using an AEAD scheme (AES-GCM) and all keys .

this is now a property of the keydata object.

pedronis

did a first pass, seal.go is nicely streamlined

pedronis · 2024-02-28T16:53:25Z

-	sha256Oid             = asn1.ObjectIdentifier{2, 16, 840, 1, 101, 3, 4, 2, 1}
-	sha384Oid             = asn1.ObjectIdentifier{2, 16, 840, 1, 101, 3, 4, 2, 2}
-	sha512Oid             = asn1.ObjectIdentifier{2, 16, 840, 1, 101, 3, 4, 2, 3}
+	KeyDataGeneration = 2


this needs a doc comment now, also as style is probably better to use a separate var stanza for exported vs not exported vars

I've added a comment to this

pedronis · 2024-02-28T16:56:11Z

-
-// MarshalKeys serializes the supplied disk unlock key and auxiliary key in
-// to a format that is ready to be encrypted by a platform's secure device.
-func MarshalKeys(key DiskUnlockKey, auxKey PrimaryKey) []byte {


snapd is calling this to interact with fde hooks that support only V1 keys? I don't remember, do we still have a way to make v1 keys if needed (non-TPM ones to be precise)?

pedronis · 2024-02-28T19:35:14Z

+			if isKeyDataError(err) {
+				return InvalidKeyDataError{fmt.Sprintf("%v (%d)", err.Error(), i+1)}
+			}
+			return xerrors.Errorf("cannot validate related key data: %w", err)


this error path doesn't seem tested

This code was copied from updateKeyPCRProtectionPoliciesCommon where it was also not tested. It's probably worth adding a case for this in a follow-up.

pedronis · 2024-02-28T20:09:02Z

 // PCR policy computed from the supplied PCRProtectionProfile. They must be persisted using
 // secboot.KeyData.WriteAtomic.
-func UpdateKeyDataPCRProtectionPolicy(tpm *Connection, authKey secboot.PrimaryKey, pcrProfile *PCRProtectionProfile, keys ...*secboot.KeyData) error {
+func UpdateKeyDataPCRProtectionPolicy(tpm *Connection, authKey secboot.PrimaryKey, pcrProfile *PCRProtectionProfile, incrementPolicyVersion bool, keys ...*secboot.KeyData) error {


this function is not tested afaict

I've added a test for this one now.

pedronis · 2024-02-28T20:12:33Z

-func (k *sealedKeyDataBase) updatePCRProtectionPolicyImpl(tpm *tpm2.TPMContext, key secboot.PrimaryKey,
-	counterPub *tpm2.NVPublic, profile *PCRProtectionProfile, session tpm2.SessionContext) error {
+func (k *sealedKeyDataBase) updatePCRProtectionPolicyNoValidate(tpm *tpm2.TPMContext, key secboot.PrimaryKey,
+	counterPub *tpm2.NVPublic, profile *PCRProtectionProfile, incrementPolicyVersion bool,


using a bool for incrementPolicyVersion is not very readable, we usually work around that keeping the bool type but always passing a const in the invocations:

const incrementPolicyVersion = true|false
updatePCRProtectionPolicyNoValidate(..., incrementPolicyVersion, ...)

or we need to switch the param to an enum or pass an option struct, the latter is not a obvious change here though

I've cleaned this up a bit with an enumerated type. I also ended up restoring the old behaviour for the SealedKeyObject type where every update increments the counter as opposed to changing the behaviour in this PR, so this function has 3 options now.

pedronis · 2024-02-28T20:13:59Z


-func (k *sealedKeyDataBase) revokeOldPCRProtectionPoliciesImpl(tpm *tpm2.TPMContext, key secboot.PrimaryKey, session tpm2.SessionContext) error {
-	pcrPolicyCounterPub, err := k.validateData(tpm, session)
+func (k *sealedKeyDataBase) updatePCRProtectionPolicy(tpm *tpm2.TPMContext, authKey secboot.PrimaryKey, role string, pcrProfile *PCRProtectionProfile, incrementPolicyVersion bool, session tpm2.SessionContext) error {


same issue with the bool

I've added a public enumerated type for the new SealedKeyData API to replace this.

pedronis · 2024-02-28T20:30:34Z

-	}
-	stream := cipher.NewCFBEncrypter(b, symKey[32:])
-	stream.XORKeyStream(payload, payload)
+func (fn keyDataConstructor) NewKeyData(skd *SealedKeyData, role string, encryptedPayload []byte, kdfAlg crypto.Hash) (*secboot.KeyData, error) {


I don't find the indirection of a method on a func type more readable or idiomatic of simply using the func type alone but maybe I'm missing something

I've removed the indirections here.

pedronis · 2024-02-28T20:30:47Z

+}

-	kd, err := secbootNewKeyData(&secboot.KeyParams{
+var makeKeyDataNoAuth keyDataConstructor = func(skd *SealedKeyData, role string, encryptedPayload []byte, kdfAlg crypto.Hash) (*secboot.KeyData, error) {


same for the var def vs just func definition

Ditto - this is just a normal function definition now.

pedronis · 2024-02-28T20:35:15Z

+			if err == nil {
+				return
+			}
+			tpm.NVUndefineSpace(tpm.OwnerHandleContext(), index, hmacSession)


this bit of undo logic does not seem tested

This code was copied from createPcrPolicyCounter where this path was also untested. It's a bit tricky to test this case without mocking the calls to go-tpm2 and there's a bunch of other paths that depend on TPM errors that aren't tested.

I think it's worth addressing these in follow-ups, but by:

adding an API to github.com/canonical/go-tpm2/mssim to put the simulator into failure mode.

adding an API to github.com/canonical/go-tpm2/testutil to call the new API after a specific number of commands are executed successfully.

using the new APIs here to trigger failures at specific points.

pedronis · 2024-02-28T20:39:32Z

-		}
-	}
-}
-


explanation PR comments or pointers to relevant commit messages if they exist of why the various tests here have been dropped would be good? some are obviously tied to model auth which is gone, but for some is less obvious why they are gone

It looks like these are dropped because there's not an equivalent API that creates multiple keys in a single call, as we aren't sharing PCR policies anymore.

This replaces the bool argument with an enum. It also restores the previous behavior for keys that use the SealedKeyObject type (where all policy updates increment the policy version).

…ameter

…ckage

…quence

chrisccoulson

I've responded to some of your comments now (I had an in-progress review that I had to finish to release them)

chrisccoulson · 2024-03-05T11:18:58Z

-	sha256Oid             = asn1.ObjectIdentifier{2, 16, 840, 1, 101, 3, 4, 2, 1}
-	sha384Oid             = asn1.ObjectIdentifier{2, 16, 840, 1, 101, 3, 4, 2, 2}
-	sha512Oid             = asn1.ObjectIdentifier{2, 16, 840, 1, 101, 3, 4, 2, 3}
+	KeyDataGeneration = 2


I've added a comment to this

chrisccoulson · 2024-03-05T11:20:07Z

+			if isKeyDataError(err) {
+				return InvalidKeyDataError{fmt.Sprintf("%v (%d)", err.Error(), i+1)}
+			}
+			return xerrors.Errorf("cannot validate related key data: %w", err)


This code was copied from updateKeyPCRProtectionPoliciesCommon where it was also not tested. It's probably worth adding a case for this in a follow-up.

chrisccoulson · 2024-03-05T11:20:17Z

 // PCR policy computed from the supplied PCRProtectionProfile. They must be persisted using
 // secboot.KeyData.WriteAtomic.
-func UpdateKeyDataPCRProtectionPolicy(tpm *Connection, authKey secboot.PrimaryKey, pcrProfile *PCRProtectionProfile, keys ...*secboot.KeyData) error {
+func UpdateKeyDataPCRProtectionPolicy(tpm *Connection, authKey secboot.PrimaryKey, pcrProfile *PCRProtectionProfile, incrementPolicyVersion bool, keys ...*secboot.KeyData) error {


I've added a test for this one now.

chrisccoulson · 2024-03-05T11:21:33Z

-func (k *sealedKeyDataBase) updatePCRProtectionPolicyImpl(tpm *tpm2.TPMContext, key secboot.PrimaryKey,
-	counterPub *tpm2.NVPublic, profile *PCRProtectionProfile, session tpm2.SessionContext) error {
+func (k *sealedKeyDataBase) updatePCRProtectionPolicyNoValidate(tpm *tpm2.TPMContext, key secboot.PrimaryKey,
+	counterPub *tpm2.NVPublic, profile *PCRProtectionProfile, incrementPolicyVersion bool,


I've cleaned this up a bit with an enumerated type. I also ended up restoring the old behaviour for the SealedKeyObject type where every update increments the counter as opposed to changing the behaviour in this PR, so this function has 3 options now.

chrisccoulson · 2024-03-05T11:21:57Z


-func (k *sealedKeyDataBase) revokeOldPCRProtectionPoliciesImpl(tpm *tpm2.TPMContext, key secboot.PrimaryKey, session tpm2.SessionContext) error {
-	pcrPolicyCounterPub, err := k.validateData(tpm, session)
+func (k *sealedKeyDataBase) updatePCRProtectionPolicy(tpm *tpm2.TPMContext, authKey secboot.PrimaryKey, role string, pcrProfile *PCRProtectionProfile, incrementPolicyVersion bool, session tpm2.SessionContext) error {


I've added a public enumerated type for the new SealedKeyData API to replace this.

chrisccoulson · 2024-03-05T11:22:07Z

-	}
-	stream := cipher.NewCFBEncrypter(b, symKey[32:])
-	stream.XORKeyStream(payload, payload)
+func (fn keyDataConstructor) NewKeyData(skd *SealedKeyData, role string, encryptedPayload []byte, kdfAlg crypto.Hash) (*secboot.KeyData, error) {


I've removed the indirections here.

chrisccoulson · 2024-03-05T11:22:20Z

+}

-	kd, err := secbootNewKeyData(&secboot.KeyParams{
+var makeKeyDataNoAuth keyDataConstructor = func(skd *SealedKeyData, role string, encryptedPayload []byte, kdfAlg crypto.Hash) (*secboot.KeyData, error) {


Ditto - this is just a normal function definition now.

chrisccoulson · 2024-03-05T11:24:43Z

+			if err == nil {
+				return
+			}
+			tpm.NVUndefineSpace(tpm.OwnerHandleContext(), index, hmacSession)


This code was copied from createPcrPolicyCounter where this path was also untested. It's a bit tricky to test this case without mocking the calls to go-tpm2 and there's a bunch of other paths that depend on TPM errors that aren't tested.

I think it's worth addressing these in follow-ups, but by:

adding an API to github.com/canonical/go-tpm2/mssim to put the simulator into failure mode.

adding an API to github.com/canonical/go-tpm2/testutil to call the new API after a specific number of commands are executed successfully.

using the new APIs here to trigger failures at specific points.

chrisccoulson · 2024-03-05T11:26:06Z

-		}
-	}
-}
-


It looks like these are dropped because there's not an equivalent API that creates multiple keys in a single call, as we aren't sharing PCR policies anymore.

sespiros · 2024-03-06T09:04:41Z

+	// incrementPolicyVersion indicates that the new policy version should be the
+	// previous policy version plus 1.
+	incrementPcrPolicyVersion


I got slightly confused by this and how it differs from the newPcrPolicyVersion option. Should we expand its comment a bit to indicate that this is used only by legacy/v0,v1,v2 TPM keydata versions? (I believe the rest of the difference in logic can be inferred by following the corresponding flows and reading other doc comments)

I agree that some further clarification about the 3 modes would be good

pedronis · 2024-03-08T19:09:10Z

 	}

-	pcrData.addRevocationCheck(trial, params.policyCounterName, params.policySequence+1)
+	pcrData.addRevocationCheck(trial, params.policyCounterName, params.policySequence)


why this change?

This PR moves incrementing the value earlier so the change here avoids a double increment that I'd missed earlier.

thanks, I followed the code a bit more and makes sense

pedronis · 2024-03-08T19:10:29Z


 	if params.policyCounterName != nil {
-		pcrData.addRevocationCheck(trial, params.policyCounterName, params.policySequence+1)
+		pcrData.addRevocationCheck(trial, params.policyCounterName, params.policySequence)


same question

pedronis · 2024-03-08T19:16:05Z

+	// incrementPolicyVersion indicates that the new policy version should be the
+	// previous policy version plus 1.
+	incrementPcrPolicyVersion


I agree that some further clarification about the 3 modes would be good

pedronis

some questions

pedronis

+1, reminder about the discussion/suggestion here though: #271 (review)

chrisccoulson · 2024-03-11T23:01:31Z

+1, reminder about the discussion/suggestion here though: #271 (review)

Thanks, I've added a comment here.

I think I've had an idea for how to clean this up and remove these values (moving the logic to the tpm2/policy_v* implementations and supplying the counter value as an argument instead), but I'll leave that for a follow-up.

sespiros force-pushed the keydata-v3-platform-tpm branch from 5c8317a to ef66445 Compare November 21, 2023 15:06

This was referenced Nov 21, 2023

[1/3] Keydata v3 platform API changes #265

Merged

[2/3] Keydata v3 scope changes #270

Merged

sespiros force-pushed the keydata-v3-platform-tpm branch 2 times, most recently from 934b170 to e30fd0c Compare November 23, 2023 10:31

sespiros requested a review from chrisccoulson December 4, 2023 12:52

sespiros force-pushed the keydata-v3-platform-tpm branch from e30fd0c to 83ad83a Compare February 7, 2024 19:26

sespiros force-pushed the keydata-v3-platform-tpm branch from 83ad83a to bc9ee04 Compare February 28, 2024 00:47

Spyros Seimenis added 22 commits February 28, 2024 15:16

keydata.go: export keyDataVersion

3b4d174

keyDataVersion should be exported so that platform implementations can refer to it.

keydata_legacy_test.go: use new MarshalV1Keys in legacy tests

a1af571

tpm2/policy.go: add role field to pcrPolicyParams

25be9d1

tpm2/keydata*.go: add new keyData Decrypt API

38a3888

Decrypt for v3 keyData objects will perform the AES-GCM decryption operation. This is not implemented for versions of keyData objects less than v3.

tpm2/keydata.go: use new PlatformKeyData API

e31fb11

new v3 keys are decrypted using AES-GCM. The auth value derivation operation has been moved to the platform agnostic API. Legacy keys are handled separately in tpm2/keydata_legacy.go and RecoverKeys() will now marshal them using ASN1.

tpm2/policy_v*.go: refactor AddPcrAssertions

0ef1774

tpm2/policy*.go: pass policySequence as an argument

612db1b

tpm2/seal_legacy.go: use new createPcrPolicyCounterLegacy

d9539e7

tpm2/policy{,_v3}.go: run PolicyAuthValue only when required

510c72b

tpm2/keydata*.go: add new v3 keyData validation methods

4e4e471

under the new v3 design the role field is taken into account when doing auth policy validation. For legacy keydata versions ValidateData will return error if the role argument is non-empty.

tpm2/platform.go: use new v3 keyData validation in ChangeAuthKey

1637206

pass the role field when validating data

tpm2/update{,_legacy}.go: use new v3 keydata in revocations

adf4015

For keydata v3, role needs to be included in data validation since it's part of the PCR policy. For legacy keydata, empty role is passed.

tpm2/seal.go: add separate constructor for keydata with passphrase

f035251

tpm2/seal_legacy.go: make use of the new updatePCRProtectionPolicyNoV…

430ed44

…alidate

tpm2/update{,_legacy.go}: add v3 support and refactor

fbc7301

keydata.go: remove MarshalKeys()

80fe2af

tpm2/keydata_v*_test.go: fix/add new tests for v3

0a18185

tpm2/keydata_v3.go: verify PolicyAuthValue only when required

508881f

this is now a property of the keydata object.

Spyros Seimenis added 3 commits February 28, 2024 15:16

tpm2: fix tests for new v3 API

6b18839

tpm2/update_legacy: fix legacy tests

e290c7d

tpm2: rename base version to generation

4facbee

sespiros force-pushed the keydata-v3-platform-tpm branch from bc9ee04 to 4facbee Compare February 28, 2024 13:16

pedronis self-requested a review February 28, 2024 13:18

pedronis reviewed Feb 28, 2024

View reviewed changes

chrisccoulson added 10 commits March 1, 2024 11:10

keydata: Add doc comment for KeyDataGeneration

df2f8c0

tpm2: Add a test for UpdateKeyDataPCRProtectionPolicy

734a8ea

tpm2: Add an enum argument to UpdatePCRProtectionPolicy

917710e

This replaces the bool argument with an enum. It also restores the previous behavior for keys that use the SealedKeyObject type (where all policy updates increment the policy version).

tpm2: remove the indirection from keyDataConstructor

a87b728

tpm2: rename baseVersion to generation

2008d6e

tpm2: add a comment explaining why we don't authenticate the role par…

18e407e

…ameter

tpm2: add a comment explaining what the constant uint32(3) is

d380e3a

Move MarshalV1Keys from export_test.go to secboot_test package

40d3ad0

tpm2: move UnmarshalProtectedKeys from export_test.go to tpm2_test pa…

5f09ab2

…ckage

tpm2: make newMockKeyData return an object with the correct policy se…

f01b4ac

…quence

chrisccoulson reviewed Mar 5, 2024

View reviewed changes

pedronis self-requested a review March 5, 2024 13:54

sespiros commented Mar 6, 2024

View reviewed changes

pedronis reviewed Mar 8, 2024

View reviewed changes

pedronis self-requested a review March 11, 2024 16:00

pedronis approved these changes Mar 11, 2024

View reviewed changes

tpm2: add some comments to pcrPolicyVersionOption

984731e

chrisccoulson merged commit 691fc82 into canonical:master Mar 11, 2024

Uh oh!

Conversation

sespiros commented Nov 17, 2023

Uh oh!

pedronis 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!

pedronis Feb 28, 2024 • 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!

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!

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!

chrisccoulson 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!

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!

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!

chrisccoulson Mar 11, 2024 • 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!

pedronis Feb 28, 2024 •

edited

Loading

chrisccoulson Mar 11, 2024 •

edited

Loading