[PATCH] Fix more bin file processing core dumps

[PATCH] 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].

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

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

diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index c722f29..d6103d8 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>
@@ -248,9 +247,9 @@ static int load_mmap(struct selabel_handle *rec, const char *path, struct stat *
 	struct mmap_area *mmap_area;
 
 	uint32_t i;
-	uint32_t *magic;
-	uint32_t *section_len;
-	uint32_t *plen;
+	uint32_t magic[2];
+	uint32_t section_len[2];
+	uint32_t plen[2];
 
 	rc = snprintf(mmap_path, sizeof(mmap_path), "%s.bin", path);
 	if (rc >= (int)sizeof(mmap_path))
@@ -304,35 +303,36 @@ 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(section_len, mmap_area, sizeof(uint32_t));
+	if (rc < 0 || *section_len > SELINUX_COMPILED_FCONTEXT_MAX_VERS)
 		return -1;
-	addr += sizeof(uint32_t);
 
 	if (*section_len >= 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))
+
+		rc = next_entry(plen, mmap_area, sizeof(uint32_t));
+		if (rc < 0)
+			return -1;
+
+		/* Check for over-run then safe to do memcmp */
+		rc = next_entry(NULL, mmap_area, *plen);
+		if (rc < 0)
+			return -1;
+
+		if (memcmp((char *)mmap_area->addr - *plen,
+				   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;
 	}
 
 	/* allocate the stems_data array */
-	section_len = (uint32_t *)addr;
-	addr += sizeof(uint32_t);
+	rc = next_entry(section_len, mmap_area, sizeof(uint32_t));
+	if (rc < 0)
+		return -1;
 
 	/*
 	 * map indexed by the stem # in the mmap file and contains the stem
@@ -349,12 +349,16 @@ static int load_mmap(struct selabel_handle *rec, const char *path, struct stat *
 		int newid;
 
 		/* the length does not inlude the nul */
-		plen = (uint32_t *)addr;
-		addr += sizeof(uint32_t);
+		rc = next_entry(plen, mmap_area, sizeof(uint32_t));
+		if (rc < 0)
+			goto err;
 
 		stem_len = *plen;
-		buf = (char *)addr;
-		addr += (stem_len + 1); // +1 is the nul
+		buf = (char *)mmap_area->addr;
+						/* +1 for null */
+		rc = next_entry(NULL, mmap_area, (stem_len + 1));
+		if (rc < 0)
+			goto err;
 
 		/* store the mapping between old and new */
 		newid = find_stem(data, buf, stem_len);
@@ -370,12 +374,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(section_len, mmap_area, sizeof(uint32_t));
+	if (rc < 0)
+		goto err;
 
 	for (i = 0; i < *section_len; i++) {
 		struct spec *spec;
-		int32_t stem_id;
+		int32_t stem_id[2];
+		int32_t meta_chars[2];
+		char *str_buf;
 
 		rc = grow_specs(data);
 		if (rc < 0)
@@ -385,53 +392,85 @@ 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 by malloc space plen size, check for NULL,
+		 * then add ptr to spec->lr.ctx_raw. */
+		rc = next_entry(plen, mmap_area, sizeof(uint32_t));
+		if (rc < 0)
 			goto err;
 
-		if (addr + *plen >= (char *)mmap_area->addr + mmap_area->len)
-			return -1;
-		addr += *plen;
+		str_buf = malloc(*plen);
+		if (!str_buf) {
+			rc = -1;
+			goto err;
+		}
+		rc = next_entry(str_buf, mmap_area, *plen);
+		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 (strlen(str_buf) != *plen - 1) {
+			free(str_buf);
+			rc = -1;
+			goto err;
+		}
+		spec->lr.ctx_raw = str_buf;
+
+		/* Process regex string by adding addr to spec->regex_str,
+		 * skip over entry to catch over-runs, then check if
+		 * NULL terminated. */
+		rc = next_entry(plen, mmap_area, sizeof(uint32_t));
+		if (rc < 0)
+			goto err;
 
-		spec->mode = *(mode_t *)addr;
-		addr += sizeof(mode_t);
+		spec->regex_str = (char *)mmap_area->addr;
+		rc = next_entry(NULL, mmap_area, *plen);
+		if (rc < 0)
+			goto err;
+
+		if (strlen(spec->regex_str) != *plen - 1) {
+			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
-			spec->stem_id = stem_map[stem_id];
-		addr += sizeof(int32_t);
+		 else
+			spec->stem_id = stem_map[*stem_id];
 
 		/* 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[0];
+
+		/* Process regex and study_data entries, skip over
+		 * plen entries to catch over-runs. */
+		rc = next_entry(plen, mmap_area, sizeof(uint32_t));
+		if (rc < 0)
+			goto err;
+		spec->regex = (pcre *)mmap_area->addr;
+		rc = next_entry(NULL, mmap_area, *plen);
+		if (rc < 0)
+			goto err;
 
-		plen = (uint32_t *)addr;
-		addr += sizeof(uint32_t);
-		spec->lsd.study_data = (void *)addr;
+		rc = next_entry(plen, mmap_area, sizeof(uint32_t));
+		if (rc < 0)
+			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, *plen);
+		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] Fix more bin file processing core dumps

[Stephen Smalley]

On 05/14/2015 10:36 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].
> 
> 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
> 
> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
> ---
>  libselinux/src/label_file.c | 167 +++++++++++++++++++++++++++-----------------
>  libselinux/src/label_file.h |  20 +++++-
>  2 files changed, 121 insertions(+), 66 deletions(-)
> 
> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> index c722f29..d6103d8 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>
> @@ -248,9 +247,9 @@ static int load_mmap(struct selabel_handle *rec, const char *path, struct stat *
>  	struct mmap_area *mmap_area;
>  
>  	uint32_t i;
> -	uint32_t *magic;
> -	uint32_t *section_len;
> -	uint32_t *plen;
> +	uint32_t magic[2];
> +	uint32_t section_len[2];
> +	uint32_t plen[2];

Why isn't this just:
uint32_t magic, section_len, plen;
and use &magic, &section_len, &plen in calls to next_entry() and magic,
section_len, and plen in place of *magic, *section_len, *plen.

> @@ -304,35 +303,36 @@ 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;

Then this becomes:
rc = next_entry(&magic, mmap_area, sizeof(uint32_t));
if (rc < 0 || magic != SELINUX_MAGIC_COMPILED_FCONTEXT)
    return -1;

Or, alternatively, you could do it as in libsepol and declare:
uint32_t buf[2];
rc = next_entry(buf, mmap_area, sizeof buf);
and read several values at one time, e.g. the magic as buf[0], the
version as buf[1], and either the pcre version string length or the stem
map length as buf[2].

BTW, there is no endianness conversion here, unlike libsepol for binary
policies?  So you have to ensure that you generate file_contexts.bin on
the same endian architecture as you use it?  That's a bit worrisome
although probably not an issue at the moment.  Can't change that though
without more substantial alteration and a version change, so don't worry
about it for now.

> -	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(section_len, mmap_area, sizeof(uint32_t));
> +	if (rc < 0 || *section_len > SELINUX_COMPILED_FCONTEXT_MAX_VERS)
>  		return -1;

Kind of strange that we reuse a variable called section_len for the
binary file format version, the pcre string version length, and the stem
array length.

> -	addr += sizeof(uint32_t);
>  
>  	if (*section_len >= 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 */

You want to retain the length comparison before doing the memcmp.

> -		addr += sizeof(uint32_t);
> -		if (memcmp((char *)addr, pcre_version(), len))
> +
> +		rc = next_entry(plen, mmap_area, sizeof(uint32_t));
> +		if (rc < 0)
> +			return -1;
> +
> +		/* Check for over-run then safe to do memcmp */
> +		rc = next_entry(NULL, mmap_area, *plen);
> +		if (rc < 0)
> +			return -1;
> +
> +		if (memcmp((char *)mmap_area->addr - *plen,
> +				   pcre_version(), len))

This is a strange idiom.  I'd prefer to either allocate a string and
copy it in, as we do in libsepol for e.g. the policydb_str, or just
perform the length check inline first and and don't call next_entry()
until after comparing.

> @@ -349,12 +349,16 @@ static int load_mmap(struct selabel_handle *rec, const char *path, struct stat *
>  		int newid;
>  
>  		/* the length does not inlude the nul */
> -		plen = (uint32_t *)addr;
> -		addr += sizeof(uint32_t);
> +		rc = next_entry(plen, mmap_area, sizeof(uint32_t));
> +		if (rc < 0)
> +			goto err;
>  
>  		stem_len = *plen;
> -		buf = (char *)addr;
> -		addr += (stem_len + 1); // +1 is the nul
> +		buf = (char *)mmap_area->addr;
> +						/* +1 for null */
> +		rc = next_entry(NULL, mmap_area, (stem_len + 1));
> +		if (rc < 0)
> +			goto err;

Likewise, and where do you check that buf[stem_len] == '\0'?
Otherwise find_stem() can run off the end.  Also, can stem_len + 1
overflow to 0?

This btw was another mistake in the file format; should have been at
least consistent throughout as to whether or not strings were
NUL-terminated and no real reason to include them in the file since we
have to verify it is NUL-terminated at load anyway - might as well just
add it at load time too.

>  
>  		/* store the mapping between old and new */
>  		newid = find_stem(data, buf, stem_len);
> @@ -370,12 +374,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(section_len, mmap_area, sizeof(uint32_t));
> +	if (rc < 0)
> +		goto err;
>  
>  	for (i = 0; i < *section_len; i++) {
>  		struct spec *spec;
> -		int32_t stem_id;
> +		int32_t stem_id[2];
> +		int32_t meta_chars[2];

Don't need arrays here either.

> +		char *str_buf;
>  
>  		rc = grow_specs(data);
>  		if (rc < 0)
> @@ -385,53 +392,85 @@ 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 by malloc space plen size, check for NULL,
> +		 * then add ptr to spec->lr.ctx_raw. */
> +		rc = next_entry(plen, mmap_area, sizeof(uint32_t));
> +		if (rc < 0)
>  			goto err;
>  
> -		if (addr + *plen >= (char *)mmap_area->addr + mmap_area->len)
> -			return -1;
> -		addr += *plen;
> +		str_buf = malloc(*plen);
> +		if (!str_buf) {
> +			rc = -1;
> +			goto err;
> +		}
> +		rc = next_entry(str_buf, mmap_area, *plen);
> +		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 (strlen(str_buf) != *plen - 1) {

No, check that str_buf[*plen-1] == '\0'; strlen(str_buf) will run off
the end, potentially unbounded, if not terminated.

> +			free(str_buf);
> +			rc = -1;
> +			goto err;
> +		}
> +		spec->lr.ctx_raw = str_buf;
> +
> +		/* Process regex string by adding addr to spec->regex_str,
> +		 * skip over entry to catch over-runs, then check if
> +		 * NULL terminated. */
> +		rc = next_entry(plen, mmap_area, sizeof(uint32_t));
> +		if (rc < 0)
> +			goto err;
>  
> -		spec->mode = *(mode_t *)addr;
> -		addr += sizeof(mode_t);
> +		spec->regex_str = (char *)mmap_area->addr;
> +		rc = next_entry(NULL, mmap_area, *plen);
> +		if (rc < 0)
> +			goto err;
> +
> +		if (strlen(spec->regex_str) != *plen - 1) {
> +			rc = -1;
> +			goto err;
> +		}

Same as above.

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

[Stephen Smalley]

On 05/14/2015 11:28 AM, Stephen Smalley wrote:
> On 05/14/2015 10:36 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].
>>
>> 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
>>
>> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
>> ---
>>  libselinux/src/label_file.c | 167 +++++++++++++++++++++++++++-----------------
>>  libselinux/src/label_file.h |  20 +++++-
>>  2 files changed, 121 insertions(+), 66 deletions(-)
>>
>> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
>> index c722f29..d6103d8 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>
>> @@ -248,9 +247,9 @@ static int load_mmap(struct selabel_handle *rec, const char *path, struct stat *
>>  	struct mmap_area *mmap_area;
>>  
>>  	uint32_t i;
>> -	uint32_t *magic;
>> -	uint32_t *section_len;
>> -	uint32_t *plen;
>> +	uint32_t magic[2];
>> +	uint32_t section_len[2];
>> +	uint32_t plen[2];
> 
> Why isn't this just:
> uint32_t magic, section_len, plen;
> and use &magic, &section_len, &plen in calls to next_entry() and magic,
> section_len, and plen in place of *magic, *section_len, *plen.
> 
>> @@ -304,35 +303,36 @@ 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;
> 
> Then this becomes:
> rc = next_entry(&magic, mmap_area, sizeof(uint32_t));
> if (rc < 0 || magic != SELINUX_MAGIC_COMPILED_FCONTEXT)
>     return -1;
> 
> Or, alternatively, you could do it as in libsepol and declare:
> uint32_t buf[2];

Sorry, should be buf[3] in that example.

> rc = next_entry(buf, mmap_area, sizeof buf);
> and read several values at one time, e.g. the magic as buf[0], the
> version as buf[1], and either the pcre version string length or the stem
> map length as buf[2].
> 
> BTW, there is no endianness conversion here, unlike libsepol for binary
> policies?  So you have to ensure that you generate file_contexts.bin on
> the same endian architecture as you use it?  That's a bit worrisome
> although probably not an issue at the moment.  Can't change that though
> without more substantial alteration and a version change, so don't worry
> about it for now.
> 
>> -	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(section_len, mmap_area, sizeof(uint32_t));
>> +	if (rc < 0 || *section_len > SELINUX_COMPILED_FCONTEXT_MAX_VERS)
>>  		return -1;
> 
> Kind of strange that we reuse a variable called section_len for the
> binary file format version, the pcre string version length, and the stem
> array length.
> 
>> -	addr += sizeof(uint32_t);
>>  
>>  	if (*section_len >= 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 */
> 
> You want to retain the length comparison before doing the memcmp.
> 
>> -		addr += sizeof(uint32_t);
>> -		if (memcmp((char *)addr, pcre_version(), len))
>> +
>> +		rc = next_entry(plen, mmap_area, sizeof(uint32_t));
>> +		if (rc < 0)
>> +			return -1;
>> +
>> +		/* Check for over-run then safe to do memcmp */
>> +		rc = next_entry(NULL, mmap_area, *plen);
>> +		if (rc < 0)
>> +			return -1;
>> +
>> +		if (memcmp((char *)mmap_area->addr - *plen,
>> +				   pcre_version(), len))
> 
> This is a strange idiom.  I'd prefer to either allocate a string and
> copy it in, as we do in libsepol for e.g. the policydb_str, or just
> perform the length check inline first and and don't call next_entry()
> until after comparing.
> 
>> @@ -349,12 +349,16 @@ static int load_mmap(struct selabel_handle *rec, const char *path, struct stat *
>>  		int newid;
>>  
>>  		/* the length does not inlude the nul */
>> -		plen = (uint32_t *)addr;
>> -		addr += sizeof(uint32_t);
>> +		rc = next_entry(plen, mmap_area, sizeof(uint32_t));
>> +		if (rc < 0)
>> +			goto err;
>>  
>>  		stem_len = *plen;
>> -		buf = (char *)addr;
>> -		addr += (stem_len + 1); // +1 is the nul
>> +		buf = (char *)mmap_area->addr;
>> +						/* +1 for null */
>> +		rc = next_entry(NULL, mmap_area, (stem_len + 1));
>> +		if (rc < 0)
>> +			goto err;
> 
> Likewise, and where do you check that buf[stem_len] == '\0'?
> Otherwise find_stem() can run off the end.  Also, can stem_len + 1
> overflow to 0?
> 
> This btw was another mistake in the file format; should have been at
> least consistent throughout as to whether or not strings were
> NUL-terminated and no real reason to include them in the file since we
> have to verify it is NUL-terminated at load anyway - might as well just
> add it at load time too.
> 
>>  
>>  		/* store the mapping between old and new */
>>  		newid = find_stem(data, buf, stem_len);
>> @@ -370,12 +374,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(section_len, mmap_area, sizeof(uint32_t));
>> +	if (rc < 0)
>> +		goto err;
>>  
>>  	for (i = 0; i < *section_len; i++) {
>>  		struct spec *spec;
>> -		int32_t stem_id;
>> +		int32_t stem_id[2];
>> +		int32_t meta_chars[2];
> 
> Don't need arrays here either.
> 
>> +		char *str_buf;
>>  
>>  		rc = grow_specs(data);
>>  		if (rc < 0)
>> @@ -385,53 +392,85 @@ 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 by malloc space plen size, check for NULL,
>> +		 * then add ptr to spec->lr.ctx_raw. */
>> +		rc = next_entry(plen, mmap_area, sizeof(uint32_t));
>> +		if (rc < 0)
>>  			goto err;
>>  
>> -		if (addr + *plen >= (char *)mmap_area->addr + mmap_area->len)
>> -			return -1;
>> -		addr += *plen;
>> +		str_buf = malloc(*plen);
>> +		if (!str_buf) {
>> +			rc = -1;
>> +			goto err;
>> +		}
>> +		rc = next_entry(str_buf, mmap_area, *plen);
>> +		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 (strlen(str_buf) != *plen - 1) {
> 
> No, check that str_buf[*plen-1] == '\0'; strlen(str_buf) will run off
> the end, potentially unbounded, if not terminated.
> 
>> +			free(str_buf);
>> +			rc = -1;
>> +			goto err;
>> +		}
>> +		spec->lr.ctx_raw = str_buf;
>> +
>> +		/* Process regex string by adding addr to spec->regex_str,
>> +		 * skip over entry to catch over-runs, then check if
>> +		 * NULL terminated. */
>> +		rc = next_entry(plen, mmap_area, sizeof(uint32_t));
>> +		if (rc < 0)
>> +			goto err;
>>  
>> -		spec->mode = *(mode_t *)addr;
>> -		addr += sizeof(mode_t);
>> +		spec->regex_str = (char *)mmap_area->addr;
>> +		rc = next_entry(NULL, mmap_area, *plen);
>> +		if (rc < 0)
>> +			goto err;
>> +
>> +		if (strlen(spec->regex_str) != *plen - 1) {
>> +			rc = -1;
>> +			goto err;
>> +		}
> 
> Same as above.
> 

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

[Richard Haines]

> On Thursday, 14 May 2015, 16:35, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > On 05/14/2015 11:28 AM, Stephen Smalley wrote:
>>  On 05/14/2015 10:36 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].
>>> 
>>>  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
>>> 
>>>  Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
>>>  ---
>>>   libselinux/src/label_file.c | 167 
> +++++++++++++++++++++++++++-----------------
>>>   libselinux/src/label_file.h |  20 +++++-
>>>   2 files changed, 121 insertions(+), 66 deletions(-)
>>> 
>>>  diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
>>>  index c722f29..d6103d8 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>
>>>  @@ -248,9 +247,9 @@ static int load_mmap(struct selabel_handle *rec, 
> const char *path, struct stat *
>>>       struct mmap_area *mmap_area;
>>>   
>>>       uint32_t i;
>>>  -    uint32_t *magic;
>>>  -    uint32_t *section_len;
>>>  -    uint32_t *plen;
>>>  +    uint32_t magic[2];
>>>  +    uint32_t section_len[2];
>>>  +    uint32_t plen[2];
>> 
>>  Why isn't this just:
>>  uint32_t magic, section_len, plen;
>>  and use &magic, &section_len, &plen in calls to next_entry() 
> and magic,
>>  section_len, and plen in place of *magic, *section_len, *plen.
>> 
>>>  @@ -304,35 +303,36 @@ 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;
>> 
>>  Then this becomes:
>>  rc = next_entry(&magic, mmap_area, sizeof(uint32_t));
>>  if (rc < 0 || magic != SELINUX_MAGIC_COMPILED_FCONTEXT)
>>      return -1;
>> 
>>  Or, alternatively, you could do it as in libsepol and declare:
>>  uint32_t buf[2];
> 
> Sorry, should be buf[3] in that example.
> 
> 
>>  rc = next_entry(buf, mmap_area, sizeof buf);
>>  and read several values at one time, e.g. the magic as buf[0], the
>>  version as buf[1], and either the pcre version string length or the stem

>>  map length as buf[2].
I'll fix all the problems highlighted and send a V2 next week

>> 
>>  BTW, there is no endianness conversion here, unlike libsepol for binary
>>  policies?  So you have to ensure that you generate file_contexts.bin on
>>  the same endian architecture as you use it?  That's a bit worrisome
>>  although probably not an issue at the moment.  Can't change that though
>>  without more substantial alteration and a version change, so don't 
> worry

>>  about it for now.
I guess this would need to be done for acceptance into Android ??. If so,
and as the overall objective of what I started was Android support, then
I'm willing to give this a shot by following the way libsepol handles
this. All advice welcome.

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

[Stephen Smalley]

On 05/15/2015 10:58 AM, Richard Haines wrote:
> 
> 
> 
> 
> 
>> On Thursday, 14 May 2015, 16:35, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>> On 05/14/2015 11:28 AM, Stephen Smalley wrote:
>>>  On 05/14/2015 10:36 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].
>>>>
>>>>  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
>>>>
>>>>  Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
>>>>  ---
>>>>   libselinux/src/label_file.c | 167 
>> +++++++++++++++++++++++++++-----------------
>>>>   libselinux/src/label_file.h |  20 +++++-
>>>>   2 files changed, 121 insertions(+), 66 deletions(-)
>>>>
>>>>  diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
>>>>  index c722f29..d6103d8 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>
>>>>  @@ -248,9 +247,9 @@ static int load_mmap(struct selabel_handle *rec, 
>> const char *path, struct stat *
>>>>       struct mmap_area *mmap_area;
>>>>   
>>>>       uint32_t i;
>>>>  -    uint32_t *magic;
>>>>  -    uint32_t *section_len;
>>>>  -    uint32_t *plen;
>>>>  +    uint32_t magic[2];
>>>>  +    uint32_t section_len[2];
>>>>  +    uint32_t plen[2];
>>>
>>>  Why isn't this just:
>>>  uint32_t magic, section_len, plen;
>>>  and use &magic, &section_len, &plen in calls to next_entry() 
>> and magic,
>>>  section_len, and plen in place of *magic, *section_len, *plen.
>>>
>>>>  @@ -304,35 +303,36 @@ 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;
>>>
>>>  Then this becomes:
>>>  rc = next_entry(&magic, mmap_area, sizeof(uint32_t));
>>>  if (rc < 0 || magic != SELINUX_MAGIC_COMPILED_FCONTEXT)
>>>      return -1;
>>>
>>>  Or, alternatively, you could do it as in libsepol and declare:
>>>  uint32_t buf[2];
>>
>> Sorry, should be buf[3] in that example.
>>
>>
>>>  rc = next_entry(buf, mmap_area, sizeof buf);
>>>  and read several values at one time, e.g. the magic as buf[0], the
>>>  version as buf[1], and either the pcre version string length or the stem
> 
>>>  map length as buf[2].
> I'll fix all the problems highlighted and send a V2 next week
> 
>>>
>>>  BTW, there is no endianness conversion here, unlike libsepol for binary
>>>  policies?  So you have to ensure that you generate file_contexts.bin on
>>>  the same endian architecture as you use it?  That's a bit worrisome
>>>  although probably not an issue at the moment.  Can't change that though
>>>  without more substantial alteration and a version change, so don't 
>> worry
> 
>>>  about it for now.
> I guess this would need to be done for acceptance into Android ??. If so,
> and as the overall objective of what I started was Android support, then
> I'm willing to give this a shot by following the way libsepol handles
> this. All advice welcome.

Don't think it is a requirement.
Also not sure if file_contexts.bin might be inherently endian-specific
due to storing the compiled regex.