Parse SBAT variable data and perform verification #3
Conversation
Extracted information is used along with data extracted from binary with SBAT header metadata to perform a verification of binary. Verification is implemented only for binaries, booted directly through shim. shim_verify SBAT verification is not yet implemented, since certain binaries do not have SBAT metadata implemented yet ( Linux kernel ). https://github.com/rhboot/shim/blob/sbat/SBAT.md Signed-off-by: Alex Burmashev <alexander.burmashev@oracle.com>
There was a problem hiding this comment.
I've put quite a bit in-line, but two over-arching points:
- I think having an ad-hoc linked list here is not great, so I sent a PR earlier to add a generic one; it's in main now.
- There's a lot of minor formatting issues; "clang-format --style=file -i include/sbat.h sbat.c" should fix them up.
I don't know if you've used the linux-like list primitives before, so I did a bit of a first pass on using them to do the parsing at vathpela@6ca51d2 . I haven't done the changes to pe.c to make that work with it.
| @@ -22,6 +29,15 @@ struct sbat { | |||
| struct sbat_entry **entries; | |||
| }; | |||
|
|
|||
| struct sbat_var* parse_sbat_var(); | |||
| if (sbat.entries) | ||
| for (i = 0; i < sbat.size; i++) | ||
| FreePool(sbat.entries[i]); | ||
| if (var) { |
| static BOOLEAN is_utf8_bom(CHAR8 *buf) | ||
| { | ||
| unsigned char bom[] = { 0xEF,0xBB,0xBF }; | ||
| return !!CompareMem(buf, bom, MIN(sizeof(bom), sizeof(buf))); |
| @@ -120,4 +121,98 @@ EFI_STATUS parse_sbat(char *sbat_base, size_t sbat_size, char *buffer, | |||
| return efi_status; | |||
| } | |||
|
|
|||
| EFI_STATUS verify_sbat (struct sbat *sbat, struct sbat_var *sbat_var_root) | |||
There was a problem hiding this comment.
This can be static. Also please run "clang-format --style=file -i include/sbat.h sbat.c".
| /* atoi returns zero for failed conversion, so essentially | ||
| badly parsed component_generation will be treated as zero | ||
| */ | ||
| if (atoi(entry->component_generation) < atoi(sbat_var_entry->component_generation)) { |
There was a problem hiding this comment.
Hrm. Not having strtoul() available might be a good argument for saying we always start them at 1. @martinezjavier @jsetje, yall have thoughts?
There was a problem hiding this comment.
Sorry @vathpela, I missed this comment before. That's a good question, maybe we could add strtoul() to shim ? Or use the one provided by Cryptlib ?
| @@ -4,6 +4,7 @@ | |||
| */ | |||
|
|
|||
| #include "sbat.h" | |||
| #include <string.h> | |||
|
|
|||
| CHAR8 * | |||
| struct sbat_var* add_entry(struct sbat_var*, const CHAR8 *comp_gen, | ||
| const CHAR8 *comp_name_size, const CHAR8 *comp_name); | ||
|
|
||
| struct sbat_var* new_entry(const CHAR8 *comp_gen, const CHAR8 *comp_name_size, | ||
| const CHAR8 *comp_name); |
There was a problem hiding this comment.
I don't think these need to be exposed?
| struct sbat_var* add_entry(struct sbat_var *n, const CHAR8 *comp_gen, const CHAR8 *comp_name_size, | ||
| const CHAR8 *comp_name) |
There was a problem hiding this comment.
This can be static as well.
Also I'd really rather something like:
static int
add_entry(list_t *list, const char *const CHAR8 *comp_gen, const CHAR8 *comp_name_size,
const CHAR8 *comp_name)
which follows the -1 on error, 0 on success pattern. (See my broader comment on list_t.)
| start = get_sbat_field(start, end, &comp_gen, ','); | ||
|
|
||
| start = get_sbat_field(start, end, &comp_name_size, ','); | ||
|
|
||
| start = get_sbat_field(start, end, &comp_name, '\n'); |
There was a problem hiding this comment.
These all need to be error checked.
I think they should also all be looking for ',', never newline, as that allows us to treat trailing fields as comments, though it doesn't really make much difference for this specific usage due to our size constraints - but it would allow you to put it in a loop with a single error handler.
|
|
||
| dprint(L"component %a with generation %a\n",comp_name, comp_gen); | ||
|
|
||
| add_entry(nodename, comp_gen, comp_name_size, comp_name); |
For some reason when we try to ever use the builtins, even with the symbol there as a fallback, something goes horribly wrong somewhere around here: | (gdb) bt | #0 strcmp (s1=0x7d492359 "MD5", s2=0x7d492359 "MD5") at include/system/string.h:57 | #1 0x000000007d460419 in getrn (lh=lh@entry=0x7e081318, data=data@entry=0x7e084398, rhash=rhash@entry=0x7f7c9268) at crypto/lhash/lhash.c:415 | #2 0x000000007d46076e in lh_insert (lh=0x7e081318, data=data@entry=0x7e084398) at crypto/lhash/lhash.c:188 | #3 0x000000007d43e027 in OBJ_NAME_add (name=name@entry=0x7d492359 "MD5", type=type@entry=1, data=data@entry=0x7d4ad3a0 <md5_md> "\004") at crypto/objects/o_names.c:202 As much as I love a Sisyphean challenge, in the interest of not having bugs or time, this patch changes it to just not use them for anything other than guaranteeing our implementations have the exact same types as you would expect. Signed-off-by: Peter Jones <pjones@redhat.com>
Extracted information is used along with data extracted from binary with SBAT
header metadata to perform a verification of binary.
Verification is implemented only for binaries, booted directly through shim.
shim_verify SBAT verification is not yet implemented, since certain binaries
do not have SBAT metadata implemented yet ( Linux kernel ).
https://github.com/rhboot/shim/blob/sbat/SBAT.md
Signed-off-by: Alex Burmashev alexander.burmashev@oracle.com