tpm2: add tpm2_get_pin_auth() #28414

ddstreet · 2023-07-15T12:42:11Z

Add function to calculate the hash digest for a provided pin, and also verify that the final byte in the digest is not 0. This is required because the TPM will always remove all trailing 0's from an auth value before using it.

Fixes: #27716

src/shared/tpm2-util.c

YHNdnzj · 2023-07-15T13:44:04Z

src/shared/tpm2-util.c

+                auth.buffer[auth.size - 1] = 0xff;
+        }
+
+        *ret_auth = auth;


Suggested change

*ret_auth = auth;

*ret_auth = TAKE_STRUCT(auth);

ack good call, updated and pushed

I mean the CLEANUP_ERASE() will clean it up also so TAKE_STRUCT() isn't strictly needed, i guess we could just use one or the other, but doesn't hurt to use both

I removed the CLEANUP_ERASE() from tpm2_get_pin_auth() in favor of using TAKE_STRUCT(), figured we dont need both.

Add function to calculate the hash digest for a provided pin, and also verify that the final byte in the digest is not 0. This is required because the TPM will always remove all trailing 0's from an auth value before using it. Fixes: systemd#27716

poettering · 2023-07-16T08:41:07Z

src/shared/tpm2-util.c

+ * 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) {


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.

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.

poettering · 2023-07-16T08:41:55Z

src/shared/tpm2-util.c

+        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;
+        }


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?

see #27716 (comment)

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?

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?

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.

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...

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)?

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.

@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.

@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?

poettering · 2023-07-19T20:32:15Z

Given that theres apparently confusion regarding NULs in at least one implementation i wonder if it wouldnt be better to just map all zero bytes to something else (maybe to their index in the string mod 9 + 1?). Just to avoid any disambiguities if some implementation assumes this is a c string or so?

Either way the NUL mapping needs a compat flag in the json data since old enrollments should continue to work without the mapping and new enrollments should use the NUL mapping.

poettering · 2023-07-19T20:33:16Z

Alternatively maybe we should just base64 encode the hmac result and truncate and thus always pass in a valid ascii string. Would reduce entropy quite a bit though

williamcroberts · 2023-07-19T20:40:42Z

Given that theres apparently confusion regarding NULs in at least one implementation i wonder if it wouldnt be better to just map all zero bytes to something else (maybe to their index in the string mod 9 + 1?). Just to avoid any disambiguities if some implementation assumes this is a c string or so?

It drives me nuts that this bug made it into prod on the TPM code. As you mentioned above, another way is to just make authValue's strings. That's what I do in the tpm2-pkcs11 library. I use base16, but base64 would be preferable. It just avoids this.

Either way the NUL mapping needs a compat flag in the json data since old enrollments should continue to work without the mapping and new enrollments should use the NUL mapping.

Yeah, I guess just shove another bit in the flags?

ddstreet · 2023-07-19T22:00:35Z

Given that theres apparently confusion regarding NULs in at least one implementation i wonder if it wouldnt be better to just map all zero bytes to something else (maybe to their index in the string mod 9 + 1?). Just to avoid any disambiguities if some implementation assumes this is a c string or so?

Either way the NUL mapping needs a compat flag in the json data since old enrollments should continue to work without the mapping and new enrollments should use the NUL mapping.

It's definitely frustrating that the TPM spec added all the totally unnecessary wording about authValues being strings. It should have just required hashing and so all authValues would be exactly the length of the object hashing alg size and the actual bytes in the authValue data could be anything. I don't see the usefulness of the spec saying that trailing 0's should be removed.

For this specific case, I am pretty sure it's safe to adjust the trailing 0 byte, because any previous sealed data is guaranteed to have not worked at all. So it shouldn't break any existing sealed objects out there; I don't think we need to use any compat flag.

Also, I don't think we should convert all 0's, since it is likely there are working sealed objects out there with 0's in their auth value (since we've always converted pins to a hash digest).

One of the things I want to add (eventually) is to include versioning in the JSON for a sealed object. It's getting to the point where unsealing is too complex, without some kind of explicit version so we can know what the exact unsealing process needs to be.

