From be9c38a77d31ceaaba1647c05ee1ee16940a8875 Mon Sep 17 00:00:00 2001 From: Ludwig Nussel Date: Mon, 19 Feb 2024 18:19:48 +0100 Subject: [PATCH 1/9] pcrlock: exclude red and green color ranges Using red and green in the output is rather confusing as that is normally associated with good or bad like in the checkmark columns. So let's exclude those colors from the display. Co-authored-by: Akarithos <277667353+Akarithos@users.noreply.github.com> --- src/pcrlock/pcrlock.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/pcrlock/pcrlock.c b/src/pcrlock/pcrlock.c index 33b35ac414..bdee629bc0 100644 --- a/src/pcrlock/pcrlock.c +++ b/src/pcrlock/pcrlock.c @@ -2098,6 +2098,18 @@ static const char *ansi_true_color(uint8_t r, uint8_t g, uint8_t b, char ret[sta return ret; } +/* red and green colors usually have good/bad meaning. So exclude color ranges that look too red- or + * greenish. See https://en.wikipedia.org/wiki/Hue */ +static double map_pcr_to_hue(uint32_t pcr) { + double exclude_red_range = 60.0; + double exclude_green_range = 60.0; + double h = (360.0 - exclude_red_range - exclude_green_range) / (TPM2_PCRS_MAX - 1) * pcr; + h += exclude_red_range / 2; + if (h > 120 - exclude_green_range / 2) + h += exclude_green_range; + return MIN(h, 360 - exclude_red_range / 2); +} + static char *color_for_pcr(EventLog *el, uint32_t pcr) { char color[ANSI_TRUE_COLOR_MAX]; uint8_t r, g, b; @@ -2108,7 +2120,7 @@ static char *color_for_pcr(EventLog *el, uint32_t pcr) { if (el->registers[pcr].color) return el->registers[pcr].color; - hsv_to_rgb(360.0 / (TPM2_PCRS_MAX - 1) * pcr, 100, 90, &r, &g, &b); + hsv_to_rgb(map_pcr_to_hue(pcr), 100, 90, &r, &g, &b); ansi_true_color(r, g, b, color); el->registers[pcr].color = strdup(color); From 18fed0d0d4b91a0c700b2288ad9b4a00195dedca Mon Sep 17 00:00:00 2001 From: Ludwig Nussel Date: Thu, 15 Feb 2024 17:08:35 +0100 Subject: [PATCH 2/9] pcrlock: shorten displayed hash bytes Shorten number of bytes show of sha256 hashes. Similar to git, makes output less wide and more readable. Co-authored-by: Akarithos <277667353+Akarithos@users.noreply.github.com> --- man/systemd-pcrlock.xml | 8 ++++++++ src/pcrlock/pcrlock.c | 16 ++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/man/systemd-pcrlock.xml b/man/systemd-pcrlock.xml index 521f3a8bde..0a80d2488b 100644 --- a/man/systemd-pcrlock.xml +++ b/man/systemd-pcrlock.xml @@ -443,6 +443,14 @@ The following options are understood: + + + + Show full hash value instead of abbreviating to 7 characters. + + + + diff --git a/src/pcrlock/pcrlock.c b/src/pcrlock/pcrlock.c index bdee629bc0..0effdd09e1 100644 --- a/src/pcrlock/pcrlock.c +++ b/src/pcrlock/pcrlock.c @@ -94,6 +94,8 @@ static BootEntryTokenType arg_entry_token_type = BOOT_ENTRY_TOKEN_AUTO; static char *arg_entry_token = NULL; static bool arg_varlink = false; static bool arg_quiet = false; +/* abbreviate to 7 chars, just like git */ +static size_t arg_abbreviate_hash = 7; STATIC_DESTRUCTOR_REGISTER(arg_components, strv_freep); STATIC_DESTRUCTOR_REGISTER(arg_pcrlock_path, freep); @@ -2274,6 +2276,9 @@ static int show_log_table(EventLog *el, sd_json_variant **ret_variant) { if (!hex) return log_oom(); + if (!sd_json_format_enabled(arg_json_format_flags)) + strshorten(hex, arg_abbreviate_hash); + r = table_add_cell(table, NULL, TABLE_STRING, hex); } else r = table_add_cell(table, NULL, TABLE_EMPTY, NULL); @@ -2422,6 +2427,9 @@ static int show_pcr_table(EventLog *el, sd_json_variant **ret_variant) { if (!hex) return log_oom(); + if (!sd_json_format_enabled(arg_json_format_flags)) + strshorten(hex, arg_abbreviate_hash); + r = table_add_many(table, TABLE_STRING, hex, TABLE_SET_COLOR, color); @@ -2441,6 +2449,9 @@ static int show_pcr_table(EventLog *el, sd_json_variant **ret_variant) { if (!hex) return log_oom(); + if (!sd_json_format_enabled(arg_json_format_flags)) + strshorten(hex, arg_abbreviate_hash); + color = !hash_match ? ansi_highlight_red() : is_unset_pcr(el->registers[pcr].banks[i].observed.buffer, el->registers[pcr].banks[i].observed.size) ? ansi_grey() : NULL; @@ -5334,6 +5345,11 @@ static int parse_argv(int argc, char *argv[], char ***ret_args) { return r; break; + OPTION_LONG("full", NULL, + "Print full hash in measurement log"): + arg_abbreviate_hash = SIZE_MAX; + break; + OPTION_LONG("raw-description", NULL, "Show raw firmware record data as description in table"): arg_raw_description = true; From dcde7c750d627e0ad92905955eb5e3ef9bce12f7 Mon Sep 17 00:00:00 2001 From: Ludwig Nussel Date: Tue, 20 Feb 2024 17:55:05 +0100 Subject: [PATCH 3/9] pcrlock: don't add checkmark for missing component Co-authored-by: Akarithos <277667353+Akarithos@users.noreply.github.com> --- src/pcrlock/pcrlock.c | 46 +++++++++++++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/src/pcrlock/pcrlock.c b/src/pcrlock/pcrlock.c index 0effdd09e1..f86ae6458d 100644 --- a/src/pcrlock/pcrlock.c +++ b/src/pcrlock/pcrlock.c @@ -254,6 +254,9 @@ struct EventLog { /* PCRs mask indicating all PCRs touched by unrecognized components */ uint32_t missing_component_pcrs; + + /* PCRs mask indicating component found for the pcr */ + uint32_t has_component_pcrs; }; static EventLogRecordBank *event_log_record_bank_free(EventLogRecordBank *bank) { @@ -2048,6 +2051,8 @@ static int event_log_map_components(EventLog *el) { continue; } + el->has_component_pcrs |= event_log_component_variant_pcrs(*ii); + r = event_log_match_component_variant(el, 0, i, 0, n_matching + n_empty == 0); if (r < 0) return r; @@ -2380,14 +2385,23 @@ static int show_pcr_table(EventLog *el, sd_json_variant **ret_variant) { /* Whether all records in this PCR have a matching component */ bool fully_recognized = el->registers[pcr].fully_recognized; + bool seen = el->registers[pcr].n_measurements > 0; + /* Whether any unmatched components touch this PCR */ bool missing_components = BIT_SET(el->missing_component_pcrs, pcr); + bool has_components = BIT_SET(el->has_component_pcrs, pcr); - const char *emoji = glyph( - !hash_match ? GLYPH_DEPRESSED_SMILEY : - !fully_recognized ? GLYPH_UNHAPPY_SMILEY : - missing_components ? GLYPH_SLIGHTLY_HAPPY_SMILEY : - GLYPH_HAPPY_SMILEY); + const char *emoji = ""; + if (seen || has_components) { + if (!hash_match) + emoji = glyph(GLYPH_DEPRESSED_SMILEY); + else if (!fully_recognized) + emoji = glyph(GLYPH_UNHAPPY_SMILEY); + else if (!missing_components) + emoji = glyph(GLYPH_HAPPY_SMILEY); + else + emoji = glyph(GLYPH_SLIGHTLY_HAPPY_SMILEY); + } r = table_add_many(table, TABLE_UINT32, pcr, @@ -2405,13 +2419,19 @@ static int show_pcr_table(EventLog *el, sd_json_variant **ret_variant) { if (r < 0) return table_log_add_error(r); - r = table_add_many(table, - TABLE_BOOLEAN_CHECKMARK, hash_match, - TABLE_SET_COLOR, ansi_highlight_green_red(hash_match), - TABLE_BOOLEAN_CHECKMARK, fully_recognized, - TABLE_SET_COLOR, ansi_highlight_green_red(fully_recognized), - TABLE_BOOLEAN_CHECKMARK, !missing_components, - TABLE_SET_COLOR, ansi_highlight_green_red(!missing_components)); + if (sd_json_format_enabled(arg_json_format_flags)) + r = table_add_many(table, + TABLE_BOOLEAN_CHECKMARK, hash_match, + TABLE_BOOLEAN_CHECKMARK, fully_recognized, + TABLE_BOOLEAN_CHECKMARK, !missing_components); + else + r = table_add_many(table, + TABLE_STRING, seen ? glyph_check_mark(hash_match) : " ", + TABLE_SET_COLOR, ansi_highlight_green_red(hash_match), + TABLE_STRING, seen ? glyph_check_mark(fully_recognized) : " ", + TABLE_SET_COLOR, ansi_highlight_green_red(fully_recognized), + TABLE_STRING, has_components ? glyph_check_mark(!missing_components) : " ", + TABLE_SET_COLOR, ansi_highlight_green_red(!missing_components)); if (r < 0) return table_log_add_error(r); @@ -2479,7 +2499,7 @@ static int show_pcr_table(EventLog *el, sd_json_variant **ret_variant) { printf("\n" "%sLegend: H → PCR hash value matches event log%s\n" "%s R → All event log records for this PCR have a matching component%s\n" - "%s C → No components that couldn't be matched with log records affect this PCR%s\n", + "%s C → Component exists and found in event log%s\n", ansi_grey(), ansi_normal(), /* less on small screens automatically resets the color after long lines, hence we set it anew for each line */ ansi_grey(), ansi_normal(), ansi_grey(), ansi_normal()); From d1cec479004734a5fe61325d72e3b8f63a7e51a9 Mon Sep 17 00:00:00 2001 From: Ludwig Nussel Date: Thu, 15 Feb 2024 17:59:53 +0100 Subject: [PATCH 4/9] pcrlock: add --strict option Add a --strict option that makes sure all PCRs for which components were found are indeed included in the set of predicted PCRs. Instead throw an error. Avoids creating a policy less secure than requested. Co-authored-by: Akarithos <277667353+Akarithos@users.noreply.github.com> --- man/systemd-pcrlock.xml | 9 +++++++++ src/pcrlock/pcrlock.c | 16 ++++++++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/man/systemd-pcrlock.xml b/man/systemd-pcrlock.xml index 0a80d2488b..dbb59f2e12 100644 --- a/man/systemd-pcrlock.xml +++ b/man/systemd-pcrlock.xml @@ -601,6 +601,15 @@ + + + + Takes a boolean. Defaults to false. Honoured by make-policy and predict. If + true all specified PCRs must be included in the policy otherwise the command returns failure. + + + + diff --git a/src/pcrlock/pcrlock.c b/src/pcrlock/pcrlock.c index f86ae6458d..8ca570a3a3 100644 --- a/src/pcrlock/pcrlock.c +++ b/src/pcrlock/pcrlock.c @@ -91,6 +91,7 @@ static RecoveryPinMode arg_recovery_pin = RECOVERY_PIN_HIDE; static char *arg_policy_path = NULL; static bool arg_force = false; static BootEntryTokenType arg_entry_token_type = BOOT_ENTRY_TOKEN_AUTO; +static bool arg_strict = false; static char *arg_entry_token = NULL; static bool arg_varlink = false; static bool arg_quiet = false; @@ -3068,6 +3069,7 @@ static int unlink_pcrlock(const char *default_pcrlock_path) { static int event_log_reduce_to_safe_pcrs(EventLog *el, uint32_t *pcrs) { _cleanup_free_ char *dropped = NULL, *kept = NULL; + bool dropped_relevant_pcr = false; assert(el); assert(pcrs); @@ -3110,6 +3112,9 @@ static int event_log_reduce_to_safe_pcrs(EventLog *el, uint32_t *pcrs) { continue; drop: + if (arg_strict) + dropped_relevant_pcr = true; + *pcrs &= ~(UINT32_C(1) << pcr); if (strextendf_with_separator(&dropped, ", ", "%" PRIu32 " (%s)", pcr, tpm2_pcr_index_to_string(pcr)) < 0) @@ -3117,7 +3122,7 @@ static int event_log_reduce_to_safe_pcrs(EventLog *el, uint32_t *pcrs) { } if (dropped) - log_notice("PCRs dropped from protection mask: %s", dropped); + log_full(dropped_relevant_pcr ? LOG_ERR : LOG_NOTICE, "PCRs dropped from protection mask: %s", dropped); else log_debug("No PCRs dropped from protection mask."); @@ -3126,7 +3131,7 @@ static int event_log_reduce_to_safe_pcrs(EventLog *el, uint32_t *pcrs) { else log_notice("No PCRs kept in protection mask."); - return 0; + return dropped_relevant_pcr ? -ENOEXEC : 0; } static int pcr_prediction_add_result( @@ -5503,6 +5508,13 @@ static int parse_argv(int argc, char *argv[], char ***ret_args) { return r; break; + OPTION_LONG("strict", "BOOL", + "Require all PCRs configured via --pcr= included in policy"): + r = parse_boolean_argument("--strict", opts.arg, &arg_strict); + if (r < 0) + return r; + break; + OPTION('q', "quiet", NULL, "Suppress unnecessary output"): arg_quiet = true; break; From 8f631ffe05e0df35d1d87e40d63ccc2a7cec9009 Mon Sep 17 00:00:00 2001 From: Ludwig Nussel Date: Thu, 22 Feb 2024 15:06:57 +0100 Subject: [PATCH 5/9] pcrlock: limit list view to selected PCRs Makes it easier to inspect specific PCRs Co-authored-by: Akarithos <277667353+Akarithos@users.noreply.github.com> --- src/pcrlock/pcrlock.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/pcrlock/pcrlock.c b/src/pcrlock/pcrlock.c index 8ca570a3a3..e59a20e9b3 100644 --- a/src/pcrlock/pcrlock.c +++ b/src/pcrlock/pcrlock.c @@ -2231,6 +2231,10 @@ static int show_log_table(EventLog *el, sd_json_variant **ret_variant) { FOREACH_ARRAY(rr, el->records, el->n_records) { EventLogRecord *record = *rr; + if (EVENT_LOG_RECORD_IS_PCR(record) && + arg_pcr_mask != 0 && !FLAGS_SET(arg_pcr_mask, UINT32_C(1) << record->pcr)) + continue; + if (EVENT_LOG_RECORD_IS_PCR(record)) r = table_add_many(table, TABLE_UINT32, record->pcr, @@ -2380,6 +2384,9 @@ static int show_pcr_table(EventLog *el, sd_json_variant **ret_variant) { (void) table_set_json_field_name(table, 7, "noMissingComponents"); for (uint32_t pcr = 0; pcr < TPM2_PCRS_MAX; pcr++) { + if (arg_pcr_mask != 0 && !FLAGS_SET(arg_pcr_mask, UINT32_C(1) << pcr)) + continue; + /* Check if the PCR hash value matches the event log data */ bool hash_match = event_log_pcr_checks_out(el, el->registers + pcr); From 230f1718374771a6f1afe6eeb859abc57dc31668 Mon Sep 17 00:00:00 2001 From: Ludwig Nussel Date: Thu, 29 Feb 2024 09:36:56 +0100 Subject: [PATCH 6/9] pcrlock: reduce verbosity a bit Not sure what a good compromise between too much and too less is. Semi happy with SYSTEMD_LOG_LEVEL=notice now. Co-authored-by: Akarithos <277667353+Akarithos@users.noreply.github.com> --- src/pcrlock/pcrlock.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/pcrlock/pcrlock.c b/src/pcrlock/pcrlock.c index e59a20e9b3..7bcfa5013e 100644 --- a/src/pcrlock/pcrlock.c +++ b/src/pcrlock/pcrlock.c @@ -1941,7 +1941,7 @@ static int event_log_validate_fully_recognized(EventLog *el) { continue; if (rec->n_mapped == 0) { - log_notice("Event log record %zu (PCR %" PRIu32 ", \"%s\") not matching any component.", + log_info("Event log record %zu (PCR %" PRIu32 ", \"%s\") not matching any component.", (size_t) (rr - el->records), rec->pcr, strna(rec->description)); fully_recognized = false; break; @@ -2073,7 +2073,7 @@ static int event_log_map_components(EventLog *el) { else if (arg_location_end && strcmp(c->id, arg_location_end) > 0) { log_info("Didn't find component '%s' in event log, but irrelevant for location window, ignoring.", c->id); } else { - log_notice("Couldn't find component '%s' in event log.", c->id); + log_info("Couldn't find component '%s' in event log.", c->id); el->n_missing_components++; el->missing_component_pcrs |= event_log_component_pcrs(c); @@ -2092,9 +2092,9 @@ static int event_log_map_components(EventLog *el) { } if (n_skipped > 0) - log_notice("Skipped %u components (%s).", n_skipped, skipped_ids); + log_info("Skipped %u components (%s).", n_skipped, skipped_ids); if (el->n_missing_components > 0) - log_notice("Unable to recognize %zu components in event log.", el->n_missing_components); + log_debug("Unable to recognize %zu components in event log.", el->n_missing_components); return event_log_validate_fully_recognized(el); } @@ -3049,7 +3049,7 @@ static int write_pcrlock(sd_json_variant *array, const char *default_pcrlock_pat return log_error_errno(r, "Failed to output JSON object: %m"); if (p) - log_info("%s written.", p); + log_debug("%s written.", p); return 0; } @@ -3111,7 +3111,7 @@ static int event_log_reduce_to_safe_pcrs(EventLog *el, uint32_t *pcrs) { goto drop; } - log_info("PCR %" PRIu32 " (%s) matches event log and fully consists of recognized measurements. Including in set of PCRs.", pcr, strna(tpm2_pcr_index_to_string(pcr))); + log_debug("PCR %" PRIu32 " (%s) matches event log and fully consists of recognized measurements. Including in set of PCRs.", pcr, strna(tpm2_pcr_index_to_string(pcr))); if (strextendf_with_separator(&kept, ", ", "%" PRIu32 " (%s)", pcr, tpm2_pcr_index_to_string(pcr)) < 0) return log_oom(); @@ -3659,7 +3659,7 @@ static int make_policy(bool force, RecoveryPinMode recovery_pin_mode) { if (r < 0) return r; - log_info("Predicted future PCRs in %s.", FORMAT_TIMESPAN(usec_sub_unsigned(now(CLOCK_MONOTONIC), predict_start_usec), 1)); + log_debug("Predicted future PCRs in %s.", FORMAT_TIMESPAN(usec_sub_unsigned(now(CLOCK_MONOTONIC), predict_start_usec), 1)); _cleanup_(sd_json_variant_unrefp) sd_json_variant *new_prediction_json = NULL; r = tpm2_pcr_prediction_to_json(&new_prediction, el->primary_algorithm, &new_prediction_json); From 2e7c9a24a1bcb06bc845ea456f52c4b34d418444 Mon Sep 17 00:00:00 2001 From: Ludwig Nussel Date: Thu, 7 Mar 2024 15:19:05 +0100 Subject: [PATCH 7/9] pcrlock: ignore empty device path Don't mark records with empty device path as invalid. When shim is in use such records are normal :-/ https://github.com/rhboot/shim/issues/642 --- src/pcrlock/pcrlock.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/pcrlock/pcrlock.c b/src/pcrlock/pcrlock.c index 7bcfa5013e..2b7850efe0 100644 --- a/src/pcrlock/pcrlock.c +++ b/src/pcrlock/pcrlock.c @@ -830,6 +830,16 @@ static int event_log_record_extract_firmware_description(EventLogRecord *rec) { goto invalid; } + /* device path could be empty. Don't mark that as invalid but leave as don't know. + * Happens eg with shim https://github.com/rhboot/shim/issues/642 */ + if (load->lengthOfDevicePath == 0) { + rec->description = strdup("File: "); + if (!rec->description) + return log_oom(); + + return 1; + } + const packed_EFI_DEVICE_PATH *dp = (const packed_EFI_DEVICE_PATH*) load->devicePath; size_t left = load->lengthOfDevicePath; From 8ab0aaea3c87563eecd854b9dfe6d28d6bc87b61 Mon Sep 17 00:00:00 2001 From: Ludwig Nussel Date: Thu, 7 Mar 2024 15:43:22 +0100 Subject: [PATCH 8/9] pcrlock: honor pcr arg for filtering list-components --- src/pcrlock/pcrlock.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/pcrlock/pcrlock.c b/src/pcrlock/pcrlock.c index 2b7850efe0..d290538aad 100644 --- a/src/pcrlock/pcrlock.c +++ b/src/pcrlock/pcrlock.c @@ -2796,6 +2796,9 @@ static int verb_list_components(int argc, char *argv[], uintptr_t _data, void *u FOREACH_ARRAY(c, el->components, el->n_components) { + if (arg_pcr_mask != 0 && (arg_pcr_mask & event_log_component_pcrs(*c)) == 0) + continue; + if (!sd_json_format_enabled(arg_json_format_flags)) { _cleanup_free_ char *marker = NULL; From d2bb93565cb5cc752992e0d0ab82f57f11b69273 Mon Sep 17 00:00:00 2001 From: Ludwig Nussel Date: Mon, 29 Apr 2024 14:34:25 +0200 Subject: [PATCH 9/9] pcrlock: skip make-policy tests if environment lacks TPM support make-policy needs to write into an NVindex. For that we need an actual TPM. Co-authored-by: Akarithos <277667353+Akarithos@users.noreply.github.com> --- test/units/TEST-70-TPM2.pcrlock.sh | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/units/TEST-70-TPM2.pcrlock.sh b/test/units/TEST-70-TPM2.pcrlock.sh index ae809be671..25f1a2537e 100755 --- a/test/units/TEST-70-TPM2.pcrlock.sh +++ b/test/units/TEST-70-TPM2.pcrlock.sh @@ -117,10 +117,12 @@ if [[ -n "$SD_STUB" ]]; then [[ "$uki_stdin" == "$uki_pipe" ]] fi -PIN=huhu "$SD_PCRLOCK" make-policy --pcr="$PCRS" --recovery-pin=query -# Repeat immediately (this call will have to reuse the nvindex, rather than create it) -"$SD_PCRLOCK" make-policy --pcr="$PCRS" -"$SD_PCRLOCK" make-policy --pcr="$PCRS" --force +if "$SD_PCRLOCK" is-supported; then + PIN=huhu "$SD_PCRLOCK" make-policy --pcr="$PCRS" --recovery-pin=query + # Repeat immediately (this call will have to reuse the nvindex, rather than create it) + "$SD_PCRLOCK" make-policy --pcr="$PCRS" + "$SD_PCRLOCK" make-policy --pcr="$PCRS" --force +fi img="/tmp/pcrlock.img" truncate -s 20M "$img"