Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add libFuzzer support for csv.c and sbat.c #584

Merged
merged 3 commits into from
Jun 29, 2023
Merged

Add libFuzzer support for csv.c and sbat.c #584

merged 3 commits into from
Jun 29, 2023

Conversation

vathpela
Copy link
Contributor

@vathpela vathpela commented Jun 28, 2023 •
edited
Loading

shim takes several forms of input from several sources that are not necessarily trustworthy. As such, we need to take measures to validate that we don't have unacceptable results from bad inputs. One such measure is "fuzzing" the inputs which parse untrusted data by running them with randomized or partially randomized input.

This change adds such testing using clang's "libFuzzer" to our CSV parser and the parser for .sbat sections.

@vathpela vathpela force-pushed the libfuzzer branch 2 times, most recently from f4fa540 to 0bc13b8 Compare June 28, 2023 20:53
@vathpela
Add libFuzzer support for csv.c
0ef5e94 
shim takes several forms of input from several sources that are not
necessarily trustworthy.  As such, we need to take measures to validate
that we don't have unacceptable results from bad inputs.  One such
measure is "fuzzing" the inputs which parse untrusted data by running
them with randomized or partially randomized input.

This change adds such testing using clang's "libFuzzer" to our CSV
parser.  I've run this on 24-cores at 4GHz for half an hour, and so far
each fuzzer has converged on 79% coverage.  I expect the 21% that's not
getting covered are the EFI API mock interfaces we're building in from
test.c and similar.  So far no errors have been found, which is what was
expected since this particular API is being manually fuzzed with ~8kB of
/dev/urandom on every build since 2021-02-23.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela vathpela changed the title Add libFuzzer support for csv.c Add libFuzzer support for csv.c and sbat.c Jun 28, 2023
@vathpela
Fix a 1-byte memory leak in .sbat parsing.
028ed14 
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>
@vathpela
Add libFuzzer support to the .sbat parser.
2a2c41a 
shim takes several forms of input from several sources that are not
necessarily trustworthy.  As such, we need to take measures to validate
that we don't have unacceptable results from bad inputs.  One such
measure is "fuzzing" the inputs which parse untrusted data by running
them with randomized or partially randomized input.

This change adds such testing using clang's "libFuzzer" to our parser
for ".sbat" sections.  I've run it for about half an hour and so far it
found one memory leak, but no other errors.

Signed-off-by: Peter Jones <pjones@redhat.com>
jsetje
jsetje approved these changes Jun 28, 2023
View reviewed changes
Copy link
Collaborator

@jsetje jsetje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to have the generic "clean" and "all" in fuzz.mk?

Otherwise this looks ok, I do get noise from -print-multiarch on a system that doesn't support it, although I did confirm it's just noise.

@vathpela
Copy link
Contributor Author

Did you mean to have the generic "clean" and "all" in fuzz.mk?

Yeah - those wind up appending the build dependency, so e.g. clean will automatically rebuild fuzz-clean.

Otherwise this looks ok, I do get noise from -print-multiarch on a system that doesn't support it, although I did confirm it's just noise.

I don't think I'm going to fix "clang made this argument print a warning instead of silent when there's nothing to do" in this one, especially since you don't need to run that to ship it at all.

@vathpela vathpela merged commit e246812 into rhboot:main Jun 29, 2023
21 checks passed
@vathpela vathpela deleted the libfuzzer branch June 29, 2023 18:35
@dennis-tseng99
Copy link
Contributor

dennis-tseng99 commented Jul 9, 2023 •
edited
Loading

