Skip to content

load_ini_buffer: allocate ini_length + 1 bytes (fixes OOB write) - #21

Open
1820893135-pixel wants to merge 1 commit into
madmurphy:masterfrom
1820893135-pixel:fix-load-ini-buffer-alloc
Open

load_ini_buffer: allocate ini_length + 1 bytes (fixes OOB write)#21
1820893135-pixel wants to merge 1 commit into
madmurphy:masterfrom
1820893135-pixel:fix-load-ini-buffer-alloc

Conversation

@1820893135-pixel

Copy link
Copy Markdown

Fixes the heap-buffer-overflow reported in #20.

strip_ini_cache() documents (and does) a mandatory one-byte write:

/* src/confini.c:2621 — first statement of strip_ini_cache() */
ini_source[ini_length] = '\0';

so its ini_source argument needs ini_length + 1 usable bytes. The bundled helper examples/utilities/load_ini_buffer.h violated that contract:

char * const ini_cache = malloc(ini_length);   /* only ini_length bytes */
memcpy(ini_cache, ini_buffer, ini_length);
const int retval = strip_ini_cache(ini_cache, ini_length, ...);

Every non-empty input therefore wrote one NUL byte past the allocation. Other in-tree callers are fine — examples/topics/strip_ini_cache.c uses strndup() and the two doc examples in src/confini.c use malloc(file_size + 1) — this helper was the only one missing the extra byte.

Fix: allocate ini_length + 1 bytes to match the documented contract.

Verification

ASan reproducer using the bundled header itself:

#include <confini.h>
#include "load_ini_buffer.h"
static int cb(IniDispatch *d, void *v) { (void)d; (void)v; return 0; }
int main(void) {
    const char *input = "[S]\nkey = value\n";
    return load_ini_buffer(input, strlen(input), INI_DEFAULT_FORMAT, NULL, cb, NULL);
}
clang -fsanitize=address -I src -I examples/utilities test.c src/confini.c -o test

Before: aborts with

ERROR: AddressSanitizer: heap-buffer-overflow ... WRITE of size 1
    #0 strip_ini_cache src/confini.c:2621:25
    #1 load_ini_buffer  examples/utilities/load_ini_buffer.h:43:22

After: exits 0 with no sanitizer report.

I also checked the UTF-8 BOM probe at src/confini.c:2636-2640 (ini_source[0..2]): it is not separately exploitable, because the mandatory ini_source[ini_length] = '\0' always terminates that three-byte chain first (ini_source[2] is only read when ini_length >= 3, in which case it is in bounds). So the allocation is the only change needed.

Fixes #20.

`strip_ini_cache()` documents that the `ini_source` buffer must be able to
hold `ini_length + 1` bytes, since the first thing it does is

    ini_source[ini_length] = '\0';

regardless of the input. The bundled `load_ini_buffer()` helper allocated
only `ini_length` bytes and then handed the buffer to `strip_ini_cache()`,
so every non-empty input wrote one byte past the allocation (an ASan
heap-buffer-overflow on the NUL terminator).

Allocate `ini_length + 1` bytes to match the documented contract.

Reproducer (ASan): calling `load_ini_buffer("[S]\nkey = value\n", ...)`
aborts with "heap-buffer-overflow ... WRITE of size 1 ... in strip_ini_cache"
before the fix and runs cleanly afterwards.

Fixes madmurphy#20.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]heap-buffer-overflow in strip_ini_cache: unconditional ini_source[ini_length] = '\0' write contradicts bundled example (CWE-122)

1 participant