Add efisecdb tooling #184

vathpela · 2021-11-15T23:10:55Z

Note that this is on top of #187 .

vathpela · 2021-11-15T23:29:17Z

This test is going to fail until I re-generate our test images with mandoc included.

frozencemetery · 2021-11-16T18:02:43Z

Unless there's a reason not to, I'd prefer to wait for the tests to pass before reviewing. (I have submitted review for the commits that are also part of #184.)

vathpela · 2021-11-16T22:05:21Z

Unless there's a reason not to, I'd prefer to wait for the tests to pass before reviewing. (I have submitted review for the commits that are also part of #184.)

That's why it's a draft, yeah ;)

vathpela · 2021-12-08T18:04:04Z

(There's still some potential problems here coverity found, so I've put this back on draft for now while I address those.)

frozencemetery

"secdb: change secdb algorithm sort order": I'm having trouble parsing this commit message, which is especially unfortunate since I don't understand the rationale for the change.
"efisecdb: add efisecdb": given how massive this commit is, I'll need to review it again after changes (sigh). You might as well have given it the message "Now draw the rest of the owl." :P
"Add security types/guids and signature database iterators": I think this one is generally okay, but want to think about it and review it again.
Style: I think usually break after the operator (&&, +, etc.), right? Or does this codebase go the other way? (Right now it seems to do both.)
No space after sizeof. (Or space after sizeof, but again, one, not both, please.)

Further stuff inline.

src/x509.h

src/error.c

src/util.h

src/secdb.c

src/esl-iter.c

src/makeguids.c

src/makeguids.h

vathpela · 2022-01-07T21:56:28Z

"secdb: change secdb algorithm sort order": I'm having trouble parsing this commit message, which is especially unfortunate since I don't understand the rationale for the change.

I've re-written it, hopefully this helps.

* "efisecdb: add efisecdb": given how massive this commit is, I'll need to review it again after changes (sigh).  You might as well have given it the message "Now draw the rest of the owl." :P

Yeah, that's why I separated out libefisec and the iterators. If you have suggestions for how to break that up more, I'm open to them.

* "Add security types/guids and signature database iterators": I think this one is generally okay, but want to think about it and review it again.

Okay.

* Style: I think usually break _after_ the operator (&&, +, etc.), right?  Or does this codebase go the other way?  (Right now it seems to do both.)

TBH my usual style is "whichever makes the line-wrapping least crazy, so long as it leaves it clear what each line is doing".

* No space after sizeof.  (Or space after sizeof, but again, one, not both, please.)

Fair enough; I've fixed the ones that are added (or adjacent) here.

frozencemetery

Thank you for your patience.

cov-emit, the part of coverity that does the compile phase and emits the report for analysis, whines thusly: > COV_TRANSLATE EXPANDED ARGS: "-Og" "-g3" "-Wall" "-Wextra" "-Werror" "-std=gnu11" "-funsigned-char" "-fvisibility=hidden" "-specs=src/include/gcc.specs" "-fno-merge-constants" "-fPIC" "-DLIBEFIVAR_VERSION=37" "-D_GNU_SOURCE" "-Isrc/include/" "-c" "-o" "linux.o" "linux.c" > [WARNING] Unsupported version: 1.2.1 > [STATUS] Compiling linux.c > cov-emit --dir=cov-int --ignore_path=/tmp/cov-pjones/78d2689a1daa6aeae021a68abca9bbb7/cov-configure --ignore_path=/tmp/cov-pjones/78d2689a1daa6aeae021a68abca9bbb7/cov-pjones/c897c3f20c04a6d04529cac42ceca900 --pre_preinclude cov-int/emit/random/config/ae451ed419029c6313a7b619272897a4/gcc-config-0/coverity-macro-compat.h --pre_preinclude cov-int/emit/random/config/ae451ed419029c6313a7b619272897a4/gcc-config-0/coverity-compiler-compat.h --add_type_modifier=__coverity___fpreg --add_type_modifier=__coverity_decimal --add_type_modifier=__coverity_float --add_type_modifier=__coverity_floatx --no_predefined_feature_test_macros --no_stdarg_builtin --no_predefined_cplusplus -w --no_predefines --comp_ver 1.2.1 --char_bit_size=8 --restrict --gnu_carriage_return_line_terminator --no_multiline_string --no_trigraphs --ignore_calling_convention --c11 --enable_80bit_float --enable_128bit_float --allow__bool --macro_stack_pragmas --inline_keyword --has_include_macro --has_include_next_macro --has_cpp_attribute_macro --no_predefines --c --no_builtin_emulation --ppp_translator "replace%#\s*ident\s*([^;]*);%#ident $1" --gcc --c11 --gnu_version 10201 --unsigned_chars --no_trigraphs --uliterals -Isrc/include/ --sys_include /usr/lib/gcc/x86_64-redhat-linux/11/include --sys_include /usr/local/include --sys_include /usr/include -DLIBEFIVAR_VERSION=37 -D_GNU_SOURCE -D__OPTIMIZE__ --type_sizes=e16Pdlx8fi4s2 --type_alignments=e16Pdlx8fi4s2 --size_t_type=m --wchar_t_type=i --ptrdiff_t_type=l linux.c > "linux.c", line 483: warning #1234: a variable-length array is not allowed > inside of a statement expression > rc = find_device_file(&filepath, "driver", "block/%s", dev->disk_name); > ^ Really not sure *why*, with -std=gnu11, this wouldn't be allowed - gcc doesn't complain about it at all. But I'm tired of looking at this when writing workarounds for /other/ coverity bugs. With that in mind, this patch switches that to use alloca() instead. It also adds several #include statements to the linux.h header, to make it independently parsable by syntax checkers. Signed-off-by: Peter Jones <pjones@redhat.com>

Signed-off-by: Peter Jones <pjones@redhat.com>

Coverity correctly noticed that sometimes we're plausibly assigning a uint32_t to a spot on the stack that's only a uint8_t, which isn't great. This patch adds a new union type for the partition signature, and takes out the broken type-casting that hides the error. Unfortunately it still has to leave cast in place on the efidp_make_hd() call, as that's part of our ABI. Signed-off-by: Peter Jones <pjones@redhat.com>

Once upon a time I made our debug logging use memfd. That was pointless, and means we need a newer kernel and libc than we otherwise might. This takes that out. Signed-off-by: Peter Jones <pjones@redhat.com>

In cff88dd, the ignore line for efivar moved from the topdir to the src/ subdirectory, but as a result became not a fully qualified path. This means it accidentally also ignores /src/include/efivar, which is not intended. This adds the / to anchor the path again. Signed-off-by: Peter Jones <pjones@redhat.com>

This adds the build infrastructure and very basic parts of libefisec, without actually including any functionality. Signed-off-by: Peter Jones <pjones@redhat.com>

This adds most of the authenticode related data structures and definitions. A few of these structures aren't specified terribly well in terms of packing and alignment, so where possible that's noted in comments and I've followed what *seems* to be industry practice. Signed-off-by: Peter Jones <pjones@redhat.com>

This adds a helper to crack open an x509 sequence and figure out its size. Signed-off-by: Peter Jones <pjones@redhat.com>

In efisecdb, we're going to need the ability to use the EFI_SIGNATURE_LIST and EFI_SIGNATURE_DATA data structures. This adds their declarations and iterators for them to libefisec. Signed-off-by: Peter Jones <pjones@redhat.com>

This adds several more trivial helpers: - global page_size that's automatically set up - xfree() that keeps you from reusing a pointer you've freed. - xclose() that keeps you from accidentally reusing a dead file descriptor - pointer list type and iterators Signed-off-by: Peter Jones <pjones@redhat.com>

log_() doesn't strictly need to be a macro, and making it one makes gcc analyzer output harder to read, so make it a real function. Signed-off-by: Peter Jones <pjones@redhat.com>

show_errors() will be used in utilities other than efivar, so it needs to be in shared code. This patch moves it to util.h, as well as changing it to use the verbosity setting API that libefivar provides. Right now it can't live in a .c file unless that file /and error.c/ are linked into makeguids, which currently results in dependency loops that break the build, so it's still in util.h . Signed-off-by: Peter Jones <pjones@redhat.com>

This converts some of the cases where we have a space after "sizeof" to not do that. Signed-off-by: Peter Jones <pjones@redhat.com>

This allows us to re-use the guids.txt parsing in utilities we're building, not just in makeguids. Signed-off-by: Peter Jones <pjones@redhat.com>

This adds a new utility, efisecdb, used to manipulate databases with the structure of UEFI's db, dbx, and KEK databases. Signed-off-by: Peter Jones <pjones@redhat.com>

There is an issue on some systems when creating EFI security database files, which is that the maximum variable size[0] turns out to be quite small. Because of this, there's a need to separate entries by class and add the most important ones first. This changes our sort order such that (by default) the most important thing comes first, and that is usually any present certificate, followed by any sha256 cert TBS hashes, followed by any individual sha256 hashes. [0] i.e., when you call: BS->QueryVariableInfo(EFI_VARIABLE_BOOTSERVICE_ACCESS, &max_storage_sz, &remaining_sz, &max_var_sz) you often get max_var_sz in the 2-page to 3-page range, which is pretty stingy for a big list of hashes plus a few X.509 certificates. Signed-off-by: Peter Jones <pjones@redhat.com>

Everybody loves documentation. Signed-off-by: Peter Jones <pjones@redhat.com>

vathpela requested a review from frozencemetery November 15, 2021 23:10

vathpela mentioned this pull request Nov 15, 2021

More misc fixes #185

Merged

vathpela force-pushed the security-features branch 2 times, most recently from 6cac166 to cb85b5a Compare November 16, 2021 22:02

vathpela force-pushed the security-features branch 15 times, most recently from f98bc57 to 0d9bc77 Compare November 18, 2021 17:35

frozencemetery force-pushed the security-features branch from 0d9bc77 to b28771f Compare November 22, 2021 21:51

vathpela marked this pull request as ready for review December 7, 2021 21:58

vathpela marked this pull request as draft December 8, 2021 18:03

vathpela force-pushed the security-features branch 4 times, most recently from 7c7468d to 8054502 Compare December 9, 2021 15:49

vathpela force-pushed the security-features branch 2 times, most recently from cdf93e9 to 67e26fd Compare December 9, 2021 18:42

vathpela marked this pull request as ready for review December 9, 2021 18:47

vathpela mentioned this pull request Dec 9, 2021

Work around coverity silliness. #191

Closed

frozencemetery requested changes Dec 10, 2021

View reviewed changes

vathpela force-pushed the security-features branch 2 times, most recently from d7f0d2f to 6a4569a Compare January 7, 2022 21:38

vathpela force-pushed the security-features branch from 6a4569a to 86f9a3d Compare January 7, 2022 21:56

frozencemetery force-pushed the security-features branch from 86f9a3d to 620af55 Compare January 10, 2022 18:35

frozencemetery approved these changes Jan 10, 2022

View reviewed changes

vathpela added 17 commits January 10, 2022 13:36

hexdump: minor deadcode fix

88adb34

Signed-off-by: Peter Jones <pjones@redhat.com>

dbglog: there was no reason to use memfd

e79089d

Once upon a time I made our debug logging use memfd. That was pointless, and means we need a newer kernel and libc than we otherwise might. This takes that out. Signed-off-by: Peter Jones <pjones@redhat.com>

Add the beginnings of our security library

672c36b

This adds the build infrastructure and very basic parts of libefisec, without actually including any functionality. Signed-off-by: Peter Jones <pjones@redhat.com>

Add an x509 sequence size helper

5006008

This adds a helper to crack open an x509 sequence and figure out its size. Signed-off-by: Peter Jones <pjones@redhat.com>

Add security types/guids and signature database iterators

f71c199

In efisecdb, we're going to need the ability to use the EFI_SIGNATURE_LIST and EFI_SIGNATURE_DATA data structures. This adds their declarations and iterators for them to libefisec. Signed-off-by: Peter Jones <pjones@redhat.com>

util.h: make log_() a proper function

a7c26f8

log_() doesn't strictly need to be a macro, and making it one makes gcc analyzer output harder to read, so make it a real function. Signed-off-by: Peter Jones <pjones@redhat.com>

Minor whitespace cleanups.

c33de8f

This converts some of the cases where we have a space after "sizeof" to not do that. Signed-off-by: Peter Jones <pjones@redhat.com>

Make the guids.txt parser available outside of makeguids.c

6a293a3

This allows us to re-use the guids.txt parsing in utilities we're building, not just in makeguids. Signed-off-by: Peter Jones <pjones@redhat.com>

efisecdb: add efisecdb

d917870

This adds a new utility, efisecdb, used to manipulate databases with the structure of UEFI's db, dbx, and KEK databases. Signed-off-by: Peter Jones <pjones@redhat.com>

Add a better man page for efisecdb

bec837e

Everybody loves documentation. Signed-off-by: Peter Jones <pjones@redhat.com>

frozencemetery force-pushed the security-features branch from 620af55 to bec837e Compare January 10, 2022 18:36

frozencemetery merged commit bec837e into rhboot:main Jan 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add efisecdb tooling #184

Add efisecdb tooling #184

vathpela commented Nov 15, 2021 •

edited

Loading

vathpela commented Nov 15, 2021

frozencemetery commented Nov 16, 2021 •

edited

Loading

vathpela commented Nov 16, 2021

vathpela commented Dec 8, 2021

frozencemetery left a comment

vathpela commented Jan 7, 2022

frozencemetery left a comment

Add efisecdb tooling #184

Add efisecdb tooling #184

Conversation

vathpela commented Nov 15, 2021 • edited Loading

vathpela commented Nov 15, 2021

frozencemetery commented Nov 16, 2021 • edited Loading

vathpela commented Nov 16, 2021

vathpela commented Dec 8, 2021

frozencemetery left a comment

Choose a reason for hiding this comment

vathpela commented Jan 7, 2022

frozencemetery left a comment

Choose a reason for hiding this comment

vathpela commented Nov 15, 2021 •

edited

Loading

frozencemetery commented Nov 16, 2021 •

edited

Loading