From 9cf9f1255cc415f07b740a472029e8da9e6045ad Mon Sep 17 00:00:00 2001 From: Edouard Schweisguth Date: Fri, 12 Jun 2026 16:43:37 +0000 Subject: [PATCH 1/2] xinetd probe: fix memory-safety bugs on malformed config Bugs found by fuzzing xiconf_parse()/xiconf_parse_section() with crafted xinetd configuration content. A regression test exercising each case is added in a follow-up commit (tests/probes/xinetd). - A line with no trailing newline set the line length to the whole file length instead of the bytes remaining from the current offset, so the fixed line buffer was overflowed by memcpy() (heap-buffer-overflow). Two sites: the top-level scanner and xiconf_parse_section(). - An unterminated service section ran its for(;;) reader past the end of the in-memory file; bound the loop to inoff < inlen and guard the section entry against inoff already being at/after the end. - *strchr(buffer, ' ') = '\0' dereferenced NULL when the copied line contained an embedded NUL (arbitrary file content), at two sites (keyword and service name). Check the result before writing. - For a keyword with no value, op was set past the keyword's NUL terminator, reading out of bounds; only step past it when a value follows. - A (name, protocol) translation key was built with strcpy()/strcat() into a fixed buffer with no NULL or length check; a NULL protocol dereferenced and an over-long name+protocol overflowed it. Guard both, matching the checks already in xiconf_getservice(). - xiconf_service_free() recursed over the ->next chain; a long crafted chain overflowed the stack. Free iteratively. - On an unrecognized type value, scur->type (strdup'd, later free'd) was reassigned to a string literal -> invalid free, and the old value leaked. Free the old value and strdup(""). - op_assign_str() leaked the previous value when an attribute was repeated within a section; free before reassigning. We are aware of the in-progress hardening in #2349, which touches this file; its scope (include-path handling) is narrower and does not cover the bugs above. Two findings overlap and are reconciled here. --- src/OVAL/probes/unix/xinetd_probe.c | 241 +++++++++++++++++++--------- 1 file changed, 165 insertions(+), 76 deletions(-) diff --git a/src/OVAL/probes/unix/xinetd_probe.c b/src/OVAL/probes/unix/xinetd_probe.c index fcb9156250..8925db8dea 100644 --- a/src/OVAL/probes/unix/xinetd_probe.c +++ b/src/OVAL/probes/unix/xinetd_probe.c @@ -406,30 +406,32 @@ static xiconf_service_t *xiconf_service_new(void) static void xiconf_service_free(xiconf_service_t *service) { - if (service == NULL) - return; - #define NONNULL_FREE(p) if (service->p != NULL) free(service->p) - if (service->id != service->name) - NONNULL_FREE(id); - - NONNULL_FREE(name); - NONNULL_FREE(type); - NONNULL_FREE(flags); - NONNULL_FREE(socket_type); - NONNULL_FREE(protocol); - NONNULL_FREE(user); - NONNULL_FREE(server); - NONNULL_FREE(server_args); - NONNULL_FREE(only_from); - NONNULL_FREE(no_access); - - if (service->next != NULL) { - xiconf_service_free(service->next); + /* Walk the ->next chain iteratively. Services that share an id are + * linked through ->next, and a long chain (easy to produce from a + * crafted config) would overflow the stack if freed recursively. */ + while (service != NULL) { + xiconf_service_t *next = service->next; + + if (service->id != service->name) + NONNULL_FREE(id); + + NONNULL_FREE(name); + NONNULL_FREE(type); + NONNULL_FREE(flags); + NONNULL_FREE(socket_type); + NONNULL_FREE(protocol); + NONNULL_FREE(user); + NONNULL_FREE(server); + NONNULL_FREE(server_args); + NONNULL_FREE(only_from); + NONNULL_FREE(no_access); + + free(service); + service = next; } - - free(service); +#undef NONNULL_FREE } static void xiconf_stree_free_cb(struct rbt_str_node *n) @@ -634,8 +636,13 @@ xiconf_t *xiconf_parse(const char *path, unsigned int max_depth) l_pend = strchr(l_pbeg, '\n'); if (l_pend == NULL) { - l_pend = l_pbeg + xifile->inlen; - l_size = xifile->inlen; + /* No newline before the end of the buffer: the + * last line runs to EOF. Its length is the number + * of bytes remaining from the current offset, not + * the whole file length (inmem is NUL-terminated + * at inlen). */ + l_size = xifile->inlen - xifile->inoff; + l_pend = l_pbeg + l_size; } else l_size = (size_t)(l_pend - l_pbeg); @@ -650,7 +657,17 @@ xiconf_t *xiconf_parse(const char *path, unsigned int max_depth) bufidx = 0; memcpy (buffer, l_pbeg, l_size); buffer[l_size] = ' '; - *strchr(buffer, ' ') = '\0'; + /* Terminate the keyword at the first whitespace. The + * sentinel space written above guarantees strchr() finds + * one -- unless the copied line contains an embedded NUL + * byte (possible with arbitrary file content), in which + * case the buffer is already NUL-terminated there and + * strchr() returns NULL. Guard against that. */ + { + char *sp = strchr(buffer, ' '); + if (sp != NULL) + *sp = '\0'; + } /* skip whitespaces before the keyword */ while(isspace(buffer[bufidx])) ++bufidx; @@ -676,7 +693,16 @@ xiconf_t *xiconf_parse(const char *path, unsigned int max_depth) return (NULL); } - *strchr(buffer + bufidx, ' ') = '\0'; + /* Terminate the service name at the next space. + * The sentinel space appended to buffer normally + * guarantees one exists; an embedded NUL in binary + * file content can make strchr() return NULL, in + * which case the name is already NUL-terminated. */ + { + char *sp = strchr(buffer + bufidx, ' '); + if (sp != NULL) + *sp = '\0'; + } if (xiconf_parse_section (xiconf, xifile, XICONF_SECTION_SERVICE, buffer + bufidx) != 0) { tmpbuf_free(buffer); @@ -727,15 +753,26 @@ xiconf_t *xiconf_parse(const char *path, unsigned int max_depth) } /* - * Get the include(dir) argument + * Get the include(dir) argument. The keyword was + * NUL-terminated where its trailing space used to be, + * so skip past it (its real length -- "include" vs + * "includedir") and the inserted NUL to reach the + * argument, staying within the line buffer. */ - bufidx += strlen("includedir"); - while(isspace(buffer[bufidx])) ++bufidx; - inclarg = buffer + bufidx + 1; + bufidx += (inctype == XICONF_INCTYPE_DIR) + ? strlen("includedir") : strlen("include"); + while (bufidx < l_size && isspace(buffer[bufidx])) + ++bufidx; buffer[l_size] = '\0'; + if (bufidx + 1 > l_size) { + dW("Found include directive without an argument!"); + break; + } + inclarg = buffer + bufidx + 1; + if (*inclarg == '\0') { - dW("Found includedir directive without an argument!"); + dW("Found include directive without an argument!"); break; } @@ -769,19 +806,23 @@ xiconf_t *xiconf_parse(const char *path, unsigned int max_depth) break; } - strcpy (pathbuf, inclarg); incllen = strlen(inclarg); - if (pathbuf[incllen - 1] != PATH_SEPARATOR) { - if (incllen < PATH_MAX) { - pathbuf[incllen++] = '/'; - pathbuf[incllen ] = '\0'; - } else { - dE("Length of the includedir argument is out of range: len=%zu, max=%zu", - incllen, PATH_MAX); - closedir(dirfp); - break; - } + /* Reject an over-long directory argument up front: + * we must fit inclarg + '/' + an entry name + NUL in + * the fixed pathbuf, so copying it unbounded would + * overflow the stack buffer. */ + if (incllen >= PATH_MAX) { + dE("Length of the includedir argument is out of range: len=%zu, max=%zu", + incllen, (size_t)PATH_MAX); + closedir(dirfp); + break; + } + memcpy(pathbuf, inclarg, incllen + 1); + + if (incllen == 0 || pathbuf[incllen - 1] != PATH_SEPARATOR) { + pathbuf[incllen++] = '/'; + pathbuf[incllen ] = '\0'; } for (;;) { @@ -800,6 +841,13 @@ xiconf_t *xiconf_parse(const char *path, unsigned int max_depth) continue; } + /* Don't overflow pathbuf when appending the + * directory entry name to the '/'-terminated + * prefix. */ + if (incllen + strlen(dent->d_name) + 1 > sizeof(pathbuf)) { + dW("includedir entry path too long, skipping: %s", dent->d_name); + continue; + } strcpy(pathbuf + incllen, dent->d_name); if (xiconf_add_cfile (xiconf, pathbuf, xifile->depth + 1) != 0) @@ -857,12 +905,12 @@ static int xiconf_service_merge_defaults(xiconf_service_t *dst, xiconf_service_t int xiconf_parse_section(xiconf_t *xiconf, xiconf_file_t *xifile, int type, char *name) { xiconf_service_t *snew, *scur; - char *buffer; + char *buffer = NULL; register size_t bufidx, keyidx; char *l_pbeg; char *l_pend; size_t l_size; - char *key, *op, *opval; + char *key = NULL, *op, *opval; void **opvar; int (*opfun)(void *, char *); struct xiconf_attr *xiattr; @@ -876,6 +924,12 @@ int xiconf_parse_section(xiconf_t *xiconf, xiconf_file_t *xifile, int type, char return (-1); } + /* The caller may have advanced inoff to (or past) the end of the + * in-memory file -- e.g. a 'service NAME' keyword on the very last line + * with no section body. Bail out instead of reading past inmem. */ + if (xifile->inoff >= xifile->inlen) + return (-1); + /* * Find the left brace. Only the left brace, whitespace * characters before and after it and a newline character @@ -902,7 +956,9 @@ int xiconf_parse_section(xiconf_t *xiconf, xiconf_file_t *xifile, int type, char snew = xiconf_service_new(); snew->name = strdup(name); - for (;;) { + /* Stop at end-of-buffer: a section that is never closed with '}' must + * not read past the in-memory copy of the file. */ + for (; xifile->inoff < xifile->inlen;) { /* * Find out the line boundaries. */ @@ -910,8 +966,11 @@ int xiconf_parse_section(xiconf_t *xiconf, xiconf_file_t *xifile, int type, char l_pend = strchr(l_pbeg, '\n'); if (l_pend == NULL) { - l_pend = l_pbeg + xifile->inlen; - l_size = xifile->inlen; + /* See xiconf_parse(): a line that runs to EOF is only + * as long as the bytes remaining from the current + * offset, not the whole file. */ + l_size = xifile->inlen - xifile->inoff; + l_pend = l_pbeg + l_size; } else l_size = (size_t)(l_pend - l_pbeg); @@ -941,6 +1000,11 @@ int xiconf_parse_section(xiconf_t *xiconf, xiconf_file_t *xifile, int type, char exit(ENOMEM); for (keyidx = 0; ! isspace(key[keyidx]) && key[keyidx]; keyidx++); + /* Remember whether the keyword is actually followed by + * whitespace (i.e. by a value). If the loop stopped on key's + * own NUL terminator instead, there is no value byte after it + * and stepping past the terminator would read out of bounds. */ + bool key_has_value = (key[keyidx] != '\0'); key[keyidx] = '\0'; switch (*key) { @@ -959,7 +1023,10 @@ int xiconf_parse_section(xiconf_t *xiconf, xiconf_file_t *xifile, int type, char goto finish_section; default: - op = key + strlen(key) + 1; + /* Step past the keyword's terminator only when a value + * follows; otherwise point op at the empty string so the + * scan below does not read past the end of key. */ + op = key_has_value ? key + keyidx + 1 : key + keyidx; while(isspace(*op)) ++op; @@ -1031,6 +1098,7 @@ int xiconf_parse_section(xiconf_t *xiconf, xiconf_file_t *xifile, int type, char } tmpbuf_free(buffer); + buffer = NULL; free(key); key = NULL; } @@ -1132,7 +1200,13 @@ int xiconf_parse_section(xiconf_t *xiconf, xiconf_file_t *xifile, int type, char if (!type_enum_match) { dE("The value of the type setting does not match" " any of the allowed values by OVAL: %s", scur->type); - scur->type = ""; + /* Replace with an owned empty string: scur->type was + * strdup()'d and is free()'d by xiconf_service_free(), + * so it must not be set to a string literal (that would + * be an invalid free) and the old value must be freed + * to avoid a leak. */ + free(scur->type); + scur->type = strdup(""); } } /* @@ -1140,36 +1214,48 @@ int xiconf_parse_section(xiconf_t *xiconf, xiconf_file_t *xifile, int type, char * (in case it's not already there) */ st = NULL; - strcpy(st_key, scur->name); - strcat(st_key, scur->protocol); - - rbt_str_get(xiconf->ttree, st_key, (void *)&st); - - if (st == NULL) { - dD("new strans record: k=%s", st_key); - - st = malloc(sizeof(xiconf_strans_t)); - st->cnt = 1; - st->srv = malloc (sizeof (xiconf_service_t *)); - st->srv[0] = scur; - - if (rbt_str_add (xiconf->ttree, strdup(st_key), st) != 0) { - dE("Can't add strans record (k=%s) into the strans tree (%p)", - st_key, xiconf->ttree); - return (-1); - } - } else { - dD("adding new strans record to an exiting one: k=%s, cnt=%u+1", - st_key, st->cnt); - - void *new_srv = realloc(st->srv, sizeof (xiconf_service_t *) * ++(st->cnt)); - if (new_srv == NULL) { - dE("Failed to re-allocate memory for st->srv"); - return (-1); + /* Register the (name, protocol) -> id translation, but only when + * we can build the key: we need both a name and a protocol, and + * the two together must fit the fixed st_key buffer. A service + * section may legitimately omit the protocol, in which case we + * just skip registration. (xiconf_getservice() applies the same + * NULL/length guards on lookup.) */ + if (scur->name != NULL && scur->protocol != NULL && + strlen(scur->name) + strlen(scur->protocol) <= XICFG_STRANS_MAXKEYLEN) { + strcpy(st_key, scur->name); + strcat(st_key, scur->protocol); + + rbt_str_get(xiconf->ttree, st_key, (void *)&st); + + if (st == NULL) { + dD("new strans record: k=%s", st_key); + + st = malloc(sizeof(xiconf_strans_t)); + st->cnt = 1; + st->srv = malloc (sizeof (xiconf_service_t *)); + st->srv[0] = scur; + + if (rbt_str_add (xiconf->ttree, strdup(st_key), st) != 0) { + dE("Can't add strans record (k=%s) into the strans tree (%p)", + st_key, xiconf->ttree); + return (-1); + } } else { - st->srv = new_srv; - st->srv[st->cnt - 1] = scur; + dD("adding new strans record to an exiting one: k=%s, cnt=%u+1", + st_key, st->cnt); + + void *new_srv = realloc(st->srv, sizeof (xiconf_service_t *) * ++(st->cnt)); + if (new_srv == NULL) { + dE("Failed to re-allocate memory for st->srv"); + return (-1); + } else { + st->srv = new_srv; + st->srv[st->cnt - 1] = scur; + } } + } else { + dW("Skipping (name, protocol) translation for service '%s'", + scur->name ? scur->name : ""); } /* @@ -1342,6 +1428,9 @@ int op_assign_str(void *var, char *val) dE("Error stripping white space from string '%s'", val); return (-1); } + /* Free any previously assigned value so that an attribute + * repeated within a section does not leak the earlier string. */ + free(*((char **)(var))); *((char **)(var)) = strndup(val, (strend-val+1)); return (0); } else From 71434563d983da2cdc03f7e8f0ef1becf97e64c4 Mon Sep 17 00:00:00 2001 From: Edouard Schweisguth Date: Fri, 12 Jun 2026 17:09:36 +0000 Subject: [PATCH 2/2] xinetd probe: add regression test for malformed-config crashes Adds the crafted inputs that triggered the memory-safety bugs fixed in the previous commit as test fixtures, and a test case that feeds each one to test_probe_xinetd and fails if the parser is killed by a signal or trips a sanitizer (the pre-existing, harmless UBSan function-pointer-type report on the rbtree free callback is ignored). test_probe_xinetd now calls xiconf_free() on the parsed config before returning, both to release it (the test leaked it previously) and so the regression also exercises the teardown path, where two of the bugs lived. --- tests/probes/xinetd/test_probe_xinetd.c | 2 ++ tests/probes/xinetd/test_xinetd_probe.sh | 32 ++++++++++++++++++ .../xinetd/xinetd_crash_keyword_no_value.conf | Bin 0 -> 250 bytes .../xinetd/xinetd_crash_section_no_eol.conf | Bin 0 -> 265 bytes .../xinetd_crash_service_name_no_space.conf | Bin 0 -> 70 bytes .../xinetd_crash_service_null_protocol.conf | 11 ++++++ .../xinetd/xinetd_crash_type_not_enum.conf | 24 +++++++++++++ 7 files changed, 69 insertions(+) create mode 100644 tests/probes/xinetd/xinetd_crash_keyword_no_value.conf create mode 100644 tests/probes/xinetd/xinetd_crash_section_no_eol.conf create mode 100644 tests/probes/xinetd/xinetd_crash_service_name_no_space.conf create mode 100644 tests/probes/xinetd/xinetd_crash_service_null_protocol.conf create mode 100644 tests/probes/xinetd/xinetd_crash_type_not_enum.conf diff --git a/tests/probes/xinetd/test_probe_xinetd.c b/tests/probes/xinetd/test_probe_xinetd.c index af765c1340..939055b1a6 100644 --- a/tests/probes/xinetd/test_probe_xinetd.c +++ b/tests/probes/xinetd/test_probe_xinetd.c @@ -48,6 +48,7 @@ int main (int argc, char *argv[]) if (xres == NULL) { fprintf(stderr, "Not found.\n"); + xiconf_free(xcfg); return (3); } else { register unsigned int l; @@ -89,5 +90,6 @@ int main (int argc, char *argv[]) } } + xiconf_free(xcfg); return (0); } diff --git a/tests/probes/xinetd/test_xinetd_probe.sh b/tests/probes/xinetd/test_xinetd_probe.sh index 9b35741db6..7be2141537 100755 --- a/tests/probes/xinetd/test_xinetd_probe.sh +++ b/tests/probes/xinetd/test_xinetd_probe.sh @@ -80,6 +80,37 @@ function test_probe_xinetd_duplicates { return 1 } +# Regression test for memory-safety bugs in xiconf_parse()/xiconf_parse_section(). +# Each of these inputs used to crash the parser (heap-buffer-overflow, NULL-pointer +# dereference, invalid free, ...) before the guards were added. The parser must now +# handle them without crashing; the service/protocol arguments are arbitrary, we only +# care that the process is neither killed by a signal nor trips a sanitizer. +function test_probe_xinetd_regression_malformed { + local ret_val=0 + local f errf rc + for f in xinetd_crash_keyword_no_value.conf \ + xinetd_crash_section_no_eol.conf \ + xinetd_crash_service_name_no_space.conf \ + xinetd_crash_service_null_protocol.conf \ + xinetd_crash_type_not_enum.conf ; do + errf=$(mktemp) + ./test_probe_xinetd "${srcdir}/${f}" foo tcp > /dev/null 2>"$errf" + rc=$? + # Ignore the pre-existing, harmless UBSan report about calling a typed + # rbtree callback through a generic function pointer; flag a termination + # by signal (exit >= 128) or any other sanitizer/runtime error. + if [ "$rc" -ge 128 ] || \ + grep -v "through pointer to incorrect function type" "$errf" \ + | grep -q "ERROR: AddressSanitizer:\|AddressSanitizer:DEADLYSIGNAL\|LeakSanitizer:\|runtime error:" ; then + echo "CRASH on malformed input ${f} (exit ${rc}):" + cat "$errf" + ret_val=$[$ret_val + 1] + fi + rm -f "$errf" + done + return $ret_val +} + # Testing. test_init @@ -88,6 +119,7 @@ if [ -z ${CUSTOM_OSCAP+x} ] ; then test_run "test_probe_xinetd_parser" test_probe_xinetd_parser test_run "xinetd parser regression test: string list" test_probe_xinetd_regression_stringlist test_run "test_probe_xinetd_duplicates" test_probe_xinetd_duplicates + test_run "xinetd parser regression test: malformed input" test_probe_xinetd_regression_malformed fi test_exit diff --git a/tests/probes/xinetd/xinetd_crash_keyword_no_value.conf b/tests/probes/xinetd/xinetd_crash_keyword_no_value.conf new file mode 100644 index 0000000000000000000000000000000000000000..8cf4ad7061709b9ff0981e1266b641ab5b5cd08d GIT binary patch literal 250 zcmc(Zu?>JQ3Xn&*45 zM=GAkCpA)Q=d$lyAKfFX4MTuma7n91!Mo^<{>+8u1hu#n7S@C$Int&knOlYn6z42h literal 0 HcmV?d00001 diff --git a/tests/probes/xinetd/xinetd_crash_section_no_eol.conf b/tests/probes/xinetd/xinetd_crash_section_no_eol.conf new file mode 100644 index 0000000000000000000000000000000000000000..aeb68f5ded54dcf32127172946b866e03739dfe6 GIT binary patch literal 265 zcmZurF>b>!3^dS9uiy)!7ii8BAcLn)g)Gqt5mhuqDrg<#?~&zp%T46*cy|$JUz3V? z7-*m@>T^KZ761ny!!WcV@#RZ@7u?7O2jk~>Iv>GTy)K3B=4k}WodUvK*Y(d$INVV8 zM+c$>{dbR93M&UryO)N=yfNf1t