[PATCH V3] Fix more bin file processing core dumps

[PATCH V3] Fix more bin file processing core dumps

[Richard Haines]

The reading of bin files has been changed to follow that of loading
policy to catch over-runs. Entries that should be NULL 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.

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

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

diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index c722f29..64839e0 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,80 @@ 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 || entry_len <= 0)
+			return -1;
+
+		/* Check version lengths */
+		if (len != entry_len)
+			return -1;
+
+		/* Check if pcre version mismatch */
+		str_buf = malloc(entry_len);
+		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 <= 0)
+		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 <= 0) {
+			rc = -1;
+			goto err;
+		}
+
+		buf = (char *)mmap_area->addr;
+		/* Check if buf is going to over-run before null check. */
+		rc = next_entry(NULL, mmap_area, (stem_len + 1));
+		if (rc < 0)
+			goto err;
 
-		stem_len = *plen;
-		buf = (char *)addr;
-		addr += (stem_len + 1); // +1 is the nul
+		if (buf[stem_len] != '\0') {
+			rc = -1;
+			goto err;
+		}
 
 		/* store the mapping between old and new */
 		newid = find_stem(data, buf, stem_len);
@@ -370,12 +389,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 <= 0) {
+		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 +407,89 @@ 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 <= 0) {
+			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;
+
+		/* Process regex string */
+		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t));
+		if (rc < 0 || entry_len <= 0) {
+			rc = -1;
+			goto err;
+		}
 
-		spec->mode = *(mode_t *)addr;
-		addr += sizeof(mode_t);
+		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;
 
-		plen = (uint32_t *)addr;
-		addr += sizeof(uint32_t);
-		spec->lsd.study_data = (void *)addr;
+		/* Process regex and study_data entries */
+		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t));
+		if (rc < 0 || entry_len <= 0) {
+			rc = -1;
+			goto err;
+		}
+		spec->regex = (pcre *)mmap_area->addr;
+		rc = next_entry(NULL, mmap_area, entry_len);
+		if (rc < 0)
+			goto err;
+
+		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t));
+		if (rc < 0 || entry_len <= 0) {
+			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;
 
 		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

Re: [PATCH V3] Fix more bin file processing core dumps

[Stephen Smalley]

On 05/19/2015 06:26 AM, Richard Haines wrote:
> The reading of bin files has been changed to follow that of loading
> policy to catch over-runs. Entries that should be NULL 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.
> 
> 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
> 
> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
> ---
>  libselinux/src/label_file.c | 204 ++++++++++++++++++++++++++++----------------
>  libselinux/src/label_file.h |  20 ++++-
>  2 files changed, 149 insertions(+), 75 deletions(-)
> 
> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> index c722f29..64839e0 100644
> --- a/libselinux/src/label_file.c
> +++ b/libselinux/src/label_file.c
> @@ -304,57 +300,80 @@ 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 || entry_len <= 0)
> +			return -1;

Can't be < 0 since it is uint32_t and don't need to separately check for
== 0 because you check the length against the expected version string
length below.

> +
> +		/* Check version lengths */
> +		if (len != entry_len)
> +			return -1;
> +
> +		/* Check if pcre version mismatch */
> +		str_buf = malloc(entry_len);

Need to malloc entry_len + 1 bytes in order to NUL-terminate it below
without buffer overflow.
Also, need to check that entry_len < UINT32_MAX before adding one to it.

> +		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;

Should probably be consistent, e.g. use = '\0' or = 0 throughout.
I think the former is preferred stylistically and maybe for portability,
although I don't know of any case where it would differ.

> +		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 <= 0)
> +		return -1;

Can't be < 0 as it is uint32_t.

>  
>  	/*
>  	 * 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 <= 0) {
> +			rc = -1;
> +			goto err;
> +		}

Likewise, can't be < 0 due to type.

> +
> +		buf = (char *)mmap_area->addr;
> +		/* Check if buf is going to over-run before null check. */
> +		rc = next_entry(NULL, mmap_area, (stem_len + 1));

