All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V4] Fix more bin file processing core dumps
@ 2015-05-24 17:35 Richard Haines
  2015-05-26 15:42 ` Stephen Smalley
  0 siblings, 1 reply; 2+ messages in thread
From: Richard Haines @ 2015-05-24 17:35 UTC (permalink / raw)
  To: selinux

The reading of bin files has been changed to follow that of loading
policy to catch over-runs. Entries that should be NUL terminated are
also checked. If any error, then process the text file. This should
fix all problems highlighted in [1] with V2 fixing those in [2].
V3 corrects int32_t/uint32_t for *_len entries and V4 fixes [3]
and adds pcre_fullinfo checks to validate regex and study data
sizes. pcre_fullinfo also validates its magic number.

Tested with bin files built using sefcontext_compile PCRE_VERS 1 and 2.

The following is a rough guide to the difference in processing a bin
file against a text file:
   6K entries - x5
   4K entries - x4
   1K entries - x3
   500 entries - x2

[1] http://marc.info/?l=selinux&m=143101983922281&w=2
[2] http://marc.info/?l=selinux&m=143161763905159&w=2
[3] http://marc.info/?l=selinux&m=143204170705586&w=2

Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
---
 libselinux/src/label_file.c | 226 ++++++++++++++++++++++++++++++--------------
 libselinux/src/label_file.h |  20 +++-
 2 files changed, 171 insertions(+), 75 deletions(-)

diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index c722f29..7da79b4 100644
--- a/libselinux/src/label_file.c
+++ b/libselinux/src/label_file.c
@@ -8,7 +8,6 @@
  * developed by Secure Computing Corporation.
  */
 
-#include <assert.h>
 #include <fcntl.h>
 #include <stdarg.h>
 #include <string.h>
@@ -242,15 +241,12 @@ static int load_mmap(struct selabel_handle *rec, const char *path, struct stat *
 	int mmapfd;
 	int rc;
 	struct stat mmap_stat;
-	char *addr;
+	char *addr, *str_buf;
 	size_t len;
-	int stem_map_len, *stem_map;
+	int *stem_map;
 	struct mmap_area *mmap_area;
-
-	uint32_t i;
-	uint32_t *magic;
-	uint32_t *section_len;
-	uint32_t *plen;
+	uint32_t i, magic, version;
+	uint32_t entry_len, stem_map_len, regex_array_len;
 
 	rc = snprintf(mmap_path, sizeof(mmap_path), "%s.bin", path);
 	if (rc >= (int)sizeof(mmap_path))
@@ -304,57 +300,86 @@ static int load_mmap(struct selabel_handle *rec, const char *path, struct stat *
 	data->mmap_areas = mmap_area;
 
 	/* check if this looks like an fcontext file */
-	magic = (uint32_t *)addr;
-	if (*magic != SELINUX_MAGIC_COMPILED_FCONTEXT)
+	rc = next_entry(&magic, mmap_area, sizeof(uint32_t));
+	if (rc < 0 || magic != SELINUX_MAGIC_COMPILED_FCONTEXT)
 		return -1;
-	addr += sizeof(uint32_t);
 
 	/* check if this version is higher than we understand */
-	section_len = (uint32_t *)addr;
-	if (*section_len > SELINUX_COMPILED_FCONTEXT_MAX_VERS)
+	rc = next_entry(&version, mmap_area, sizeof(uint32_t));
+	if (rc < 0 || version > SELINUX_COMPILED_FCONTEXT_MAX_VERS)
 		return -1;
-	addr += sizeof(uint32_t);
 
-	if (*section_len >= SELINUX_COMPILED_FCONTEXT_PCRE_VERS) {
+	if (version >= SELINUX_COMPILED_FCONTEXT_PCRE_VERS) {
 		len = strlen(pcre_version());
-		plen = (uint32_t *)addr;
-		if (*plen > mmap_area->len)
-			return -1; /* runs off the end of the map */
-		if (len != *plen)
-			return -1; /* pcre version length mismatch */
-		addr += sizeof(uint32_t);
-		if (memcmp((char *)addr, pcre_version(), len))
-			return -1; /* pcre version content mismatch */
-		if (addr + *plen >= (char *)mmap_area->addr + mmap_area->len)
-			return -1; /* Buffer over-run */
-		addr += *plen;
+
+		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t));
+		if (rc < 0)
+			return -1;
+
+		/* Check version lengths */
+		if (len != entry_len)
+			return -1;
+
+		/* Check if pcre version mismatch */
+		str_buf = malloc(entry_len + 1);
+		if (!str_buf)
+			return -1;
+
+		rc = next_entry(str_buf, mmap_area, entry_len);
+		if (rc < 0) {
+			free(str_buf);
+			return -1;
+		}
+
+		str_buf[entry_len] = '\0';
+		if ((strcmp(str_buf, pcre_version()) != 0)) {
+			free(str_buf);
+			return -1;
+		}
+		free(str_buf);
 	}
 
 	/* allocate the stems_data array */
-	section_len = (uint32_t *)addr;
-	addr += sizeof(uint32_t);
+	rc = next_entry(&stem_map_len, mmap_area, sizeof(uint32_t));
+	if (rc < 0 || !stem_map_len)
+		return -1;
 
 	/*
 	 * map indexed by the stem # in the mmap file and contains the stem
 	 * number in the data stem_arr
 	 */
-	stem_map_len = *section_len;
 	stem_map = calloc(stem_map_len, sizeof(*stem_map));
 	if (!stem_map)
 		return -1;
 
-	for (i = 0; i < *section_len; i++) {
+	for (i = 0; i < stem_map_len; i++) {
 		char *buf;
 		uint32_t stem_len;
 		int newid;
 
 		/* the length does not inlude the nul */
-		plen = (uint32_t *)addr;
-		addr += sizeof(uint32_t);
+		rc = next_entry(&stem_len, mmap_area, sizeof(uint32_t));
+		if (rc < 0 || !stem_len) {
+			rc = -1;
+			goto err;
+		}
 
-		stem_len = *plen;
-		buf = (char *)addr;
-		addr += (stem_len + 1); // +1 is the nul
+		/* Check for stem_len wrap around. */
+		if (stem_len < UINT32_MAX) {
+			buf = (char *)mmap_area->addr;
+			/* Check if over-run before null check. */
+			rc = next_entry(NULL, mmap_area, (stem_len + 1));
+			if (rc < 0)
+				goto err;
+
+			if (buf[stem_len] != '\0') {
+				rc = -1;
+				goto err;
+			}
+		} else {
+			rc = -1;
+			goto err;
+		}
 
 		/* store the mapping between old and new */
 		newid = find_stem(data, buf, stem_len);
@@ -370,12 +395,15 @@ static int load_mmap(struct selabel_handle *rec, const char *path, struct stat *
 	}
 
 	/* allocate the regex array */
-	section_len = (uint32_t *)addr;
-	addr += sizeof(*section_len);
+	rc = next_entry(&regex_array_len, mmap_area, sizeof(uint32_t));
+	if (rc < 0 || !regex_array_len) {
+		rc = -1;
+		goto err;
+	}
 
-	for (i = 0; i < *section_len; i++) {
+	for (i = 0; i < regex_array_len; i++) {
 		struct spec *spec;
-		int32_t stem_id;
+		int32_t stem_id, meta_chars;
 
 		rc = grow_specs(data);
 		if (rc < 0)
@@ -385,53 +413,105 @@ static int load_mmap(struct selabel_handle *rec, const char *path, struct stat *
 		spec->from_mmap = 1;
 		spec->regcomp = 1;
 
-		plen = (uint32_t *)addr;
-		addr += sizeof(uint32_t);
-		rc = -1;
-		spec->lr.ctx_raw = strdup((char *)addr);
-		if (!spec->lr.ctx_raw)
+		/* Process context */
+		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t));
+		if (rc < 0 || !entry_len) {
+			rc = -1;
 			goto err;
+		}
 
-		if (addr + *plen >= (char *)mmap_area->addr + mmap_area->len)
-			return -1;
-		addr += *plen;
+		str_buf = malloc(entry_len);
+		if (!str_buf) {
+			rc = -1;
+			goto err;
+		}
+		rc = next_entry(str_buf, mmap_area, entry_len);
+		if (rc < 0)
+			goto err;
 
-		plen = (uint32_t *)addr;
-		addr += sizeof(uint32_t);
-		spec->regex_str = (char *)addr;
-		if (addr + *plen >= (char *)mmap_area->addr + mmap_area->len)
-			return -1;
-		addr += *plen;
+		if (str_buf[entry_len - 1] != '\0') {
+			free(str_buf);
+			rc = -1;
+			goto err;
+		}
+		spec->lr.ctx_raw = str_buf;
 
-		spec->mode = *(mode_t *)addr;
-		addr += sizeof(mode_t);
+		/* Process regex string */
+		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t));
+		if (rc < 0 || !entry_len) {
+			rc = -1;
+			goto err;
+		}
+
+		spec->regex_str = (char *)mmap_area->addr;
+		rc = next_entry(NULL, mmap_area, entry_len);
+		if (rc < 0)
+			goto err;
+
+		if (spec->regex_str[entry_len - 1] != '\0') {
+			rc = -1;
+			goto err;
+		}
+
+		/* Process mode */
+		rc = next_entry(&spec->mode, mmap_area, sizeof(mode_t));
+		if (rc < 0)
+			goto err;
 
 		/* map the stem id from the mmap file to the data->stem_arr */
-		stem_id = *(int32_t *)addr;
-		if (stem_id == -1 || stem_id >= stem_map_len)
+		rc = next_entry(&stem_id, mmap_area, sizeof(int32_t));
+		if (rc < 0)
+			goto err;
+
+		if (stem_id < 0 || stem_id >= stem_map_len)
 			spec->stem_id = -1;
-		else
+		 else
 			spec->stem_id = stem_map[stem_id];
-		addr += sizeof(int32_t);
 
 		/* retrieve the hasMetaChars bit */
-		spec->hasMetaChars = *(uint32_t *)addr;
-		addr += sizeof(uint32_t);
+		rc = next_entry(&meta_chars, mmap_area, sizeof(uint32_t));
+		if (rc < 0)
+			goto err;
 
-		plen = (uint32_t *)addr;
-		addr += sizeof(uint32_t);
-		spec->regex = (pcre *)addr;
-		if (addr + *plen >= (char *)mmap_area->addr + mmap_area->len)
-			return -1;
-		addr += *plen;
+		spec->hasMetaChars = meta_chars;
+
+		/* Process regex and study_data entries */
+		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t));
+		if (rc < 0 || !entry_len) {
+			rc = -1;
+			goto err;
+		}
+		spec->regex = (pcre *)mmap_area->addr;
+		rc = next_entry(NULL, mmap_area, entry_len);
+		if (rc < 0)
+			goto err;
+
+		/* Check that regex lengths match. pcre_fullinfo()
+		 * also validates its magic number. */
+		rc = pcre_fullinfo(spec->regex, NULL, PCRE_INFO_SIZE, &len);
+		if (rc < 0 || len != entry_len) {
+			rc = -1;
+			goto err;
+		}
 
-		plen = (uint32_t *)addr;
-		addr += sizeof(uint32_t);
-		spec->lsd.study_data = (void *)addr;
+		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t));
+		if (rc < 0 || !entry_len) {
+			rc = -1;
+			goto err;
+		}
+		spec->lsd.study_data = (void *)mmap_area->addr;
 		spec->lsd.flags |= PCRE_EXTRA_STUDY_DATA;
-		if (addr + *plen >= (char *)mmap_area->addr + mmap_area->len)
-			return -1;
-		addr += *plen;
+		rc = next_entry(NULL, mmap_area, entry_len);
+		if (rc < 0)
+			goto err;
+
+		/* Check that study data lengths match. */
+		rc = pcre_fullinfo(spec->regex, &spec->lsd,
+				    PCRE_INFO_STUDYSIZE, &len);
+		if (rc < 0 || len != entry_len) {
+			rc = -1;
+			goto err;
+		}
 
 		data->nspec++;
 	}
diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
index d37008f..3d963b4 100644
--- a/libselinux/src/label_file.h
+++ b/libselinux/src/label_file.h
@@ -43,8 +43,8 @@ struct stem {
 
 /* Where we map the file in during selabel_open() */
 struct mmap_area {
-	void *addr;
-	size_t len;
+	void *addr;	/* Start of area - gets incremented by next_entry() */
+	size_t len;	/* Length - gets decremented by next_entry() */
 	struct mmap_area *next;
 };
 
@@ -297,4 +297,20 @@ static inline int find_stem_from_spec(struct saved_data *data, const char *buf)
 	return store_stem(data, stem, stem_len);
 }
 
+/* This will always check for buffer over-runs and either read the next entry
+ * if buf != NULL or skip over the entry (as these areas are mapped in the
+ * current buffer). */
+static inline int next_entry(void *buf, struct mmap_area *fp, size_t bytes)
+{
+	if (bytes > fp->len)
+		return -1;
+
+	if (buf)
+		memcpy(buf, fp->addr, bytes);
+
+	fp->addr = (char *)fp->addr + bytes;
+	fp->len -= bytes;
+	return 0;
+}
+
 #endif /* _SELABEL_FILE_H_ */
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2015-05-26 15:42 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-24 17:35 [PATCH V4] Fix more bin file processing core dumps Richard Haines
2015-05-26 15:42 ` Stephen Smalley

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.