[PATCH 1/6] libselinux: do not dereference a NULL pointer when calloc() fails

[PATCH 1/6] libselinux: do not dereference a NULL pointer when calloc() fails

[Nicolas Iooss]

selabel_is_digest_set() contains the following code:

        digest = calloc(1, sizeof(*digest));
        if (!digest)
            goto err;

    /* ... */

    err:
        free(digest->digest);

If calloc() failed, digest is NULL but is dereferenced when the
execution jumps to label err.

Check that digest is not NULL before freeing its fields.

This issue has been found using clang's static analyzer.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libselinux/src/label.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/libselinux/src/label.c b/libselinux/src/label.c
index 60639cfcfb74..3300ddc0ab31 100644
--- a/libselinux/src/label.c
+++ b/libselinux/src/label.c
@@ -191,9 +191,11 @@ static inline struct selabel_digest *selabel_is_digest_set
 	return NULL;
 
 err:
-	free(digest->digest);
-	free(digest->specfile_list);
-	free(digest);
+	if (digest) {
+		free(digest->digest);
+		free(digest->specfile_list);
+		free(digest);
+	}
 	return NULL;
 }
 
-- 
2.12.0

[PATCH 2/6] libsemanage: drop checks on semanage_module_info_destroy() value

[Nicolas Iooss]

semanage_module_info_destroy() always returns 0. Nevertheless
semanage_direct_list_all() uses its return value in a surprising way:

    cleanup:
        if (priorities != NULL) {
            /* ... */
            free(priorities);
        }
        /* ... */
        ret = semanage_module_info_destroy(sh, modinfo_tmp);
        if (ret != 0) {
            status = -1;
            goto cleanup;
        }

The last "goto cleanup;" leads clang's static analyzer to believe a
double free is possible. Even though this is a false positive, the
body of condition "if (ret != 0)" contains dead code. Remove it.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsemanage/src/direct_api.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index 568732355f54..1d53c0d64e0c 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -2499,11 +2499,7 @@ static int semanage_direct_list_all(semanage_handle_t *sh,
 				goto cleanup;
 			}
 
-			ret = semanage_module_info_destroy(sh, modinfo_tmp);
-			if (ret != 0) {
-				status = -1;
-				goto cleanup;
-			}
+			semanage_module_info_destroy(sh, modinfo_tmp);
 			free(modinfo_tmp);
 			modinfo_tmp = NULL;
 
@@ -2528,11 +2524,7 @@ cleanup:
 		free(modules);
 	}
 
-	ret = semanage_module_info_destroy(sh, modinfo_tmp);
-	if (ret != 0) {
-		status = -1;
-		goto cleanup;
-	}
+	semanage_module_info_destroy(sh, modinfo_tmp);
 	free(modinfo_tmp);
 	modinfo_tmp = NULL;
 
-- 
2.12.0

Re: [PATCH 2/6] libsemanage: drop checks on semanage_module_info_destroy() value

[Stephen Smalley]

On Fri, 2017-04-07 at 22:44 +0200, Nicolas Iooss wrote:
> semanage_module_info_destroy() always returns 0. Nevertheless
> semanage_direct_list_all() uses its return value in a surprising way:
> 
>     cleanup:
>         if (priorities != NULL) {
>             /* ... */
>             free(priorities);
>         }
>         /* ... */
>         ret = semanage_module_info_destroy(sh, modinfo_tmp);
>         if (ret != 0) {
>             status = -1;
>             goto cleanup;
>         }
> 
> The last "goto cleanup;" leads clang's static analyzer to believe a
> double free is possible. Even though this is a false positive, the
> body of condition "if (ret != 0)" contains dead code. Remove it.

I'm merging this one but we should likely follow up with a patch to
change semodule_module_info_destroy() to explicitly return void.

> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>  libsemanage/src/direct_api.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/libsemanage/src/direct_api.c
> b/libsemanage/src/direct_api.c
> index 568732355f54..1d53c0d64e0c 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -2499,11 +2499,7 @@ static int
> semanage_direct_list_all(semanage_handle_t *sh,
>  				goto cleanup;
>  			}
>  
> -			ret = semanage_module_info_destroy(sh,
> modinfo_tmp);
> -			if (ret != 0) {
> -				status = -1;
> -				goto cleanup;
> -			}
> +			semanage_module_info_destroy(sh,
> modinfo_tmp);
>  			free(modinfo_tmp);
>  			modinfo_tmp = NULL;
>  
> @@ -2528,11 +2524,7 @@ cleanup:
>  		free(modules);
>  	}
>  
> -	ret = semanage_module_info_destroy(sh, modinfo_tmp);
> -	if (ret != 0) {
> -		status = -1;
> -		goto cleanup;
> -	}
> +	semanage_module_info_destroy(sh, modinfo_tmp);
>  	free(modinfo_tmp);
>  	modinfo_tmp = NULL;
>  

[PATCH 3/6] libselinux: make process_boolean() fail on invalid lines

[Nicolas Iooss]

When security_load_booleans() calls process_boolean() to parse a boolean
definition, process_boolean() returns a successful value when it fails
to use strtok_r() (e.g. when there is no "=" in the parsed line). This
leads security_load_booleans() to use uninitialized name and/or val when
setting the boolean into the policy.

This issue has been found using clang's static analyzer and is similar
to the one which has been fixed in libsepol with commit 76f8c04c197f
("libsepol: make process_boolean() fail on invalid lines"). Fix it in
the same way.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libselinux/src/booleans.c | 58 ++++++++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 23 deletions(-)