williamcroberts · 2023-07-20T14:29:36Z

One of the things I want to add (eventually) is to include versioning in the JSON for a sealed object. It's getting to the point where unsealing is too complex, without some kind of explicit version so we can know what the exact unsealing process needs to be.

Yes my eyes bleed going through all the conditional statements to figure out what should be done based on what is NULL and what isn't.

For this specific case, I am pretty sure it's safe to adjust the trailing 0 byte, because any previous sealed data is guaranteed to have not worked at all. So it shouldn't break any existing sealed objects out there; I don't think we need to use any compat flag.

Contrary to my email in support of setting the last byte, it occurred to me now that if you ever want to support importing existing sealed keys, modifying the last byte over trimming it would mean you cant without having logic to trim these keys authValue bod modify these keys, instead of always trim. Plus if we trim, it will match ESAPI and when you set the minimum version you can drop this code.

…ired by tpm spec To keep compatibility with any existing object authValues with trailing 0's, change tpm2_get_pin_auth() to trim trailing 0's, which is what the TPM implementation will do. This should retain compatibility with any existing authValues that contain trailing 0's. Note that any existing authValues with trailing 0's are unlikely to have worked in the way that systemd uses them in object sealing, which is as a bind key for the encryption (and policy) session. However, it is better to be compatible with the TPM spec (and implementations) even if previously created objects that are affected may not have worked. Fixes: systemd#28414

ddstreet · 2023-07-21T15:29:45Z

One of the things I want to add (eventually) is to include versioning in the JSON for a sealed object. It's getting to the point where unsealing is too complex, without some kind of explicit version so we can know what the exact unsealing process needs to be.

Yes my eyes bleed going through all the conditional statements to figure out what should be done based on what is NULL and what isn't.

For this specific case, I am pretty sure it's safe to adjust the trailing 0 byte, because any previous sealed data is guaranteed to have not worked at all. So it shouldn't break any existing sealed objects out there; I don't think we need to use any compat flag.

Contrary to my email in support of setting the last byte, it occurred to me now that if you ever want to support importing existing sealed keys, modifying the last byte over trimming it would mean you cant without having logic to trim these keys authValue bod modify these keys, instead of always trim. Plus if we trim, it will match ESAPI and when you set the minimum version you can drop this code.

yeah, that makes sense, there is really no advantage to adjusting the final 0 instead of just trimming as required by spec. I opened #28486 can you take a look?

…ired by tpm spec To keep compatibility with any existing object authValues with trailing 0's, change tpm2_get_pin_auth() to trim trailing 0's, which is what the TPM implementation will do. This should retain compatibility with any existing authValues that contain trailing 0's. Note that any existing authValues with trailing 0's are unlikely to have worked in the way that systemd uses them in object sealing, which is as a bind key for the encryption (and policy) session. However, it is better to be compatible with the TPM spec (and implementations) even if previously created objects that are affected may not have worked. Fixes: systemd#28414

…ired by tpm spec To keep compatibility with any existing object authValues with trailing 0's, change tpm2_get_pin_auth() to trim trailing 0's, which is what the TPM implementation will do. This should retain compatibility with any existing authValues that contain trailing 0's. Note that any existing authValues with trailing 0's are unlikely to have worked in the way that systemd uses them in object sealing, which is as a bind key for the encryption (and policy) session. However, it is better to be compatible with the TPM spec (and implementations) even if previously created objects that are affected may not have worked. Fixes: #28414

…ired by tpm spec To keep compatibility with any existing object authValues with trailing 0's, change tpm2_get_pin_auth() to trim trailing 0's, which is what the TPM implementation will do. This should retain compatibility with any existing authValues that contain trailing 0's. Note that any existing authValues with trailing 0's are unlikely to have worked in the way that systemd uses them in object sealing, which is as a bind key for the encryption (and policy) session. However, it is better to be compatible with the TPM spec (and implementations) even if previously created objects that are affected may not have worked. Fixes: systemd#28414 (cherry picked from commit 63477a7) Related: RHEL-16182

