From c588cf9720eec3cc4f7438fa9a36cf2e2d8667f1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 24 Oct 2023 22:42:21 +0200 Subject: [PATCH 1/4] repart: reduce scope of two variables This replicates what 475c473d328c12f5e9fd43cee959154a0d0c78a4 did for cryptenroll for repart, which has very similar code for this. --- src/partition/repart.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/partition/repart.c b/src/partition/repart.c index e8a492c7ce..a64ba292d7 100644 --- a/src/partition/repart.c +++ b/src/partition/repart.c @@ -150,10 +150,8 @@ static X509 *arg_certificate = NULL; static char *arg_tpm2_device = NULL; static Tpm2PCRValue *arg_tpm2_hash_pcr_values = NULL; static size_t arg_tpm2_n_hash_pcr_values = 0; -static bool arg_tpm2_hash_pcr_values_use_default = true; static char *arg_tpm2_public_key = NULL; static uint32_t arg_tpm2_public_key_pcr_mask = 0; -static bool arg_tpm2_public_key_pcr_mask_use_default = true; static bool arg_split = false; static GptPartitionType *arg_filter_partitions = NULL; static size_t arg_n_filter_partitions = 0; @@ -6487,6 +6485,7 @@ static int parse_argv(int argc, char *argv[]) { {} }; + bool auto_hash_pcr_values = true, auto_public_key_pcr_mask = true; int c, r; assert(argc >= 0); @@ -6707,7 +6706,7 @@ static int parse_argv(int argc, char *argv[]) { } case ARG_TPM2_PCRS: - arg_tpm2_hash_pcr_values_use_default = false; + auto_hash_pcr_values = false; r = tpm2_parse_pcr_argument_append(optarg, &arg_tpm2_hash_pcr_values, &arg_tpm2_n_hash_pcr_values); if (r < 0) return r; @@ -6722,7 +6721,7 @@ static int parse_argv(int argc, char *argv[]) { break; case ARG_TPM2_PUBLIC_KEY_PCRS: - arg_tpm2_public_key_pcr_mask_use_default = false; + auto_public_key_pcr_mask = false; r = tpm2_parse_pcr_argument_to_mask(optarg, &arg_tpm2_public_key_pcr_mask); if (r < 0) return r; @@ -6936,15 +6935,21 @@ static int parse_argv(int argc, char *argv[]) { return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "A path to an image file must be specified when --split is used."); - if (arg_tpm2_public_key_pcr_mask_use_default && arg_tpm2_public_key) + if (auto_public_key_pcr_mask && arg_tpm2_public_key) { + assert(arg_tpm2_public_key_pcr_mask == 0); arg_tpm2_public_key_pcr_mask = INDEX_TO_MASK(uint32_t, TPM2_PCR_KERNEL_BOOT); + } - if (arg_tpm2_hash_pcr_values_use_default && !GREEDY_REALLOC_APPEND( - arg_tpm2_hash_pcr_values, - arg_tpm2_n_hash_pcr_values, - &TPM2_PCR_VALUE_MAKE(TPM2_PCR_INDEX_DEFAULT, /* hash= */ 0, /* value= */ {}), - 1)) - return log_oom(); + if (auto_hash_pcr_values) { + assert(arg_tpm2_n_hash_pcr_values == 0); + + if (!GREEDY_REALLOC_APPEND( + arg_tpm2_hash_pcr_values, + arg_tpm2_n_hash_pcr_values, + &TPM2_PCR_VALUE_MAKE(TPM2_PCR_INDEX_DEFAULT, /* hash= */ 0, /* value= */ {}), + 1)) + return log_oom(); + } if (arg_pretty < 0 && isatty(STDOUT_FILENO)) arg_pretty = true; From d66ff6fe77d1f0e3fcdd90413a81750f923b008e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 24 Oct 2023 22:41:57 +0200 Subject: [PATCH 2/4] cryptenroll: add addition asserts --- src/cryptenroll/cryptenroll.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/cryptenroll/cryptenroll.c b/src/cryptenroll/cryptenroll.c index a3332f5e7d..e0e2b40cf1 100644 --- a/src/cryptenroll/cryptenroll.c +++ b/src/cryptenroll/cryptenroll.c @@ -493,15 +493,21 @@ static int parse_argv(int argc, char *argv[]) { if (r < 0) return r; - if (auto_public_key_pcr_mask && arg_tpm2_public_key) + if (auto_public_key_pcr_mask && arg_tpm2_public_key) { + assert(arg_tpm2_public_key_pcr_mask == 0); arg_tpm2_public_key_pcr_mask = INDEX_TO_MASK(uint32_t, TPM2_PCR_KERNEL_BOOT); + } - if (auto_hash_pcr_values && !GREEDY_REALLOC_APPEND( - arg_tpm2_hash_pcr_values, - arg_tpm2_n_hash_pcr_values, - &TPM2_PCR_VALUE_MAKE(TPM2_PCR_INDEX_DEFAULT, /* hash= */ 0, /* value= */ {}), - 1)) - return log_oom(); + if (auto_hash_pcr_values) { + assert(arg_tpm2_n_hash_pcr_values == 0); + + if (!GREEDY_REALLOC_APPEND( + arg_tpm2_hash_pcr_values, + arg_tpm2_n_hash_pcr_values, + &TPM2_PCR_VALUE_MAKE(TPM2_PCR_INDEX_DEFAULT, /* hash= */ 0, /* value= */ {}), + 1)) + return log_oom(); + } return 1; } From 996d22e50a3b35657bc3bc1c6f419c2c27d84344 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 24 Oct 2023 22:41:26 +0200 Subject: [PATCH 3/4] cryptenroll: add section headers to --help text --- src/cryptenroll/cryptenroll.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/src/cryptenroll/cryptenroll.c b/src/cryptenroll/cryptenroll.c index e0e2b40cf1..4362e43529 100644 --- a/src/cryptenroll/cryptenroll.c +++ b/src/cryptenroll/cryptenroll.c @@ -102,28 +102,35 @@ static int help(void) { if (r < 0) return log_oom(); - printf("%s [OPTIONS...] BLOCK-DEVICE\n" - "\n%sEnroll a security token or authentication credential to a LUKS volume.%s\n\n" + printf("%1$s [OPTIONS...] BLOCK-DEVICE\n\n" + "%5$sEnroll a security token or authentication credential to a LUKS volume.%6$s\n\n" " -h --help Show this help\n" " --version Show package version\n" - " --password Enroll a user-supplied password\n" - " --recovery-key Enroll a recovery key\n" + " --wipe-slot=SLOT1,SLOT2,…\n" + " Wipe specified slots\n" + "\n%3$sUnlocking:%4$s\n" " --unlock-key-file=PATH\n" " Use a file to unlock the volume\n" " --unlock-fido2-device=PATH\n" " Use a FIDO2 device to unlock the volume\n" + "\n%3$sSimple Enrollment:%4$s\n" + " --password Enroll a user-supplied password\n" + " --recovery-key Enroll a recovery key\n" + "\n%3$sPKCS11 Enrollment:%4$s\n" " --pkcs11-token-uri=URI\n" " Specify PKCS#11 security token URI\n" - " --fido2-credential-algorithm=STRING\n" - " Specify COSE algorithm for FIDO2 credential\n" + "\n%3$sFIDO2 Enrollment:%4$s\n" " --fido2-device=PATH\n" " Enroll a FIDO2-HMAC security token\n" + " --fido2-credential-algorithm=STRING\n" + " Specify COSE algorithm for FIDO2 credential\n" " --fido2-with-client-pin=BOOL\n" " Whether to require entering a PIN to unlock the volume\n" " --fido2-with-user-presence=BOOL\n" " Whether to require user presence to unlock the volume\n" " --fido2-with-user-verification=BOOL\n" " Whether to require user verification to unlock the volume\n" + "\n%3$sTPM2 Enrollment:%4$s\n" " --tpm2-device=PATH\n" " Enroll a TPM2 device\n" " --tpm2-seal-key-handle=HANDLE\n" @@ -139,13 +146,13 @@ static int help(void) { " file\n" " --tpm2-with-pin=BOOL\n" " Whether to require entering a PIN to unlock the volume\n" - " --wipe-slot=SLOT1,SLOT2,…\n" - " Wipe specified slots\n" - "\nSee the %s for details.\n", + "\nSee the %2$s for details.\n", program_invocation_short_name, - ansi_highlight(), + link, + ansi_underline(), ansi_normal(), - link); + ansi_highlight(), + ansi_normal()); return 0; } From 51abb74eabb8351e319fd12865c2e11abd8c8966 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 24 Oct 2023 22:54:39 +0200 Subject: [PATCH 4/4] cryptenroll: validate positional arguments before looking at detail parameters Let's switch the order in which we process positional arguments and analyze/tweak detail parameters. Let's look at the positional arguments first (i.e. the "big picture") and then look at the switches (i.e. "little details"). THis doesn't matter much, but makes for better error messages I think. At least I was very confused that a completely borked cmdline I passed to cryptenrolled complained about some detail and let the major fuckup pass... --- src/cryptenroll/cryptenroll.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/cryptenroll/cryptenroll.c b/src/cryptenroll/cryptenroll.c index 4362e43529..174247907d 100644 --- a/src/cryptenroll/cryptenroll.c +++ b/src/cryptenroll/cryptenroll.c @@ -474,6 +474,18 @@ static int parse_argv(int argc, char *argv[]) { } } + if (optind >= argc) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "No block device node specified, refusing."); + + if (argc > optind+1) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Too many arguments, refusing."); + + r = parse_path_argument(argv[optind], false, &arg_node); + if (r < 0) + return r; + if (arg_enroll_type == ENROLL_FIDO2) { if (arg_unlock_type == UNLOCK_FIDO2 && !(arg_fido2_device && arg_unlock_fido2_device)) @@ -488,18 +500,6 @@ static int parse_argv(int argc, char *argv[]) { } } - if (optind >= argc) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "No block device node specified, refusing."); - - if (argc > optind+1) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Too many arguments, refusing."); - - r = parse_path_argument(argv[optind], false, &arg_node); - if (r < 0) - return r; - if (auto_public_key_pcr_mask && arg_tpm2_public_key) { assert(arg_tpm2_public_key_pcr_mask == 0); arg_tpm2_public_key_pcr_mask = INDEX_TO_MASK(uint32_t, TPM2_PCR_KERNEL_BOOT);