| Age | Commit message (Collapse) | Author |
|---|
|
In original sbat.c:
...
else if (preserve_sbat_uefi_variable(sbat, sbatsize, attributes,
sbat_var_candidate) &&
!reset_sbat) {
...
The time omplexity of preserve_sbat_uefi_variable() is higher than
reset_sbat. Maybe we could swap both of them to calculate reset_sbat
first. Such that the shortcut performance can be improved.
Signed-off-by: Dennis Tseng <dennis.tseng@suse.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>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
Support for updating SBAT revocations to latest or previous revocations.
Allow SBAT revocations to be reset to empty metadata only when UEFI
Secure Boot is disabled.
Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.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>
|
|
Valgrind was showing me a memory leak in the tests, and it's because of
several minor problems:
- the allocation isn't actually ever a list entry, because the entry
array was before the linked list in the allocations
- the comparison for "first" when trying to free it is incorrect, so
that was never getting set.
- we never free the test variable data that was parsed
- we're never calling cleanup_sbat_var() from several test cases.
This fixes these issues.
Before:
==2525955==
==2525955== HEAP SUMMARY:
==2525955== in use at exit: 181 bytes in 3 blocks
==2525955== total heap usage: 17 allocs, 14 frees, 2,310 bytes allocated
==2525955==
==2525955== 15 bytes in 1 blocks are definitely lost in loss record 1 of 3
==2525955== at 0x4845464: calloc (vg_replace_malloc.c:1117)
==2525955== by 0x401D21: UnknownInlinedFun (test-sbat.c:937)
==2525955== by 0x401D21: main (test-sbat.c:1043)
==2525955==
==2525955== 56 bytes in 1 blocks are definitely lost in loss record 2 of 3
==2525955== at 0x4845464: calloc (vg_replace_malloc.c:1117)
==2525955== by 0x402ACB: parse_sbat_var_data (sbat.c:234)
==2525955== by 0x40189D: UnknownInlinedFun (test-sbat.c:445)
==2525955== by 0x40189D: main (test-sbat.c:1029)
==2525955==
==2525955== 110 bytes in 1 blocks are definitely lost in loss record 3 of 3
==2525955== at 0x4845464: calloc (vg_replace_malloc.c:1117)
==2525955== by 0x402ACB: parse_sbat_var_data (sbat.c:234)
==2525955== by 0x401D67: UnknownInlinedFun (test-sbat.c:943)
==2525955== by 0x401D67: main (test-sbat.c:1043)
==2525955==
==2525955== LEAK SUMMARY:
==2525955== definitely lost: 181 bytes in 3 blocks
==2525955== indirectly lost: 0 bytes in 0 blocks
==2525955== possibly lost: 0 bytes in 0 blocks
==2525955== still reachable: 0 bytes in 0 blocks
==2525955== suppressed: 0 bytes in 0 blocks
==2525955==
==2525955== For lists of detected and suppressed errors, rerun with: -s
==2525955== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)
After:
==2591367== Memcheck, a memory error detector
==2591367== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==2591367== Using Valgrind-3.17.0 and LibVEX; rerun with -h for copyright info
==2591367== Command: ./test-sbat
==2591367==
==2591367==
==2591367== HEAP SUMMARY:
==2591367== in use at exit: 56 bytes in 1 blocks
==2591367== total heap usage: 17 allocs, 16 frees, 5,382 bytes allocated
==2591367==
==2591367== 56 bytes in 1 blocks are definitely lost in loss record 1 of 1
==2591367== at 0x4845464: calloc (vg_replace_malloc.c:1117)
==2591367== by 0x402AEB: parse_sbat_var_data (sbat.c:234)
==2591367== by 0x40189D: UnknownInlinedFun (test-sbat.c:445)
==2591367== by 0x40189D: main (test-sbat.c:1033)
==2591367==
==2591367== LEAK SUMMARY:
==2591367== definitely lost: 56 bytes in 1 blocks
==2591367== indirectly lost: 0 bytes in 0 blocks
==2591367== possibly lost: 0 bytes in 0 blocks
==2591367== still reachable: 0 bytes in 0 blocks
==2591367== suppressed: 0 bytes in 0 blocks
==2591367==
==2591367== For lists of detected and suppressed errors, rerun with: -s
==2591367== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
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>
|
|
Currently, the check for the contents of the SBAT variable has an
inverted strncmp() test, causing it to delete the variable
inappropriately.
This patch fixes that check, preventing shim from always stepping on the
sbat variable, and adds test cases to validate the correct logic.
Signed-off-by: Jan Setje-Eilers <jan.setjeeilers@oracle.com>
|
|
This moves the check for the SBAT variable's attributes and contents
into its own function, so that test cases can be written against it.
Signed-off-by: Jan Setje-Eilers <jan.setjeeilers@oracle.com>
|
|
Fix a couple of small off-by-one errors in the SBAT variable initial
setup and validation path.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This makes it so we can tell what it's actually doing and why.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This makes it so that if you build with ENABLE_SHIM_DEVEL, the SBAT we
use is named SBAT_DEVEL instead of SBAT, and it's expected to have
EFI_VARIABLE_RUNTIME_ACCESS set.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
A few cleanups:
- Ensure that the data we get from get_variable() is at least big enough
to actually be an SBAT variable
- Only try to delete if the variable is actually set
- Don't set the variable again if deleting it failed
- We don't actually need to get the size of the variable, allocate,
and then get the variable; get_variable() does the allocation for us.
- Don't compare the variable data when get_variable() failed
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
|
|
Signed-off-by: Peter Jones <pjones@redhat.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>
|
|
Fix a typo that inverted the check that validates that a CSV
column is not empty.
Signed-off-by: Thomas Frauendorfer | Miray Software <tf@miray.de>
|
|
This commit fixes the following gcc warnings for casting.
sbat.c: In function 'verify_single_entry':
sbat.c:157:13: error: pointer targets in passing argument 1 of 'strcmp' differ in signedness [-Werror=pointer-sign]
if (strcmp(entry->component_name, sbat_var_entry->component_name) == 0) {
^~~~~
In file included from Cryptlib/Include/string.h:15:0,
from sbat.c:7:
Cryptlib/Include/OpenSslSupport.h:299:16: note: expected 'const char *' but argument is of type 'const CHAR8 * {aka const unsigned char *}'
int strcmp (const char *, const char *);
^~~~~~
sbat.c:157:36: error: pointer targets in passing argument 2 of 'strcmp' differ in signedness [-Werror=pointer-sign]
if (strcmp(entry->component_name, sbat_var_entry->component_name) == 0) {
^~~~~~~~~~~~~~
In file included from Cryptlib/Include/string.h:15:0,
from sbat.c:7:
Cryptlib/Include/OpenSslSupport.h:299:16: note: expected 'const char *' but argument is of type 'const CHAR8 * {aka const unsigned char *}'
int strcmp (const char *, const char *);
^~~~~~
sbat.c:165:19: error: pointer targets in passing argument 1 of 'AsciiStrDecimalToUintn' differ in signedness [-Werror=pointer-sign]
sbat_gen = atoi(entry->component_generation);
^
Cryptlib/Include/OpenSslSupport.h:380:66: note: in definition of macro 'atoi'
#define atoi(nptr) AsciiStrDecimalToUintn(nptr)
^~~~
In file included from Cryptlib/Include/OpenSslSupport.h:21:0,
from Cryptlib/Include/string.h:15,
from sbat.c:7:
Cryptlib/Library/BaseLib.h:9:7: note: expected 'const char *' but argument is of type 'const CHAR8 * {aka const unsigned char *}'
UINTN AsciiStrDecimalToUintn(const char *String);
^~~~~~~~~~~~~~~~~~~~~~
In file included from Cryptlib/Include/string.h:15:0,
from sbat.c:7:
sbat.c:166:23: error: pointer targets in passing argument 1 of 'AsciiStrDecimalToUintn' differ in signedness [-Werror=pointer-sign]
sbat_var_gen = atoi(sbat_var_entry->component_generation);
^
Cryptlib/Include/OpenSslSupport.h:380:66: note: in definition of macro 'atoi'
#define atoi(nptr) AsciiStrDecimalToUintn(nptr)
^~~~
In file included from Cryptlib/Include/OpenSslSupport.h:21:0,
from Cryptlib/Include/string.h:15,
from sbat.c:7:
Cryptlib/Library/BaseLib.h:9:7: note: expected 'const char *' but argument is of type 'const CHAR8 * {aka const unsigned char *}'
UINTN AsciiStrDecimalToUintn(const char *String);
^~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [<builtin>: sbat.o] Error 1
Signed-off-by: Gary Lin <glin@suse.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This makes our SBAT variable parser use the generic CSV parser, and also
changes its API slightly to produce a more testable intermediate
interface.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This makes the .sbat section parser use parse_csv_data(). It also
re-names a couple of the structs, because they were still too easy to
get lost in.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This moves is_utf8_bom() to str.h, and also adds two #defines, UTF8_BOM
and UTF8_BOM_SIZE.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
"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>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Parse the SBAT [0] Version-Based Revocation Metadata that's contained in a
.sbat data section of the loaded PE binary. This information is used along
with data in a SBAT variable to determine if a EFI binary has been revoked.
[0]: https://github.com/rhboot/shim/blob/sbat/SBAT.md
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
The Secure Boot Advanced Targeting (SBAT) [0] is a Generation Number Based
Revocation mechanism that is meant to replace the DBX revocation file list.
Binaries must contain a .sbat data section that has a set entries, each of
them consisting of UTF-8 strings as comma separated values. Allow to embed
this information into the fwupd EFI binary at build time.
The SBAT metadata must contain at least two entries. One that defines the
SBAT version used and another one that defines the component generation.
This patch adds a sbat.csv that contains these two entries and downstream
users can override if additional entries are needed due changes that make
them diverge from upstream code and potentially add other vulnerabilities.
The same SBAT metadata is added to the fallback and MOK manager binaries
because these are built from the same shim source. These need to have SBAT
metadata as well to be booted if a .sbat section is mandatory.
[0]: https://github.com/rhboot/shim/blob/sbat/SBAT.md
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
|
