| Age | Commit message (Collapse) | Author |
|---|
|
shim uses path delimiters to determine the file path for the second
stage. Currently only / (slash) is detected, but some DHCP
implementations supply \ (backslash) as the path specifier.
This patch changes it to accept either delimiter.
Fixes issue #524.
Signed-off-by: Alberto Perez <aperezguevar@microsoft.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
A rendering error which caused the `<Vendor>_key.EFI` text to be
rendered as `_key.EFI` has been fixed. The text was being rendered incorrectly by GitHub since the <Vendor> part was being treated as an HTML tag and therefore ignored.
Two misspellings have been fixed
Tables have been reformatted to be more readable as plaintext. Rendering remains the same.
Signed-off-by: Kamil Aronowski <kamil.aronowski@yahoo.com>
|
|
Following the discovery of more problematic firmwares and drivers
affected by the issue f23883ccf78f1f605a272f9e5700f47e5494a71d is
designed to address (e.g. https://github.com/rhboot/shim/issues/558),
this patch further improves the code so that, instead of simply bailing
out, we progressively increase the buffer sizes, until either success
or a maximum size limit is reached.
In most cases, this workaround should be enough to ensure completion
of the directory read and thus provide full shim functionality (while
still warning the user about the non-compliance of their environment).
Signed-off-by: Pete Batard <pete@akeo.ie>
|
|
(See https://bugs.debian.org/1024617)
One of the Debian builds of grub bumped the SBAT to 3, but didn't
include the patches needed. Add "grub.debian,4" to block those
binaries.
Signed-off-by: Steve McIntyre <steve@einval.com>
|
|
Fix the `main` function in `test-sbat.c` to return the `status` variable
like the other `test-*.c` files.
Signed-off-by: Nicholas Bishop <nicholasbishop@google.com>
|
|
These calls did not check the return value. If they had, it would have
shown that the calls were failing due to passing `NULL` for the
`realloc` function pointer. That causes an early return, so the calls
weren't actually doing anything.
The `malloc`/`realloc`/`free` functions defined in
Cryptlib/SysCall/BaseMemAllocation.c are what actually get used, so just
drop the explicit call to `CRYPTO_set_mem_functions`.
Signed-off-by: Nicholas Bishop <nicholasbishop@google.com>
|
|
If the ShimRetainProtocol variable is set, avoid uninstalling our
protocol.
For example, this allows sd-stub in a UKI to use the shim protocol to
validate PE binaries, even if it is executed by a second stage, before
the kernel is loaded.
Ensure that the variable is volatile and for BootServices access.
Also delete it on startup, so that we can be sure it was really set by
a second stage.
Example use case in sd-boot/sd-stub:
https://github.com/systemd/systemd/pull/27358
Signed-off-by: Luca Boccassi <bluca@debian.org>
|
|
On DELL R350 booting DVD through RFS with BIOS 1.4.2 in Secure Boot,
firmware returns EFI_BUFFER_TOO_SMALL but with new buffersize set to 0,
which causes the load_certs() code to loop forever:
while (1) {
efi_status = dir->Read(dir, &buffersize, buffer);
if (efi_status == EFI_BUFFER_TOO_SMALL) {
...
continue;
}
...
}
This commit prevents such infinite loop.
Signed-off-by: Renaud Métrich <rmetrich@redhat.com>
|
|
This makes some checks in `get_mem_attrs` and `update_mem_attrs`
clearer.
Also add `test-pe-util.c` with a test for the new macro. The file is
named that way instead of `test-pe.c` to avoid having to get `pe.c`
building in the unit test environment.
Signed-off-by: Nicholas Bishop <nicholasbishop@google.com>
|
|
Setting memory attributes is generally done at page granularity, and
this is enforced by checks in `get_mem_attrs` and
`update_mem_attrs`. But unlike the section address, the section size
isn't necessarily aligned to 4KiB. Round up the section size to fix
this.
Signed-off-by: Nicholas Bishop <nicholasbishop@google.com>
|
|
There is one long-standing problem in CRT realloc wrapper, which will
cause the obvious buffer overflow issue when re-allocating one bigger
memory block:
void *realloc (void *ptr, size_t size)
{
//
// BUG: hardcode OldSize == size! We have no any knowledge about
// memory size of original pointer ptr.
//
return ReallocatePool ((UINTN) size, (UINTN) size, ptr);
}
This patch introduces one extra header to record the memory buffer size
information when allocating memory block from malloc routine, and re-wrap
the realloc() and free() routines to remove this BUG.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ting Ye <ting.ye@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qin Long <qin.long@intel.com>
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
Validated-by: Jian J Wang <jian.j.wang@intel.com>
Cherry picked from https://github.com/tianocore/edk2.git, commit
cf8197a39d07179027455421a182598bd6989999. Changes:
* `SIGNATURE_32` -> `EFI_SIGNATURE_32`
* Added definition of `MIN`
Fixes https://github.com/rhboot/shim/issues/538
Signed-off-by: Nicholas Bishop <nicholasbishop@google.com>
|
|
Currently by default, when we build shim we do not set the PE
NX-compatibility DLL Characteristic flag. This signifies to the
firmware that shim (including the components it loads) is not prepared
for several related firmware changes:
- non-executable stack
- non-executable pages from AllocatePages()/AllocatePool()/etc.
- non-writable 0 page (not strictly related but some firmware will be
transitioning at the same time)
- the need to use the UEFI 2.10 Memory Attribute Protocol to set page
permissions.
This patch changes that default to be enabled by default. Distributors
of shim will need to ensure that either their builds disable this bit
(using "post-process-pe -N"), or that the bootloaders and kernels you
support loading are all compliant with this change. A new make
variable, POST_PROCESS_PE_FLAGS, has been added to simplify doing so.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
In https://github.com/rhboot/shim/issues/533 , iokomin noticed that
gas in binutils before 2.36 appears to be incorrectly concatenating
string literals in '.asciz' directives, including an extra NUL character
in between the strings, and this will cause us to incorrectly parse the
.sbatlevel section in shim binaries.
This patch adds test cases that will cause the build to fail if this has
happened, as well as changing sbat_var.S to to use '.ascii' and '.byte'
to construct the data, rather than using '.asciz'.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Due to the issues addressed in the 2022-11-15 batch of grub CVEs[0], we
need to bump the sbat version from grub. This patch changes it from 2
to 3.
[0] https://lists.gnu.org/archive/html/grub-devel/2022-11/msg00059.html
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Though we don't need to bump SBAT_LEVEL for this, we've decided to
change the level to 3 here in case
53509eaf2253e23bfb552e9386fd0877abe592b4 turns out to be worse than we
think it is, so we can fix that easily later.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This adds a few more UEFI functions to our coverity model, so we see a
few less false positives during scanning. It also fixes an error in our
model for OpenSSL's OBJ_dup().
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
AuthenticodeVerify() calls OpenSSLs d2i_PKCS7() API to parse asn encoded
signed authenticode pkcs#7 data. when this successfully returns, a type
check is done by calling PKCS7_type_is_signed() and then
Pkcs7->d.sign->contents->type is used. It is possible to construct an asn1
blob that successfully decodes and have d2i_PKCS7() return a valid pointer
and have PKCS7_type_is_signed() also return success but have Pkcs7->d.sign
be a NULL pointer.
Looking at how PKCS7_verify() [inside of OpenSSL] implements checking for
pkcs7 structs it does the following:
- call PKCS7_type_is_signed()
- call PKCS7_get_detached()
Looking into how PKCS7_get_detatched() is implemented, it checks to see if
p7->d.sign is NULL or if p7->d.sign->contents->d.ptr is NULL.
As such, the fix is to do the same as OpenSSL after calling d2i_PKCS7().
- Add call to PKS7_get_detached() to existing error handling
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
Cherry-picked-from: https://github.com/tianocore/edk2/commit/26442d11e620a9e81c019a24a4ff38441c64ba10
|
|
MokListTrusted was added by mistake to PCR 7 in 4e513405. The value of
MokListTrusted does not alter the behavior of secure boot so, as per
https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClient_PFP_r1p05_v23_pub.pdf#page=36
(section 3.3.4 PCR usage) so it should not be factored in the value of
PCR 7.
See:
https://github.com/rhboot/shim/pull/423
https://github.com/rhboot/shim/commit/4e513405b4f1641710115780d19dcec130c5208f
Fixes https://github.com/rhboot/shim/issues/484
Fixes https://github.com/rhboot/shim/issues/492
Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>
|
|
Remove timestamps, user names, etc. from the tarball so that
it can be built reproducibly by multiple people, on different
machines.
The outer bzip2 layer might still be different, no reproducible
bzip2 known.
Signed-off-by: Julian Andres Klode <julian.klode@canonical.com>
|
|
Seen mokmanager image load failure '2 sections contain entry point'
for shim built on Oracle Linux 9 aarch64. found_entry_point counter in
handle_image() uses SizeOfRawData to calculate section boundary.
PE spec defines VirtualSize for the total size of the section when loaded
into memory. SizeOfRawData is the size of the section (for object files)
or the size of the initialized data on disk.
Fix this issue by updating section in-memory size limit to VirtualSize.
Resolves: #517
Signed-off-by: Ilya Okomin <ilya.okomin@oracle.com>
|
|
This changes the alignment of UINT64 data to 8 bytes on IA32, which
matches EDK2's understanding of alignment. In particular this change
affects the offset where shim writes `EFI_LOADED_IMAGE.ImageSize`.
Fixes https://github.com/rhboot/shim/issues/515
Signed-off-by: Nicholas Bishop <nicholasbishop@google.com>
|
|
Use the EFI RT memory function CopyMem instead of memcpy in load_cert_file.
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
|
|
0214cd9cef5a fixes a NULL pointer dereference problem, it introduces two
new problems. First it incorrectly assumes li.FilePath is a string.
Second, it puts EFI_LOADED_IMAGE li on the stack. It has been found
that not all archectures can handle this being on the stack.
The shim_li variable will be setup properly from the read_image
call. Use the global shim_li variable instead when calling
verify_image.
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
|
|
We've seen crashes in early GRUB code on an ARM Cortex-A72-based
platform that point at seemingly harmless instructions. Flushing
the i-cache of those instructions prior to executing has been
shown to avoid the problem, which has parallels with this story:
https://www.mail-archive.com/osv-dev@googlegroups.com/msg06203.html
Add a cache flushing utility function and provide an implementation
using a GCC intrinsic. This will need to be extended to support other
compilers. Note that this intrinsic is a no-op for x86 platforms.
This fixes issue #498.
Signed-off-by: dann frazier <dann.frazier@canonical.com>
|
|
In 6c8d08c0af4768c715b79c8ec25141d56e34f8b4 ("shim: Ignore UEFI
LoadOptions that are just NUL characters."), a check was added to
discard load options that are entirely NUL. We now see some firmwares
that start LoadOptions with a NUL, and then follow it with garbage (path
to directory containing loaders). Widen the check to just discard
anything that starts with a NUL.
Resolves: #490
Related: #95
See-also: https://bugzilla.redhat.com/show_bug.cgi?id=2113005
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
|
|
Intel Trust Domain Extensions (Intel TDX) extends Virtual Machine
Extensions (VMX) and Multi-Key Total Memory Encryption (MK-TME) with a
new kind of virtual machine guest called a Trust Domain(TD)[1]. A TD
runs in a CPU mode that is designed to protect the confidentiality of
its memory contents and its CPU state from any other software, including
the hosting Virtual Machine Monitor (VMM).
Trust Domain Virtual Firmware (TDVF) is required to provide Intel TDX
implementation and service for EFI_CC_MEASUREMENT_PROTOCOL[2]. The bugzilla
for TDVF is at https://bugzilla.tianocore.org/show_bug.cgi?id=3625.
To support CC measurement/attestation with Intel TDX technology, these 4
RTMR registers will be extended by TDX service like TPM/TPM2 PCR:
- RTMR[0] for TDVF configuration
- RTMR[1] for the TD OS loader and kernel
- RTMR[2] for the OS application
- RTMR[3] reserved for special usage only
Add a TDX Implementation for CC Measurement protocol along with
TPM/TPM2 protocol.
References:
[1] https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-whitepaper-v4.pdf
[2] https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-virtual-firmware-design-guide-rev-1.pdf
[3] https://software.intel.com/content/dam/develop/external/us/en/documents/intel-tdx-guest-hypervisor-communication-interface-1.0-344426-002.pdf
Signed-off-by: Lu Ken <ken.lu@intel.com>
[rharwood: style pass on code and commit message]
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
|
|
It's been suggested that we should link to the test plan in the readme.
This seems pretty reasonable to me, so here it is.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
When calling back into shim from grub, the MokListRT may contain additional
entries not available in the original MokList, an example being the certs
included via user_cert. Use the MokListRT instead when calling check_db_cert
and check_db_hash.
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
|
|
Given a set of EFI variables and boot assets, it should be possible
to compute what the value of PCR 7 will be on the next boot.
As shim manages the contents of the SbatLevel variable and this is
measured to PCR 7, export the payloads that shim contains in a new
COFF section (.sbatlevel) so that it can be introspected by code
outside of shim.
The new section works a bit like .vendor_cert - it contains a header
and then the payload. In this case, the header contains no size fields
because the strings are NULL terminated. Shim uses this new section
internally in set_sbat_uefi_variable.
The .sbatlevel section starts with a 4 byte version field which is
not used by shim but may be useful for external auditors if the
format of the section contents change in the future.
Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
PR 446 ("Add verify_image") duplicates some of the code affected by
Chris Coulson's defense in depth patch against CVE-2022-28737 ("pe:
Perform image verification earlier when loading grub").
This patch makes the same change to the new function.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
bump shim SBAT generation requirement to 2 for CVE-2022-28737
bump GRUB2 SBAT generation requirement to 2 for CVE-2021-3695
Signed-off-by: Jan Setje-Eilers <jan.setjeeilers@oracle.com>
|
|
Signed-off-by: Jan Setje-Eilers <jan.setjeeilers@oracle.com>
|
|
The second stage loader was being verified after loading it into
memory. As an additional hardening measure to avoid performing risky
memcpys using header fields from a potentially specially crafted image,
perform the verification before this so that it can be rejected earlier.
Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
|
|
During image loading, the size of the destination buffer for the image
is determined by the SizeOfImage field in the optional header. The start
and end virtual addresses of each section, as determined by each section's
VirtualAddress and VirtualSize fields, are bounds checked against the
allocated buffer. However, the amount of data copied to the destination
buffer is determined by the section's SizeOfRawData filed. If this is
larger than the VirtualSize, then the copy can overflow the destination
buffer.
Fix this by limiting the amount of data to copy to the section's
VirtualSize. In the case where a section has SizeOfRawData > VirtualSize,
the excess data is discarded.
This fixes CVE-2022-28737
Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
|
|
Since booting from removable media can be hard to detect,
setting a persistent latest SBAT policy is risky in a typical
client system. This changes latest to be a one-shot operation
that could be set at the time of an OS update if desired.
Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
|
|
|
|
We're told what the size limit is for one of the two things this needs
to handle, we should honor it.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Coverity pointed out that AllocateZeroPool() can error, and then we
immediately dereference the NULL.
This handles the error case.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Coverity noticed that the refactoring of handle_image() wildly
misunderstood how we deal with file paths.
This reworks it to not have a bunch of NULL dereferences.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Coverity complains:
CID 373676 (#3 of 3): Unrecoverable parse warning (PARSE_ERROR)
1. arguments_provided_for_attribute: attribute "__malloc__" does not take arguments
This is, of course, just plain wrong. Even so, I'm tired of looking at
it, so this patch wraps the #define we use for that attribute in a check
to see if it's being built by Coverity.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
preserve_sbat_uefi_variable() shoud really deal with the sbat
metadata version as a numerical value that could gain more digits.
It also needs to only compare the datestamp since the actual
metadata can grow and shrink
Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
|
|
scan-build noticed that when we split this out, this assignment was no
longer in a loop, and so doesn't do anything.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
scan-build says info->FileName returned from a successful call to
dir->Read() can be NULL. I don't think that would be a compliant
implementation, but anything's possible.
This patch checks it for NULL-ness before the StrCaseCmp().
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
There are a couple of places where the code we've got right now just
uses integers to decode one of our MoK variables. That's bad.
This patch replaces those with symbolic names.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
scan-build noticed that there's a path where we'll pass some data from
the read_image() to e.g. the string functions, but it might be an
unassigned pointer on one of the code paths. I don't think you can
actually hit it without returning from an error first, but best to
initialize these anyway.
This patch initializes data to NULL and datasize to 0.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
