heap-buffer-overflow (WRITE of size 1) in string_copy()

[geeknik]

HackerOne triage said this bug was out of scope because it requires "physical i can touch your server" access in order to exploit. I believe they are wrong in their assessment, however, there is no point trying to change their mind and this issue probably needs to be fixed, so here I am.

Describe the bug
We discovered a heap-buffer-overflow in this chunk of code (twemproxy/src/nc_string.c:96):

string_copy(struct string *dst, const uint8_t *src, uint32_t srclen)
{
    ASSERT(dst->len == 0 && dst->data == NULL);
    ASSERT(src != NULL && srclen != 0);

    dst->data = nc_strndup(src, srclen + 1);
    if (dst->data == NULL) {
        return NC_ENOMEM;
    }

    dst->len = srclen;
    dst->data[dst->len] = '\0';

    return NC_OK;
}

To Reproduce

1. compile with clang-12 + address sanitizer
2. run ./nutcracker -c poc.yml

poc.yml

0:
 0:
 - "\0"

Expected behavior
No crash.

Actual behavior

==4129277==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020000007d1 at pc 0x00000036a85e bp 0x7ffc23d0c730 sp 0x7ffc23d0c728
WRITE of size 1 at 0x6020000007d1 thread T0
    #0 0x36a85d in string_copy /root/twemproxy/src/nc_string.c:96:25
    #1 0x35044c in conf_push_scalar /root/twemproxy/src/nc_conf.c:481:14
    #2 0x35044c in conf_parse_core /root/twemproxy/src/nc_conf.c:668:18
    #3 0x35044c in conf_parse /root/twemproxy/src/nc_conf.c:740:14
    #4 0x35044c in conf_create /root/twemproxy/src/nc_conf.c:1377:14
    #5 0x327e5a in core_ctx_create /root/twemproxy/src/nc_core.c:70:15
    #6 0x327e5a in core_start /root/twemproxy/src/nc_core.c:167:11
    #7 0x3747e8 in nc_run /root/twemproxy/src/nc.c:523:11
    #8 0x3732b1 in main /root/twemproxy/src/nc.c:594:5
    #9 0x7f6a4d8c5564 in __libc_start_main csu/../csu/libc-start.c:332:16
    #10 0x2783ed in _start (/root/twemproxy/src/nutcracker+0x2783ed)

0x6020000007d1 is located 0 bytes to the right of 1-byte region [0x6020000007d0,0x6020000007d1)
allocated by thread T0 here:
    #0 0x2f364d in malloc (/root/twemproxy/src/nutcracker+0x2f364d)
    #1 0x28a8d2 in strndup (/root/twemproxy/src/nutcracker+0x28a8d2)
    #2 0x36a6e6 in string_copy /root/twemproxy/src/nc_string.c:90:17
    #3 0x327e5a in core_ctx_create /root/twemproxy/src/nc_core.c:70:15
    #4 0x327e5a in core_start /root/twemproxy/src/nc_core.c:167:11
    #5 0x3747e8 in nc_run /root/twemproxy/src/nc.c:523:11
    #6 0x7f6a4d8c5564 in __libc_start_main csu/../csu/libc-start.c:332:16

SUMMARY: AddressSanitizer: heap-buffer-overflow /root/twemproxy/src/nc_string.c:96:25 in string_copy

Screenshots

Environment
Clang 12, Ubuntu 18 or 21

Additional context
HackerOne triage gatekeeping bugs they don't understand is silly and needs to stop.

[TysonAndre]

HackerOne triage said this bug was out of scope because it requires "physical i can touch your server" access in order to exploit.

It does require "physical i can touch your server" access to create config files. Having the attacker be able to create/modify files owned by your application in their location of choice causes many, many other issues

https://en.cppreference.com/w/c/experimental/dynamic/strndup is used in string_copy, in config parsing code. It should be using something other than nc_strncmp and returning an NC_ERROR rstatus_t if there's a null byte before srclen, there's not a good reason right now to support null bytes in config file entries