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.

Sign up for GitHub

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

Jump to bottom

tpm2: add tpm2_get_pin_auth() #28414

Merged
merged 1 commit into from
Jul 16, 2023
Merged

tpm2: add tpm2_get_pin_auth() #28414

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions src/shared/tpm2-util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2244,6 +2244,34 @@ int tpm2_digest_many_digests(
return tpm2_digest_many(alg, digest, iovecs, n_data, extend);
}

/* This hashes the provided pin into a digest value, but also verifies that the final byte is not 0, because
* the TPM specification Part 1 ("Architecture") section Authorization Values (subsection "Authorization Size
* Convention") states "Trailing octets of zero are to be removed from any string before it is used as an
* authValue". Since the TPM doesn't know if the auth value is a "string" or just a hash digest, any hash
* digest that randomly happens to end in 0 must have the final 0 changed, or the TPM will remove it before
* using the value in its HMAC calculations, resulting in failed HMAC checks. */
static int tpm2_get_pin_auth(TPMI_ALG_HASH hash, const char *pin, TPM2B_AUTH *ret_auth) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might as well drop the first argument. We chose the hash function ourselves here, and hence we can just hardcode it inside this function, no need to pass it in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was trying to leave is as easy as possible to allow using a different hash alg in the future, and reduce the number of places we have sha256 hardcoded.

TPM2B_AUTH auth = {};
int r;

assert(pin);
assert(ret_auth);

r = tpm2_digest_buffer(hash, &auth, pin, strlen(pin), /* extend= */ false);
if (r < 0)
return r;

assert(auth.size > 0);
if (auth.buffer[auth.size - 1] == 0) {
log_debug("authValue digest ends in 0 which the TPM will remove and cause HMAC authorization failures, adjusting.");
auth.buffer[auth.size - 1] = 0xff;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, i don't fully grok why this is necessary. can you explain?

I mean, as long as everyone drops the trailing NUL bytes we should be fine, no? or are you saying some code does and other code does not? can you be more specific on that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #27716 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not getting this. As long as the zeroes at the end are dropped systematically things should always result in the same state in the TPM, hence should be OK.

Why specifically does that trigger any issue? can you elaborate for somebody a bit thick, like me?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, while the tpm drops the trailing 0, tpm2-tss doesn't, and so when tpm2-tss makes the hmac calculations they don't match what the tpm is expecting. If/when tpm2-tss is fixed, we could drop this, but of course we'd then need to set a minimum tpm2-tss version (that includes the fix).

@williamcroberts do you happen to know if there's an open bug in tpm2-tss for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it's possible there are some tpms that dont drop the trailing 0 - as i noted in the issue, the spec is horribly ambiguous on this point; it says:

Trailing octets of zero are to be removed from any string before it is used as an authValue

but then also

If the password/phrase, with trailing zeros removed, is longer than the digest produced by the nameAlg of the object, then the password/phrase – with trailing octets of zero removed – is hashed using nameAlg and the resulting hash given to the TPM as the authValue for the object.

Without acknowledging that a hash digest can also end in a 0.

Basically, the safest thing to do to ensure all parts work correctly is to never, ever use a auth value that ends in a 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the TPM stops processing auth values when it hits a NUL byte. This was a bug in the earlier implementation, but they kept the behavior around. For clarity, consider you had the string "passw\0rd", the auth value would get truncated to "passw". Probably what I said is wrong, trying to dig that out of memory. @AndreasFuchsTPM might remember better.

Ahh I'm an idiot, I thought this was in the tpm2-tss code. Yeah their is no need to do this. The TPM will always trim the auth value on the ending NUL bytes... the TPM actually removes all trailing zeroes, this can be seen in: MemoryRemoveTrailingZeros() within CheckPWAuthSession() in SessionProcess.c

right, but tpm2-tss doesnt trim those trailing null bytes, which is what causes the HMAC errors...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see in the spec it does have a buried note that clarifies; in HMAC Computation under authValue there is "NOTE 2": Trailing zeros are always removed from an authValue before it is used in an authorization computation.

So yeah, this does seem like tpm2-tss should strip trailing 0's (probably in Esys_TR_SetAuth)?

Copy link
Contributor

@williamcroberts williamcroberts Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the TPM stops processing auth values when it hits a NUL byte. This was a bug in the earlier implementation, but they kept the behavior around. For clarity, consider you had the string "passw\0rd", the auth value would get truncated to "passw". Probably what I said is wrong, trying to dig that out of memory. @AndreasFuchsTPM might remember better.

Ahh I'm an idiot, I thought this was in the tpm2-tss code. Yeah their is no need to do this. The TPM will always trim the auth value on the ending NUL bytes... the TPM actually removes all trailing zeroes, this can be seen in: MemoryRemoveTrailingZeros() within CheckPWAuthSession() in SessionProcess.c

right, but tpm2-tss doesnt trim those trailing null bytes, which is what causes the HMAC errors...

Ahh OK yeah I see that linked issue now, makes sense. I filed a bug in tpm2-tss: tpm2-software/tpm2-tss#2664. However, the code here needs to strip all of them, just not one. But you can just wrap the calls to Esys_TR_SetAuth to essentially right trim the array and re-adjust the auth buffer size field, then you won't need to set a minimum ESYS version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ddstreet I can't reprod with the tpm2-tss stack, but I did file a bug 2664 and Juergen is looking into it on our side. However, can I nack this patch so it doesn't get released and could you respin a fix to address a passphrase ending in more than one zero byte (ie foopass\0\0\0\0\0\0\0), which is possible. It needs to rtrim all 0s.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ddstreet I can't reprod with the tpm2-tss stack, but I did file a bug 2664 and Juergen is looking into it on our side. However, can I nack this patch so it doesn't get released and could you respin a fix to address a passphrase ending in more than one zero byte (ie foopass\0\0\0\0\0\0\0), which is possible. It needs to rtrim all 0s.

since we're using a hash digest, the actual text passphrase doesn't really matter, right?

And also we shouldn't need to handle any 0's other than the last one, since we're not actually trimming it; we are adjusting it (from 0->0xff). So any 0's before it should be handled normally. No?


*ret_auth = TAKE_STRUCT(auth);

return 0;
}

static int tpm2_set_auth(Tpm2Context *c, const Tpm2Handle *handle, const char *pin) {
TPM2B_AUTH auth = {};
TSS2_RC rc;
Expand All @@ -2257,7 +2285,7 @@ static int tpm2_set_auth(Tpm2Context *c, const Tpm2Handle *handle, const char *p

CLEANUP_ERASE(auth);

r = tpm2_digest_buffer(TPM2_ALG_SHA256, &auth, pin, strlen(pin), /* extend= */ false);
r = tpm2_get_pin_auth(TPM2_ALG_SHA256, pin, &auth);
if (r < 0)
return r;

Expand Down Expand Up @@ -3228,7 +3256,7 @@ int tpm2_seal(const char *device,
CLEANUP_ERASE(hmac_sensitive);

if (pin) {
r = tpm2_digest_buffer(TPM2_ALG_SHA256, &hmac_sensitive.userAuth, pin, strlen(pin), /* extend= */ false);
r = tpm2_get_pin_auth(TPM2_ALG_SHA256, pin, &hmac_sensitive.userAuth);
if (r < 0)
return r;
}
Expand Down