From 3c4b4c1ff868b94d5884f05a85b593cee6e601483874deb5f97b0b5c10686bc1 Mon Sep 17 00:00:00 2001 From: Michael Chang Date: Mon, 20 Mar 2023 02:50:55 +0000 Subject: [PATCH 1/2] Accepting request 1072324 from home:frispete:Tumbleweed - Meanwhile, memtest86+ gained EFI support, but using the grub command line to run it manually is quite tedious... Adapt 20_memtest86+ to provide a proper menu entry. Executing memtest requires to turn security off in BIOS: (Boot Mode: Other OS). OBS-URL: https://build.opensuse.org/request/show/1072324 OBS-URL: https://build.opensuse.org/package/show/Base:System/grub2?expand=0&rev=445 --- 20_memtest86+ | 29 ++++++++++++++++++----------- grub2.changes | 8 ++++++++ grub2.spec | 4 ++++ 3 files changed, 30 insertions(+), 11 deletions(-) diff --git a/20_memtest86+ b/20_memtest86+ index 97c5bfd..e0b4149 100644 --- a/20_memtest86+ +++ b/20_memtest86+ @@ -4,6 +4,7 @@ set -e # grub-mkconfig helper script. # Copyright (C) 2011 Michal Ambroz # Adapted for openSUSE by Andrey Borzenkov +# Adapted for EFI by Hans-Peter Jansen # # you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -18,17 +19,11 @@ set -e # You should have received a copy of the GNU General Public License # along with the script. If not, see . -. /usr/share/grub2/grub-mkconfig_lib +. "$pkgdatadir/grub-mkconfig_lib" export TEXTDOMAIN=grub2 export TEXTDOMAINDIR=/usr/share/locale -# memset86+ requires the x86 real mode -# which is not available with UEFI booting. -if [ -d /sys/firmware/efi ]; then - exit 0 -fi - CLASS="--class memtest86 --class gnu --class tools" if [ "x${GRUB_DISTRIBUTOR}" = "x" ] ; then @@ -38,7 +33,19 @@ else CLASS="--class $(echo ${GRUB_DISTRIBUTOR} | tr 'A-Z' 'a-z' | cut -d' ' -f1) ${CLASS}" fi -memtest=/boot/memtest.bin +# memtest86+ comes in two flavours, one EFI and one suitable for x86 real mode. +# The EFI module requires security disabled in BIOS (Boot Mode: Other OS) +if [ -d /sys/firmware/efi -a -f /boot/efi/EFI/memtest86/memtest.efi ]; then + memtest=/boot/efi/EFI/memtest86/memtest.efi + loader='linux ' + message="$(gettext_printf "Loading EFI memtest ...\n" | grub_quote)" + # locate the real EFI partition + GRUB_DEVICE_BOOT=$(grub2-probe -t device "$memtest") +else + memtest=/boot/memtest.bin + loader='linux16' + message="$(gettext_printf "Loading x86 memtest ...\n" | grub_quote)" +fi if grub_file_is_not_garbage "$memtest" ; then gettext_printf "Found memtest image: %s\n" "$memtest" >&2 @@ -50,10 +57,10 @@ if grub_file_is_not_garbage "$memtest" ; then printf "menuentry '%s' %s \$menuentry_id_option '%s' {\n" "${OS}" "${CLASS}" "memtest-$boot_device_id" prepare_boot_cache="$(prepare_grub_to_access_device ${GRUB_DEVICE_BOOT} | sed -e "s/^/\t/")" printf '%s\n' "${prepare_boot_cache}" - message="$(gettext_printf "Loading memtest ...\n" | grub_quote)" cat << EOF - echo '$message' - linux16 ${rel_dirname}/${basename} + echo '$message' + ${loader} ${rel_dirname}/${basename} } + EOF fi diff --git a/grub2.changes b/grub2.changes index 51a1f51..e807bde 100644 --- a/grub2.changes +++ b/grub2.changes @@ -1,3 +1,11 @@ +------------------------------------------------------------------- +Wed Mar 15 21:46:00 UTC 2023 - Hans-Peter Jansen + +- Meanwhile, memtest86+ gained EFI support, but using the grub + command line to run it manually is quite tedious... + Adapt 20_memtest86+ to provide a proper menu entry. Executing + memtest requires to turn security off in BIOS: (Boot Mode: Other OS). + ------------------------------------------------------------------- Mon Mar 13 15:43:01 UTC 2023 - rw@suse.com diff --git a/grub2.spec b/grub2.spec index 2223e31..f3154da 100644 --- a/grub2.spec +++ b/grub2.spec @@ -528,6 +528,10 @@ Requires: s390-tools %ifarch ppc64 ppc64le Requires: powerpc-utils %endif +%ifarch %{ix86} +# meanwhile, memtest is available as EFI executable +Recommends: memtest86+ +%endif %if 0%{?only_x86_64:1} ExclusiveArch: x86_64 From 448b5a32b00d4f772cdff9b561a72916f46384940e5fbc37dc8d74a9f5d16356 Mon Sep 17 00:00:00 2001 From: Michael Chang Date: Wed, 22 Mar 2023 02:43:43 +0000 Subject: [PATCH 2/2] Accepting request 1073013 from home:michael-chang:branches:Base:System - Restrict cryptsetup key file permission for better security (bsc#1207499) * 0001-loader-linux-Ensure-the-newc-pathname-is-NULL-termin.patch * 0002-Restrict-cryptsetup-key-file-permission-for-better-s.patch OBS-URL: https://build.opensuse.org/request/show/1073013 OBS-URL: https://build.opensuse.org/package/show/Base:System/grub2?expand=0&rev=446 --- ...ure-the-newc-pathname-is-NULL-termin.patch | 146 ++++++++++++++++++ ...tup-key-file-permission-for-better-s.patch | 64 ++++++++ grub2.changes | 7 + grub2.spec | 2 + 4 files changed, 219 insertions(+) create mode 100644 0001-loader-linux-Ensure-the-newc-pathname-is-NULL-termin.patch create mode 100644 0002-Restrict-cryptsetup-key-file-permission-for-better-s.patch diff --git a/0001-loader-linux-Ensure-the-newc-pathname-is-NULL-termin.patch b/0001-loader-linux-Ensure-the-newc-pathname-is-NULL-termin.patch new file mode 100644 index 0000000..b2a7ec1 --- /dev/null +++ b/0001-loader-linux-Ensure-the-newc-pathname-is-NULL-termin.patch @@ -0,0 +1,146 @@ +From 1dcab5bf3843abc997f7e7dba32e5dbcb9bf66b2 Mon Sep 17 00:00:00 2001 +From: Gary Lin +Date: Fri, 25 Nov 2022 15:37:35 +0800 +Subject: [PATCH 1/2] loader/linux: Ensure the newc pathname is NULL-terminated + +Per "man 5 cpio", the namesize in the cpio header includes the trailing +NUL byte of the pathname and the pathname is followed by NUL bytes, but +the current implementation ignores the trailing NUL byte when making +the newc header. Although make_header() tries to pad the pathname string, +the padding won't happen when strlen(name) + sizeof(struct newc_head) +is a multiple of 4, and the non-NULL-terminated pathname may lead to +unexpected results. + +Assume that a file is created with 'echo -n aaaa > /boot/test12' and +loaded by grub2: + + linux /boot/vmlinuz + initrd newc:test12:/boot/test12 /boot/initrd + +The initrd command eventually invoked grub_initrd_load() and sent +'t''e''s''t''1''2' to make_header() to generate the header: + +00000070 30 37 30 37 30 31 33 30 31 43 41 30 44 45 30 30 |070701301CA0DE00| +00000080 30 30 38 31 41 34 30 30 30 30 30 33 45 38 30 30 |0081A4000003E800| +00000090 30 30 30 30 36 34 30 30 30 30 30 30 30 31 36 33 |0000640000000163| +000000a0 37 36 45 34 35 32 30 30 30 30 30 30 30 34 30 30 |76E4520000000400| +000000b0 30 30 30 30 30 38 30 30 30 30 30 30 31 33 30 30 |0000080000001300| +000000c0 30 30 30 30 30 30 30 30 30 30 30 30 30 30 30 30 |0000000000000000| +000000d0 30 30 30 30 30 36 30 30 30 30 30 30 30 30 74 65 |00000600000000te| + ^namesize +000000e0 73 74 31 32 61 61 61 61 30 37 30 37 30 31 30 30 |st12aaaa07070100| + ^^ end of the pathname + +Since strlen("test12") + sizeof(struct newc_head) is 116 = 29 * 4, +make_header() didn't pad the pathname, and the file content followed +"test12" immediately. This violates the cpio format and may trigger such +error during linux boot: + + Initramfs unpacking failed: ZSTD-compressed data is trunc + +To avoid the potential problems, this commit counts the trailing NUL byte +in when calling make_header() and adjusts the initrd size accordingly. + +Now the header becomes + +00000070 30 37 30 37 30 31 33 30 31 43 41 30 44 45 30 30 |070701301CA0DE00| +00000080 30 30 38 31 41 34 30 30 30 30 30 33 45 38 30 30 |0081A4000003E800| +00000090 30 30 30 30 36 34 30 30 30 30 30 30 30 31 36 33 |0000640000000163| +000000a0 37 36 45 34 35 32 30 30 30 30 30 30 30 34 30 30 |76E4520000000400| +000000b0 30 30 30 30 30 38 30 30 30 30 30 30 31 33 30 30 |0000080000001300| +000000c0 30 30 30 30 30 30 30 30 30 30 30 30 30 30 30 30 |0000000000000000| +000000d0 30 30 30 30 30 37 30 30 30 30 30 30 30 30 74 65 |00000700000000te| + ^namesize +000000e0 73 74 31 32 00 00 00 00 61 61 61 61 30 37 30 37 |st12....aaaa0707| + ^^ end of the pathname + +Besides the trailing NUL byte, make_header() pads 3 more NUL bytes, and +the user can safely read the pathname without a further check. + +To conform to the cpio format, the headers for "TRAILER!!!" are also +adjusted to include the trailing NUL byte, not ignore it. + +Signed-off-by: Gary Lin +Reviewed-by: Daniel Kiper +--- + grub-core/loader/linux.c | 25 ++++++++++++++++++------- + 1 file changed, 18 insertions(+), 7 deletions(-) + +diff --git a/grub-core/loader/linux.c b/grub-core/loader/linux.c +index 8f0fad805..e4018e65e 100644 +--- a/grub-core/loader/linux.c ++++ b/grub-core/loader/linux.c +@@ -130,12 +130,23 @@ insert_dir (const char *name, struct dir **root, + n->name = grub_strndup (cb, ce - cb); + if (ptr) + { ++ /* ++ * Create the substring with the trailing NUL byte ++ * to be included in the cpio header. ++ */ ++ char *tmp_name = grub_strndup (name, ce - name); ++ if (!tmp_name) { ++ grub_free (n->name); ++ grub_free (n); ++ return grub_errno; ++ } + grub_dprintf ("linux", "Creating directory %s, %s\n", name, ce); +- ptr = make_header (ptr, name, ce - name, ++ ptr = make_header (ptr, tmp_name, ce - name + 1, + 040777, 0); ++ grub_free (tmp_name); + } + if (grub_add (*size, +- ALIGN_UP ((ce - (char *) name) ++ ALIGN_UP ((ce - (char *) name + 1) + + sizeof (struct newc_head), 4), + size)) + { +@@ -260,7 +271,7 @@ grub_initrd_init (int argc, char *argv[], + grub_initrd_close (initrd_ctx); + return grub_errno; + } +- name_len = grub_strlen (initrd_ctx->components[i].newc_name); ++ name_len = grub_strlen (initrd_ctx->components[i].newc_name) + 1; + if (grub_add (initrd_ctx->size, + ALIGN_UP (sizeof (struct newc_head) + name_len, 4), + &initrd_ctx->size) || +@@ -274,7 +285,7 @@ grub_initrd_init (int argc, char *argv[], + { + if (grub_add (initrd_ctx->size, + ALIGN_UP (sizeof (struct newc_head) +- + sizeof ("TRAILER!!!") - 1, 4), ++ + sizeof ("TRAILER!!!"), 4), + &initrd_ctx->size)) + goto overflow; + free_dir (root); +@@ -302,7 +313,7 @@ grub_initrd_init (int argc, char *argv[], + initrd_ctx->size = ALIGN_UP (initrd_ctx->size, 4); + if (grub_add (initrd_ctx->size, + ALIGN_UP (sizeof (struct newc_head) +- + sizeof ("TRAILER!!!") - 1, 4), ++ + sizeof ("TRAILER!!!"), 4), + &initrd_ctx->size)) + goto overflow; + free_dir (root); +@@ -378,7 +389,7 @@ grub_initrd_load (struct grub_linux_initrd_context *initrd_ctx, + } + else if (newc) + { +- ptr = make_header (ptr, "TRAILER!!!", sizeof ("TRAILER!!!") - 1, ++ ptr = make_header (ptr, "TRAILER!!!", sizeof ("TRAILER!!!"), + 0, 0); + free_dir (root); + root = 0; +@@ -406,7 +417,7 @@ grub_initrd_load (struct grub_linux_initrd_context *initrd_ctx, + { + grub_memset (ptr, 0, ALIGN_UP_OVERHEAD (cursize, 4)); + ptr += ALIGN_UP_OVERHEAD (cursize, 4); +- ptr = make_header (ptr, "TRAILER!!!", sizeof ("TRAILER!!!") - 1, 0, 0); ++ ptr = make_header (ptr, "TRAILER!!!", sizeof ("TRAILER!!!"), 0, 0); + } + free_dir (root); + root = 0; +-- +2.39.2 + diff --git a/0002-Restrict-cryptsetup-key-file-permission-for-better-s.patch b/0002-Restrict-cryptsetup-key-file-permission-for-better-s.patch new file mode 100644 index 0000000..e7caa33 --- /dev/null +++ b/0002-Restrict-cryptsetup-key-file-permission-for-better-s.patch @@ -0,0 +1,64 @@ +From 9f18541245858f53fea72d8d60304f9015d88b5f Mon Sep 17 00:00:00 2001 +From: Michael Chang +Date: Fri, 17 Mar 2023 22:00:23 +0800 +Subject: [PATCH 2/2] Restrict cryptsetup key file permission for better + security + +GRUB's default permission 777 for concatenated initrd files was too +permissive for the cryptsetup key file, causing a complaint from +systemd-cryptsetup during boot. This commit replaces the 0777 permission +with a more secure 0400 permission for the key file. + +Signed-off-by: Michael Chang +--- + grub-core/loader/linux.c | 6 +++++- + 1 file changed, 5 insertions(+), 1 deletion(-) + +diff --git a/grub-core/loader/linux.c b/grub-core/loader/linux.c +index e4018e65e..fc71a78d7 100644 +--- a/grub-core/loader/linux.c ++++ b/grub-core/loader/linux.c +@@ -32,6 +32,7 @@ struct grub_linux_initrd_component + char *buf; + char *newc_name; + grub_off_t size; ++ grub_uint32_t mode; + }; + + struct dir +@@ -202,6 +203,7 @@ grub_initrd_component (const char *buf, int bufsz, const char *newc_name, + grub_memcpy (comp->buf, buf, bufsz); + initrd_ctx->nfiles++; + comp->size = bufsz; ++ comp->mode = 0100400; + if (grub_add (initrd_ctx->size, comp->size, + &initrd_ctx->size)) + goto overflow; +@@ -271,6 +273,7 @@ grub_initrd_init (int argc, char *argv[], + grub_initrd_close (initrd_ctx); + return grub_errno; + } ++ initrd_ctx->components[i].mode = 0100777; + name_len = grub_strlen (initrd_ctx->components[i].newc_name) + 1; + if (grub_add (initrd_ctx->size, + ALIGN_UP (sizeof (struct newc_head) + name_len, 4), +@@ -372,6 +375,7 @@ grub_initrd_load (struct grub_linux_initrd_context *initrd_ctx, + if (initrd_ctx->components[i].newc_name) + { + grub_size_t dir_size; ++ grub_uint32_t mode = initrd_ctx->components[i].mode; + + if (insert_dir (initrd_ctx->components[i].newc_name, &root, ptr, + &dir_size)) +@@ -383,7 +387,7 @@ grub_initrd_load (struct grub_linux_initrd_context *initrd_ctx, + ptr += dir_size; + ptr = make_header (ptr, initrd_ctx->components[i].newc_name, + grub_strlen (initrd_ctx->components[i].newc_name) + 1, +- 0100777, ++ mode, + initrd_ctx->components[i].size); + newc = 1; + } +-- +2.39.2 + diff --git a/grub2.changes b/grub2.changes index e807bde..9e36a34 100644 --- a/grub2.changes +++ b/grub2.changes @@ -1,3 +1,10 @@ +------------------------------------------------------------------- +Mon Mar 20 05:02:01 UTC 2023 - Michael Chang + +- Restrict cryptsetup key file permission for better security (bsc#1207499) + * 0001-loader-linux-Ensure-the-newc-pathname-is-NULL-termin.patch + * 0002-Restrict-cryptsetup-key-file-permission-for-better-s.patch + ------------------------------------------------------------------- Wed Mar 15 21:46:00 UTC 2023 - Hans-Peter Jansen diff --git a/grub2.spec b/grub2.spec index f3154da..67d28d0 100644 --- a/grub2.spec +++ b/grub2.spec @@ -503,6 +503,8 @@ Patch974: 0001-clean-up-crypttab-and-linux-modules-dependency.patch Patch975: 0002-discard-cached-key-before-entering-grub-shell-and-ed.patch # Make grub more robust against storage race condition causing system boot failures (bsc#1189036) Patch976: 0001-ieee1275-ofdisk-retry-on-open-and-read-failure.patch +Patch977: 0001-loader-linux-Ensure-the-newc-pathname-is-NULL-termin.patch +Patch978: 0002-Restrict-cryptsetup-key-file-permission-for-better-s.patch Requires: gettext-runtime %if 0%{?suse_version} >= 1140