diff --git a/libselinux/src/booleans.c b/libselinux/src/booleans.c
index ba9d9348e2c7..49452756ffb2 100644
--- a/libselinux/src/booleans.c
+++ b/libselinux/src/booleans.c
@@ -342,30 +342,42 @@ static int process_boolean(char *buffer, char *name, int namesize, int *val)
 {
 	char name1[BUFSIZ];
 	char *ptr = NULL;
-	char *tok = strtok_r(buffer, "=", &ptr);
-	if (tok) {
-		strncpy(name1, tok, BUFSIZ - 1);
-		strtrim(name, name1, namesize - 1);
-		if (name[0] == '#')
-			return 0;
-		tok = strtok_r(NULL, "\0", &ptr);
-		if (tok) {
-			while (isspace(*tok))
-				tok++;
-			*val = -1;
-			if (isdigit(tok[0]))
-				*val = atoi(tok);
-			else if (!strncasecmp(tok, "true", sizeof("true") - 1))
-				*val = 1;
-			else if (!strncasecmp
-				 (tok, "false", sizeof("false") - 1))
-				*val = 0;
-			if (*val != 0 && *val != 1) {
-				errno = EINVAL;
-				return -1;
-			}
+	char *tok;
 
-		}
+	/* Skip spaces */
+	while (isspace(buffer[0]))
+		buffer++;
+	/* Ignore comments */
+	if (buffer[0] == '#')
+		return 0;
+
+	tok = strtok_r(buffer, "=", &ptr);
+	if (!tok) {
+		errno = EINVAL;
+		return -1;
+	}
+	strncpy(name1, tok, BUFSIZ - 1);
+	strtrim(name, name1, namesize - 1);
+
+	tok = strtok_r(NULL, "\0", &ptr);
+	if (!tok) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	while (isspace(*tok))
+		tok++;
+
+	*val = -1;
+	if (isdigit(tok[0]))
+		*val = atoi(tok);
+	else if (!strncasecmp(tok, "true", sizeof("true") - 1))
+		*val = 1;
+	else if (!strncasecmp(tok, "false", sizeof("false") - 1))
+		*val = 0;
+	if (*val != 0 && *val != 1) {
+		errno = EINVAL;
+		return -1;
 	}
 	return 1;
 }
-- 
2.12.0

Re: [PATCH 3/6] libselinux: make process_boolean() fail on invalid lines

[Stephen Smalley]

On Fri, 2017-04-07 at 22:44 +0200, Nicolas Iooss wrote:
> When security_load_booleans() calls process_boolean() to parse a
> boolean
> definition, process_boolean() returns a successful value when it
> fails
> to use strtok_r() (e.g. when there is no "=" in the parsed line).
> This
> leads security_load_booleans() to use uninitialized name and/or val
> when
> setting the boolean into the policy.
> 
> This issue has been found using clang's static analyzer and is
> similar
> to the one which has been fixed in libsepol with commit 76f8c04c197f
> ("libsepol: make process_boolean() fail on invalid lines"). Fix it in
> the same way.
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Merging this one but just for reference, I think this code (and the
corresponding libsepol code) is all legacy at this point.  I'd kill it
except it would be an ABI break.

> ---
>  libselinux/src/booleans.c | 58 ++++++++++++++++++++++++++++---------
> ----------
>  1 file changed, 35 insertions(+), 23 deletions(-)
> 
> diff --git a/libselinux/src/booleans.c b/libselinux/src/booleans.c
> index ba9d9348e2c7..49452756ffb2 100644
> --- a/libselinux/src/booleans.c
> +++ b/libselinux/src/booleans.c
> @@ -342,30 +342,42 @@ static int process_boolean(char *buffer, char
> *name, int namesize, int *val)
>  {
>  	char name1[BUFSIZ];
>  	char *ptr = NULL;
> -	char *tok = strtok_r(buffer, "=", &ptr);
> -	if (tok) {
> -		strncpy(name1, tok, BUFSIZ - 1);
> -		strtrim(name, name1, namesize - 1);
> -		if (name[0] == '#')
> -			return 0;
> -		tok = strtok_r(NULL, "\0", &ptr);
> -		if (tok) {
> -			while (isspace(*tok))
> -				tok++;
> -			*val = -1;
> -			if (isdigit(tok[0]))
> -				*val = atoi(tok);
> -			else if (!strncasecmp(tok, "true",
> sizeof("true") - 1))
> -				*val = 1;
> -			else if (!strncasecmp
> -				 (tok, "false", sizeof("false") -
> 1))
> -				*val = 0;
> -			if (*val != 0 && *val != 1) {
> -				errno = EINVAL;
> -				return -1;
> -			}
> +	char *tok;
>  
> -		}
> +	/* Skip spaces */
> +	while (isspace(buffer[0]))
> +		buffer++;
> +	/* Ignore comments */
> +	if (buffer[0] == '#')
> +		return 0;
> +
> +	tok = strtok_r(buffer, "=", &ptr);
> +	if (!tok) {
> +		errno = EINVAL;
> +		return -1;
> +	}
> +	strncpy(name1, tok, BUFSIZ - 1);
> +	strtrim(name, name1, namesize - 1);
> +
> +	tok = strtok_r(NULL, "\0", &ptr);
> +	if (!tok) {
> +		errno = EINVAL;
> +		return -1;
> +	}
> +
> +	while (isspace(*tok))
> +		tok++;
> +
> +	*val = -1;
> +	if (isdigit(tok[0]))
> +		*val = atoi(tok);
> +	else if (!strncasecmp(tok, "true", sizeof("true") - 1))
> +		*val = 1;
> +	else if (!strncasecmp(tok, "false", sizeof("false") - 1))
> +		*val = 0;
> +	if (*val != 0 && *val != 1) {
> +		errno = EINVAL;
> +		return -1;
>  	}
>  	return 1;
>  }

[PATCH 4/6] libselinux: ensure that 4 columns are read from /proc/mounts

[Nicolas Iooss]

If exclude_non_seclabel_mounts() ever gets run on a kernel where
/proc/mounts only contains three columns, mount_info[3] will be used
"without being initialized in "strtok(mount_info[3], ",")" because
variable index would be 3 at the end of this loop:

    index = 0;
    item = strtok(buf, " ");
    while (item != NULL) {
        mount_info[index] = item;
        if (index == 3)
            break;
        index++;
        item = strtok(NULL, " ");
    }

Swap the condition on index and its increment so that it gets to 4 only
when there are at least four columns.

This issue has been found using clang's static analyzer.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libselinux/src/selinux_restorecon.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
index 9fdafea17de7..eefd2cf83e32 100644
--- a/libselinux/src/selinux_restorecon.c
+++ b/libselinux/src/selinux_restorecon.c
@@ -252,12 +252,12 @@ static int exclude_non_seclabel_mounts(void)
 		item = strtok(buf, " ");
 		while (item != NULL) {
 			mount_info[index] = item;
-			if (index == 3)
-				break;
 			index++;
+			if (index == 4)
+				break;
 			item = strtok(NULL, " ");
 		}
-		if (index < 3) {
+		if (index < 4) {
 			selinux_log(SELINUX_ERROR,
 				    "/proc/mounts record \"%s\" has incorrect format.\n",
 				    buf);
-- 
2.12.0

[PATCH 5/6] libsepol: refuse to load policies with no block

[Nicolas Iooss]

Some functions assumes that p->global is not NULL. For example
range_read() contains:

    p->global->enabled->range_tr_rules = rtr;

However p->global may currently be NULL when loading a policy module
with no avrule block. Avoid a NULL pointer dereference by making such a
policy invalid.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/src/policydb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index 3cff6d276d68..7093b29833bf 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -4044,6 +4044,10 @@ int policydb_read(policydb_t * p, struct policy_file *fp, unsigned verbose)
 		if (avrule_block_read(p, &p->global, info->sym_num, fp) == -1) {
 			goto bad;
 		}
+		if (p->global == NULL) {
+			ERR(fp->handle, "no avrule block in policy");
+			goto bad;
+		}
 		for (i = 0; i < info->sym_num; i++) {
 			if ((rc = next_entry(buf, fp, sizeof(uint32_t))) < 0) {
 				goto bad;
-- 
2.12.0

[PATCH 6/6] libsepol: do not wrap integers when checking bound

[Nicolas Iooss]

Some invalid policies might have p->p_types.nprim = 0. When parsing
such a policy, "i > p->p_types.nprim - 1" is always false even though
reading p->type_val_to_struct[i] triggers a segmentation fault.

Make type_set_expand() return an error when parsing such a policy by
handling correctly when p->p_types.nprim is zero.

This issue has been found while fuzzing semodule_package with the
American Fuzzy Lop.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/src/expand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
index 315fc65cfd7e..54bf781d335f 100644
--- a/libsepol/src/expand.c
+++ b/libsepol/src/expand.c
@@ -2527,7 +2527,7 @@ int type_set_expand(type_set_t * set, ebitmap_t * t, policydb_t * p,
 				 * invalid policies might have more types set in the ebitmap than
 				 * what's available in the type_val_to_struct mapping
 				 */
-				if (i > p->p_types.nprim - 1)
+				if (i >= p->p_types.nprim)
 					goto err_types;
 
 				if (!p->type_val_to_struct[i]) {
-- 
2.12.0

Re: [PATCH 6/6] libsepol: do not wrap integers when checking bound

[Stephen Smalley]

On Fri, 2017-04-07 at 22:44 +0200, Nicolas Iooss wrote:
> Some invalid policies might have p->p_types.nprim = 0. When parsing
> such a policy, "i > p->p_types.nprim - 1" is always false even though
> reading p->type_val_to_struct[i] triggers a segmentation fault.
> 
> Make type_set_expand() return an error when parsing such a policy by
> handling correctly when p->p_types.nprim is zero.
> 
> This issue has been found while fuzzing semodule_package with the
> American Fuzzy Lop.
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Thanks, applied these six patches.

> ---
>  libsepol/src/expand.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> index 315fc65cfd7e..54bf781d335f 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -2527,7 +2527,7 @@ int type_set_expand(type_set_t * set, ebitmap_t
> * t, policydb_t * p,
>  				 * invalid policies might have more
> types set in the ebitmap than
>  				 * what's available in the
> type_val_to_struct mapping
>  				 */
> -				if (i > p->p_types.nprim - 1)
> +				if (i >= p->p_types.nprim)
>  					goto err_types;
>  
>  				if (!p->type_val_to_struct[i]) {