But you should test that stem_len < UINT32_MAX before adding one to it
so it doesn't wrap around.

> +		if (rc < 0)
> +			goto err;
>  
> -		stem_len = *plen;
> -		buf = (char *)addr;
> -		addr += (stem_len + 1); // +1 is the nul
> +		if (buf[stem_len] != '\0') {
> +			rc = -1;
> +			goto err;
> +		}
>  
>  		/* store the mapping between old and new */
>  		newid = find_stem(data, buf, stem_len);
> @@ -370,12 +389,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 <= 0) {
> +		rc = -1;
> +		goto err;
> +	}

Can't be < 0.

>  
> -	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 +407,89 @@ 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 <= 0) {
> +			rc = -1;
>  			goto err;
> +		}

Only need to check for entry_len != 0, < not possible.

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

This is why you need the != 0 test above.

> +		spec->lr.ctx_raw = str_buf;
> +
> +		/* Process regex string */
> +		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t));
> +		if (rc < 0 || entry_len <= 0) {
> +			rc = -1;
> +			goto err;
> +		}

Don't need the < 0 part of the test, just != 0.
>  
> -		spec->mode = *(mode_t *)addr;
> -		addr += sizeof(mode_t);
> +		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;
>  
> -		plen = (uint32_t *)addr;
> -		addr += sizeof(uint32_t);
> -		spec->lsd.study_data = (void *)addr;
> +		/* Process regex and study_data entries */
> +		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t));
> +		if (rc < 0 || entry_len <= 0) {
> +			rc = -1;
> +			goto err;
> +		}

Don't need the < 0 test, just != 0.

> +		spec->regex = (pcre *)mmap_area->addr;
> +		rc = next_entry(NULL, mmap_area, entry_len);
> +		if (rc < 0)
> +			goto err;

Seems unsafe (we don't do anything to ensure that spec->regex is
pointing to a valid pcre, or that its entry_len is some minimal size
required for the underlying structure) but offhand I don't know if there
is any function exported by libpcre that would allow us to validate it.
 You don't have to address it; just noting it.  Was wondering if we
could use pcre_fullinfo() to get the expected sizes and compare as we do
when writing in sefcontext_compile(), but that presumes that re and sd
are in fact already valid structures so I don't think it helps here.

> +
> +		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t));
> +		if (rc < 0 || entry_len <= 0) {
> +			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;

Likewise for spec->lsd.study_data.

>  
>  		data->nspec++;
>  	}

Re: [PATCH V3] Fix more bin file processing core dumps

[Stephen Smalley]

On 05/19/2015 09:18 AM, Stephen Smalley wrote:
> On 05/19/2015 06:26 AM, Richard Haines wrote:
>> The reading of bin files has been changed to follow that of loading
>> policy to catch over-runs. Entries that should be NULL terminated are

NUL-terminated, not NULL terminated.  NUL == '\0'.  NULL == (void *)0.

>> 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.
>>
>> 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
>>
>> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
>> ---
>>  libselinux/src/label_file.c | 204 ++++++++++++++++++++++++++++----------------
>>  libselinux/src/label_file.h |  20 ++++-
>>  2 files changed, 149 insertions(+), 75 deletions(-)
>>
>> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
>> index c722f29..64839e0 100644
>> --- a/libselinux/src/label_file.c
>> +++ b/libselinux/src/label_file.c
>> @@ -304,57 +300,80 @@ 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 || entry_len <= 0)
>> +			return -1;
> 
> Can't be < 0 since it is uint32_t and don't need to separately check for
> == 0 because you check the length against the expected version string
> length below.
> 
>> +
>> +		/* Check version lengths */
>> +		if (len != entry_len)
>> +			return -1;
>> +
>> +		/* Check if pcre version mismatch */
>> +		str_buf = malloc(entry_len);
> 
> Need to malloc entry_len + 1 bytes in order to NUL-terminate it below
> without buffer overflow.
> Also, need to check that entry_len < UINT32_MAX before adding one to it.

