From e7508839afdd59619342a25c0c39c11fa934fd72 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Fri, 23 Sep 2022 09:47:26 +0200 Subject: [PATCH 1/4] fuzz-bcd: Do not include bcd.c This is not needed anymore, so do it the proper way now. --- src/boot/efi/fuzz-bcd.c | 3 +-- src/boot/efi/meson.build | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/boot/efi/fuzz-bcd.c b/src/boot/efi/fuzz-bcd.c index e56183c4f0..6d76533e8f 100644 --- a/src/boot/efi/fuzz-bcd.c +++ b/src/boot/efi/fuzz-bcd.c @@ -1,12 +1,11 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #include "alloc-util.h" +#include "bcd.h" #include "fd-util.h" #include "fuzz.h" #include "utf8.h" -#include "bcd.c" - int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { _cleanup_free_ void *p = NULL; diff --git a/src/boot/efi/meson.build b/src/boot/efi/meson.build index 22d28e4197..3bb0827f8c 100644 --- a/src/boot/efi/meson.build +++ b/src/boot/efi/meson.build @@ -419,7 +419,7 @@ if efi_arch[1] in ['ia32', 'x86_64', 'arm', 'aarch64'] 'HAVE_ZSTD'], ] fuzzers += [ - [files('fuzz-bcd.c', 'efi-string.c')], + [files('fuzz-bcd.c', 'bcd.c', 'efi-string.c')], ] endif From 4b8eb86360b106afbc69cc014e8f1a15017c9d1f Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Fri, 23 Sep 2022 09:54:03 +0200 Subject: [PATCH 2/4] fuzz: Introduce DO_NOT_OPTIMIZE The compiler may decide computations like these are not doing anything and decide to optimize them away. This would defeat the whole fuzzing exercise. This macro will force the compiler to materialize the value no matter what. It should be less prone to accidents compared to using log functions, which would either slow things down or still optimize the value away (or simply move it into the if branch the log macros create). The benefit over assert_se would be that no requirement is made on the value itself. If we are fine getting a string of any size (including zero), an assert_se would either create a noisy compiler warning about conditions that would alawys be met or yet again optimize the whole thing away. --- src/boot/efi/fuzz-bcd.c | 8 ++------ src/fuzz/fuzz.h | 3 +++ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/boot/efi/fuzz-bcd.c b/src/boot/efi/fuzz-bcd.c index 6d76533e8f..297b71f60c 100644 --- a/src/boot/efi/fuzz-bcd.c +++ b/src/boot/efi/fuzz-bcd.c @@ -2,7 +2,6 @@ #include "alloc-util.h" #include "bcd.h" -#include "fd-util.h" #include "fuzz.h" #include "utf8.h" @@ -13,14 +12,11 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { if (outside_size_range(size, 0, 100*1024)) return 0; - if (!getenv("SYSTEMD_LOG_LEVEL")) - log_set_max_level(LOG_CRIT); - p = memdup(data, size); assert_se(p); char16_t *title = get_bcd_title(p, size); - if (title) - (void) char16_strlen(title); + /* If we get something, it must be NUL-terminated, but an empty string is still valid! */ + DO_NOT_OPTIMIZE(title && char16_strlen(title)); return 0; } diff --git a/src/fuzz/fuzz.h b/src/fuzz/fuzz.h index 04c438edaf..a7d3a89fe2 100644 --- a/src/fuzz/fuzz.h +++ b/src/fuzz/fuzz.h @@ -27,3 +27,6 @@ static inline bool outside_size_range(size_t size, size_t lower, size_t upper) { return FUZZ_USE_SIZE_LIMIT; return false; } + +/* Force value to not be optimized away. */ +#define DO_NOT_OPTIMIZE(value) ({ asm volatile("" : : "g"(value) : "memory"); }) From ac37f132a1f5f00d35632d2ae1618b94d993ac61 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Fri, 23 Sep 2022 10:07:25 +0200 Subject: [PATCH 3/4] fuzz: Add fuzzer for some efi string functions --- src/boot/efi/fuzz-efi-string.c | 40 ++++++++++++++++++++++++++++++++++ src/boot/efi/meson.build | 1 + 2 files changed, 41 insertions(+) create mode 100644 src/boot/efi/fuzz-efi-string.c diff --git a/src/boot/efi/fuzz-efi-string.c b/src/boot/efi/fuzz-efi-string.c new file mode 100644 index 0000000000..4d4d01f026 --- /dev/null +++ b/src/boot/efi/fuzz-efi-string.c @@ -0,0 +1,40 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "alloc-util.h" +#include "efi-string.h" +#include "fuzz.h" +#include "utf8.h" + +static char16_t *memdup_str16(const uint8_t *data, size_t size) { + char16_t *ret = memdup(data, size); + assert_se(ret); + ret[size / sizeof(char16_t) - 1] = '\0'; + return ret; +} + +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { + if (outside_size_range(size, sizeof(size_t), 1024 * 1024)) + return 0; + + size_t len, len2; + memcpy(&len, data, sizeof(len)); + data += sizeof(len); + size -= sizeof(len); + + len2 = size - len; + if (len > size || len < sizeof(char16_t) || len2 < sizeof(char16_t)) + return 0; + + const char *tail8 = NULL; + _cleanup_free_ char *str8 = ASSERT_SE_PTR(memdup_suffix0(data, size)); + DO_NOT_OPTIMIZE(parse_number8(str8, &(uint64_t){ 0 }, size % 2 == 0 ? NULL : &tail8)); + + const char16_t *tail16 = NULL; + _cleanup_free_ char16_t *str16 = memdup_str16(data, size); + DO_NOT_OPTIMIZE(parse_number16(str16, &(uint64_t){ 0 }, size % 2 == 0 ? NULL : &tail16)); + + _cleanup_free_ char16_t *pattern = memdup_str16(data, len), *haystack = memdup_str16(data + len, len2); + DO_NOT_OPTIMIZE(efi_fnmatch(pattern, haystack)); + + return 0; +} diff --git a/src/boot/efi/meson.build b/src/boot/efi/meson.build index 3bb0827f8c..e0cd4ebad9 100644 --- a/src/boot/efi/meson.build +++ b/src/boot/efi/meson.build @@ -420,6 +420,7 @@ if efi_arch[1] in ['ia32', 'x86_64', 'arm', 'aarch64'] ] fuzzers += [ [files('fuzz-bcd.c', 'bcd.c', 'efi-string.c')], + [files('fuzz-efi-string.c', 'efi-string.c')], ] endif From af4b8cef3810e185ec4a38d135006cd7de51eb5c Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Sat, 24 Sep 2022 13:33:10 +0200 Subject: [PATCH 4/4] boot: Make efi_fnmatch non-backtracking --- src/boot/efi/efi-string.c | 60 +++++++++++++++++++++++++--------- src/boot/efi/test-efi-string.c | 16 +++++++++ 2 files changed, 61 insertions(+), 15 deletions(-) diff --git a/src/boot/efi/efi-string.c b/src/boot/efi/efi-string.c index 5c05dfeb06..b877c6f224 100644 --- a/src/boot/efi/efi-string.c +++ b/src/boot/efi/efi-string.c @@ -138,13 +138,11 @@ DEFINE_STRCHR(char16_t, strchr16); DEFINE_STRNDUP(char, xstrndup8, strnlen8); DEFINE_STRNDUP(char16_t, xstrndup16, strnlen16); -/* Patterns are fnmatch-compatible (with reduced feature support). */ -static bool efi_fnmatch_internal(const char16_t *p, const char16_t *h, int max_depth) { +static bool efi_fnmatch_prefix(const char16_t *p, const char16_t *h, const char16_t **ret_p, const char16_t **ret_h) { assert(p); assert(h); - - if (max_depth == 0) - return false; + assert(ret_p); + assert(ret_h); for (;; p++, h++) switch (*p) { @@ -166,17 +164,12 @@ static bool efi_fnmatch_internal(const char16_t *p, const char16_t *h, int max_d break; case '*': - /* No need to recurse for consecutive '*'. */ + /* Point ret_p at the remainder of the pattern. */ while (*p == '*') p++; - - for (; *h != '\0'; h++) - /* Try matching haystack with remaining pattern. */ - if (efi_fnmatch_internal(p, h, max_depth - 1)) - return true; - - /* End of haystack. Pattern needs to be empty too for a match. */ - return *p == '\0'; + *ret_p = p; + *ret_h = h; + return true; case '[': if (*h == '\0') @@ -236,8 +229,45 @@ static bool efi_fnmatch_internal(const char16_t *p, const char16_t *h, int max_d } } +/* Patterns are fnmatch-compatible (with reduced feature support). */ bool efi_fnmatch(const char16_t *pattern, const char16_t *haystack) { - return efi_fnmatch_internal(pattern, haystack, 32); + /* Patterns can be considered as simple patterns (without '*') concatenated by '*'. By doing so we + * simply have to make sure the very first simple pattern matches the start of haystack. Then we just + * look for the remaining simple patterns *somewhere* within the haystack (in order) as any extra + * characters in between would be matches by the '*'. We then only have to ensure that the very last + * simple pattern matches at the actual end of the haystack. + * + * This means we do not need to use backtracking which could have catastrophic runtimes with the + * right input data. */ + + for (bool first = true;;) { + const char16_t *pattern_tail = NULL, *haystack_tail = NULL; + bool match = efi_fnmatch_prefix(pattern, haystack, &pattern_tail, &haystack_tail); + if (first) { + if (!match) + /* Initial simple pattern must match. */ + return false; + if (!pattern_tail) + /* No '*' was in pattern, we can return early. */ + return true; + first = false; + } + + if (pattern_tail) { + assert(match); + pattern = pattern_tail; + haystack = haystack_tail; + } else { + /* If we have a match this must be at the end of the haystack. Note that + * efi_fnmatch_prefix compares the NUL-bytes at the end, so we cannot match the end + * of pattern in the middle of haystack). */ + if (match || *haystack == '\0') + return match; + + /* Match one character using '*'. */ + haystack++; + } + } } #define DEFINE_PARSE_NUMBER(type, name) \ diff --git a/src/boot/efi/test-efi-string.c b/src/boot/efi/test-efi-string.c index 0bfb564e3d..2b2359fe5c 100644 --- a/src/boot/efi/test-efi-string.c +++ b/src/boot/efi/test-efi-string.c @@ -345,6 +345,11 @@ TEST(efi_fnmatch) { TEST_FNMATCH_ONE("**", "abcd", true); TEST_FNMATCH_ONE("*b*", "abcd", true); TEST_FNMATCH_ONE("abc*d", "abc", false); + TEST_FNMATCH_ONE("start*end", "startend", true); + TEST_FNMATCH_ONE("start*end", "startendend", true); + TEST_FNMATCH_ONE("start*end", "startenddne", false); + TEST_FNMATCH_ONE("start*end", "startendstartend", true); + TEST_FNMATCH_ONE("start*end", "starten", false); TEST_FNMATCH_ONE("*.conf", "arch.conf", true); TEST_FNMATCH_ONE("debian-*.conf", "debian-wheezy.conf", true); TEST_FNMATCH_ONE("debian-*.*", "debian-wheezy.efi", true); @@ -369,6 +374,17 @@ TEST(efi_fnmatch) { TEST_FNMATCH_ONE("[b]", "z-a", false); TEST_FNMATCH_ONE("[a\\-z]", "b", false); TEST_FNMATCH_ONE("?a*b[.-0]c", "/a/b/c", true); + TEST_FNMATCH_ONE("debian-*-*-*.*", "debian-jessie-2018-06-17-kernel-image-5.10.0-16-amd64.efi", true); + + /* These would take forever with a backtracking implementation. */ + TEST_FNMATCH_ONE( + "a*b*c*d*e*f*g*h*i*j*k*l*m*n*o*p*q*r*s*t*u*v*w*x*y*z*", + "aaaabbbbccccddddeeeeffffgggghhhhiiiijjjjkkkkllllmmmmnnnnooooppppqqqqrrrrssssttttuuuuvvvvwwwwxxxxyyyy", + false); + TEST_FNMATCH_ONE( + "a*b*c*d*e*f*g*h*i*j*k*l*m*n*o*p*q*r*s*t*u*v*w*x*y*z*", + "aaaabbbbccccddddeeeeffffgggghhhhiiiijjjjkkkkllllmmmmnnnnooooppppqqqqrrrrssssttttuuuuvvvvwwwwxxxxyyyyzzzz!!!!", + true); } TEST(parse_number8) {