Re: [SECURITY PATCH 001/117] verifiers: Move verifiers API to kernel image
On Thu, Mar 18, 2021 at 01:22:19AM +, Colin Watson wrote: > On Tue, Mar 02, 2021 at 07:00:08PM +0100, Daniel Kiper wrote: [snip] > I believe the practical threshold is 62 512-byte sectors, i.e. 31744 > bytes. > > As you can see, the biggest single change was induced by this patch, > which moves the verifiers API into the kernel image. Makes sense. Is > there anything we can do about this? > > I'm a little confused why this change had to be made in this way. > grub_load_modules is called pretty early during kernel initialization, > and it initializes all embedded modules. Wouldn't it have been > sufficient to leave verifiers as a module and simply include that module > in all UEFI-platform images? > > If that wouldn't have worked for some reason, then perhaps it would be > possible to restructure things a bit more so that we could leave the > verifiers API as a module on i386-pc, e.g. by moving it back to > grub-core/commands/verifiers.c and having conditional code that either > registers/unregisters the filter in a module or registers it at kernel > startup, depending on the platform. It wouldn't be especially pretty, > but I think we could tolerate that for the sake of fixing this > regression. I fully concur with Colin's idea. It is unfortunate that short MBR gap is still used, but it is also unnecessary to increase core image size to support nonexistent efi lockdown on i386-pc platform. The only consumer of the verifiers on the i386-pc platform is pgp module so it is good to keep verifiers as module as long as autoload can keep existing configuation to work transparently. For that I've also worked out a patch and will post here for review. Thanks, Michael > > Thanks, > > -- > Colin Watson (he/him) [cjwat...@debian.org] > > ___ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [SECURITY PATCH 001/117] verifiers: Move verifiers API to kernel image
On Tue, Mar 02, 2021 at 07:00:08PM +0100, Daniel Kiper wrote: > From: Marco A Benatto > > Move verifiers API from a module to the kernel image, so it can be > used there as well. There are no functional changes in this patch. I've had reports in Debian that the i386-pc image no longer fits in the MBR in some configurations (e.g. https://bugs.debian.org/984488, https://bugs.debian.org/985374). (Yes, I know, MBR is awful and even people who have to use it should put the first partition at more like 1MiB rather than 63 sectors, but it isn't practical for non-experts to fix up existing systems without a complete reinstallation, so breaking this in a security update is pretty bad.) Since I suspected that a lot of this was due to organic growth of the image as the security patch series made various bits of code more careful, I wrote a script to build each revision along the upstream changes from Debian version 2.04-15 to 2.04-16 and build an image with the following modules, extracted from one of those bug reports: ext2 part_msdos diskfilter mdraid09 biosdisk. Here's a report of the resulting image sizes and commits: 30884 2bd6855d2 grub-install: Fix inverted test for NLS enabled when copying locales 31427 0d324ad1b verifiers: Move verifiers API to kernel image 31446 6e14c57c6 kern: Add lockdown support 31446 f1d70c97b kern/lockdown: Set a variable if the GRUB is locked down 31446 71b48a193 efi: Lockdown the GRUB when the UEFI Secure Boot is enabled 31446 3d8afd579 efi: Use grub_is_lockdown() instead of hardcoding a disabled modules list 31446 c3037730d acpi: Don't register the acpi command when locked down 31446 5d58cce5c mmap: Don't register cutmem and badram commands when lockdown is enforced 31446 22f08600d commands: Restrict commands that can load BIOS or DT blobs when locked down 31446 bf939ef4e commands/setpci: Restrict setpci command when locked down 31446 ad9d55e50 commands/hdparm: Restrict hdparm command when locked down 31446 13a1fa9c1 gdb: Restrict GDB access when locked down 31446 b1e1dd471 loader/xnu: Don't allow loading extension and packages when locked down 31446 9042c1bc8 docs: Document the cutmem command 31452 9e6b789fa dl: Only allow unloading modules that are not dependencies 31452 d26f10df9 usb: Avoid possible out-of-bound accesses caused by malicious devices 31452 a993a2006 mmap: Fix memory leak when iterating over mapped memory 31452 60709e32e net/net: Fix possible dereference to of a NULL pointer 31452 118fe4df3 net/tftp: Fix dangling memory pointer 31473 967b95c4e kern/parser: Fix resource leak if argc == 0 31473 42b46cb07 kern/efi: Fix memory leak on failure 31473 10f42aeff kern/efi/mm: Fix possible NULL pointer dereference 31473 ad3b3b125 gnulib/regexec: Resolve unused variable 31473 a0b08bad3 gnulib/regcomp: Fix uninitialized token structure 31473 3131d3ff8 gnulib/argp-help: Fix dereference of a possibly NULL state 31473 dc28cd75d gnulib/regexec: Fix possible null-dereference 31473 711dd9d97 gnulib/regcomp: Fix uninitialized re_token 31473 28314f6c1 io/lzopio: Resolve unnecessary self-assignment errors 31473 f4eb2c3dd zstd: Initialize seq_t structure fully 31482 6d368ec03 kern/partition: Check for NULL before dereferencing input string 31482 e743b06fc disk/ldm: Make sure comp data is freed before exiting from make_vg() 31482 af94bf626 disk/ldm: If failed then free vg variable too 31482 8e43b154c disk/ldm: Fix memory leak on uninserted lv references 31482 0beb60002 disk/cryptodisk: Fix potential integer overflow 31482 20ddfae56 hfsplus: Check that the volume name length is valid 31482 d8fa680fe zfs: Fix possible negative shift operation 31482 1b80d2dde zfs: Fix resource leaks while constructing path 31482 2b07acad0 zfs: Fix possible integer overflows 31482 0283863c7 zfsinfo: Correct a check for error allocating memory 31482 ad663e4ea affs: Fix memory leaks 31482 8d9e05f24 libgcrypt/mpi: Fix possible unintended sign extension 31482 3120a6835 libgcrypt/mpi: Fix possible NULL dereference 31482 6d38008dd syslinux: Fix memory leak while parsing 31482 06f86bc0d normal/completion: Fix leaking of memory when processing a completion 31482 e31e8ecbc commands/hashsum: Fix a memory leak 31482 74d544182 video/efi_gop: Remove unnecessary return value of grub_video_gop_fill_mode_info() 31482 e07f13cfa video/fb/fbfill: Fix potential integer overflow 31482 fffc476df video/fb/video_fb: Fix multiple integer overflows 31482 786656dc8 video/fb/video_fb: Fix possible integer overflow 31482 bf3df4eeb video/readers/jpeg: Test for an invalid next marker reference from a jpeg file 31482 f9b9c56e2 gfxmenu/gui_list: Remove code that coverity is flagging as dead 31482 11cf998c2 loader/bsd: Check for NULL arg up-front 31482 d311599e4 loader/xnu: Fix memory leak 31482 986de6735 loader/xnu: Free driverkey data when an error is detected in grub_xnu_writetree_toheap() 31482 f851813cd
[SECURITY PATCH 001/117] verifiers: Move verifiers API to kernel image
From: Marco A Benatto
Move verifiers API from a module to the kernel image, so it can be
used there as well. There are no functional changes in this patch.
Signed-off-by: Marco A Benatto
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Daniel Kiper
---
grub-core/Makefile.am| 1 +
grub-core/Makefile.core.def | 6 +-
grub-core/kern/main.c| 4
grub-core/{commands => kern}/verifiers.c | 8 ++--
include/grub/verify.h| 9 ++---
5 files changed, 14 insertions(+), 14 deletions(-)
rename grub-core/{commands => kern}/verifiers.c (97%)
diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
index c6ba5b2d7..cc6fc7dfa 100644
--- a/grub-core/Makefile.am
+++ b/grub-core/Makefile.am
@@ -91,6 +91,7 @@ KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/parser.h
KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/partition.h
KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/term.h
KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/time.h
+KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/verify.h
KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/mm_private.h
KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/net.h
KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/memory.h
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 68b9e9f68..0cecc3875 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -140,6 +140,7 @@ kernel = {
common = kern/rescue_parser.c;
common = kern/rescue_reader.c;
common = kern/term.c;
+ common = kern/verifiers.c;
noemu = kern/compiler-rt.c;
noemu = kern/mm.c;
@@ -944,11 +945,6 @@ module = {
};
module = {
- name = verifiers;
- common = commands/verifiers.c;
-};
-
-module = {
name = shim_lock;
common = commands/efi/shim_lock.c;
enable = efi;
diff --git a/grub-core/kern/main.c b/grub-core/kern/main.c
index 9cad0c448..73967e2f5 100644
--- a/grub-core/kern/main.c
+++ b/grub-core/kern/main.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#ifdef GRUB_MACHINE_PCBIOS
#include
@@ -274,6 +275,9 @@ grub_main (void)
grub_printf ("Welcome to GRUB!\n\n");
grub_setcolorstate (GRUB_TERM_COLOR_STANDARD);
+ /* Init verifiers API. */
+ grub_verifiers_init ();
+
grub_load_config ();
grub_boot_time ("Before loading embedded modules.");
diff --git a/grub-core/commands/verifiers.c b/grub-core/kern/verifiers.c
similarity index 97%
rename from grub-core/commands/verifiers.c
rename to grub-core/kern/verifiers.c
index aae4d84bb..75d7994cf 100644
--- a/grub-core/commands/verifiers.c
+++ b/grub-core/kern/verifiers.c
@@ -221,12 +221,8 @@ grub_verify_string (char *str, enum
grub_verify_string_type type)
return GRUB_ERR_NONE;
}
-GRUB_MOD_INIT(verifiers)
+void
+grub_verifiers_init (void)
{
grub_file_filter_register (GRUB_FILE_FILTER_VERIFY, grub_verifiers_open);
}
-
-GRUB_MOD_FINI(verifiers)
-{
- grub_file_filter_unregister (GRUB_FILE_FILTER_VERIFY);
-}
diff --git a/include/grub/verify.h b/include/grub/verify.h
index ea0491433..cd129c398 100644
--- a/include/grub/verify.h
+++ b/include/grub/verify.h
@@ -64,7 +64,10 @@ struct grub_file_verifier
grub_err_t (*verify_string) (char *str, enum grub_verify_string_type type);
};
-extern struct grub_file_verifier *grub_file_verifiers;
+extern struct grub_file_verifier *EXPORT_VAR (grub_file_verifiers);
+
+extern void
+grub_verifiers_init (void);
static inline void
grub_verifier_register (struct grub_file_verifier *ver)
@@ -78,7 +81,7 @@ grub_verifier_unregister (struct grub_file_verifier *ver)
grub_list_remove (GRUB_AS_LIST (ver));
}
-grub_err_t
-grub_verify_string (char *str, enum grub_verify_string_type type);
+extern grub_err_t
+EXPORT_FUNC (grub_verify_string) (char *str, enum grub_verify_string_type
type);
#endif /* ! GRUB_VERIFY_HEADER */
--
2.11.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
