Fix/xinetd probe memory safety

src/OVAL/probes/unix/xinetd_probe.c

@@ -406,30 +406,32 @@
 
 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 @@
 			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 @@
 			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 @@
 						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 @@
 				}
 
 				/*
-				 * 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 @@
 						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 @@
 							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 @@
 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 @@
 		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,16 +956,21 @@
 	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.
 		 */
 		l_pbeg = xifile->inmem + xifile->inoff;
 		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 @@
 			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 @@
 
 			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 @@
 		}
 
 		tmpbuf_free(buffer);
+		buffer = NULL;
 		free(key);
 		key = NULL;
 	}
@@ -1132,44 +1200,62 @@
 			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("");
 			}
 		}
 		/*
 		 * Add entry to the ttree for (name, protocol) -> (id) translation
 		 * (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 @@
 			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

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);
 }