From 2c90b5ec63ab420d074ebe4f5c6881737c9bc155 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Sun, 21 May 2023 15:18:21 +0100 Subject: [PATCH 01/11] stub: measure SMBIOS kernel-cmdline-extra in PCR12 PCR1, where SMBIOS strings are measured, is filled with data that is not under the control of the machine owner. Measure cmdline extensions in PCR12 too, where we measure other optional addons that are loaded by sd-stub. --- man/systemd-stub.xml | 5 ++++- src/boot/efi/stub.c | 8 +++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/man/systemd-stub.xml b/man/systemd-stub.xml index 21b79cd35f..4cbf9cde85 100644 --- a/man/systemd-stub.xml +++ b/man/systemd-stub.xml @@ -63,6 +63,9 @@ A compiled binary DeviceTree will be looked for in the .dtb PE section. + Kernel version information, i.e. the output of uname -r for the + kernel included in the UKI, in the .uname PE section. + The kernel command line to pass to the invoked kernel will be looked for in the .cmdline PE section. @@ -391,7 +394,7 @@ io.systemd.stub.kernel-cmdline-extra If set, the value of this string is added to the list of kernel command line - arguments that are passed to the kernel. + arguments that are measured in PCR12 and passed to the kernel. diff --git a/src/boot/efi/stub.c b/src/boot/efi/stub.c index eb4bd77ac3..c8bbd36f3c 100644 --- a/src/boot/efi/stub.c +++ b/src/boot/efi/stub.c @@ -277,11 +277,17 @@ static EFI_STATUS run(EFI_HANDLE image) { mangle_stub_cmdline(cmdline); } - /* SMBIOS strings are measured in PCR1, so we do not re-measure these command line extensions. */ const char *extra = smbios_find_oem_string("io.systemd.stub.kernel-cmdline-extra"); if (extra) { _cleanup_free_ char16_t *tmp = TAKE_PTR(cmdline), *extra16 = xstr8_to_16(extra); cmdline = xasprintf("%ls %ls", tmp, extra16); + + /* SMBIOS strings are measured in PCR1, but we also want to measure them in our specific + * PCR12, as firmware-owned PCRs are very difficult to use as they'll contain unpredictable + * measurements that are not under control of the machine owner. */ + m = false; + (void) tpm_log_load_options(extra16, &m); + parameters_measured = parameters_measured < 0 ? m : (parameters_measured && m); } export_variables(loaded_image); From 3e8cb05e1263d684cdc315e32e933e5895bd6257 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Fri, 12 May 2023 00:49:25 +0100 Subject: [PATCH 02/11] efi: move get_dropin_dir to util.c Will be used elsewhere in a later commit. Rename to clarify that it provides .extra.d/ directories. --- src/boot/efi/cpio.c | 25 +------------------------ src/boot/efi/util.c | 24 ++++++++++++++++++++++++ src/boot/efi/util.h | 2 ++ 3 files changed, 27 insertions(+), 24 deletions(-) diff --git a/src/boot/efi/cpio.c b/src/boot/efi/cpio.c index f82a31b475..bb3754a3bc 100644 --- a/src/boot/efi/cpio.c +++ b/src/boot/efi/cpio.c @@ -301,29 +301,6 @@ static EFI_STATUS pack_cpio_trailer( return EFI_SUCCESS; } -static char16_t *get_dropin_dir(const EFI_DEVICE_PATH *file_path) { - if (!file_path) - return NULL; - - /* A device path is allowed to have more than one file path node. If that is the case they are - * supposed to be concatenated. Unfortunately, the device path to text protocol simply converts the - * nodes individually and then combines those with the usual '/' for device path nodes. But this does - * not create a legal EFI file path that the file protocol can use. */ - - /* Make sure we really only got file paths. */ - for (const EFI_DEVICE_PATH *node = file_path; !device_path_is_end(node); - node = device_path_next_node(node)) - if (node->Type != MEDIA_DEVICE_PATH || node->SubType != MEDIA_FILEPATH_DP) - return NULL; - - _cleanup_free_ char16_t *file_path_str = NULL; - if (device_path_to_str(file_path, &file_path_str) != EFI_SUCCESS) - return NULL; - - convert_efi_path(file_path_str); - return xasprintf("%ls.extra.d", file_path_str); -} - EFI_STATUS pack_cpio( EFI_LOADED_IMAGE_PROTOCOL *loaded_image, const char16_t *dropin_dir, @@ -363,7 +340,7 @@ EFI_STATUS pack_cpio( return log_error_status(err, "Unable to open root directory: %m"); if (!dropin_dir) - dropin_dir = rel_dropin_dir = get_dropin_dir(loaded_image->FilePath); + dropin_dir = rel_dropin_dir = get_extra_dir(loaded_image->FilePath); err = open_directory(root, dropin_dir, &extra_dir); if (err == EFI_NOT_FOUND) diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c index c5f1742466..d5a23338fe 100644 --- a/src/boot/efi/util.c +++ b/src/boot/efi/util.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ +#include "device-path-util.h" #include "proto/device-path.h" #include "proto/simple-text-io.h" #include "ticks.h" @@ -665,3 +666,26 @@ void *find_configuration_table(const EFI_GUID *guid) { return NULL; } + +char16_t *get_extra_dir(const EFI_DEVICE_PATH *file_path) { + if (!file_path) + return NULL; + + /* A device path is allowed to have more than one file path node. If that is the case they are + * supposed to be concatenated. Unfortunately, the device path to text protocol simply converts the + * nodes individually and then combines those with the usual '/' for device path nodes. But this does + * not create a legal EFI file path that the file protocol can use. */ + + /* Make sure we really only got file paths. */ + for (const EFI_DEVICE_PATH *node = file_path; !device_path_is_end(node); + node = device_path_next_node(node)) + if (node->Type != MEDIA_DEVICE_PATH || node->SubType != MEDIA_FILEPATH_DP) + return NULL; + + _cleanup_free_ char16_t *file_path_str = NULL; + if (device_path_to_str(file_path, &file_path_str) != EFI_SUCCESS) + return NULL; + + convert_efi_path(file_path_str); + return xasprintf("%ls.extra.d", file_path_str); +} diff --git a/src/boot/efi/util.h b/src/boot/efi/util.h index 929f3bca8c..aadbbdad0d 100644 --- a/src/boot/efi/util.h +++ b/src/boot/efi/util.h @@ -212,3 +212,5 @@ static inline bool efi_guid_equal(const EFI_GUID *a, const EFI_GUID *b) { } void *find_configuration_table(const EFI_GUID *guid); + +char16_t *get_extra_dir(const EFI_DEVICE_PATH *file_path); From e715d82de6694d82a17921b5ccbcf47398604068 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Fri, 12 May 2023 00:49:57 +0100 Subject: [PATCH 03/11] efi: support passing empty cmdline to mangle_stub_cmdline() Just return instead of crashing --- src/boot/efi/util.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c index d5a23338fe..42526d7005 100644 --- a/src/boot/efi/util.c +++ b/src/boot/efi/util.c @@ -268,6 +268,9 @@ char16_t *xstr8_to_path(const char *str8) { void mangle_stub_cmdline(char16_t *cmdline) { char16_t *p = cmdline; + if (!cmdline) + return; + for (; *cmdline != '\0'; cmdline++) /* Convert ASCII control characters to spaces. */ if (*cmdline <= 0x1F) From e1f1b5fc62f721a3a4c14d97ad01447b2ac07d6d Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Fri, 12 May 2023 00:51:19 +0100 Subject: [PATCH 04/11] efi: set EFIVAR to stop Shim from uninstalling its protocol We'll use it from the stub to validate files. Requires Shim 5.18. By default, Shim uninstalls its protocol when calling StartImage(), so when loading systemd-boot via shim and then loading an UKI, the UKI's sd-stub will no longer be able to use the shim verification protocol by default. --- src/boot/efi/boot.c | 4 ++++ src/boot/efi/shim.c | 9 +++++++++ src/boot/efi/shim.h | 1 + 3 files changed, 14 insertions(+) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 65294f3c09..67f4a5ea62 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -2641,6 +2641,10 @@ static EFI_STATUS run(EFI_HANDLE image) { init_usec = time_usec(); + /* Ask Shim to leave its protocol around, so that the stub can use it to validate PEs. + * By default, Shim uninstalls its protocol when calling StartImage(). */ + shim_retain_protocol(); + err = BS->OpenProtocol( image, MAKE_GUID_PTR(EFI_LOADED_IMAGE_PROTOCOL), diff --git a/src/boot/efi/shim.c b/src/boot/efi/shim.c index dda727ee8e..df136ed6d9 100644 --- a/src/boot/efi/shim.c +++ b/src/boot/efi/shim.c @@ -97,3 +97,12 @@ EFI_STATUS shim_load_image(EFI_HANDLE parent, const EFI_DEVICE_PATH *device_path return ret; } + +void shim_retain_protocol(void) { + uint8_t value = 1; + + /* Ask Shim to avoid uninstalling its security protocol, so that we can use it from sd-stub to + * validate PE addons. By default, Shim uninstalls its protocol when calling StartImage(). + * Requires Shim 15.8. */ + (void) efivar_set_raw(MAKE_GUID_PTR(SHIM_LOCK), u"ShimRetainProtocol", &value, sizeof(value), 0); +} diff --git a/src/boot/efi/shim.h b/src/boot/efi/shim.h index 44155d21fc..e0cb39f795 100644 --- a/src/boot/efi/shim.h +++ b/src/boot/efi/shim.h @@ -13,3 +13,4 @@ bool shim_loaded(void); EFI_STATUS shim_load_image(EFI_HANDLE parent, const EFI_DEVICE_PATH *device_path, EFI_HANDLE *ret_image); +void shim_retain_protocol(void); From b6f2e6860220aa89550f690b12246c4e8eb6e908 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Sun, 21 May 2023 14:32:09 +0100 Subject: [PATCH 05/11] stub/measure: document and measure .uname UKI section --- man/systemd-stub.xml | 3 +++ src/boot/measure.c | 3 +++ src/fundamental/tpm-pcr.c | 1 + src/fundamental/tpm-pcr.h | 1 + src/ukify/ukify.py | 2 +- 5 files changed, 9 insertions(+), 1 deletion(-) diff --git a/man/systemd-stub.xml b/man/systemd-stub.xml index 4cbf9cde85..e0513f2136 100644 --- a/man/systemd-stub.xml +++ b/man/systemd-stub.xml @@ -57,6 +57,9 @@ os-release5 file of the OS the kernel belongs to, in the .osrel PE section. + Kernel version information, i.e. the output of uname -r for the + kernel included in the UKI, in the .uname PE section. + The initrd will be loaded from the .initrd PE section. diff --git a/src/boot/measure.c b/src/boot/measure.c index 072f38f200..9b677a2a48 100644 --- a/src/boot/measure.c +++ b/src/boot/measure.c @@ -83,6 +83,7 @@ static int help(int argc, char *argv[], void *userdata) { " --initrd=PATH Path to initrd image file %7$s .initrd\n" " --splash=PATH Path to splash bitmap file %7$s .splash\n" " --dtb=PATH Path to Devicetree file %7$s .dtb\n" + " --uname=PATH Path to 'uname -r' file %7$s .uname\n" " --pcrpkey=PATH Path to public key for PCR signatures %7$s .pcrpkey\n" "\nSee the %2$s for details.\n", program_invocation_short_name, @@ -122,6 +123,7 @@ static int parse_argv(int argc, char *argv[]) { ARG_INITRD, ARG_SPLASH, ARG_DTB, + ARG_UNAME, _ARG_PCRSIG, /* the .pcrsig section is not input for signing, hence not actually an argument here */ _ARG_SECTION_LAST, ARG_PCRPKEY = _ARG_SECTION_LAST, @@ -144,6 +146,7 @@ static int parse_argv(int argc, char *argv[]) { { "initrd", required_argument, NULL, ARG_INITRD }, { "splash", required_argument, NULL, ARG_SPLASH }, { "dtb", required_argument, NULL, ARG_DTB }, + { "uname", required_argument, NULL, ARG_UNAME }, { "pcrpkey", required_argument, NULL, ARG_PCRPKEY }, { "current", no_argument, NULL, 'c' }, { "bank", required_argument, NULL, ARG_BANK }, diff --git a/src/fundamental/tpm-pcr.c b/src/fundamental/tpm-pcr.c index 7609d83c2e..0685d37b05 100644 --- a/src/fundamental/tpm-pcr.c +++ b/src/fundamental/tpm-pcr.c @@ -11,6 +11,7 @@ const char* const unified_sections[_UNIFIED_SECTION_MAX + 1] = { [UNIFIED_SECTION_INITRD] = ".initrd", [UNIFIED_SECTION_SPLASH] = ".splash", [UNIFIED_SECTION_DTB] = ".dtb", + [UNIFIED_SECTION_UNAME] = ".uname", [UNIFIED_SECTION_PCRSIG] = ".pcrsig", [UNIFIED_SECTION_PCRPKEY] = ".pcrpkey", NULL, diff --git a/src/fundamental/tpm-pcr.h b/src/fundamental/tpm-pcr.h index e12b4ff607..4989d93f0c 100644 --- a/src/fundamental/tpm-pcr.h +++ b/src/fundamental/tpm-pcr.h @@ -29,6 +29,7 @@ typedef enum UnifiedSection { UNIFIED_SECTION_INITRD, UNIFIED_SECTION_SPLASH, UNIFIED_SECTION_DTB, + UNIFIED_SECTION_UNAME, UNIFIED_SECTION_PCRSIG, UNIFIED_SECTION_PCRPKEY, _UNIFIED_SECTION_MAX, diff --git a/src/ukify/ukify.py b/src/ukify/ukify.py index d87670eb24..3167f5dbc5 100755 --- a/src/ukify/ukify.py +++ b/src/ukify/ukify.py @@ -658,10 +658,10 @@ def make_uki(opts): ('.osrel', opts.os_release, True ), ('.cmdline', opts.cmdline, True ), ('.dtb', opts.devicetree, True ), + ('.uname', opts.uname, True ), ('.splash', opts.splash, True ), ('.pcrpkey', pcrpkey, True ), ('.initrd', initrd, True ), - ('.uname', opts.uname, False), # linux shall be last to leave breathing room for decompression. # We'll add it later. From e18e3c4305537ee18a0b61676b0a8f30efedac19 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Tue, 23 May 2023 01:43:59 +0100 Subject: [PATCH 06/11] elf2efi: add parameter to increase reserved space for headers When building a minimal empty addon it would not have enough space to append sections. Add an option that will later be used to reserve enough space. --- tools/elf2efi.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tools/elf2efi.py b/tools/elf2efi.py index 6179ba8213..20d25321c4 100755 --- a/tools/elf2efi.py +++ b/tools/elf2efi.py @@ -512,10 +512,11 @@ def elf2efi(args: argparse.Namespace): opt.SizeOfImage = align_to( sections[-1].VirtualAddress + sections[-1].VirtualSize, SECTION_ALIGNMENT ) + opt.SizeOfHeaders = align_to( PE_OFFSET + coff.SizeOfOptionalHeader - + sizeof(PeSection) * coff.NumberOfSections, + + sizeof(PeSection) * max(coff.NumberOfSections, args.minimum_sections), FILE_ALIGNMENT, ) # DYNAMIC_BASE|NX_COMPAT|HIGH_ENTROPY_VA or DYNAMIC_BASE|NX_COMPAT @@ -578,6 +579,12 @@ def main(): type=argparse.FileType("wb"), help="Output PE/EFI file", ) + parser.add_argument( + "--minimum-sections", + type=int, + default=0, + help="Minimum number of sections to leave space for", + ) elf2efi(parser.parse_args()) From e78fc81d30f254dec3ce998ba19ecee22bb3eb04 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Tue, 23 May 2023 02:12:12 +0100 Subject: [PATCH 07/11] elf2efi: ensure minimum gap between .text and other sections When linking an almost empty binary the linker can merged .text with a later section, creating a RWE segment, that then it rejects. --- tools/elf2efi.lds | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tools/elf2efi.lds b/tools/elf2efi.lds index 805efc156d..6e9eff0763 100644 --- a/tools/elf2efi.lds +++ b/tools/elf2efi.lds @@ -6,6 +6,13 @@ SECTIONS { .text ALIGN(CONSTANT(MAXPAGESIZE)) : { *(.text .text.*) } + + /* When linking a minimal addon stub, the linker can merge .text and .dynsym, creating a RWE + * segment, and then rejects it. Ensure there's a gap so that we end up with two separate segments. + * The alignments for the next sections are only applied if the section exists, so they are not + * enough, and we need to have this unconditional one. */ + . = ALIGN(CONSTANT(MAXPAGESIZE)); + .rodata ALIGN(CONSTANT(MAXPAGESIZE)) : { *(.rodata .rodata.*) *(.srodata .srodata.*) From f644ea3ed7ec22c28814b194e4e5bbbf2fa98560 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Tue, 23 May 2023 01:45:40 +0100 Subject: [PATCH 08/11] ukify: use empty stub for addons Instead of picking up sd-stub, which is runnable, add an empty addon stub that just returns an error if executed --- src/boot/efi/addon.c | 14 ++++++++++++++ src/boot/efi/meson.build | 22 +++++++++++++++++++++- src/ukify/ukify.py | 5 ++++- 3 files changed, 39 insertions(+), 2 deletions(-) create mode 100644 src/boot/efi/addon.c diff --git a/src/boot/efi/addon.c b/src/boot/efi/addon.c new file mode 100644 index 0000000000..d2ca770c65 --- /dev/null +++ b/src/boot/efi/addon.c @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "efi.h" + +/* Magic string for recognizing our own binaries */ +_used_ _section_(".sdmagic") static const char magic[] = + "#### LoaderInfo: systemd-addon " GIT_VERSION " ####"; + +/* This is intended to carry data, not to be executed */ + +EFIAPI EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *system_table); +EFIAPI EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *system_table) { + return EFI_UNSUPPORTED; +} diff --git a/src/boot/efi/meson.build b/src/boot/efi/meson.build index 9a560cf193..485f5d2e9d 100644 --- a/src/boot/efi/meson.build +++ b/src/boot/efi/meson.build @@ -259,6 +259,10 @@ stub_sources = files( 'stub.c', ) +addon_sources = files( + 'addon.c', +) + if get_option('b_sanitize') == 'undefined' libefi_sources += files('ubsan.c') endif @@ -328,12 +332,27 @@ foreach archspec : efi_archspecs override_options : efi_override_options, name_suffix : 'elf.stub', pie : true) + + efi_elf_binaries += executable( + 'addon' + archspec['arch'], + addon_sources, + include_directories : efi_includes, + c_args : archspec['c_args'], + link_args : archspec['link_args'], + link_depends : elf2efi_lds, + gnu_symbol_visibility : 'hidden', + override_options : efi_override_options, + name_suffix : 'elf.stub', + pie : true) endforeach foreach efi_elf_binary : efi_elf_binaries # FIXME: Use build_tgt.name() with meson >= 0.54.0 name = fs.name(efi_elf_binary.full_path()).split('.')[0] - name += name.startswith('linux') ? '.efi.stub' : '.efi' + name += name.startswith('systemd-boot') ? '.efi' : '.efi.stub' + # For the addon, given it's empty, we need to explicitly reserve space in the header to account for + # the sections that ukify will add. + minimum_sections = name.startswith('addon') ? '7' : '0' exe = custom_target( name, output : name, @@ -348,6 +367,7 @@ foreach efi_elf_binary : efi_elf_binaries '--efi-major=1', '--efi-minor=1', '--subsystem=10', + '--minimum-sections=' + minimum_sections, '@INPUT@', '@OUTPUT@', ]) diff --git a/src/ukify/ukify.py b/src/ukify/ukify.py index 3167f5dbc5..3a0c7af362 100755 --- a/src/ukify/ukify.py +++ b/src/ukify/ukify.py @@ -1141,7 +1141,10 @@ def finalize_options(opts): opts.efi_arch = guess_efi_arch() if opts.stub is None: - opts.stub = pathlib.Path(f'/usr/lib/systemd/boot/efi/linux{opts.efi_arch}.efi.stub') + if opts.linux is not None: + opts.stub = pathlib.Path(f'/usr/lib/systemd/boot/efi/linux{opts.efi_arch}.efi.stub') + else: + opts.stub = pathlib.Path(f'/usr/lib/systemd/boot/efi/addon{opts.efi_arch}.efi.stub') if opts.signing_engine is None: if opts.sb_key: From c67d5a027d7a34ab19a12f7585dd7c143d82481d Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Sun, 21 May 2023 14:32:39 +0100 Subject: [PATCH 09/11] ukify: add default .sbat section for addons In order to ensure addons can always be revoked via SBAT, and it is not left out by mistake, have a default metadata entry if none is specified by the caller. https://github.com/rhboot/shim/blob/main/SBAT.md --- man/ukify.xml | 26 +++++++++++++++++++++++++- src/ukify/ukify.py | 14 +++++++++++++- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/man/ukify.xml b/man/ukify.xml index 2e22b1f42e..4531ac89b2 100644 --- a/man/ukify.xml +++ b/man/ukify.xml @@ -50,6 +50,7 @@ Splash=/, PCRPKey=/, Uname=/, + SBAT=/, and below. @@ -369,6 +370,27 @@ + + + [Addon:<replaceable>NAME</replaceable>] section + + Currently, these options only apply when building PE addons. + + + + SBAT=TEXT|@PATH + + + SBAT metadata associated with the addon. SBAT policies are useful to revoke whole + groups of addons with a single, static policy update that does not take space in DBX/MOKX. If not + specified manually, a default metadata entry consisting of + uki.addon.systemd,1,UKI Addon,uki.addon.systemd,1,https://www.freedesktop.org/software/systemd/man/systemd-stub.html + will be used, to ensure it is always possible to revoke addons. For more information on SBAT see + Shim's documentation. + + + + @@ -464,11 +486,13 @@ Phases=enter-initrd:leave-initrd --secureboot-private-key=sb.key \ --secureboot-certificate=sb.cert \ --cmdline='debug' \ + --sbat='sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md + uki.addon.author,1,UKI Addon for System,uki.addon.author,1,https://www.freedesktop.org/software/systemd/man/systemd-stub.html' --output=debug.cmdline This creates a signed PE binary that contains the additional kernel command line parameter - debug. + debug with SBAT metadata referring to the owner of the addon. diff --git a/src/ukify/ukify.py b/src/ukify/ukify.py index 3a0c7af362..66e176cd0c 100755 --- a/src/ukify/ukify.py +++ b/src/ukify/ukify.py @@ -679,10 +679,12 @@ def make_uki(opts): call_systemd_measure(uki, linux, opts=opts) - # UKI creation + # UKI or addon creation - addons don't use the stub so we add SBAT manually if linux is not None: uki.add_section(Section.create('.linux', linux, measure=True)) + elif opts.sbat: + uki.add_section(Section.create('.sbat', opts.sbat, measure=False)) if sign_args_present: unsigned = tempfile.NamedTemporaryFile(prefix='uki') @@ -927,6 +929,16 @@ CONFIG_ITEMS = [ config_key = 'UKI/Stub', ), + ConfigItem( + '--sbat', + metavar = 'TEXT|@PATH', + help = 'SBAT policy [.sbat section] for addons', + default = """sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md +uki.addon,1,UKI Addon,uki.addon,1,https://www.freedesktop.org/software/systemd/man/systemd-stub.html +""", + config_key = 'Addon/SBAT', + ), + ConfigItem( '--section', dest = 'sections', From 05c9f9c2517c54b98d55f08f8afa67c79be861e8 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Fri, 12 May 2023 00:55:58 +0100 Subject: [PATCH 10/11] stub: allow loading and verifying cmdline addons Files placed in /EFI/Linux/UKI.efi.extra.d/ and /loader/addons/ are opened and verified using the LoadImage protocol, and will thus get verified via shim/firmware. If they are valid signed PE files, the .cmdline section will be extracted and appended. If there are multiple addons in each directory, they will be parsed in alphanumerical order. Optionally the .uname sections are also matched if present, so that they can be used to filter out addons as well if needed, and only addons that correspond exactly to the UKI being loaded are used. It is recommended to also always add a .sbat section to addons, so that they can be mass-revoked with just a policy update. The files must have a .addon.efi suffix. Files in the per-UKI directory are parsed, sorted, measured and appended first. Then, files in the generic directory are processed. --- man/systemd-stub.xml | 32 +++ mkosi.presets/00-base/mkosi.build | 12 + .../lib/systemd/mkosi-check-and-shutdown.sh | 2 + src/boot/efi/addon.c | 1 + src/boot/efi/meson.build | 3 +- src/boot/efi/stub.c | 216 ++++++++++++++++++ 6 files changed, 265 insertions(+), 1 deletion(-) diff --git a/man/systemd-stub.xml b/man/systemd-stub.xml index e0513f2136..497f51c4f9 100644 --- a/man/systemd-stub.xml +++ b/man/systemd-stub.xml @@ -28,8 +28,10 @@ /usr/lib/systemd/boot/efi/linuxx64.efi.stub /usr/lib/systemd/boot/efi/linuxia32.efi.stub /usr/lib/systemd/boot/efi/linuxaa64.efi.stub + ESP/.../foo.efi.extra.d/*.addon.efi ESP/.../foo.efi.extra.d/*.cred ESP/.../foo.efi.extra.d/*.raw + ESP/loader/addons/*.addon.efi ESP/loader/credentials/*.cred @@ -148,11 +150,41 @@ details on system extension images. The generated cpio archive containing these system extension images is measured into TPM PCR 13 (if a TPM is present). + Similarly, files + foo.efi.extra.d/*.addon.efi + are loaded and verified as PE binaries, and a .cmdline section is parsed from them. + In case Secure Boot is enabled, these files will be validated using keys in UEFI DB, Shim's DB or + Shim's MOK, and will be rejected otherwise. Additionally, if the both the addon and the UKI contain a + a .uname section, the addon will be rejected if they do not exactly match. It is + recommended to always add a .sbat section to all signed addons, so that they may be + revoked with a SBAT policy update, without requiring blocklisting via DBX/MOKX. The + ukify1 tool will + add a SBAT policy by default if none is passed when building addons. For more information on SBAT see + Shim's documentation. + Addons are supposed to be used to pass additional kernel command line parameters, regardless of the + kernel image being booted, for example to allow platform vendors to ship platform-specific + configuration. The loaded command line addon files are sorted, loaded, measured into TPM PCR 12 (if a + TPM is present) and appended to the kernel command line. UKI command line options are listed first, + then options from addons in /loader/addons/*.addon.efi are appended next, and + finally UKI-specific addons are appended last. Addons are always loaded in the same order based on the + filename, so that, given the same set of addons, the same set of measurements can be expected in + PCR12, however note that the filename is not protected by the PE signature, and as such an attacker + with write access to the ESP could potentially rename these files to change the order in which they + are loaded, in a way that could alter the functionality of the kernel, as some options might be order + dependent. If you sign such addons, you should pay attention to the PCR12 values and make use of an + attestation service so that improper use of your signed addons can be detected and dealt with using + one of the aforementioned revocation mechanisms. + Files /loader/credentials/*.cred are packed up in a cpio archive and placed in the /.extra/global_credentials/ directory of the initrd file hierarchy. This is supposed to be used to pass additional credentials to the initrd, regardless of the kernel being booted. The generated cpio archive is measured into TPM PCR 12 (if a TPM is present) + + Additionally, files /loader/addons/*.addon.efi are loaded and + verified as PE binaries, and a .cmdline section is parsed from them. This is + supposed to be used to pass additional command line parameters to the kernel, regardless of the kernel + being booted. These mechanisms may be used to parameterize and extend trusted (i.e. signed), immutable initrd diff --git a/mkosi.presets/00-base/mkosi.build b/mkosi.presets/00-base/mkosi.build index 167bd751d3..627df80684 100755 --- a/mkosi.presets/00-base/mkosi.build +++ b/mkosi.presets/00-base/mkosi.build @@ -193,3 +193,15 @@ if [ "$WITH_TESTS" = 1 ]; then fi ( set -x; meson install -C "$BUILDDIR" --quiet --no-rebuild --only-changed ) + +# Ensure that side-loaded PE addons are loaded if signed, and ignored if not +if [ -d "${DESTDIR}/boot/loader" ]; then + addons_dir="${DESTDIR}/boot/loader/addons" +elif [ -d "${DESTDIR}/efi/loader" ]; then + addons_dir="${DESTDIR}/efi/loader/addons" +fi +if [ -n "${addons_dir}" ]; then + mkdir -p "${addons_dir}" + ukify --secureboot-private-key mkosi.secure-boot.key --secureboot-certificate mkosi.secure-boot.crt --cmdline this_should_be_here -o "${addons_dir}/good.addon.efi" + ukify --cmdline this_should_not_be_here -o "${addons_dir}/bad.addon.efi" +fi diff --git a/mkosi.presets/20-final/mkosi.extra/usr/lib/systemd/mkosi-check-and-shutdown.sh b/mkosi.presets/20-final/mkosi.extra/usr/lib/systemd/mkosi-check-and-shutdown.sh index e6259c42db..ae1385b98b 100755 --- a/mkosi.presets/20-final/mkosi.extra/usr/lib/systemd/mkosi-check-and-shutdown.sh +++ b/mkosi.presets/20-final/mkosi.extra/usr/lib/systemd/mkosi-check-and-shutdown.sh @@ -7,6 +7,8 @@ systemctl --failed --no-legend | tee /failed-services if [[ -d /sys/firmware/efi/efivars/ ]]; then cmp /sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c <(printf '\6\0\0\0\1') cmp /sys/firmware/efi/efivars/SetupMode-8be4df61-93ca-11d2-aa0d-00e098032b8c <(printf '\6\0\0\0\0') + grep -q this_should_be_here /proc/cmdline + grep -q this_should_not_be_here /proc/cmdline && exit 1 fi # Exit with non-zero EC if the /failed-services file is not empty (we have -e set) diff --git a/src/boot/efi/addon.c b/src/boot/efi/addon.c index d2ca770c65..53e8812205 100644 --- a/src/boot/efi/addon.c +++ b/src/boot/efi/addon.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #include "efi.h" +#include "version.h" /* Magic string for recognizing our own binaries */ _used_ _section_(".sdmagic") static const char magic[] = diff --git a/src/boot/efi/meson.build b/src/boot/efi/meson.build index 485f5d2e9d..b573e6996d 100644 --- a/src/boot/efi/meson.build +++ b/src/boot/efi/meson.build @@ -242,6 +242,7 @@ libefi_sources = files( 'pe.c', 'random-seed.c', 'secure-boot.c', + 'shim.c', 'ticks.c', 'util.c', 'vmm.c', @@ -249,7 +250,6 @@ libefi_sources = files( systemd_boot_sources = files( 'boot.c', - 'shim.c', ) stub_sources = files( @@ -340,6 +340,7 @@ foreach archspec : efi_archspecs c_args : archspec['c_args'], link_args : archspec['link_args'], link_depends : elf2efi_lds, + dependencies : versiondep, gnu_symbol_visibility : 'hidden', override_options : efi_override_options, name_suffix : 'elf.stub', diff --git a/src/boot/efi/stub.c b/src/boot/efi/stub.c index c8bbd36f3c..c315a4d51a 100644 --- a/src/boot/efi/stub.c +++ b/src/boot/efi/stub.c @@ -11,6 +11,7 @@ #include "proto/shell-parameters.h" #include "random-seed.h" #include "secure-boot.h" +#include "shim.h" #include "splash.h" #include "tpm-pcr.h" #include "util.h" @@ -180,6 +181,189 @@ static bool use_load_options( return true; } +static EFI_STATUS load_addons_from_dir( + EFI_FILE *root, + const char16_t *prefix, + char16_t ***items, + size_t *n_items, + size_t *n_allocated) { + + _cleanup_(file_closep) EFI_FILE *extra_dir = NULL; + _cleanup_free_ EFI_FILE_INFO *dirent = NULL; + size_t dirent_size = 0; + EFI_STATUS err; + + assert(root); + assert(prefix); + assert(items); + assert(n_items); + assert(n_allocated); + + err = open_directory(root, prefix, &extra_dir); + if (err == EFI_NOT_FOUND) + /* No extra subdir, that's totally OK */ + return EFI_SUCCESS; + if (err != EFI_SUCCESS) + return log_error_status(err, "Failed to open addons directory '%ls': %m", prefix); + + for (;;) { + _cleanup_free_ char16_t *d = NULL; + + err = readdir(extra_dir, &dirent, &dirent_size); + if (err != EFI_SUCCESS) + return log_error_status(err, "Failed to read addons directory of loaded image: %m"); + if (!dirent) /* End of directory */ + break; + + if (dirent->FileName[0] == '.') + continue; + if (FLAGS_SET(dirent->Attribute, EFI_FILE_DIRECTORY)) + continue; + if (!is_ascii(dirent->FileName)) + continue; + if (strlen16(dirent->FileName) > 255) /* Max filename size on Linux */ + continue; + if (!endswith_no_case(dirent->FileName, u".addon.efi")) + continue; + + d = xstrdup16(dirent->FileName); + + if (*n_items + 2 > *n_allocated) { + /* We allocate 16 entries at a time, as a matter of optimization */ + if (*n_items > (SIZE_MAX / sizeof(uint16_t)) - 16) /* Overflow check, just in case */ + return log_oom(); + + size_t m = *n_items + 16; + *items = xrealloc(*items, *n_allocated * sizeof(uint16_t *), m * sizeof(uint16_t *)); + *n_allocated = m; + } + + (*items)[(*n_items)++] = TAKE_PTR(d); + (*items)[*n_items] = NULL; /* Let's always NUL terminate, to make freeing via strv_free() easy */ + } + + return EFI_SUCCESS; + +} + +static EFI_STATUS cmdline_append_and_measure_addons( + EFI_HANDLE stub_image, + EFI_LOADED_IMAGE_PROTOCOL *loaded_image, + const char16_t *prefix, + const char *uname, + bool *ret_parameters_measured, + char16_t **cmdline_append) { + + _cleanup_(strv_freep) char16_t **items = NULL; + _cleanup_(file_closep) EFI_FILE *root = NULL; + _cleanup_free_ char16_t *buffer = NULL; + size_t n_items = 0, n_allocated = 0; + EFI_STATUS err; + + assert(stub_image); + assert(loaded_image); + assert(prefix); + assert(ret_parameters_measured); + assert(cmdline_append); + + if (!loaded_image->DeviceHandle) + return EFI_SUCCESS; + + err = open_volume(loaded_image->DeviceHandle, &root); + if (err == EFI_UNSUPPORTED) + /* Error will be unsupported if the bootloader doesn't implement the file system protocol on + * its file handles. */ + return EFI_SUCCESS; + if (err != EFI_SUCCESS) + return log_error_status(err, "Unable to open root directory: %m"); + + err = load_addons_from_dir(root, prefix, &items, &n_items, &n_allocated); + if (err != EFI_SUCCESS) + return err; + + if (n_items == 0) + return EFI_SUCCESS; /* Empty directory */ + + /* Now, sort the files we found, to make this uniform and stable (and to ensure the TPM measurements + * are not dependent on read order) */ + sort_pointer_array((void**) items, n_items, (compare_pointer_func_t) strcmp16); + + for (size_t i = 0; i < n_items; i++) { + size_t addrs[_UNIFIED_SECTION_MAX] = {}, szs[_UNIFIED_SECTION_MAX] = {}; + _cleanup_free_ EFI_DEVICE_PATH *addon_path = NULL; + _cleanup_(unload_imagep) EFI_HANDLE addon = NULL; + EFI_LOADED_IMAGE_PROTOCOL *loaded_addon = NULL; + _cleanup_free_ char16_t *addon_spath = NULL; + + addon_spath = xasprintf("%ls\\%ls", prefix, items[i]); + err = make_file_device_path(loaded_image->DeviceHandle, addon_spath, &addon_path); + if (err != EFI_SUCCESS) + return log_error_status(err, "Error making device path for %ls: %m", addon_spath); + + /* By using shim_load_image, we cover both the case where the PE files are signed with MoK + * and with DB, and running with or without shim. */ + err = shim_load_image(stub_image, addon_path, &addon); + if (err != EFI_SUCCESS) { + log_error_status(err, + "Failed to read '%ls' from '%ls', ignoring: %m", + items[i], + addon_spath); + continue; + } + + err = BS->HandleProtocol(addon, + MAKE_GUID_PTR(EFI_LOADED_IMAGE_PROTOCOL), + (void **) &loaded_addon); + if (err != EFI_SUCCESS) + return log_error_status(err, "Failed to find protocol in %ls: %m", items[i]); + + err = pe_memory_locate_sections(loaded_addon->ImageBase, unified_sections, addrs, szs); + if (err != EFI_SUCCESS || szs[UNIFIED_SECTION_CMDLINE] == 0) { + if (err == EFI_SUCCESS) + err = EFI_NOT_FOUND; + log_error_status(err, + "Unable to locate embedded .cmdline section in %ls, ignoring: %m", + items[i]); + continue; + } + + /* We want to enforce that addons are not UKIs, i.e.: they must not embed a kernel. */ + if (szs[UNIFIED_SECTION_LINUX] > 0) { + log_error_status(EFI_INVALID_PARAMETER, "%ls is a UKI, not an addon, ignoring: %m", items[i]); + continue; + } + + /* Also enforce that, in case it is specified, .uname matches as a quick way to allow + * enforcing compatibility with a specific UKI only */ + if (uname && szs[UNIFIED_SECTION_UNAME] > 0 && + !strneq8(uname, + (char *)loaded_addon->ImageBase + addrs[UNIFIED_SECTION_UNAME], + szs[UNIFIED_SECTION_UNAME])) { + log_error(".uname mismatch between %ls and UKI, ignoring", items[i]); + continue; + } + + _cleanup_free_ char16_t *tmp = TAKE_PTR(buffer), + *extra16 = xstrn8_to_16((char *)loaded_addon->ImageBase + addrs[UNIFIED_SECTION_CMDLINE], + szs[UNIFIED_SECTION_CMDLINE]); + buffer = xasprintf("%ls%ls%ls", strempty(tmp), isempty(tmp) ? u"" : u" ", extra16); + } + + mangle_stub_cmdline(buffer); + + if (!isempty(buffer)) { + _cleanup_free_ char16_t *tmp = TAKE_PTR(*cmdline_append); + bool m = false; + + (void) tpm_log_load_options(buffer, &m); + *ret_parameters_measured = m; + + *cmdline_append = xasprintf("%ls%ls%ls", strempty(tmp), isempty(tmp) ? u"" : u" ", buffer); + } + + return EFI_SUCCESS; +} + static EFI_STATUS run(EFI_HANDLE image) { _cleanup_free_ void *credential_initrd = NULL, *global_credential_initrd = NULL, *sysext_initrd = NULL, *pcrsig_initrd = NULL, *pcrpkey_initrd = NULL; size_t credential_initrd_size = 0, global_credential_initrd_size = 0, sysext_initrd_size = 0, pcrsig_initrd_size = 0, pcrpkey_initrd_size = 0; @@ -190,6 +374,7 @@ static EFI_STATUS run(EFI_HANDLE image) { size_t addrs[_UNIFIED_SECTION_MAX] = {}, szs[_UNIFIED_SECTION_MAX] = {}; _cleanup_free_ char16_t *cmdline = NULL; int sections_measured = -1, parameters_measured = -1; + _cleanup_free_ char *uname = NULL; bool sysext_measured = false, m; uint64_t loader_features = 0; EFI_STATUS err; @@ -262,6 +447,10 @@ static EFI_STATUS run(EFI_HANDLE image) { /* Show splash screen as early as possible */ graphics_splash((const uint8_t*) loaded_image->ImageBase + addrs[UNIFIED_SECTION_SPLASH], szs[UNIFIED_SECTION_SPLASH]); + if (szs[UNIFIED_SECTION_UNAME] > 0) + uname = xstrndup8((char *)loaded_image->ImageBase + addrs[UNIFIED_SECTION_UNAME], + szs[UNIFIED_SECTION_UNAME]); + if (use_load_options(image, loaded_image, szs[UNIFIED_SECTION_CMDLINE] > 0, &cmdline)) { /* Let's measure the passed kernel command line into the TPM. Note that this possibly * duplicates what we already did in the boot menu, if that was already used. However, since @@ -277,6 +466,33 @@ static EFI_STATUS run(EFI_HANDLE image) { mangle_stub_cmdline(cmdline); } + /* If we have any extra command line to add via PE addons, load them now and append, and + * measure the additions separately, after the embedded options, but before the smbios ones, + * so that the order is reversed from "most hardcoded" to "most dynamic". The global addons are + * loaded first, and the image-specific ones later, for the same reason. */ + err = cmdline_append_and_measure_addons( + image, + loaded_image, + u"\\loader\\addons", + uname, + &m, + &cmdline); + if (err != EFI_SUCCESS) + log_error_status(err, "Error loading global addons, ignoring: %m"); + parameters_measured = parameters_measured < 0 ? m : (parameters_measured && m); + + _cleanup_free_ char16_t *dropin_dir = get_extra_dir(loaded_image->FilePath); + err = cmdline_append_and_measure_addons( + image, + loaded_image, + dropin_dir, + uname, + &m, + &cmdline); + if (err != EFI_SUCCESS) + log_error_status(err, "Error loading UKI-specific addons, ignoring: %m"); + parameters_measured = parameters_measured < 0 ? m : (parameters_measured && m); + const char *extra = smbios_find_oem_string("io.systemd.stub.kernel-cmdline-extra"); if (extra) { _cleanup_free_ char16_t *tmp = TAKE_PTR(cmdline), *extra16 = xstr8_to_16(extra); From f19b62756071cd6fc28b800062f591a3af88fe6a Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Wed, 24 May 2023 11:18:18 +0100 Subject: [PATCH 11/11] TODO: remove fixed item --- TODO | 6 ------ 1 file changed, 6 deletions(-) diff --git a/TODO b/TODO index f0aa26e021..a746f2b409 100644 --- a/TODO +++ b/TODO @@ -467,12 +467,6 @@ Features: parametrization, if needed. This matches our usual rule that admin config should win over vendor defaults. -* sd-stub: optionally allow users to configure manual kernel command line even - in SecureBoot by authenticating it via shim's APIs, integrating with MOK and - similar: instead of authenticating just PE code shim should be capable of - authenticating any kind of data for us, including files containing kernel - command lines. - * write a "search path" spec, that documents the prefixes to search in (i.e. the usual /etc/, /run/, /usr/lib/ dance, potentially /usr/etc/), how to sort found entries, how masking works and overriding.