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 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 0000000000..8cf4ad7061 Binary files /dev/null and b/tests/probes/xinetd/xinetd_crash_keyword_no_value.conf differ 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 0000000000..aeb68f5ded Binary files /dev/null and b/tests/probes/xinetd/xinetd_crash_section_no_eol.conf differ diff --git a/tests/probes/xinetd/xinetd_crash_service_name_no_space.conf b/tests/probes/xinetd/xinetd_crash_service_name_no_space.conf new file mode 100644 index 0000000000..fe3b5fb7f3 Binary files /dev/null and b/tests/probes/xinetd/xinetd_crash_service_name_no_space.conf differ diff --git a/tests/probes/xinetd/xinetd_crash_service_null_protocol.conf b/tests/probes/xinetd/xinetd_crash_service_null_protocol.conf new file mode 100644 index 0000000000..e7fb5e3223 --- /dev/null +++ b/tests/probes/xinetd/xinetd_crash_service_null_protocol.conf @@ -0,0 +1,11 @@ +defau = 60 + log_type = SYSLOG authpriv} +service fuzz +{ + ÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿ socket_type wiat = no + user = no + user = root + server = /usr/sbble = /usr/sbin/in.fuzzd + disable = no +} +incudledir /etc/xinetd.d diff --git a/tests/probes/xinetd/xinetd_crash_type_not_enum.conf b/tests/probes/xinetd/xinetd_crash_type_not_enum.conf new file mode 100644 index 0000000000..46718e7d03 --- /dev/null +++ b/tests/probes/xinetd/xinetd_crash_type_not_enum.conf @@ -0,0 +1,24 @@ +defaults +{ + innsstceSLOG authpriv +} +service fu~… +{ + socket_!(= SYSLOG authpriv +} +service fqzz +{ + socket_thpriv +} +service fu~… +{ + iv +} +service fqzz +{ + socket_SYSLOG authpriv +} +service fqzz +{ + type =t e ear +sm ! protocouzetd.d