@vathpela Because .ascii directive expects zero or more string literals separated by commas (http://web.mit.edu/gnu/doc/html/as_7.html), rather than by space, could you change sbat_var.S file to something like fig-1 ? So that the compiling error(fig-2) can be avoided with clang when make fuzz. Thanks so much.

[fig-1]
image

[fig-2]
image

vathpela added a commit that referenced this pull request Jan 23, 2024
@vathpela
Bump version to 15.8
5914984 
What's changed
* Various CVE fixes:
  CVE-2023-40546 mok: fix LogError() invocation
  CVE-2023-40547 - avoid incorrectly trusting HTTP headers
  CVE-2023-40548 Fix integer overflow on SBAT section size on 32-bit system
  CVE-2023-40549 Authenticode: verify that the signature header is in bounds.
  CVE-2023-40550 pe: Fix an out-of-bound read in verify_buffer_sbat()
  CVE-2023-40551: pe-relocate: Fix bounds check for MZ binaries
* Add make infrastructure to set the NX_COMPAT flag by @vathpela in #530
* Make sbat_var.S parse right with buggy gcc/binutils by @vathpela in #535
* Drop invalid calls to CRYPTO_set_mem_functions by @nicholasbishop in #537
* pe: Align section size up to page size for mem attrs by @nicholasbishop in #539
* test-sbat: Fix exit code by @vathpela in #540
* pe: Add IS_PAGE_ALIGNED macro by @nicholasbishop in #541
* CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper by @nicholasbishop in #546
* Don't loop forever in load_certs() with buggy firmware by @rmetrich in #547
* Block Debian grub binaries with SBAT < 4 by @steve-mcintyre in #550
* Shim unable to locate grubx64 in PXE boot mode when grubx64 is stored in a different file path by @Alberto-Perez-Guevara in #551
* Further improve load_certs() for non-compliant drivers/firmwares by @pbatard in #560
* pe: only process RelocDir->Size of reloc section by @mikebeaton in #562
* Rename 'msecs' to 'usecs' to avoid potential confusion by @aronowski in #563
* Optionally allow to keep shim protocol installed by @bluca in #565
* SBAT-related documents formatting and spelling by @aronowski in #566
* Add SbatLevel_Variable.txt to document the various revocations by @jsetje in #569
* Add a security contact email address in README.md by @vathpela in #572
* Use -Wno-unused-but-set-variable for Cryptlib and OpenSSL by @vathpela in #576
* mok: fix LogError() invocation by @vathpela in #577
* Minor housekeeping by @vathpela in #578
* Test ImageAddress() by @vathpela in #579
* FreePages() is used to return memory allocated by AllocatePages() by @dennis-tseng99 in #580
* Size should minus 1 when calculating 'RelocBaseEnd' by @jsetje in #581
* Verify signature before verifying sbat levels by @jsetje in #583
* Add libFuzzer support for csv.c and sbat.c by @vathpela in #584
* mok: Avoid underflow in maximum variable size calculation by @alpernebbi in #587
* Housekeeping by @vathpela in #605

Signed-off-by: Peter Jones <pjones@redhat.com>
brianredbeard pushed a commit to brianredbeard/redhat-efi-boot-shim that referenced this pull request Feb 22, 2024
@vathpela @brianredbeard
Bump version to 15.8
891edcf 
What's changed
* Various CVE fixes:
  CVE-2023-40546 mok: fix LogError() invocation
  CVE-2023-40547 - avoid incorrectly trusting HTTP headers
  CVE-2023-40548 Fix integer overflow on SBAT section size on 32-bit system
  CVE-2023-40549 Authenticode: verify that the signature header is in bounds.
  CVE-2023-40550 pe: Fix an out-of-bound read in verify_buffer_sbat()
  CVE-2023-40551: pe-relocate: Fix bounds check for MZ binaries
* Add make infrastructure to set the NX_COMPAT flag by @vathpela in rhboot#530
* Make sbat_var.S parse right with buggy gcc/binutils by @vathpela in rhboot#535
* Drop invalid calls to CRYPTO_set_mem_functions by @nicholasbishop in rhboot#537
* pe: Align section size up to page size for mem attrs by @nicholasbishop in rhboot#539
* test-sbat: Fix exit code by @vathpela in rhboot#540
* pe: Add IS_PAGE_ALIGNED macro by @nicholasbishop in rhboot#541
* CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper by @nicholasbishop in rhboot#546
* Don't loop forever in load_certs() with buggy firmware by @rmetrich in rhboot#547
* Block Debian grub binaries with SBAT < 4 by @steve-mcintyre in rhboot#550
* Shim unable to locate grubx64 in PXE boot mode when grubx64 is stored in a different file path by @Alberto-Perez-Guevara in rhboot#551
* Further improve load_certs() for non-compliant drivers/firmwares by @pbatard in rhboot#560
* pe: only process RelocDir->Size of reloc section by @mikebeaton in rhboot#562
* Rename 'msecs' to 'usecs' to avoid potential confusion by @aronowski in rhboot#563
* Optionally allow to keep shim protocol installed by @bluca in rhboot#565
* SBAT-related documents formatting and spelling by @aronowski in rhboot#566
* Add SbatLevel_Variable.txt to document the various revocations by @jsetje in rhboot#569
* Add a security contact email address in README.md by @vathpela in rhboot#572
* Use -Wno-unused-but-set-variable for Cryptlib and OpenSSL by @vathpela in rhboot#576
* mok: fix LogError() invocation by @vathpela in rhboot#577
* Minor housekeeping by @vathpela in rhboot#578
* Test ImageAddress() by @vathpela in rhboot#579
* FreePages() is used to return memory allocated by AllocatePages() by @dennis-tseng99 in rhboot#580
* Size should minus 1 when calculating 'RelocBaseEnd' by @jsetje in rhboot#581
* Verify signature before verifying sbat levels by @jsetje in rhboot#583
* Add libFuzzer support for csv.c and sbat.c by @vathpela in rhboot#584
* mok: Avoid underflow in maximum variable size calculation by @alpernebbi in rhboot#587
* Housekeeping by @vathpela in rhboot#605

Signed-off-by: Peter Jones <pjones@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsetje jsetje jsetje approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vathpela @dennis-tseng99 @jsetje