| Age | Commit message (Collapse) | Author |
|---|
|
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>
|
