-
Notifications
You must be signed in to change notification settings - Fork 60
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
wolfTPM Support for sealing/unsealing based on a PCR that is signed externally #294
Conversation
…xternally. Use an external key to sign a PCR digest. Allows a new signed policy to be sent with updates to continue allowing a sealed secret to be unsealed when PCR's change. This resolves the issue with PCR brittleness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
general review, found a few issues
examples/boot/secret_seal.c
Outdated
|
||
/* Start an authenticated session (salted / unbound) */ | ||
rc = wolfTPM2_StartSession(&dev, &tpmSession, &storage, NULL, | ||
TPM_SE_HMAC, paramEncAlg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be TPM_SE_POLICY? I thought you had to use a policy session or a trial session to do make policies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a sealed object does not require a policy session. We have a signed PCR policy digest we write when creating the keyed hashed (seal) object. No policy session is needed.
printf("Session Handle 0x%x\n", (word32)tpmSession.handle.hndl); | ||
printf("Parameter Encryption: %s\n", TPM2_GetAlgName(paramEncAlg)); | ||
|
||
rc = wolfTPM2_SetAuthSession(&dev, 1, &tpmSession, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use auth index 0, otherwise I think it uses the default session handle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will not enable parameter encryption if index 0 is used.
|
||
/* Load Key Public Info */ | ||
#if !defined(NO_FILESYSTEM) | ||
static int LoadAuthKeyInfo(WOLFTPM2_DEV* dev, WOLFTPM2_KEY* authKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not DRY, maybe this should go in the test common code, along with the hex to byte we repeat everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the code for this is already in wolfTPM2_ImportPublicKeyBuffer. This is just to load a file, which I don't want in the library proper.
printf("Parameter Encryption: %s\n", TPM2_GetAlgName(paramEncAlg)); | ||
|
||
/* enable parameter encryption for unseal */ | ||
rc = wolfTPM2_SetAuthSession(&dev, 1, &tpmSession, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use session index 0
examples/pcr/policy_sign.c
Outdated
return rc; | ||
} | ||
|
||
if (XSTRNCMP(keyFile, ".pem", XSTRLEN(".pem")) == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't seem right, wouldn't this just check the first 4 characters of the filename to match .pem?
if (rc == 0) { | ||
if (authKey->pub.publicArea.type == TPM_ALG_ECC) { | ||
sigAlg = authKey->pub.publicArea.parameters.eccDetail.scheme.scheme; | ||
rc = TPM2_GetHashType(pcrAlg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why set rc if you're not going to check it before overwriting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just realized you cast it to hashType, whoops
&policyDigestSz); | ||
} | ||
inSz = *digestSz; /* capture input digest size (for policyDigestOld) */ | ||
rc = TPM2_GetHashType(pcrAlg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why set rc if you're going to overwrite it before checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't found a better way to solve this. Same pattern is used elsewhere. Resolves a compiler warning about int to enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I didn't see hashType was being set to it, which is fine. I thought it was being overwritten by 7163
src/tpm2_wrap.c
Outdated
policyDigestSz, sizeof(TPM_ALG_ID)); | ||
/* policyRef */ | ||
if (rc == 0 && policyRefSz > 0) { | ||
rc = wc_HashUpdate(&hash_ctx, hashType, digest, inSz); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should hash policyRef, not digest
policyAuthNVIn->authHandle = parent->hndl; | ||
|
||
rc = TPM2_PolicyAuthorizeNV(policyAuthNVIn); | ||
rc = wc_HashFinal(&hash_ctx, hashType, digest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will overwrite the contents of digest, might not be wise if the caller needs the buffer's contents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I designed it this way. The "digest" are in both input and output (as documented). This is because some policies take in an "old" digest to get a new one (chaining them).
policyAuthNVIn->authHandle = parent->hndl; | ||
|
||
rc = TPM2_PolicyAuthorizeNV(policyAuthNVIn); | ||
rc = wc_HashFinal(&hash_ctx, hashType, digest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this also overwrites digest, this might be okay since it's the new policyDigest anyway right?
b9858a7
to
c2191c5
Compare
… avoid parallel builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, CI running all the examples is awesome
Use an external key to sign a PCR digest. Allows a new signed policy to be sent with updates to continue allowing a sealed secret to be unsealed when PCR's change. This resolves the issue with PCR brittleness.
Removed experimental policy examples (will put back as draft PR).