github-actions bot added util-lib tpm2 please-review PR is ready for (re-)review by a maintainer labels Jul 15, 2023

This comment was marked as outdated.

Sign in to view

ddstreet mentioned this pull request Jul 15, 2023

TEST-70-TPM2 is unstable #27716

Closed

bluca reviewed Jul 15, 2023

View reviewed changes

src/shared/tpm2-util.c Outdated Show resolved Hide resolved

bluca reviewed Jul 15, 2023

View reviewed changes

src/shared/tpm2-util.c Outdated Show resolved Hide resolved

bluca approved these changes Jul 15, 2023

View reviewed changes

ddstreet force-pushed the tpm2_pin_trailing_zero branch from 0f0ad6f to ba6f31f Compare July 15, 2023 13:03

bluca added good-to-merge/with-minor-suggestions and removed please-review PR is ready for (re-)review by a maintainer labels Jul 15, 2023

ddstreet force-pushed the tpm2_pin_trailing_zero branch 2 times, most recently from e16c0ce to d0787f2 Compare July 15, 2023 13:07

bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Jul 15, 2023

bluca added this to the v254 milestone Jul 15, 2023

YHNdnzj reviewed Jul 15, 2023

View reviewed changes

ddstreet force-pushed the tpm2_pin_trailing_zero branch from d0787f2 to 8062501 Compare July 15, 2023 14:20

tpm2: add tpm2_get_pin_auth()

204a2b1

Add function to calculate the hash digest for a provided pin, and also verify that the final byte in the digest is not 0. This is required because the TPM will always remove all trailing 0's from an auth value before using it. Fixes: systemd#27716

ddstreet force-pushed the tpm2_pin_trailing_zero branch from 8062501 to 204a2b1 Compare July 15, 2023 18:12

DaanDeMeyer approved these changes Jul 15, 2023

View reviewed changes

bluca merged commit f230572 into systemd:main Jul 16, 2023
40 checks passed

github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Jul 16, 2023

poettering reviewed Jul 16, 2023

View reviewed changes

ddstreet deleted the tpm2_pin_trailing_zero branch July 18, 2023 13:45

ddstreet mentioned this pull request Jul 21, 2023

tpm2: instead of adjusting authValue trailing 0(s), trim them as required by spec #28486

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tpm2: add tpm2_get_pin_auth() #28414

tpm2: add tpm2_get_pin_auth() #28414

ddstreet commented Jul 15, 2023 •

edited by github-actions bot

Loading

This comment was marked as outdated.

YHNdnzj Jul 15, 2023

ddstreet Jul 15, 2023

ddstreet Jul 15, 2023

ddstreet Jul 15, 2023

poettering Jul 16, 2023

ddstreet Jul 17, 2023

poettering Jul 16, 2023

bluca Jul 16, 2023

poettering Jul 16, 2023

ddstreet Jul 17, 2023

ddstreet Jul 17, 2023

ddstreet Jul 17, 2023

ddstreet Jul 17, 2023

williamcroberts Jul 18, 2023 •

edited

Loading

williamcroberts Jul 19, 2023

ddstreet Jul 19, 2023

poettering commented Jul 19, 2023

poettering commented Jul 19, 2023

williamcroberts commented Jul 19, 2023

ddstreet commented Jul 19, 2023

williamcroberts commented Jul 20, 2023

ddstreet commented Jul 21, 2023

tpm2: add tpm2_get_pin_auth() #28414

tpm2: add tpm2_get_pin_auth() #28414

Conversation

ddstreet commented Jul 15, 2023 • edited by github-actions bot Loading

This comment was marked as outdated.

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

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

Choose a reason for hiding this comment

williamcroberts Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poettering commented Jul 19, 2023

poettering commented Jul 19, 2023

williamcroberts commented Jul 19, 2023

ddstreet commented Jul 19, 2023

williamcroberts commented Jul 20, 2023

ddstreet commented Jul 21, 2023

ddstreet commented Jul 15, 2023 •

edited by github-actions bot

Loading

williamcroberts Jul 18, 2023 •

edited

Loading