Fix several stack buffer overflows and pointer overflows

[skeeto]

Several overflows may occur in switch case '[' when the input pattern contains many escaped characters. The added backslashes leave too little space in the output pattern when processing nested brackets such that the remaining input length exceeds the output capacity. Therefore all these concatenations must also be checked.

The ADD_CHAR was missed in 41281ea (#87). The switch can exit exactly at capacity, leaving no room for the finishing '$', causing an overflow.

In STRING_CAT, the end pointer is positioned one past the end of the destination, and it is undefined behavior to compute an address beyond the end pointer, including for comparisons, even temporarily. The UB occurs exactly when buffer overflow would have occurred, so the buffer overflow check could be optimized away by compilers. Even if this wasn't the case, the check could produce a false negative if the computed address overflowed the address space, which is, after all, why the C standard doesn't define behavior in the first place.

The fix is simple: Check using sizes, not addresses. The explicit cast suppresses warnings around signed-unsigned comparisons. It would also be worthwhile to assert(end > p), which would have caught some of these issues sooner, but there is no established assertion discipline and I didn't want to introduce it here.

---

These issues were discovered with this quick-and-dirty afl fuzz target:

#define _GNU_SOURCE
#define PCRE2_CODE_UNIT_WIDTH 8
#define UNIX
#define EC_VERSION_SUFFIX ""
#define EC_VERSION_MAJOR 0
#define EC_VERSION_MINOR 0
#define EC_VERSION_PATCH 0
#include "src/lib/ec_glob.c"
#include "src/lib/editorconfig.c"
#include "src/lib/ini.c"
#include "src/lib/misc.c"
#include <unistd.h>
#include <sys/mman.h>

__AFL_FUZZ_INIT();

int main(void)
{
    __AFL_INIT();
    int fd = memfd_create("fuzz", 0);
    unsigned char *buf = __AFL_FUZZ_TESTCASE_BUF;
    while (__AFL_LOOP(10000)) {
        int len = __AFL_FUZZ_TESTCASE_LEN;
        ftruncate(fd, 0);
        write(fd, buf, len);
        struct editorconfig_handle ec = {0};
        ec.conf_file_name = (char[]){'0'+fd, 0};
        editorconfig_parse("/proc/self/fd/", &ec);
    }
}

The interface only accepts input through a file path, which makes testing a bit more complex. memfd_create() avoids writing fuzz input through a real file and allows for parallel fuzzing because the virtual file path is private. I built and ran it like so:

$ echo >include/config.h
$ afl-gcc-fast -Iinclude -g3 -fsanitize=address,undefined fuzz.c -lpcre2-8
$ afl-fuzz -i tests/parser/comments.in -o fuzzout ./a.out

overflow-input.txt is an input that triggers all overflows addressed by these commits.

[xuhdev]

Thanks for the contribution and detailed commit message! The PR looks good, only two minor comments.

...the end pointer is positioned one past the end of the destination, and it is undefined behavior to compute an address beyond the end pointer...

• Do you have a reference to this?
• Is using the address one past the end of an array defined behavior?

[skeeto]

- Do you have a reference to this? - Is using the address one past the end of an array defined behavior?

C99 6.5.6-8 answers both at once:

If both the pointer operand and the result point to elements of the same array object, or one past the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined.

One past the end makes so many things simpler, including cases like this, which is why it's permitted despite its special cases. Some languages do not allow one-past-the-end pointers, and it's surprising how much more complicated, and error prone, programs become as a result. I've made the two suggestions and updated the second commit message to match. As the first assertion, I didn't want to make a decision about dealing with -DNDEBUG in the build. (This particular case is a great example of why sizes ought to be signed, and why I use ptrdiff_t for memory quantities in my own programs. Without edge cases near zero, many problems evaporate. The maximum object size is PTRDIFF_MAX either implicitly or explicitly (e.g. in GCC and Clang), so a strlen result, and especially this one, cannot exceed it despite returning size_t.)

[xuhdev]

Thanks for the detailed explanation!