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.

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

Fix secure boot issue when measuring the kernel #51

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion Makefile.in
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ ORACLE_SRCS = oracle.c \
store.c \
util.c \
sd-boot.c \
uapi.c
uapi.c \
secure_boot.c
ORACLE_OBJS = $(addprefix build/,$(patsubst %.c,%.o,$(ORACLE_SRCS)))

all: $(TOOLS) $(MANPAGES)
Expand Down
2 changes: 1 addition & 1 deletion configure
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
# Invoke with --help for a description of options
#
# microconf:begin
# version 0.5.4
# version 0.5.5
# require libtss2
# require json
# disable debug-authenticode
Expand Down
2 changes: 1 addition & 1 deletion microconf/version
Original file line number Diff line number Diff line change
@@ -1 +1 @@
uc_version=0.5.4
uc_version=0.5.5
62 changes: 55 additions & 7 deletions src/efi-application.c
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@
*/
static const tpm_evdigest_t * __tpm_event_efi_bsa_rehash(const tpm_event_t *, const tpm_parsed_event_t *, tpm_event_log_rehash_ctx_t *);
static bool __tpm_event_efi_bsa_extract_location(tpm_parsed_event_t *parsed);
static bool __tpm_event_efi_bsa_inspect_image(tpm_parsed_event_t *parsed);
static bool __tpm_event_efi_bsa_inspect_image(struct efi_bsa_event *evspec);

static bool __is_shim_issue(const tpm_event_t *ev, const struct efi_bsa_event *evspec);

static void
__tpm_event_efi_bsa_destroy(tpm_parsed_event_t *parsed)
Expand Down Expand Up @@ -111,9 +113,18 @@ __tpm_event_parse_efi_bsa(tpm_event_t *ev, tpm_parsed_event_t *parsed, buffer_t
assign_string(&ctx->efi_partition, evspec->efi_partition);
else
assign_string(&evspec->efi_partition, ctx->efi_partition);
__tpm_event_efi_bsa_inspect_image(parsed);
__tpm_event_efi_bsa_inspect_image(evspec);
}

/* When the shim issue is present the efi_application will be
* empty. The binary path will be reconstructed with the
* --next-kernel parameter, but to generate the full path the
* `efi_partition` is needed.
*/
if (__is_shim_issue(ev, evspec))
assign_string(&evspec->efi_partition, ctx->efi_partition);


return true;
}

Expand Down Expand Up @@ -150,9 +161,8 @@ __tpm_event_efi_bsa_extract_location(tpm_parsed_event_t *parsed)
}

static bool
__tpm_event_efi_bsa_inspect_image(tpm_parsed_event_t *parsed)
__tpm_event_efi_bsa_inspect_image(struct efi_bsa_event *evspec)
{
struct efi_bsa_event *evspec = &parsed->efi_bsa_event;
char path[PATH_MAX];
const char *display_name;
buffer_t *img_data;
Expand Down Expand Up @@ -274,6 +284,31 @@ efi_application_extract_signer(const tpm_parsed_event_t *parsed)
return authenticode_get_signer(evspec->img_info);
}

static bool __is_shim_issue(const tpm_event_t *ev, const struct efi_bsa_event *evspec)
{
/* When secure boot is enabled and shim is installed,
* systemd-boot installs some security overrides that will
* delegate into shim (via shim_validate from systemd-boot)
* the validation of the kernel signature.
*
* The shim_validate function receives the device path from
* the firmware, and is used to load the kernel into memory.
* At the end call shim_verify from shim, but pass only the
* buffer with the loaded image.
*
* The net result is that the event log
* EV_EFI_BOOT_SERVICES_APPLICATION registered by shim_verify
* will not contain the device path that pcr-oracle requires
* to rehash the binary.
*
* So far only the kernel is presenting this issue (when
* systemd-boot is used, GRUB2 needs to be evaluated), so this
* can be detected if there is an event registered in PCR 4
* without path.
*/
return (secure_boot_enabled() && ev->pcr_index == 4 && !evspec->efi_application);
}

