| Age | Commit message (Collapse) | Author |
|---|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
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>
|
|
The ability to automatically apply SBATLevel revocations varies
from distro to distro. This allows distros that are able to
automatically apply SBATLevel revocations when shim is updated to
select a level by supplying SBAT_AUTOMATIC_DATE=<datestamp> on the
make command line. Currently the following options are available:
2021030218 no revocations - useful for distros that need to rely on
an externally delivered revocations.efi
2022052400 grub,2
2022111500 shim,2
grub,3
2023012900 shim,2
grub,3
grub.debian,4
If no datestamp is specified the build will default to the
most recent 2023012900.
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>
|
|
GCC 4 doesn't have __builtin_add_overflow() and friends, so this results
in a compiler error.
On platforms using that version, do the arithmetic without it.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
We thought we would fully support NX compatibility in the full stack for
this release, but all of the necessary components aren't *quite* ready
for this release.
This patch switches back the default that was changed in a53b9f7ceec1d,
but it leaves the build infrastructure in place.
Signed-off-by: Peter Jones <pjones@redhat.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>
|
|
otherwise our build won't be reproducible, doh!
|
|
Since shim is inherently updated by shipping a new shim, the
latest built in revocations can include the most recent shim
revocations. Since CVE-2023-40547 is high impact, this revocation
should be available to everyone as soon as possible.
GRUB2 CVE-2023-4692 and CVE-2023-4693 are in the ntfs module that
only some vendors ship. Since some vendors did not ship an updated
GRUB2 for these issues, the revocation for these CVEs is not
included in the payload at this time.
Signed-off-by: Jan Setje-Eilers <jan.setjeeilers@oracle.com>
|
|
When working on issues with the memory attributes API, shim does not
currently display any errors which are returned from the API itself.
This adds those error messages.
Resolves #575
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
When retrieving files via HTTP or related protocols, shim attempts to
allocate a buffer to store the received data. Unfortunately, this means
getting the size from an HTTP header, which can be manipulated to
specify a size that's smaller than the received data. In this case, the
code accidentally uses the header for the allocation but the protocol
metadata to copy it from the rx buffer, resulting in an out-of-bounds
write.
This patch adds an additional check to test that the rx buffer is not
larger than the allocation.
Resolves: CVE-2023-40547
Reported-by: Bill Demirkapi, Microsoft Security Response Center
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>
|
|
If shim detects a self revocation in a new proposed SbatLevel
and refuses to apply this new set of revocations a message should
be printed even in non-verbose modes.
Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.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>
|
|
Unless an explict sbat policy is specified, always delete SbatLevel
when secure boot is disabled.
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 CVE-2023-40546, an incorrect invocation of LogError()
causes a read from the page at address 0, which on newer systems will
correctly cause a fault. The immediate fix for this CVE is to fix the
invocation so that the error is logged correctly, but there is more that
can be done.
This patch adds additional checks to ensure that the format specifier on
any of these invocations can not be NULL, thereby mitigating this entire
class of error from creating a fault. Additionally, most of these
checks are done using _Static_assert(), so they should normally be
triggered at compile time.
Signed-off-by: Peter Jones <pjones@redhat.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 CVE-2023-40550, we scan the section headers for the section
name without having verified that the section header is actually in the
binary.
This patch adds such verification to read_headers()
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>
|
|
Since the fuzzer already found one problem here, and none of that data
is intended to be trusted to begin with, it makes sense to use checked
math for all of the values read from the PE headers.
This updates all of that math to use checked arithmetic operations.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
In read_header(), we attempt to parse the PE binary headers. In doing
so, if there is an MZ (i.e. MS-DOS) header, we locate the PE header by
finding the offset in that header. Unfortunately that is not correctly
bounds checked, and carefully chosen values can cause an out-of-bounds
ready beyond the end of the loaded binary.
Unfortunately the trivial fix (bounds check that value) also makes it
clear that the way we were determining if an image is loadable on this
platform and distinguishing between PE32 and PE32+ binaries has the
exact same issue going on, and so the fix includes reworking that logic
to correctly bounds check all of those tests as well.
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-40551
Reported-by: gkirkpatrick@google.com
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This adds a fuzz harness for read_header().
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
We need to do arithmetic on untrusted values sometimes, so this patch
adds the following primitives as macros that wrap the compiler builtins.
bool checked_add(TYPE addend0, TYPE addend1, TYPE *sum)
bool checked_sub(TYPE minuend, TYPE subtrahend, TYPE *difference)
bool checked_mul(TYPE factor0, TYPE factor1, TYPE *product)
And also the following primitive which returns True if divisor is 0 and
False otherwise:
bool checked_div(TYPE dividend, TYPE divisor, TYPE *quotient)
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
We used to use efisiglist to generate the DBX list. Newer versions of
the pesign package don't include it any more, and the recommended
replacement tool is now efisecdb from efivar. Tweak the
generate_dbx_list script to work with both old and new. Let's make
backports easy...
|
|
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 <pjones@redhat.com>
|
|
This is a "creature comfort" change to make it so gcc-specific options
don't make it into compile_commands.json.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
With "gcc -fanalyzer" and "scan-build", it's convenient to be able to
continue even though the compiler has returned error on one or more
source files.
This makes it so compiler errors are ignored in some of those cases.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
The code that mirrors MOK database to EFI variables gets the remaining
variable storage size from the firmware and subtracts the size needed
for any overhead to see if there is enough space to create a new entry.
However these calculations are on unsigned integer types, they can
underflow and result in huge values when the firmware is about to run
out of usable variable space. Explicitly check against this.
Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
|
|
Currently pe's handle_image() function has two related issues, which are
a memory leak in most error paths and an incorrect FreePool() call in
some error paths.
This patch adds the correct FreePages() calls to most error paths, and
switches the FreePool() call to match them.
[commit message re-written to be more informative by pjones]
Signed-off-by: Dennis Tseng <dennis.tseng@suse.com>
|
|
In 569270d8603d68308ad8bf8ef4cad4b09101d35e, the PE loader's address
sanitizing function, ImageAddress(), was changed to match the intended
behavior and the accompanying test case. Unfortunately, the PE
relocator uses this function to compute the last address in the
relocation directory, and as a result, any binary with a relocations
will trigger that edge condition and fail to load.
This patch changes that call to compute the address that's one byte
earlier. The only things the computed value is used for are a) testing
that the relocation *section* is valid, and b) serving as a limit for
iterating the relocations. Since a relocation is never less than two
bytes, this will still work.
[commit message re-written to be more informative by pjones]
Signed-off-by: Dennis Tseng <dennis.tseng@suse.com>
|
|
shim takes several forms of input from several sources that are not
necessarily trustworthy. As such, we need to take measures to validate
that we don't have unacceptable results from bad inputs. One such
measure is "fuzzing" the inputs which parse untrusted data by running
them with randomized or partially randomized input.
This change adds such testing using clang's "libFuzzer" to our parser
for ".sbat" sections. I've run it for about half an hour and so far it
found one memory leak, but no other errors.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
On the occasion that .sbat is entirely made of characters that aren't
meaningfully CSV /data/, but which the parser accepts (i.e. newline), we
currently allocate a byte of memory which then gets leaked.
This patch tests for that condition and skips the allocation when there
aren't any actual /entries/ to parse.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
shim takes several forms of input from several sources that are not
necessarily trustworthy. As such, we need to take measures to validate
that we don't have unacceptable results from bad inputs. One such
measure is "fuzzing" the inputs which parse untrusted data by running
them with randomized or partially randomized input.
This change adds such testing using clang's "libFuzzer" to our CSV
parser. I've run this on 24-cores at 4GHz for half an hour, and so far
each fuzzer has converged on 79% coverage. I expect the 21% that's not
getting covered are the EFI API mock interfaces we're building in from
test.c and similar. So far no errors have been found, which is what was
expected since this particular API is being manually fuzzed with ~8kB of
/dev/urandom on every build since 2021-02-23.
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>
|
|
This adds a test case for our address sanitation checking function
ImageAddresS(). In doing so it addresses two issues:
- previously we allowed the address after the last byte of the image to
be computed (may need to revert this or fix some callers, we'll see...)
- bespoke overflow checking and using + directly instead of using
__builtin_add_overflow()
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This moves the parts of pe.c that *don't* depend on Cryptlib into
pe-relocate.c, so we can write test cases for them without having to
make a second openssl build without EFI support.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
CentOS 7 and RHEL 7 are EOL, and their ancient compiler is EOL along
with them. Removing them from our test environments means we'll have to
do some minor backports if they come back from the dead, but lets us use
__builtin_add_overflow() and friends without gross hacks today.
This change removes those builds from our PR tests.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
In a few places we put dprintf() at places where the compiler will get
confused if it isn't a block or a statement.
Obviously, it should be a statement, so this makes it one.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
In binutils-2.39, GNU ld has added a warning when linking objects that
don't have a .note.GNU-stack section. Since we build with
"--fatal-warnings", this causes a failure:
ld: warning: cert.o: missing .note.GNU-stack section implies executable stack
ld: NOTE: This behaviour is deprecated and will be removed in a future version of the linker
This patch adds those sections to all of the .S files, so that they will
be in the objects, as well as updating our gnu-efi tree the same way.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This adds a make rule to generate compile_commands.json, which some
verifier tools depend on.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Cryptlib and OpenSSL both currently throw warnings with some compilers
using -Wunused-but-set-variable:
clang -std=gnu11 -ggdb -ffreestanding -fmacro-prefix-map=/home/pjones/devel/github.com/shim/main/= -fno-stack-protector -fno-strict-aliasing -fpic -fshort-wchar -nostdinc -m64 -mno-mmx -mno-sse -mno-red-zone -Os -Wall -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Werror -I/home/pjones/devel/github.com/shim/main/Cryptlib -I/home/pjones/devel/github.com/shim/main/Cryptlib/Include -I/home/pjones/devel/github.com/shim/main/gnu-efi/inc -I/home/pjones/devel/github.com/shim/main/gnu-efi/inc/x86_64 -I/home/pjones/devel/github.com/shim/main/gnu-efi/inc/protocol -isystem /home/pjones/devel/github.com/shim/main/include/system -isystem /usr/lib64/clang/16/include -DMDE_CPU_X64 -c -o Pk/CryptX509.o Pk/CryptX509.c
Pk/CryptX509.c:94:19: error: variable 'Index' set but not used [-Werror,-Wunused-but-set-variable]
UINTN Index;
^
clang -std=gnu11 -ggdb -ffreestanding -fmacro-prefix-map=/home/pjones/devel/github.com/shim/main/= -fno-stack-protector -fno-strict-aliasing -fpic -fshort-wchar -nostdinc -m64 -mno-mmx -mno-sse -mno-red-zone -Os -Wall -Wextra -Wno-missing-field-initializers -Wno-empty-body -Wno-implicit-fallthrough -Wno-unused-parameter -Werror -I/home/pjones/devel/github.com/shim/main/Cryptlib/OpenSSL -I/home/pjones/devel/github.com/shim/main/Cryptlib -I/home/pjones/devel/github.com/shim/main/Cryptlib/OpenSSL/Include/ -I/home/pjones/devel/github.com/shim/main/Cryptlib/OpenSSL/crypto -I/home/pjones/devel/github.com/shim/main/Cryptlib/Include -I/home/pjones/devel/github.com/shim/main/gnu-efi/inc -I/home/pjones/devel/github.com/shim/main/gnu-efi/inc/x86_64 -I/home/pjones/devel/github.com/shim/main/gnu-efi/inc/protocol -I/home/pjones/devel/github.com/shim/main/Cryptlib/OpenSSL/crypto/asn1 -I/home/pjones/devel/github.com/shim/main/Cryptlib/OpenSSL/crypto/evp -I/home/pjones/devel/github.com/shim/main/Cryptlib/OpenSSL/crypto/modes -I/home/pjones/devel/github.com/shim/main/Cryptlib/OpenSSL/crypto/include -isystem /home/pjones/devel/github.com/shim/main/include/system -isystem /usr/lib64/clang/16/include -DL_ENDIAN -D_CRT_SECURE_NO_DEPRECATE -D_CRT_NONSTDC_NO_DEPRECATE -DOPENSSL_SMALL_FOOTPRINT -DPEDANTIC -DMDE_CPU_X64 -c -o crypto/asn1/t_x509.o crypto/asn1/t_x509.c
crypto/asn1/t_x509.c:504:18: error: variable 'l' set but not used [-Werror,-Wunused-but-set-variable]
int ret = 0, l, i;
^
Since we normally build with -Werror, these cause builds to fail in
these cases. While the bad code should be addressed, it appears
generally safe, so we should solve it upstream.
This patch adds -Wno-unused-but-set-variable to the Cryptlib Makefile,
and removes the conditionalization on gcc in the OpenSSL Makefile, as
clang now has this argument, and since we don't support building with
clang for the final build, it's useful to have clang-based tools
working.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This serves to document the SbatLevel Boot Services variable so that
other boot services code, such as bootmgr can update the revocation
level.
Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
|
|
The variable fallback_verbose_wait from now on is of the type unsigned
long to match the type of the argument usleep() accepts.
Signed-off-by: Kamil Aronowski <kamil.aronowski@yahoo.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>
|
|
In preparation for renaming msleep() to usleep(), in some cases tests
were failing due to a mismatch between our declaration of the usleep()
function and what is being provided by unistd.h. This change simply
makes our function declared only when not in a unit test environment.
Signed-off-by: Kamil Aronowski <kamil.aronowski@yahoo.com>
|
