| Age | Commit message (Collapse) | Author |
|---|
|
Network booting tends to expose things like a tfpt server
as a filesystem that doesn't implement directory listing
This will blindly try to ingest a revocations.efi file in
those cases, even if that may result in some console noise
when the file does not exist.
Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
|
|
The netboot path up until now hardcodes DEFAULT_LOADER as
the only possible filename to load. This is pretty limiting
and needs to be fixed.
Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
|
|
When the term previous was introduced for revocations to be
automatically applied there was a hope that everytime a new
revocation was built into shim, the previous revocation could
be applied automatically. Further experience has shown the
real world to be more complex than that. The automatic payload
will realistically contain a set of revocations governed by
both the cadence at which a distro's customer base updates
as well as the severity of the issue being revoked.
This is not a functional change.
Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
|
|
Attempting to call loadimage on revocations.efi when it isn't present
should results in error messages being printed to the console on at
least some firmware:
Failed to open \EFI\distro\revocations.efi - Not Found
Failed to load image ...: Not Found
Of course this is going to be the normal case on nearly all systems, at
least to begin with. Since we are about to loop through the directory
entries anyway, we can just make two passes, first looking for
revocations.efi and then looking for shim_certificate.efi. This will
still ensure that any revocations in revocations.efi are picked up
before shim_certificate.efi is loaded without resulting in any noise on
the console.
Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
|
|
Several of our CVE fixes apparently were not well tested on 32-bit, and
needed some (uintptr_t) casts sprinkled about to build with
-Werror=pointer-to-int-cast.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Inline PE section names (i.e. when not using the string table) are 8
byte arrays. They often look like ASCII strings, but they are not ASCII
strings.
This patch changes load_revocations_file() to always check that the full
array matches the section name in question.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Before applying an updated SbatLevel shim should re-run
introspection and never apply a revocation level that would
prevent the currently running shim from booting. The proper
way forward is to update shim first.
Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
|
|
This adds support for applying SkuSiPolicy UEFI BS variables. These
varaibles are needed for non-dbx based Windows revocations and are
described here:
https://support.microsoft.com/en-us/topic/kb5027455-guidance-for-blocking-vulnerable-windows-boot-managers-522bb851-0a61-44ad-aa94-ad11119c5e91
Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
|
|
Ingest SBAT Levels from revocations binary thereby allowing level
requirements to be updated independently from shipping a new shim.
Do not automatically apply any revocations from a stock shim at
this point.
Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
|
|
In verify_sbat_section(), we do some math on data that comes from the
binary being verified - in this case, we add 1 to the size of the
".sbat" section as reported in the section header, which is then used as
the input to the size of an allocation. The original value is then used
for a size in a memcpy(), which means there's an out-of-bounds write in
the overflow case.
Due to the type of the variable being size_t, but the type in the
section header being uint32_t, this is only plausibly accomplished on
32-bit systems.
This patch makes the arithmetic use a checked add operation to avoid
overflow. Additionally, it adds a check in verify_buffer_sbat() to
guarantee that the data is within the binary.
It's not currently known if this is actually exploitable on such
systems; the memory layout on a particular machine may further mitigate
this scenario.
Resolves: CVE-2023-40548
Reported-by: gkirkpatrick@google.com
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
In the validation logic in verify_buffer_authenticode(), there is yet
another case where we need to guarantee an object is in the binary but
we're only validating the pointer to it. In this case, we're validating
that the actual signature data is in the binary, but unfortunately we
failed to validate that the header describing it is, so a malformed
binary can cause us to take an out-of-bounds read (probably but not
necessarily on the same page) past the end of the buffer.
This patch adds a bounds check to verify that the signature is
actually within the bounds.
It seems unlikely this can be used for more than a denial of service,
and if you can get shim to try to verify a malformed binary, you've
effectively already accomplished a DoS.
Resolves: CVE-2023-40549
Reported-by: gkirkpatrick@google.com
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
In verify_buffer_sbat(), we have a goal-seeking loop to find the .sbat
section header. Unfortunately, while the actual contents of the section
are checked for being inside the binary, no such check exists for the
contents of the section table entry.
As a result, a carefully constructed binary will cause an out-of-bounds
read checking if the section name is ".sbat\0\0\0" or not.
This patch adds a check that each section table entry is within the
bounds of the binary.
It's not currently known if this is actually exploitable beyond creating
a denial of service, and an attacker who is in a position to use it for
a denial of service attack must already be able to do so.
Resolves: CVE-2023-40550
Reported-by: gkirkpatrick@google.com
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Verifying the validity of a files signature can protect from
an attacker creating a file that exploits a potential issue
in the sbat validation. If the signature is not checked first,
an attacker can just create a file with a valid .sbat section
and can still attack the signature validation.
Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
|
|
The function msleep uses gBS->Stall which waits for a specified number
of microseconds.
Reference: https://edk2-docs.gitbook.io/edk-ii-uefi-driver-writer-s-guide/5_uefi_services/51_services_that_uefi_drivers_commonly_use/517_stall
This reference even mentions an example sleeping for 10 microseconds: // Wait 10 uS. Notice the letter 'u'.
Therefore it's a good idea to call the function 'usleep' rather than
'msleep', so no one confuses it with milliseconds, and to change the
argument name to match as well.
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>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
Heavily inspired by Matthew Garrett's patch "Allow additional certificates
to be loaded from a signed binary".
Add support for loading a binary, verifying its signature, and then
scanning it for embedded certificates. This is intended to make it
possible to decouple shim builds from vendor signatures. In order to
add new signatures to shim, an EFI Signature List should be generated
and then added to the .db section of a well-formed EFI binary. This
binary should then be signed with a key that shim already trusts (either
a built-in key, one present in the platform firmware or
one present in MOK) and placed in the same directory as shim with a
filename starting "shim_certificate" (eg, "shim_certificate_oracle").
Shim will read multiple files and incorporate the signatures from all of
them. Note that each section *must* be an EFI Signature List, not a raw
certificate.
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
|
|
Separate out image reading from image launch in order to be able to load
an image without executing it.
Signed-off-by: Matthew Garrett <mgarrett@aurora.tech>
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
|
|
This makes SHIM_VERBOSE / SHIM_DEVEL_VERBOSE work the same way as
SHIM_DEBUG / SHIM_DEVEL_DEBUG when shim is built with ENABLE_SHIM_DEVEL
set.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This implements SBAT verification via the shim_lock protocol
by moving verification inside the existing verify_buffer()
function that is shared by both shim_verify() and handle_image().
The .sbat section is optional for code verified via the shim_lock
protocol, unlike for code that is verified and executed directly
by shim. For executables that don't have a .sbat section,
verification is skipped when using the protocol.
A vendor can enforce SBAT verification for code verified via the
shim_lock protocol by revoking all pre-SBAT binaries via a dbx
update or by using vendor_dbx and then only signing binaries that
have a .sbat section from that point.
Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
|
|
Requesting a keystroke when Secure Boot is not enabled and verbosity is
enabled is really annoying.
Signed-off-by: Renaud Métrich <rmetrich@redhat.com>
|
|
We see various reports of boot failures because the generated
boot entries contain garbage/tagging that we do not expect, and
that we then parse as a second stage boot loader.
|
|
Simple refactoring that extracts the path checking on the given
loaded image. This will be useful to check if we were booted via
removable media path in other places.
|
|
If the specified second stage loader does not exist (invalid
parameter), fall back to the DEFAULT_LOADER. This avoids failing
the boot on any garbage that made it through the load option parser
as a second stage loader name.
Bug-Ubuntu: https://bugs.launchpad.net/bugs/1937115
|
|
This moves the globals from shim.c (and lib/console.c) into their own
file, to make it so that unit tests can more easily link against code
that uses that state.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This just makes one less thing we have to make sure is the same between
the test harnesses and the runtime code.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
An error message complaining about missing file BOOTx64.EFI makes no
sense on other architectures.
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
|
|
This moves set_second_stage() and some of the helper functions it uses
out of shim.c, so that it's easier to write test cases for.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
pause() is a posix function, and having it named the same as this makes
it hard to include the asm.h header in some test cases.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
The load options handling is quite complicated and tries to accomodate
several scenarios, but there are currently multiple issues:
- If the supplied LoadOptions is an EFI_LOAD_OPTION structure,
second_stage gets initialized to the entire contents of the OptionalData
field and load_options is initialized to NULL, which means it isn't
possible to pass additional options to the second stage loader (and it
looks like the intention is for this to be supported).
- If the supplied LoadOptions contains 2 or more strings, the code seems
to assume that shim was executed from the UEFI shell and that the first
argument is the path of the shim executable, so it's ignored. But this
breaks the ability to pass additional options to the second stage loader
from BDS on firmware implementations that initialize LoadOptions to just
the OptionalData field of the EFI_LOAD_OPTION, which is what EDK2 seems
to do.
This is moot anyway because this case (strings == 2) doesn't actually seem
to work, as nothing sets loader_len and therefore second_stage is not set
to the custom loader path.
- If the supplied LoadOptions contains a single string that isn't shim's
path, nothing sets loader_len and therefore second_stage isn't set at the
end of set_second_stage.
- set_second_stage replaces L' ' characters with L'\0' - whilst this is
useful to NULL terminate the path for the second stage, it doesn't seem
quite right to do this for the remaining LoadOptions data. Grub's
chainloader command supplies additional arguments as a NULL-terminated
space-delimited string via LoadOptions. Making it NULL-delimited seems to
be incompatible with the kernel's commandline handling, which wouldn't
work for scenarios where you might want to direct-boot a kernel image
(wrapped in systemd's EFI stub) from shim.
- handle_image passes the original LoadOptions to the second stage if
load_options is NULL, which means that the second stage currently always
gets shim's load options.
I've made an attempt to try to fix things. After the initial
checks in set_second_stage, it now does this:
- Tries to parse LoadOptions as an EFI_LOAD_OPTION in order to extract
the OptionalData if it is.
- If it's not an EFI_LOAD_OPTION, check if the first string is the
current shim path and ignore it if it is (the UEFI shell case).
- Split LoadOptions in to a single NULL terminated string (used to
initialize second_stage) and the unmodified remaining data (used to
initialize load_options and load_options_size).
I've also modified handle_image to always set LoadOptions and
LoadOptionsSize. If shim is executed with no options, or is only
executed with a single option to override the second stage loader
path, the second stage is executed with LoadOptions = NULL and
LoadOptionsSize = 0 now.
I've tested this on EDK2 and I can load a custom loader with extra
options from both BDS and the UEFI shell:
FS0:\> shimx64.efi test.efi
LoadOptionsSize: 0
LoadOptions: (null)
FS0:\> shimx64.efi test.efi
LoadOptionsSize: 0
LoadOptions: (null)
FS0:\> shimx64.efi test.efi foo bar
LoadOptionsSize: 16
LoadOptions: foo bar
|
|
An openSUSE user reported(*) that shim 15.4 failed to boot the system
with the following message:
"Could not create MokListXRT: Out of Resources"
In the beginning, I thought it's caused by the growing size of
vendor-dbx. However, we found the following messages after set
SHIM_VERBOSE:
max_var_sz:8000 remaining_sz:85EC max_storage_sz:9000
SetVariable(“MokListXRT”, ... varsz=0x1404) = Out of Resources
Even though the firmware claimed the remaining storage size is 0x85EC
and the maximum variable size is 0x8000, it still rejected MokListXRT
with size 0x1404. It seems that the return values from QueryVariableInfo()
are not reliable. Since this firmware didn't really support Secure Boot,
the variable mirroring is not so critical, so we can just accept the
failure of import_mok_state() and continue boot.
(*) https://bugzilla.suse.com/show_bug.cgi?id=1185261
Signed-off-by: Gary Lin <glin@suse.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This adds dprint() to a bunch of our error returns.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Because a few shim builds were signed that did not properly initialize
the SBAT variable, and in doing so deleted valid SBAT variables, we need
to use a different name.
This changes the name from "SBAT" to "SbatLevel".
Signed-off-by: Jan Setje-Eilers <jan.setjeeilers@oracle.com>
|
|
If the LoadOptions string count is zero, then it's assumed that it is an
EFI_LOAD_OPTION and the OptionalData field attempt to be parsed. If that
fails as well, in the second stage was set to the default loader path.
But this behaviour was changed by the commit 018b74d2 ("shim: attempt to
improve the argument handling"), and not in that case the LoadOptions is
attempted to be used as a single string. This breaks some firmwares that
return something in the LoadOptions but are not a proper EFI device path.
Instead of making assumptions about the LoadOptions if can't be parsed
correctly, just use the default loader as it was done before that commit.
This fixes booting on a Gigabyte GA-Z97X-SLI mainboard that contains the
following bytes as LoadOptions: 0x41 0x4d 0x42 0x4f ('AMBO').
Reported-by: Thomas Frauendorfer | Miray Software <tf@miray.de>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
|
|
Commit 018b74d2d69 ("shim: attempt to improve the argument handling") added
added workarounds for a couple of LoadOption problems on some systems, but
introduced a regression since the is_our_path() function can be called with
a NULL start UCS-2 string.
If there's only one string, set start to the start of LoadOptions.
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
|
|
scan-build kindly pointed out:
| shim.c:1568:10: warning: Array access (from variable 'start') results in a null pointer dereference [core.NullDereference]
| while (start[loader_len++] != L'\0');
| ^~~~~~~~~~~~~~~~~~~
| 1 warning generated.
It thinks that because of a bad assumption it's making because of the
test immediately before it, which isn't currently necessary /at all/.
In fact, neither is this loop; it appears to be vestigial and the goal
was done in the loop above it.
This patch just solves for how much space is left arithmetically
instead.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This fixes ENABLE_SHIM_DEVEL to actually work, and also makes our "goto
die" failure behavior change (to wait considerably longer) based on it.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
There's no reason to do the work to set an initial SBAT variable twice,
or to do it /after/ the self check.
This changes it to do it once, before the self check, and then only
raise an error if we're in secure mode.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
|
|
"So, load options are a giant pain in the ass." - Peter Jones
This patch attempts to workaround two LoadOption problems seen on
my test system: arguments separated with only whitespace (L" ") and
strings that aren't properly NULL terminated.
In addition to my test system, it appears at least one other person
has run into a similar problem and I can reproduce the whitespace
delimiter issue with QEMU+OVMF (OEMU v5.2.0, OVMF v202011).
* https://github.com/rhboot/shim/issues/181
For reference, using QEMU+OVMF and a simple test application,
"test.efi", which does a hexdump of LoadOptions I see the following
when run via the UEFI shell:
FS0:\> test.efi one two three
LoadOptions:
0000: 74 00 65 00 73 00 74 00 2E 00 65 00 66 00 69 00 t.e.s.t...e.f.i.
0016: 20 00 6F 00 6E 00 65 00 20 00 74 00 77 00 6F 00 ..o.n.e...t.w.o.
0032: 20 00 74 00 68 00 72 00 65 00 65 00 00 00 ..t.h.r.e.e...
Signed-off-by: Paul Moore <pmoore2@cisco.com>
|
|
Signed-off-by: Alex Burmashev <alexander.burmashev@oracle.com>
|
