From 0db32a3bfb59af61aa8e79f2082dc053b9bf22d1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 21 Dec 2024 13:09:29 +0100 Subject: [PATCH 1/4] tpm2-util: drop ret_x prefix from two arguments that are not just return but also input params --- src/shared/tpm2-util.c | 12 ++++++------ src/shared/tpm2-util.h | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/shared/tpm2-util.c b/src/shared/tpm2-util.c index 890d77dcc2..a1b2695b67 100644 --- a/src/shared/tpm2-util.c +++ b/src/shared/tpm2-util.c @@ -8091,14 +8091,14 @@ int tpm2_parse_pcr_argument_append(const char *arg, Tpm2PCRValue **pcr_values, s * algorithm is included in the pcr values array this results in error. This retains the previous behavior of * tpm2_parse_pcr_argument() of clearing the mask if 'arg' is empty, replacing the mask if it is set to * UINT32_MAX, and or-ing the mask otherwise. */ -int tpm2_parse_pcr_argument_to_mask(const char *arg, uint32_t *ret_mask) { +int tpm2_parse_pcr_argument_to_mask(const char *arg, uint32_t *mask) { #if HAVE_TPM2 _cleanup_free_ Tpm2PCRValue *pcr_values = NULL; size_t n_pcr_values; int r; assert(arg); - assert(ret_mask); + assert(mask); r = tpm2_parse_pcr_argument(arg, &pcr_values, &n_pcr_values); if (r < 0) @@ -8106,7 +8106,7 @@ int tpm2_parse_pcr_argument_to_mask(const char *arg, uint32_t *ret_mask) { if (n_pcr_values == 0) { /* This retains the previous behavior of clearing the mask if the arg is empty */ - *ret_mask = 0; + *mask = 0; return 0; } @@ -8123,10 +8123,10 @@ int tpm2_parse_pcr_argument_to_mask(const char *arg, uint32_t *ret_mask) { if (r < 0) return log_error_errno(r, "Could not get pcr values mask: %m"); - if (*ret_mask == UINT32_MAX) - *ret_mask = new_mask; + if (*mask == UINT32_MAX) + *mask = new_mask; else - *ret_mask |= new_mask; + *mask |= new_mask; return 0; #else diff --git a/src/shared/tpm2-util.h b/src/shared/tpm2-util.h index 76b4dd3cc1..4a44c43f0f 100644 --- a/src/shared/tpm2-util.h +++ b/src/shared/tpm2-util.h @@ -479,7 +479,7 @@ static inline bool tpm2_is_fully_supported(void) { int verb_has_tpm2_generic(bool quiet); int tpm2_parse_pcr_argument(const char *arg, Tpm2PCRValue **ret_pcr_values, size_t *ret_n_pcr_values); -int tpm2_parse_pcr_argument_append(const char *arg, Tpm2PCRValue **ret_pcr_values, size_t *ret_n_pcr_values); +int tpm2_parse_pcr_argument_append(const char *arg, Tpm2PCRValue **pcr_values, size_t *n_pcr_values); int tpm2_parse_pcr_argument_to_mask(const char *arg, uint32_t *mask); int tpm2_load_pcr_signature(const char *path, sd_json_variant **ret); From 982e791f1c46cf6dc6429ffeebae93eed80879ff Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sun, 22 Dec 2024 18:12:37 +0100 Subject: [PATCH 2/4] tpm2-util: optionally do wildcard hash check in tpm2_pcr_values_to_mask() If TPM2_ALG_ERROR (aka "0") is specified as algorithm in tpm2_pcr_values_to_mask() we'll simply match all algorithms. This allows us to shorten tpm2_parse_pcr_argument_to_mask() a bit. The function accepts but ignores a hash algorithm specification currently, hence this should not really much effect. --- src/shared/tpm2-util.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/shared/tpm2-util.c b/src/shared/tpm2-util.c index a1b2695b67..f9a805964a 100644 --- a/src/shared/tpm2-util.c +++ b/src/shared/tpm2-util.c @@ -1845,8 +1845,11 @@ int tpm2_pcr_values_from_mask(uint32_t mask, TPMI_ALG_HASH hash, Tpm2PCRValue ** return 0; } -int tpm2_pcr_values_to_mask(const Tpm2PCRValue *pcr_values, size_t n_pcr_values, TPMI_ALG_HASH hash, uint32_t *ret_mask) { - uint32_t mask = 0; +int tpm2_pcr_values_to_mask( + const Tpm2PCRValue *pcr_values, + size_t n_pcr_values, + TPMI_ALG_HASH hash, + uint32_t *ret_mask) { assert(pcr_values || n_pcr_values == 0); assert(ret_mask); @@ -1854,12 +1857,12 @@ int tpm2_pcr_values_to_mask(const Tpm2PCRValue *pcr_values, size_t n_pcr_values, if (!tpm2_pcr_values_valid(pcr_values, n_pcr_values)) return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid PCR values."); + uint32_t mask = 0; FOREACH_ARRAY(v, pcr_values, n_pcr_values) - if (v->hash == hash) + if (hash == 0 || v->hash == hash) SET_BIT(mask, v->index); *ret_mask = mask; - return 0; } @@ -8093,13 +8096,13 @@ int tpm2_parse_pcr_argument_append(const char *arg, Tpm2PCRValue **pcr_values, s * UINT32_MAX, and or-ing the mask otherwise. */ int tpm2_parse_pcr_argument_to_mask(const char *arg, uint32_t *mask) { #if HAVE_TPM2 - _cleanup_free_ Tpm2PCRValue *pcr_values = NULL; - size_t n_pcr_values; int r; assert(arg); assert(mask); + _cleanup_free_ Tpm2PCRValue *pcr_values = NULL; + size_t n_pcr_values; r = tpm2_parse_pcr_argument(arg, &pcr_values, &n_pcr_values); if (r < 0) return r; @@ -8110,16 +8113,8 @@ int tpm2_parse_pcr_argument_to_mask(const char *arg, uint32_t *mask) { return 0; } - size_t hash_count; - r = tpm2_pcr_values_hash_count(pcr_values, n_pcr_values, &hash_count); - if (r < 0) - return log_error_errno(r, "Could not get hash count from pcr values: %m"); - - if (hash_count > 1) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Multiple PCR hash banks selected."); - uint32_t new_mask; - r = tpm2_pcr_values_to_mask(pcr_values, n_pcr_values, pcr_values[0].hash, &new_mask); + r = tpm2_pcr_values_to_mask(pcr_values, n_pcr_values, /* algorithm= */ 0, &new_mask); if (r < 0) return log_error_errno(r, "Could not get pcr values mask: %m"); From 308578202069c288e7f7dc395fa9f74582ab101e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sun, 22 Dec 2024 18:32:09 +0100 Subject: [PATCH 3/4] tpm2-util: refuse hash algorithm/value specification when we only parse a mask tpm2_parse_pcr_argument_to_mask() is supposed to parse a PCR mask string, and uses the full blown tpm2_parse_pcr_argument() call at its core, which parses more than just a mask, i.e. values and algorithms too. Which is very confusing at times, because commands such as "systemd-cryptenroll --tpm2-device=auto --tpm2-public-key-pcrs=1:sha1=09dbdbc7f6cdd8029cc90b57a915c19a0ac21bce" are very confusing, since they suggest enrollment with a specific algorithm and has value, but this is not in fact what happens: both are entirely ignored. That this was accepted this way was more an accident than intended, which is already visible in the fact that extensive test case entirely ignores the fact that strings like this are accepted. --- src/shared/tpm2-util.c | 18 ++++++++++++++---- src/test/test-tpm2.c | 13 +++++++++++-- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/shared/tpm2-util.c b/src/shared/tpm2-util.c index f9a805964a..29d9dbbe2e 100644 --- a/src/shared/tpm2-util.c +++ b/src/shared/tpm2-util.c @@ -8090,14 +8090,15 @@ int tpm2_parse_pcr_argument_append(const char *arg, Tpm2PCRValue **pcr_values, s #endif } -/* Same as tpm2_parse_pcr_argument() but converts the pcr values to a pcr mask. If more than one hash - * algorithm is included in the pcr values array this results in error. This retains the previous behavior of - * tpm2_parse_pcr_argument() of clearing the mask if 'arg' is empty, replacing the mask if it is set to - * UINT32_MAX, and or-ing the mask otherwise. */ int tpm2_parse_pcr_argument_to_mask(const char *arg, uint32_t *mask) { #if HAVE_TPM2 int r; + /* Same as tpm2_parse_pcr_argument() but converts the pcr values to a pcr mask. If a hash algorithm or + * hash value is specified an error is generated (after all we only return the mask here, nothing + * else). This retains the previous behavior of tpm2_parse_pcr_argument() of clearing the mask if + * 'arg' is empty, replacing the mask if it is set to UINT32_MAX, and or-ing the mask otherwise. */ + assert(arg); assert(mask); @@ -8113,6 +8114,15 @@ int tpm2_parse_pcr_argument_to_mask(const char *arg, uint32_t *mask) { return 0; } + FOREACH_ARRAY(v, pcr_values, n_pcr_values) { + if (v->hash != 0) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Not expecting hash algorithm specification in PCR mask value, refusing: %s", arg); + if (v->value.size != 0) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Not expecting hash value specification in PCR mask value, refusing: %s", arg); + } + uint32_t new_mask; r = tpm2_pcr_values_to_mask(pcr_values, n_pcr_values, /* algorithm= */ 0, &new_mask); if (r < 0) diff --git a/src/test/test-tpm2.c b/src/test/test-tpm2.c index e4f1f861b4..31ef6192b2 100644 --- a/src/test/test-tpm2.c +++ b/src/test/test-tpm2.c @@ -556,8 +556,13 @@ static void check_parse_pcr_argument( assert_se(tpm2_pcr_values_to_mask(expected_values, n_expected_values, expected_values[0].hash, &expected_mask) == 0); - assert_se(tpm2_parse_pcr_argument_to_mask(arg, &mask) == 0); - assert_se(mask == expected_mask); + _cleanup_free_ Tpm2PCRValue *arg_pcr_values = NULL; + size_t n_arg_pcr_values = 0; + assert_se(tpm2_parse_pcr_argument(arg, &arg_pcr_values, &n_arg_pcr_values) >= 0); + uint32_t mask2 = UINT32_MAX; + assert_se(tpm2_pcr_values_to_mask(arg_pcr_values, n_arg_pcr_values, /* algorithm= */ 0, &mask2) >= 0); + + assert_se((mask == UINT32_MAX ? mask2 : (mask|mask2)) == expected_mask); } size_t old_n_values = n_values; @@ -701,6 +706,10 @@ TEST(parse_pcr_argument) { check_parse_pcr_argument_to_mask("sysexts+17+23", 0x822000); check_parse_pcr_argument_to_mask("6+boot-loader-code,44", -EINVAL); check_parse_pcr_argument_to_mask("debug+24", -EINVAL); + check_parse_pcr_argument_to_mask("5:sha1=f013d66c7f6817d08b7eb2a93e6d0440c1f3e7f8", -EINVAL); + check_parse_pcr_argument_to_mask("0:sha256=f013d66c7f6817d08b7eb2a93e6d0440c1f3e7f8", -EINVAL); + check_parse_pcr_argument_to_mask("5:sha1=f013d66c7f6817d08b7eb2a93e6d0440c1f3e7f8,3", -EINVAL); + check_parse_pcr_argument_to_mask("3,0:sha256=f013d66c7f6817d08b7eb2a93e6d0440c1f3e7f8", -EINVAL); } static const TPMT_PUBLIC test_rsa_template = { From f16e08f18e1df537401e4849bad74bd48f6624a5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 3 Jan 2025 10:44:38 +0100 Subject: [PATCH 4/4] update TODO --- TODO | 4 ---- 1 file changed, 4 deletions(-) diff --git a/TODO b/TODO index a7b6a9d847..f1e7e449aa 100644 --- a/TODO +++ b/TODO @@ -330,10 +330,6 @@ Features: probably should measure the dm-verity root hash from the kernel side, but DDI meta info from userspace. -* rework tpm2_parse_pcr_argument_to_mask() to refuse literal hash value - specifications. They are currently parsed but ignored. We should refuse them - however, to not confuse people. - * use name_to_handle_at() with AT_HANDLE_FID instead of .st_ino (inode number) for identifying inodes, for example in copy.c when finding hard links, or loop-util.c for tracking backing files, and other places.