Only include the hashes for the architecture we're building for - no
point in adding bloat and delay here.
Add a script "block_signed_deb" to scan a set of .deb files, extract
the hashes for .efi binaries and list them in the format wanted for
the dbx hashes file.
Split out the code to use that file from the rules file into a
separate helper.
At the default -Os optimization level, gcc emits ".text.startup"
and ".text.unlikely" sections for static initializers and noreturn
functions which end up in the intermediate ELF binary:
$ objdump -h build-x64/shimx64.efi.so
build-x64/shimx64.efi.so: file format elf64-x86-64
Sections:
Idx Name Size VMA LMA File off Algn
0 .text 00046e7b 0000000000001000 0000000000001000 00001000 2**10
CONTENTS, ALLOC, LOAD, READONLY, CODE
1 .text.startup 00000118 0000000000047e7b 0000000000047e7b 00047e7b 2**0
CONTENTS, ALLOC, LOAD, READONLY, CODE
2 .text.unlikely 00000046 0000000000047f93 0000000000047f93 00047f93 2**0
CONTENTS, ALLOC, LOAD, READONLY, CODE
3 .data 000315e8 0000000000048000 0000000000048000 00048000 2**9
These additional .text.* sections are omitted from the final PE/COFF
binary, resulting in a crash when processing the ctors. Taking a look at
_init_array in gdb:
(gdb) p/x &_init_array
$1 = 0x78510
(gdb) p/x &_init_array_end
$2 = 0x7851c
(gdb) x/x (void*)&_init_array
0x78510 <_init_array>: 0x00047e7b
(gdb) x/x (void*)(&_init_array)+8
0x78518 <_init_array+8>: 0x00000000
See that 0x00047e7b falls inside the padding between the .text and .data
sections:
$ objdump -h build-x64/shimx64.efi
build-x64/shimx64.efi: file format pei-x86-64
Sections:
Idx Name Size VMA LMA File off Algn
0 .text 00046e7b 0000000000001000 0000000000001000 00000400 2**10
CONTENTS, ALLOC, LOAD, READONLY, CODE
1 .data 000315e8 0000000000048000 0000000000048000 00047400 2**9
Adjust the linker script to merge the .text.startup and .text.unlikely
sections in to the .text section.
[edited by pjones to use .text.* instead of naming the sections
individually, and to sync up with what other arches have in .text]
"gcc -fanalyzer" found two NULL pointer checks we're missing in sbat.c:
include/str.h: In function ‘get_sbat_field.part.0’:
sbat.c:20:14: error: dereference of NULL ‘offset’ [CWE-476] [-Werror=analyzer-null-dereference]
20 | if (!*offset)
and
include/str.h: In function ‘parse_sbat’:
sbat.c:140:27: error: dereference of NULL ‘current’ [CWE-476] [-Werror=analyzer-null-dereference]
140 | } while (entry && *current != '\0');
Both are simple, and this patch fixes them.
Signed-off-by: Peter Jones <pjones@redhat.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>
The parse_sbat() function is currently removing the last character of the
passed buffer, which will usually be a null-terminated string to parse.
There's no reason to do this and just take the whole size as specified by
the caller.
Reported-by: Chris Coulson <chris.coulson@canonical.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
handle_image() is quite huge and complex.
This patch moves the SBAT validation code from handle_image() to a new
function, handle_sbat().
Signed-off-by: Peter Jones <pjones@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>
Per Peter Jones suggestion, we will be flexible in what data we expect
while parsing the variable. Three fields are mandatory:
component_generation, component_name_size, component_name
However we also support adding comments and additional information to be
added after component name, with ',' as a separator. Those information
will be ignored and not used for verification purposes.
So:
grub,1
and
grub,1,wow,this,is,my,comment
will provide exactly same set of data for verification.
[0]: https://github.com/rhboot/shim/blob/main/SBAT.md
Signed-off-by: Alex Burmashev <alexander.burmashev@oracle.com>
Signed-off-by: Peter Jones <pjones@redhat.com>
The struct sbat isn't doing anything and only has two fields so let's pass
pass those two to the functions directly instead of storing it in a struct.
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>
Not all distributions put the crt0-efi-$(ARCH).o file under
$LIB_DIR/gnuefi, some stash it directly in $LIB_DIR. In an effort
to make the build a bit more user friendly, check if $LIB_DIR/gnuefi
exits before setting $EFI_PATH to that value; if $LIB_DIR/gnuefi does
not exist, fallback to $LIB_DIR for $EFI_PATH.
Signed-off-by: Paul Moore <pmoore2@cisco.com>
I wrote a test case for strnlena() and strndupa() and of course both
were off by one in the opposite directions...
... but the next patch obviates the need for them, hopefully, so this
will wind up getting dropped.
We noticed that we'd originally specified the SBAT variable as binary
records, but talked as if they're CSV. Woops. Anyway, this makes them
CSV, which also means they don't need the size field.
Signed-off-by: Peter Jones <pjones@redhat.com>
Add parameter checking to parse_sbat().
Set end pointer to be sbat_base + sbat_size - 1. We directly
dereference the end pointer but this is technically outside of
our sbat_base buffer range.
Remove current and end while loops that account for extra CRLF
or LF characters before and after the .sbat section. We will
rely on automated tooling to verify the .sbat section is sane.
Remove the overwriting of *(end - 1) with '\0'. This behavior
causes a segfault in the unit test. parse_sbat_entry() expects
a very specific pattern "_,_,_,_,_,_\n" for every entry and uses
strchrnul() to process each individual field. When *(end - 1)='\0'
is present, it short-circuits the final \n and causes the final
get_sbat_field() to return NULL, thereby setting current = NULL.
Eventually parse_sbat attempts to access current in the do-while
condition and the segfault happens.
Signed-off-by: Chris Co <chrco@microsoft.com>
It's a left over from an early implementation that was never cleaned.
Reported-by: Christopher Co <christopher.co@microsoft.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
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>
