From f8edb06159b80ee2e7cf60104e916cbc0238421e Mon Sep 17 00:00:00 2001 From: aizu-m Date: Sat, 13 Jun 2026 23:07:57 +0530 Subject: [PATCH 1/2] Fixed out-of-bounds read in module protocol when a '%' line has no '=' AddressSanitizer, parsing a "%" module-protocol line with no "=": ERROR: AddressSanitizer: SEGV on a READ #0 strlen #1 BufferNewFrom buffer.c:68 #2 ModuleProtocol evalfunction.c:10252 Came across this whilst reading the module protocol parser. The "%" case builds the JSON payload with BufferNewFrom(line + strlen(name) + 1 + 1, length - strlen(name) - 1 - 1); which assumes a well-formed "%name=" line. But sscanf("%256[^=]=", name) also returns with name set when the line carries no "=": it leaves the whole tail in name. So strlen(name) == length - 1, the length argument underflows to SIZE_MAX, and the data pointer steps one past the terminating NUL. BufferAppend() then scans from there and walks off the heap line buffer. read_module_protocol() hands arbitrary file lines straight to ModuleProtocol(), so a single line "%x" is enough to reproduce it. The other directives ("=", "@", ...) already guard their input; the "%" case did not. Only do the arithmetic once the "=" delimiter is present. Changelog: Title Signed-off-by: aizu-m --- libpromises/evalfunction.c | 5 ++++- tests/unit/evalfunction_test.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/libpromises/evalfunction.c b/libpromises/evalfunction.c index b65b514af1..f9d160528d 100644 --- a/libpromises/evalfunction.c +++ b/libpromises/evalfunction.c @@ -10246,7 +10246,10 @@ void ModuleProtocol(EvalContext *ctx, const char *command, const char *line, int // context name once it's in the vartable. Maybe this can be relaxed. sscanf(line + 1, "%256[^=]=", name); - if (CheckID(name)) + /* A well-formed line is "%name=". Without the '=' delimiter + * sscanf() leaves the whole tail in name, so strlen(name) == length-1 + * and the "length - strlen(name) - 1 - 1" below underflows. */ + if (CheckID(name) && line[strlen(name) + 1] == '=') { JsonElement *json = NULL; Buffer *holder = BufferNewFrom(line+strlen(name)+1+1, diff --git a/tests/unit/evalfunction_test.c b/tests/unit/evalfunction_test.c index b0ba60194a..cbbd082a4e 100644 --- a/tests/unit/evalfunction_test.c +++ b/tests/unit/evalfunction_test.c @@ -126,6 +126,39 @@ static void test_basename(void) basename_single_testcase("//a//b///c.csv////", ".csv", "c"); } +static void test_module_protocol_percent_no_delimiter(void) +{ + EvalContext *ctx = EvalContextNew(); + StringSet *tags = StringSetNew(); + long persistence = 0; + char context[CF_BUFSIZE] = "test"; + + /* Lines come from CfReadLine() on the heap, so allocate them that way: + * the out-of-bounds read past the line is then flagged by ASan/valgrind. */ + + /* A well-formed '%name=' line still defines the container. */ + char *ok = xstrdup("%good={\"k\":1}"); + ModuleProtocol(ctx, "/dev/null", ok, false, context, sizeof(context), + tags, &persistence); + VarRef *good = VarRefParseFromScope("good", context); + assert_true(EvalContextVariableGet(ctx, good, NULL) != NULL); + VarRefDestroy(good); + free(ok); + + /* A '%' line with no '=' delimiter must be skipped, not underflow the + * BufferNewFrom() length and read past the line buffer. */ + char *bad = xstrdup("%bad"); + ModuleProtocol(ctx, "/dev/null", bad, false, context, sizeof(context), + tags, &persistence); + VarRef *ref = VarRefParseFromScope("bad", context); + assert_true(EvalContextVariableGet(ctx, ref, NULL) == NULL); + VarRefDestroy(ref); + free(bad); + + StringSetDestroy(tags); + EvalContextDestroy(ctx); +} + int main() { PRINT_TEST_BANNER(); @@ -133,6 +166,7 @@ int main() unit_test(test_hostinnetgroup_found), unit_test(test_hostinnetgroup_not_found), unit_test(test_basename), + unit_test(test_module_protocol_percent_no_delimiter), }; return run_tests(tests); From 5f94cc1ddbafd2087a19190f83e53c2d8e2cf1f5 Mon Sep 17 00:00:00 2001 From: aizu-m Date: Fri, 19 Jun 2026 21:50:24 +0530 Subject: [PATCH 2/2] Make the '%' no-delimiter test catch the out-of-bounds read The behavioural test only tripped under ASan: in a plain build the scan past the line buffer reads adjacent heap, JsonParse() rejects the garbage, and the variable is left undefined either way, so it passed without the fix. Place the malformed '%bad' line so its terminating NUL is the last readable byte before a PROT_NONE guard page. The unpatched arithmetic steps one past the NUL and BufferAppend() scans into the guard page, so the test now crashes (caught as a failure) without the fix and passes with it. Changelog: none Signed-off-by: aizu-m --- tests/unit/evalfunction_test.c | 39 ++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/tests/unit/evalfunction_test.c b/tests/unit/evalfunction_test.c index cbbd082a4e..16db5c1acd 100644 --- a/tests/unit/evalfunction_test.c +++ b/tests/unit/evalfunction_test.c @@ -7,6 +7,13 @@ #include #include +#ifndef __MINGW32__ +#include +#if !defined(MAP_ANONYMOUS) && defined(MAP_ANON) +#define MAP_ANONYMOUS MAP_ANON +#endif +#endif + static bool netgroup_more = false; #if SETNETGRENT_RETURNS_INT @@ -133,10 +140,8 @@ static void test_module_protocol_percent_no_delimiter(void) long persistence = 0; char context[CF_BUFSIZE] = "test"; - /* Lines come from CfReadLine() on the heap, so allocate them that way: - * the out-of-bounds read past the line is then flagged by ASan/valgrind. */ - - /* A well-formed '%name=' line still defines the container. */ + /* A well-formed '%name=' line still defines the container, so the + * added delimiter check does not reject valid input. */ char *ok = xstrdup("%good={\"k\":1}"); ModuleProtocol(ctx, "/dev/null", ok, false, context, sizeof(context), tags, &persistence); @@ -145,15 +150,31 @@ static void test_module_protocol_percent_no_delimiter(void) VarRefDestroy(good); free(ok); - /* A '%' line with no '=' delimiter must be skipped, not underflow the - * BufferNewFrom() length and read past the line buffer. */ - char *bad = xstrdup("%bad"); - ModuleProtocol(ctx, "/dev/null", bad, false, context, sizeof(context), +#ifndef __MINGW32__ + /* A '%' line with no '=' makes "length - strlen(name) - 1 - 1" underflow + * to SIZE_MAX and steps the source pointer one past the terminating NUL, + * so BufferAppend() scans off the end of the line buffer. Place the line + * so its NUL is the last readable byte before a PROT_NONE guard page: the + * unpatched code walks into the guard page (crashes), the patched code + * skips the line. */ + const size_t pagesize = (size_t) sysconf(_SC_PAGESIZE); + char *region = mmap(NULL, pagesize * 2, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + assert_true(region != MAP_FAILED); + assert_int_equal(mprotect(region + pagesize, pagesize, PROT_NONE), 0); + + const char *bad = "%bad"; + char *line = region + pagesize - strlen(bad) - 1; /* NUL at page boundary */ + memcpy(line, bad, strlen(bad) + 1); + + ModuleProtocol(ctx, "/dev/null", line, false, context, sizeof(context), tags, &persistence); VarRef *ref = VarRefParseFromScope("bad", context); assert_true(EvalContextVariableGet(ctx, ref, NULL) == NULL); VarRefDestroy(ref); - free(bad); + + assert_int_equal(munmap(region, pagesize * 2), 0); +#endif StringSetDestroy(tags); EvalContextDestroy(ctx);