static const tpm_evdigest_t *
__tpm_event_efi_bsa_rehash(const tpm_event_t *ev, const tpm_parsed_event_t *parsed, tpm_event_log_rehash_ctx_t *ctx)
{
Expand All @@ -285,17 +320,30 @@ __tpm_event_efi_bsa_rehash(const tpm_event_t *ev, const tpm_parsed_event_t *pars
* We're not yet prepared to handle these, so we hope the user doesn't mess with them, and
* return the original digest from the event log.
*/
if (!evspec->efi_application) {
debug("Unable to locate boot service application - probably not a file\n");
if (!evspec->efi_application && !(__is_shim_issue(ev, evspec) && ctx->boot_entry)) {
if (__is_shim_issue(ev, evspec) && !ctx->boot_entry)
debug("Unable to locate boot service application - missing device path because shim issue");
else
debug("Unable to locate boot service application - probably not a file\n");
return tpm_event_get_digest(ev, ctx->algo);
}

/* The next boot can have a different kernel */
if (sdb_is_kernel(evspec->efi_application) && ctx->boot_entry) {
if ((sdb_is_kernel(evspec->efi_application) || __is_shim_issue(ev, evspec)) && ctx->boot_entry) {
if (__is_shim_issue(ev, evspec))
debug("Empty device path for the kernel - building one based on next kernel\n");

/* TODO: the parsed data type did not change, so all
* the description correspond to the current event
* log, and not the asset that has been measured. The
* debug output can then be missleading.
*/
debug("Measuring %s\n", ctx->boot_entry->image_path);
new_application = ctx->boot_entry->image_path;
if (new_application) {
evspec_clone = *evspec;
evspec_clone.efi_application = strdup(new_application);
__tpm_event_efi_bsa_inspect_image(&evspec_clone);
evspec = &evspec_clone;
}
}
Expand Down
16 changes: 9 additions & 7 deletions src/eventlog.c
Original file line number Diff line number Diff line change
Expand Up @@ -790,8 +790,8 @@ static const tpm_evdigest_t *
__tpm_event_systemd_rehash(const tpm_event_t *ev, const tpm_parsed_event_t *parsed, tpm_event_log_rehash_ctx_t *ctx)
{
const uapi_boot_entry_t *boot_entry = ctx->boot_entry;
char initrd[2048];
char initrd_utf16[4096];
char cmdline[2048];
char cmdline_utf16[4096];
unsigned int len;

/* If no --next-kernel option was given, do not rehash anything */
Expand All @@ -804,15 +804,16 @@ __tpm_event_systemd_rehash(const tpm_event_t *ev, const tpm_parsed_event_t *pars
}

debug("Next boot entry expected from: %s %s\n", boot_entry->title, boot_entry->version? : "");
snprintf(initrd, sizeof(initrd), "initrd=%s %s",
snprintf(cmdline, sizeof(cmdline), "initrd=%s %s",
path_unix2dos(boot_entry->initrd_path),
boot_entry->options? : "");
debug("Measuring Kernel command line: %s\n", cmdline);

len = (strlen(initrd) + 1) << 1;
assert(len <= sizeof(initrd_utf16));
__convert_to_utf16le(initrd, strlen(initrd) + 1, initrd_utf16, len);
len = (strlen(cmdline) + 1) << 1;
assert(len <= sizeof(cmdline_utf16));
__convert_to_utf16le(cmdline, strlen(cmdline) + 1, cmdline_utf16, len);

return digest_compute(ctx->algo, initrd_utf16, len);
return digest_compute(ctx->algo, cmdline_utf16, len);
}

/*
Expand Down Expand Up @@ -876,6 +877,7 @@ __tpm_event_tag_initrd_rehash(const tpm_event_t *ev, const tpm_parsed_event_t *p
}

debug("Next boot entry expected from: %s %s\n", boot_entry->title, boot_entry->version? : "");
debug("Measuring initrd: %s\n", boot_entry->initrd_path);
return runtime_digest_efi_file(ctx->algo, boot_entry->initrd_path);
}

Expand Down
2 changes: 2 additions & 0 deletions src/eventlog.h
Original file line number Diff line number Diff line change
Expand Up @@ -323,4 +323,6 @@ extern bool shim_variable_name_valid(const char *name);
extern const char * shim_variable_get_rtname(const char *name);
extern const char * shim_variable_get_full_rtname(const char *name);

extern bool secure_boot_enabled();

#endif /* EVENTLOG_H */
1 change: 1 addition & 0 deletions src/oracle.c
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,7 @@ pcr_bank_extend_register(tpm_pcr_bank_t *bank, unsigned int pcr_index, const tpm
static void
predictor_extend_hash(struct predictor *pred, unsigned int pcr_index, const tpm_evdigest_t *d)
{
debug("Extend PCR#%d: %s\n", pcr_index, digest_print(d));
pcr_bank_extend_register(&pred->prediction, pcr_index, d);
}

Expand Down
3 changes: 3 additions & 0 deletions src/sd-boot.c
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ sdb_is_kernel(const char *application)
char *path_copy;
int found = 0;

if (!application)
return false;

match = get_valid_kernel_entry_tokens();
path_copy = strdup(application);

Expand Down
44 changes: 44 additions & 0 deletions src/secure_boot.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Copyright (C) 2023 SUSE LLC
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*
* Written by Alberto Planas <[email protected]>
*/

#include <stdio.h>
#include "bufparser.h"
#include "runtime.h"

#define SECURE_BOOT_EFIVAR_NAME "SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c"


bool
secure_boot_enabled()
{
buffer_t *data;
uint8_t enabled;

data = runtime_read_efi_variable(SECURE_BOOT_EFIVAR_NAME);
if (data == NULL) {
return false;
}

if (!buffer_get_u8(data, &enabled)) {
return false;
}

return enabled == 1;
}