ucl_strnstr: fix remaining off-by-one over-read (follow-up to 852f752)

[huanghuihui0904]

Summary

This is the root cause of #337 (CVE-2025-11010), which was closed without a fix and is still present on master. That report has the crash stack; what it does not have is why the bounds check lets it through, or a patch. The CVE record places the defect in ucl_include_common; that is the caller that happens to hand in a buffer with no NUL after it. The off-by-one is in ucl_strnstr, and every call site inherits it. This is also distinct from #378, which reports a one-byte over-read in ucl_parse_macro_value() (src/ucl_parser.c) reached by input of a similar shape; this PR does not touch that code path, and in the reproducer below ucl_parse_macro_value has already returned by the time the read happens.

ucl_strnstr() reads one byte past the end of the haystack when the haystack ends with a partial match of the needle. The check on src/ucl_util.c:2192 admits one byte fewer than the strncmp on line 2195 goes on to read, so with the needle "://" any buffer whose last two bytes are :/ is read one past its end.

• Affected: the partial-match path has been present since 08c933c (2015), when ucl_strnstr was written, and remains on master as of 04e5e70 (2026-08-16). 852f752708 rewrote this same guard in 2022 but covered a different path through the loop, which is why Bug Report: Heap-Buffer-Overflow in ucl_strnstr at ucl_util.c:2207 #337 could still reproduce in the same function.
• Reached by: the .include and .try_include macro handlers. ucl_include_common() calls ucl_strnstr() with a pointer straight into the caller's chunk and the macro argument's length, so the read lands one byte past the buffer the application owns. The other in-tree caller, ucl_schema.c:777, passes strlen(p) and reads the terminating NUL instead.
• Impact: control flow depends on the byte read out of bounds. If it happens to be /, strncmp returns 0 and the include is routed to ucl_include_url() rather than ucl_include_file(). A non-sanitized build can also fault if the buffer ends at an inaccessible page boundary.

How to reproduce

Through the public API, on 04e5e70, with a chunk that is not NUL-terminated. That is what a caller feeding a mmap'd file or a network buffer hands to ucl_parser_add_chunk().

/* poc.c */
#include <stdlib.h>
#include <string.h>
#include "ucl.h"

int main(void)
{
    const char *doc = ".include(url=true) x:/";
    size_t n = strlen(doc);

    /* Exactly n bytes: no NUL terminator, nothing after. */
    unsigned char *buf = malloc(n);
    memcpy(buf, doc, n);

    struct ucl_parser *p = ucl_parser_new(0);
    ucl_parser_add_chunk(p, buf, n);
    ucl_parser_free(p);
    free(buf);
    return 0;
}

cmake -S . -B b -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_FLAGS="-fsanitize=address -g -O0"
cmake --build b -j
cc -fsanitize=address -g -O0 -Iinclude poc.c b/libucl.a -o poc
ASAN_OPTIONS=detect_leaks=0 ./poc

==3334997==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x503000000056
READ of size 2 at 0x503000000056 thread T0
    #0 in strncmp                    sanitizer_common_interceptors.inc:501
    #1 in ucl_strnstr                src/ucl_util.c:2195
    #2 in ucl_include_common         src/ucl_util.c:1612
    #3 in ucl_include_handler        src/ucl_util.c:1668
    #4 in ucl_state_machine          src/ucl_parser.c:2722
    #5 in ucl_parser_add_chunk_full  src/ucl_parser.c:3093
    #6 in ucl_parser_add_chunk       src/ucl_parser.c:3138
    #7 in main                       poc.c:15

0x503000000056 is located 0 bytes after 22-byte region [0x503000000040,0x503000000056)

SUMMARY: AddressSanitizer: heap-buffer-overflow in strncmp

The 22-byte region is the input chunk. The macro argument is a pointer straight into it (macro_start in ucl_parser.c), so data[len] is one byte past what the application owns.

Two things matter when varying the input. The chunk must end exactly at the /: .include(url=true) x:/\n does not trigger it, because the \n is inside the buffer and the over-read lands on it. And url = true is what sets allow_url and reaches the ucl_strnstr call at ucl_util.c:1612.

Root cause

	if ((c = *find++) != 0) {
		mlen = strlen(find);          /* needle length MINUS the first char */
		do {
			do {
				if ((sc = *s++) == 0 || len-- < mlen)
					return (NULL);
			} while (sc != c);
		} while (strncmp(s, find, mlen) != 0);   /* reads mlen bytes from s */

mlen is computed after find++, so it is the length of the needle's tail. When the loop consumes s[k] the remaining count is len - k, and the check lets it continue while len - k >= mlen. The strncmp that follows starts at s + k + 1 and reads mlen bytes, so it touches s[k + mlen] and needs len - k >= mlen + 1. The two differ by one, so len - k == mlen passes the check and strncmp reads s[len].

For the needle "://" (mlen == 2) that is a haystack ending in :/: the : matches c, the / matches the first byte strncmp compares, and the second byte it compares is one past the end.

What 852f752 covered and what it missed

ucl_strnstr on a heap buffer of exactly the stated size, no NUL:

haystack	needle	before 852f752	04e5e70	with this PR
abc	://	over-read	NULL	NULL
x:/	://	over-read	over-read	NULL
abc	@	over-read	over-read	NULL
a://b	://	found at 1	found at 1	found at 1

Row 1 is what 852f752 addressed: running out of haystack without ever meeting the needle's first character.

Row 2 is the case it missed, and is the one .include reaches.

Row 3 is a third path through the same loop, and the one place where 852f752 made the behaviour worse rather than leaving it unchanged. With a single-character needle mlen is 0, so the guard reads len-- < 0 and only fires once len has gone negative; because the dereference is evaluated first, s[len] and s[len + 1] are read before it does. That is a bounded two-byte over-read, independent of the haystack's length. The old len-- == 0 stopped one iteration earlier, so this path used to read one byte past the end and now reads two.

It also has a consequence the other rows do not. If s[len] happens to equal the needle character, the inner loop exits, strncmp(s, "", 0) compares zero bytes and returns 0, and the function returns a pointer one byte past the end of the buffer. The pre-852f752708 code returned NULL on that input.

Every in-tree call site passes "://", so row 3 is reachable only through ucl_internal.h, but it falls out of the same fix.

Row 4 is the control: a needle that is present is still found, at the same offset, in all three versions.

What this fix does

len <= mlen leaves mlen + 1 bytes, which is what the following strncmp needs. Checking the remaining length before dereferencing s also prevents s[len] from being read when mlen is zero, which is row 3 above. With this branch the reproducer above exits 0 with no output.

[vstakhov]

This is the root cause of #337 (CVE-2025-11010), which was closed without a fix and is still present on master.

Yes, because it's bullshit.

[vstakhov]

I have explained like 100500 times that all macros are not intended to use to parse untrusted data. So yes, OOB read in this path is not worth attention at all.

[vstakhov]

I don't mean this should not be addressed, sorry for confusion - this function could be easily used in another places after some changes.

[huanghuihui0904]

Thanks for the clarification and the follow-up commit! I reran the reproducer with your patch, and the bug is fixed on my side as well. The changes look good to me.