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

mok: fix LogError() invocation #577

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

vathpela
Copy link
Contributor

On some ARM platform, jlinton noticed that when we fail to set a variable (because it isn't supported at all, presumably), our error message has an extra argument that doesn't match the format string.

This patch removes the extra argument.

@alpernebbi
Copy link
Contributor

alpernebbi commented Jun 23, 2023

On some ARM platform, jlinton noticed that when we fail to set a variable (because it isn't supported at all, presumably), our error message has an extra argument that doesn't match the format string.

This has been triggering a Synchronous Abort on my chromebook (rk3399-gru-kevin) when trying to run Debian's signed shim with U-Boot, thanks for fixing it. From what I can tell, U-Boot now has QueryVariableInfo() available, but can misreport maximum variable size when its variable store buffer is about to run out, due to an underflow in size calculation probably here adressed by this patch. Debian's shim sets too many variables, and eventually SetVariable() fails with EFI_OUT_OF_RESOURCES and falls into this LogError.

trini pushed a commit to trini/u-boot that referenced this pull request Jul 11, 2023
Debian's arm64 UEFI Secure Boot shim makes the EFI variable store run
out of space while mirroring its MOK database to variables. This can be
observed in QEMU like so:

  $ tools/buildman/buildman -o build/qemu_arm64 --boards=qemu_arm64 -w
  $ cd build/qemu_arm64
  $ curl -L -o debian.iso \
      https://cdimage.debian.org/debian-cd/current/arm64/iso-cd/debian-12.0.0-arm64-netinst.iso
  $ qemu-system-aarch64 \
      -nographic -bios u-boot.bin \
      -machine virt -cpu cortex-a53 -m 1G -smp 2 \
      -drive if=virtio,file=debian.iso,index=0,format=raw,readonly=on,media=cdrom
  [...]
  => # interrupt autoboot
  => env set -e -bs -nv -rt -guid 605dab50-e046-4300-abb6-3dd810dd8b23 SHIM_VERBOSE 1
  => boot
  [...]
  mok.c:296:mirror_one_esl() SetVariable("MokListXRT43", ... varsz=0x4C) = Out of Resources
  mok.c:452:mirror_mok_db() esd:0x7DB92D20 adj:0x30
  Failed to set MokListXRT: Out of Resources
  mok.c:767:mirror_one_mok_variable() mirror_mok_db("MokListXRT",  datasz=17328) returned Out of Resources
  mok.c:812:mirror_one_mok_variable() returning Out of Resources
  Could not create MokListXRT: Out of Resources
  [...]
  Welcome to GRUB!

This would normally be fine as shim would continue to run grubaa64.efi,
but shim's error handling code for this case has a bug [1] that causes a
synchronous abort on at least chromebook_kevin (but apparently not on
QEMU arm64).

Double the default variable store size so the variables fit. There is a
note about this value matching PcdFlashNvStorageVariableSize when
EFI_MM_COMM_TEE is enabled, so keep the old default in that case.

[1] rhboot/shim#577

Signed-off-by: Alper Nebi Yasak <[email protected]>
Reviewed-by: Heinrich Schuchardt <[email protected]>
Copy link
Collaborator

@jsetje jsetje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we could also check that we weren't called with a null name, but that's arguably a separate issue.

On some ARM platform, jlinton noticed that when we fail to set a
variable (because it isn't supported at all, presumably), our error
message has an extra argument that doesn't match the format string.

This patch removes the extra argument.

Resolves: CVE-2023-40546
Signed-off-by: Peter Jones <[email protected]>
Copy link
Collaborator

@steve-mcintyre steve-mcintyre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@steve-mcintyre steve-mcintyre merged commit 66e6579 into rhboot:main Oct 19, 2023
vathpela added a commit that referenced this pull request Jan 23, 2024
What's changed
* Various CVE fixes:
  CVE-2023-40546 mok: fix LogError() invocation
  CVE-2023-40547 - avoid incorrectly trusting HTTP headers
  CVE-2023-40548 Fix integer overflow on SBAT section size on 32-bit system
  CVE-2023-40549 Authenticode: verify that the signature header is in bounds.
  CVE-2023-40550 pe: Fix an out-of-bound read in verify_buffer_sbat()
  CVE-2023-40551: pe-relocate: Fix bounds check for MZ binaries
* Add make infrastructure to set the NX_COMPAT flag by @vathpela in #530
* Make sbat_var.S parse right with buggy gcc/binutils by @vathpela in #535
* Drop invalid calls to CRYPTO_set_mem_functions by @nicholasbishop in #537
* pe: Align section size up to page size for mem attrs by @nicholasbishop in #539
* test-sbat: Fix exit code by @vathpela in #540
* pe: Add IS_PAGE_ALIGNED macro by @nicholasbishop in #541
* CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper by @nicholasbishop in #546
* Don't loop forever in load_certs() with buggy firmware by @rmetrich in #547
* Block Debian grub binaries with SBAT < 4 by @steve-mcintyre in #550
* Shim unable to locate grubx64 in PXE boot mode when grubx64 is stored in a different file path by @Alberto-Perez-Guevara in #551
* Further improve load_certs() for non-compliant drivers/firmwares by @pbatard in #560
* pe: only process RelocDir->Size of reloc section by @mikebeaton in #562
* Rename 'msecs' to 'usecs' to avoid potential confusion by @aronowski in #563
* Optionally allow to keep shim protocol installed by @bluca in #565
* SBAT-related documents formatting and spelling by @aronowski in #566
* Add SbatLevel_Variable.txt to document the various revocations by @jsetje in #569
* Add a security contact email address in README.md by @vathpela in #572
* Use -Wno-unused-but-set-variable for Cryptlib and OpenSSL by @vathpela in #576
* mok: fix LogError() invocation by @vathpela in #577
* Minor housekeeping by @vathpela in #578
* Test ImageAddress() by @vathpela in #579
* FreePages() is used to return memory allocated by AllocatePages() by @dennis-tseng99 in #580
* Size should minus 1 when calculating 'RelocBaseEnd' by @jsetje in #581
* Verify signature before verifying sbat levels by @jsetje in #583
* Add libFuzzer support for csv.c and sbat.c by @vathpela in #584
* mok: Avoid underflow in maximum variable size calculation by @alpernebbi in #587
* Housekeeping by @vathpela in #605

Signed-off-by: Peter Jones <[email protected]>
@eslerm
Copy link

eslerm commented Jan 24, 2024

Is this issue actually a security vulnerability, or just a bug? Why was a CVE assigned?

brianredbeard pushed a commit to brianredbeard/redhat-efi-boot-shim that referenced this pull request Feb 22, 2024
What's changed
* Various CVE fixes:
  CVE-2023-40546 mok: fix LogError() invocation
  CVE-2023-40547 - avoid incorrectly trusting HTTP headers
  CVE-2023-40548 Fix integer overflow on SBAT section size on 32-bit system
  CVE-2023-40549 Authenticode: verify that the signature header is in bounds.
  CVE-2023-40550 pe: Fix an out-of-bound read in verify_buffer_sbat()
  CVE-2023-40551: pe-relocate: Fix bounds check for MZ binaries
* Add make infrastructure to set the NX_COMPAT flag by @vathpela in rhboot#530
* Make sbat_var.S parse right with buggy gcc/binutils by @vathpela in rhboot#535
* Drop invalid calls to CRYPTO_set_mem_functions by @nicholasbishop in rhboot#537
* pe: Align section size up to page size for mem attrs by @nicholasbishop in rhboot#539
* test-sbat: Fix exit code by @vathpela in rhboot#540
* pe: Add IS_PAGE_ALIGNED macro by @nicholasbishop in rhboot#541
* CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper by @nicholasbishop in rhboot#546
* Don't loop forever in load_certs() with buggy firmware by @rmetrich in rhboot#547
* Block Debian grub binaries with SBAT < 4 by @steve-mcintyre in rhboot#550
* Shim unable to locate grubx64 in PXE boot mode when grubx64 is stored in a different file path by @Alberto-Perez-Guevara in rhboot#551
* Further improve load_certs() for non-compliant drivers/firmwares by @pbatard in rhboot#560
* pe: only process RelocDir->Size of reloc section by @mikebeaton in rhboot#562
* Rename 'msecs' to 'usecs' to avoid potential confusion by @aronowski in rhboot#563
* Optionally allow to keep shim protocol installed by @bluca in rhboot#565
* SBAT-related documents formatting and spelling by @aronowski in rhboot#566
* Add SbatLevel_Variable.txt to document the various revocations by @jsetje in rhboot#569
* Add a security contact email address in README.md by @vathpela in rhboot#572
* Use -Wno-unused-but-set-variable for Cryptlib and OpenSSL by @vathpela in rhboot#576
* mok: fix LogError() invocation by @vathpela in rhboot#577
* Minor housekeeping by @vathpela in rhboot#578
* Test ImageAddress() by @vathpela in rhboot#579
* FreePages() is used to return memory allocated by AllocatePages() by @dennis-tseng99 in rhboot#580
* Size should minus 1 when calculating 'RelocBaseEnd' by @jsetje in rhboot#581
* Verify signature before verifying sbat levels by @jsetje in rhboot#583
* Add libFuzzer support for csv.c and sbat.c by @vathpela in rhboot#584
* mok: Avoid underflow in maximum variable size calculation by @alpernebbi in rhboot#587
* Housekeeping by @vathpela in rhboot#605

Signed-off-by: Peter Jones <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants