| Age | Commit message (Collapse) | Author |
|---|
|
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>
|
|
This re-structures our includes so we can be sure everything is always
including all the system headers in a uniform, predictable way.
Temporarily it also adds a bunch of junk at all the places we use
variadic functions to specifically pick either the MS (cdecl) or ELF
ABIs.
I'm not 100% sure that's all correct (see later patch) but it's enough
to allow this to build.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
When grub2 invoked Exit() in AArch64 AAVMF, the VM crashed with the
following messsages:
Unloading driver at 0x000B7D7B000
Synchronous Exception at 0x00000000BF5D5E68
AllocatePool: failed to allocate 800 bytes
Synchronous Exception at 0x00000000BF5D5E68
The similar error also showed when I modified MokManager to call
gBS->Exit() at the end of efi_main(). However, if MokManager just
returned, the error never showed. One significant difference is
whether the loaded image was restored or not, and the firmware seems
to need the original ImageBase pointer to do clean-up.
To avoid the potential crash, this commit adds restore_loaded_image() so
that we can restore the loaded image both in start_image() and
do_exit().
Signed-off-by: Gary Lin <glin@suse.com>
|
|
This is needed for shim to verify itself when booting, to make sure that
shim binaries can't be executed anymore after been revoked by SBAT.
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
|
|
A following patch will make shim to verify its .sbat section and it
should be done before doing the OpenSSL initialization. But having
the debugger attached may be useful at this point.
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
|
|
On a typical boot we validate at least two binaries; parsing the SBAT
EFI variable each time, when it should not be changing, is not worth the
effort.
This patch moves the parsing out to some setup code, instead of doing it
during the verification stage.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Numbering the error messages in efi_main directly was a mistake, and the
following patches just make it more apparent.
This makes it an enum so we don't have to re-number at more than one
place when we add or remove them.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Currently, for an EV_EFI_VARIABLE_AUTHORITY event, the shim puts only
EFI_SIGNATURE_DATA.SignatureData in the VariableData field, but omits
EFI_SIGNATURE_DATA.SignatureOwner. According to reference implementation
in EDK2, the entire EFI_SIGNATURE_DATA is put into the VariableData
field, shown here:
https://github.com/tianocore/edk2/blob/master/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c#L1032
|
|
v2 - updated for conflicts and to include documentation (pjones)
|
|
Currently, if you have two boot entries, say one for
\EFI\fedora\shimx64.efi and one for \EFI\devel\shimx64.efi, and you set
the efi variable SHIM_DEBUG=1, both of these will trigger, and you need
to write your debugging scripts to allow each of the builds to continue.
This is a pain.
This patch makes it so on your development build, it will instead check
SHIM_DEVEL_DEBUG, thus meaning you can have it pause for a debugger only
on the development branch and not the OS you need to boot to scp in a
new development build.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This is a backport from devel of:
commit 634fd72ac6a6c6c9010c32506d524586826a8637
Author: Peter Jones <pjones@redhat.com>
Date: Fri Nov 22 15:14:22 2019 -0500
Make httpboot.c always get built.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
The license statements in our source files were getting to be a giant
mess, and mostly they all just say the same thing. I've switched most
of it to SPDX labels, but left copyright statements in place (where they
were not obviously incorrect copy-paste jobs that I did...).
If there's some change here you don't think is valid, let me know and
we can fix it up together.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
In the version of clang-format I've got locally[0],
WhitespaceSensitiveMacros seems to only work sometimes. That means that
if we ever run it on some particular things, it could seriously mess up
a bunch of our debugging output. That's not great.
In this patch, I've gone ahead and run clang-format on all the macros
that use __LINE__, which are the obvious places this is dangerous, and
then audited the result and fixed anything that's broken (including a
couple of places where it was already broken.)
[0] random:~/devel/github.com/shim/clang-format$ clang-format --version
clang-format version 11.0.0 (Fedora 11.0.0-2.fc33)
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
The DEFAULT_LOADER is a UCS-2 string and the StrLen() function returns the
number of UCS-2 encoded characters in the string. But the allocated memory
is in bytes, so only half of the needed memory to store it is allocated.
This leads to a buffer overrun when the StrCpy() function attempts to copy
the DEFAULT_LOADER to the allocated buffer.
Fixes: 354bd9b1931 ("Actually check for errors from set_second_stage()")
Reported-by: Stuart Hayes <stuart_hayes@dell.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
Upstream: pr#213
|
|
This adds support for multiple signatures. It first tries validating
the binary by hash, first against our dbx lists, then against our db
lists. If it isn't allowed or rejected at that step, it continues to
the normal routine of checking all the signatures.
At this point it does *not* reject a binary just because a signature is
by a cert on a dbx list, though that will override any db list that
certificate is listed on. If at any point any assertion about the
binary or signature list being well-formed fails, the binary is
immediately rejected, though we do allow skipping over signatures
which have an unsupported sig->Hdr.wCertificateType.
Signed-off-by: Peter Jones <pjones@redhat.com>
Upstream: pr#210
|
|
Potential new signing strategies ( for example signing grub, fwupdate
and vmlinuz with separate certificates ) require shim to support a
vendor provided bundle of trusted certificates and hashes, which allows
shim to trust EFI binaries matching either certificate by signature or
hash in the vendor_db. Functionality is similar to vendor_dbx.
This also improves the mirroring quite a bit.
Upstream: pr#206
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
Upstream: pr#206
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
Upstream: pr#212
|
|
The "TCG PC Client Specific Platform Firmware Profile Specification" says
that when measuring a PE/COFF image, the TCG_PCR_EVENT2 structure Event
field MUST contain a UEFI_IMAGE_LOAD_EVENT structure.
Currently an empty UEFI_IMAGE_LOAD_EVENT structure is passed so users only
have the hash of the PE/COFF image, but not information such the file path
of the binary.
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Upstream-commit-id: c252b9ee94c
|
|
When shim loads the second stage loader (e.g: GRUB) the FilePath field of
the EFI_LOADED_IMAGE structure isn't updated with the path of the loaded
binary. So it still contains the file path of the shim binary.
This isn't a problem since the file path is currently not used. But should
be used to set the DevicePath field of the EFI_IMAGE_LOAD_EVENT structure
that is logged when measuring the PE/COFF binaries. In that case the TPM
Event Log will have an incorrect file path for the measured binary, i.e:
$ hexdump -Cv /sys/kernel/security/tpm0/binary_bios_measurements
...
00000a50 00 00 00 00 00 00 04 04 34 00 5c 00 45 00 46 00 |........4.\.E.F.|
00000a60 49 00 5c 00 72 00 65 00 64 00 68 00 61 00 74 00 |I.\.r.e.d.h.a.t.|
00000a70 5c 00 73 00 68 00 69 00 6d 00 78 00 36 00 34 00 |\.s.h.i.m.x.6.4.|
00000a80 2e 00 65 00 66 00 69 00 00 00 7f ff 04 00 00 00 |..e.f.i.........|
00000a90 00 00 00 00 00 00 af 08 00 00 00 0d 00 00 00 b5 |................|
00000aa0 cd d0 8f bb 16 31 e2 80 8b e8 58 75 c9 89 18 95 |.....1....Xu....|
00000ab0 d2 de 15 15 00 00 00 67 72 75 62 5f 63 6d 64 20 |.......grub_cmd |
00000ac0 73 65 74 20 70 61 67 65 72 3d 31 00 08 00 00 00 |set pager=1.....|
...
So update the EFI_LOADED_IMAGE structure with the second stage loader file
path to have the correct value in the log, i.e:
$ hexdump -Cv /sys/kernel/security/tpm0/binary_bios_measurements
...
00000a50 00 00 00 00 00 00 04 04 34 00 5c 00 45 00 46 00 |........4.\.E.F.|
00000a60 49 00 5c 00 72 00 65 00 64 00 68 00 61 00 74 00 |I.\.r.e.d.h.a.t.|
00000a70 5c 00 67 00 72 00 75 00 62 00 78 00 36 00 34 00 |\.g.r.u.b.x.6.4.|
00000a80 2e 00 65 00 66 00 69 00 00 00 7f ff 04 00 00 00 |..e.f.i.........|
00000a90 00 00 00 00 00 00 af 08 00 00 00 0d 00 00 00 b5 |................|
00000aa0 cd d0 8f bb 16 31 e2 80 8b e8 58 75 c9 89 18 95 |.....1....Xu....|
00000ab0 d2 de 15 15 00 00 00 67 72 75 62 5f 63 6d 64 20 |.......grub_cmd |
00000ac0 73 65 74 20 70 61 67 65 72 3d 31 00 08 00 00 00 |set pager=1.....|
...
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Upstream-commit-id: cd7d42d493d
|
|
This changes shim_init() to check for errors from set_second_stage().
In order to make that work, it also does the following:
- correctly /always/ allocate second_stage, not sometimes allocate and
sometimes point at .data
- test for LoadOptionSize == 0 and return success
- print an error message for the failure so we can see it.
Signed-off-by: Peter Jones <pjones@redhat.com>
Upstream-commit-id: 354bd9b1931
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
Upstream-commit-id: 173d35fe8f5
|
|
Some versions of OpenSSL seem to go back and forth as to whether NULL
for these names are okay. Don't risk it.
Signed-off-by: Peter Jones <pjones@redhat.com>
Upstream-commit-id: 46b76a01717
|