Actually, you don't need the entry_len < UINT32_MAX test here because
you compare it with the expected length above already.  But you do need
it elsewhere when there is no expected length comparison and you add one
for a NUL byte.

> 
>> +		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;
> 
> Should probably be consistent, e.g. use = '\0' or = 0 throughout.
> I think the former is preferred stylistically and maybe for portability,
> although I don't know of any case where it would differ.
> 
>> +		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 <= 0)
>> +		return -1;
> 
> Can't be < 0 as it is uint32_t.
> 
>>  
>>  	/*
>>  	 * 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 <= 0) {
>> +			rc = -1;
>> +			goto err;
>> +		}
> 
> Likewise, can't be < 0 due to type.
> 
>> +
>> +		buf = (char *)mmap_area->addr;
>> +		/* Check if buf is going to over-run before null check. */
>> +		rc = next_entry(NULL, mmap_area, (stem_len + 1));
> 
> But you should test that stem_len < UINT32_MAX before adding one to it
> so it doesn't wrap around.
> 
>> +		if (rc < 0)
>> +			goto err;
>>  
>> -		stem_len = *plen;
>> -		buf = (char *)addr;
>> -		addr += (stem_len + 1); // +1 is the nul
>> +		if (buf[stem_len] != '\0') {
>> +			rc = -1;
>> +			goto err;
>> +		}
>>  
>>  		/* store the mapping between old and new */
>>  		newid = find_stem(data, buf, stem_len);
>> @@ -370,12 +389,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 <= 0) {
>> +		rc = -1;
>> +		goto err;
>> +	}
> 
> Can't be < 0.
> 
>>  
>> -	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 +407,89 @@ 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 <= 0) {
>> +			rc = -1;
>>  			goto err;
>> +		}
> 
> Only need to check for entry_len != 0, < not possible.
> 
>>  
>> -		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;
>> +		}
> 
> This is why you need the != 0 test above.
> 
>> +		spec->lr.ctx_raw = str_buf;
>> +
>> +		/* Process regex string */
>> +		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t));
>> +		if (rc < 0 || entry_len <= 0) {
>> +			rc = -1;
>> +			goto err;
>> +		}
> 
> Don't need the < 0 part of the test, just != 0.
>>  
>> -		spec->mode = *(mode_t *)addr;
>> -		addr += sizeof(mode_t);
>> +		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;
>>  
>> -		plen = (uint32_t *)addr;
>> -		addr += sizeof(uint32_t);
>> -		spec->lsd.study_data = (void *)addr;
>> +		/* Process regex and study_data entries */
>> +		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t));
>> +		if (rc < 0 || entry_len <= 0) {
>> +			rc = -1;
>> +			goto err;
>> +		}
> 
> Don't need the < 0 test, just != 0.
> 
>> +		spec->regex = (pcre *)mmap_area->addr;
>> +		rc = next_entry(NULL, mmap_area, entry_len);
>> +		if (rc < 0)
>> +			goto err;
> 
> Seems unsafe (we don't do anything to ensure that spec->regex is
> pointing to a valid pcre, or that its entry_len is some minimal size
> required for the underlying structure) but offhand I don't know if there
> is any function exported by libpcre that would allow us to validate it.
>  You don't have to address it; just noting it.  Was wondering if we
> could use pcre_fullinfo() to get the expected sizes and compare as we do
> when writing in sefcontext_compile(), but that presumes that re and sd
> are in fact already valid structures so I don't think it helps here.
> 
>> +
>> +		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t));
>> +		if (rc < 0 || entry_len <= 0) {
>> +			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;
> 
> Likewise for spec->lsd.study_data.
> 
>>  
>>  		data->nspec++;
>>  	}
> 
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
> 
>