Re: [SECURITY PATCH 001/117] verifiers: Move verifiers API to kernel image

[Colin Watson <cjwatson@debian.org>]

On Tue, Mar 02, 2021 at 07:00:08PM +0100, Daniel Kiper wrote:
> From: Marco A Benatto <mbenatto@redhat.com>
> 
> 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 loader/xnu: Check if pointer is NULL before using it
  31482 4f7bde3ab util/grub-install: Fix NULL pointer dereferences
  31482 fd0e3f964 util/grub-editenv: Fix incorrect casting of a signed value
  31482 d86e80fe0 util/glue-efi: Fix incorrect use of a possibly negative value
  31482 6a84527d4 script/execute: Fix NULL dereference in grub_script_execute_cmdline()
  31482 f785f176a commands/ls: Require device_name is not NULL before printing
  31482 12f5e77dc script/execute: Avoid crash when using "$#" outside a function scope
  31482 82446d230 lib/arg: Block repeated short options that require an argument
  31482 121811a98 script/execute: Don't crash on a "for" loop with no items
  31482 3c5bfae9e commands/menuentry: Fix quoting in setparams_prefix()
  31474 abdc1e40a kern/misc: Always set *end in grub_strtoull()
  31474 b189d92cb video/readers/jpeg: Catch files with unsupported quantization or Huffman tables
  31474 7b5a6dc77 video/readers/jpeg: Catch OOB reads/writes in grub_jpeg_decode_du()
  31474 14cc3bde4 video/readers/jpeg: Don't decode data before start of stream
  31474 0013a6fa9 term/gfxterm: Don't set up a font with glyphs that are too big
  31508 2832f9ed4 fs/fshelp: Catch impermissibly large block sizes in read helper
  31508 0a8501280 fs/hfsplus: Don't fetch a key beyond the end of the node
  31508 1632d4751 fs/hfsplus: Don't use uninitialized data on corrupt filesystems
  31508 3dbfcb563 fs/hfs: Disable under lockdown
  31508 0e5a7bb86 fs/sfs: Fix over-read of root object name
  31508 4089be10f fs/jfs: Do not move to leaf level if name length is negative
  31508 2de73233c fs/jfs: Limit the extents that getblk() can consider
  31508 751f1ad0b fs/jfs: Catch infinite recursion
  31508 64b8e6ab3 fs/nilfs2: Reject too-large keys
  31508 e5b544089 fs/nilfs2: Don't search children if provided number is too large
  31508 6187f84c0 fs/nilfs2: Properly bail on errors in grub_nilfs2_btree_node_lookup()
  31508 ab6d7615d io/gzio: Bail if gzio->tl/td is NULL
  31508 b3c863498 io/gzio: Add init_dynamic_block() clean up if unpacking codes fails
  31508 fd737860d io/gzio: Catch missing values in huft_build() and bail
  31508 2df30bf33 io/gzio: Zero gzio->tl/td in init_dynamic_block() if huft_build() fails
  31508 64eb78f6c disk/lvm: Don't go beyond the end of the data we read from disk
  31508 b448c78a1 disk/lvm: Don't blast past the end of the circular metadata buffer
  31508 ff49d996d disk/lvm: Bail on missing PV list
  31508 87625cadf disk/lvm: Do not crash if an expected string is not found
  31508 aed3e7107 disk/lvm: Do not overread metadata
  31508 3fc149f18 disk/lvm: Sanitize rlocn->offset to prevent wild read
  31508 e8f1ae648 disk/lvm: Do not allow a LV to be it's own segment's node's LV
  31508 bd87498f4 fs/btrfs: Validate the number of stripes/parities in RAID5/6
  31508 99572884b fs/btrfs: Squash some uninitialized reads
  31529 3f1acab9c kern/parser: Fix a memory leak
  31529 7feb878e8 kern/parser: Introduce process_char() helper
  31556 3ab27438b kern/parser: Introduce terminate_arg() helper
  31541 782fb1971 kern/parser: Refactor grub_parser_split_cmdline() cleanup
  31698 fc938b31c kern/buffer: Add variable sized heap buffer
  31826 596d36219 kern/parser: Fix a stack buffer overflow
  31826 149524eaa kern/efi: Add initial stack protector implementation
  31826 039725fe7 util/mkimage: Remove unused code to add BSS section
  31826 0a6005c74 util/mkimage: Use grub_host_to_target32() instead of grub_cpu_to_le32()
  31826 cf179bd80 util/mkimage: Always use grub_host_to_target32() to initialize PE stack and heap stuff
  31826 2aa0ef41c util/mkimage: Unify more of the PE32 and PE32+ header set-up
  31826 1aa253780 util/mkimage: Reorder PE optional header fields set-up
  31826 99f02aab1 util/mkimage: Improve data_size value calculation
  31826 1c810627e util/mkimage: Refactor section setup to use a helper
  31826 26ff81d1e util/mkimage: Add an option to import SBAT metadata into a .sbat section
  31826 2564455e7 grub-install-common: Add --sbat option
  31826 55c7de529 kern/misc: Split parse_printf_args() into format parsing and va_list handling
  31787 c8bc04397 kern/misc: Add STRING type for internal printf() format handling
  32060 834e5d238 kern/misc: Add function to check printf() format against expected format
  32060 c652b0e86 gfxmenu/gui: Check printf() format in the gui_progress_bar and gui_label
  32060 9cd32c576 kern/mm: Fix grub_debug_calloc() compilation error

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.

Thanks,

-- 
Colin Watson (he/him)                              [cjwatson@debian.org]