[PATCH 1/7] libsepol: do not dereference a NULL pointer when stack_init() fails

[PATCH 1/7] libsepol: do not dereference a NULL pointer when stack_init() fails

[Nicolas Iooss]

In cond_expr_to_cil() when stack_init(&stack) fails, stack is set to
NULL and the execution flow jumps to label "exit". This triggers a call
to stack_pop(stack) which dereferences a NULL pointer in "if (stack->pos
== -1)".

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

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

diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index 308ada4f1381..5c98c29bcf13 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -1363,11 +1363,12 @@ exit:
 	free(new_val);
 	free(val1);
 	free(val2);
-	while ((val1 = stack_pop(stack)) != NULL) {
-		free(val1);
+	if (stack != NULL) {
+		while ((val1 = stack_pop(stack)) != NULL) {
+			free(val1);
+		}
+		stack_destroy(&stack);
 	}
-	stack_destroy(&stack);
-
 	return rc;
 }
 
-- 
2.12.0

[PATCH 2/7] libsepol: make process_boolean() fail on invalid lines

[Nicolas Iooss]

When 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 load_booleans() to use uninitialized name and/or val when setting
the boolean into the policy.

Rework process_boolean() in order to report errors when a boolean
definition is incorrect.

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

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

diff --git a/libsepol/src/genbools.c b/libsepol/src/genbools.c
index c1f540558bf1..d79433531f76 100644
--- a/libsepol/src/genbools.c
+++ b/libsepol/src/genbools.c
@@ -34,31 +34,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) {
-				ERR(NULL, "illegal value for boolean "
-				    "%s=%s", name, tok);
-				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) {
+		ERR(NULL, "illegal boolean definition %s", buffer);
+		return -1;
+	}
+	strncpy(name1, tok, BUFSIZ - 1);
+	strtrim(name, name1, namesize - 1);
 
-		}
+	tok = strtok_r(NULL, "\0", &ptr);
+	if (!tok) {
+		ERR(NULL, "illegal boolean definition %s=%s", name, buffer);
+		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) {
+		ERR(NULL, "illegal value for boolean %s=%s", name, tok);
+		return -1;
 	}
 	return 1;
 }
-- 
2.12.0

[PATCH 3/7] libsepol: constify sepol_genbools()'s boolpath parameter

[Nicolas Iooss]

This allows removing an unnecessary cast to (char *) in libselinux.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libselinux/src/load_policy.c       | 5 ++---
 libsepol/include/sepol/booleans.h  | 2 +-
 libsepol/man/man3/sepol_genbools.3 | 2 +-
 libsepol/src/genbools.c            | 2 +-
 4 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
index b7e1a6fa6b5c..e788c25cfe68 100644
--- a/libselinux/src/load_policy.c
+++ b/libselinux/src/load_policy.c
@@ -79,7 +79,7 @@ int selinux_mkload_policy(int preservebools)
 	int (*policydb_to_image)(sepol_handle_t *, sepol_policydb_t *, void **, size_t *) = NULL;
 	int (*genbools_array)(void *data, size_t len, char **names, int *values, int nel) = NULL;
 	int (*genusers)(void *data, size_t len, const char *usersdir, void **newdata, size_t * newlen) = NULL;
-	int (*genbools)(void *data, size_t len, char *boolpath) = NULL;
+	int (*genbools)(void *data, size_t len, const char *boolpath) = NULL;
 
 #ifdef SHARED
 	char *errormsg = NULL;
@@ -275,8 +275,7 @@ checkbool:
 				free(names);
 			}
 		} else if (setlocaldefs) {
-			(void)genbools(data, size,
-				       (char *)selinux_booleans_path());
+			(void)genbools(data, size, selinux_booleans_path());
 		}
 	}
 
diff --git a/libsepol/include/sepol/booleans.h b/libsepol/include/sepol/booleans.h
index 02356d18fd5d..2966903d4564 100644
--- a/libsepol/include/sepol/booleans.h
+++ b/libsepol/include/sepol/booleans.h
@@ -17,7 +17,7 @@ extern "C" {
    policy for the boolean settings in the boolean configuration file.
    The binary policy is rewritten in place in memory.
    Returns 0 upon success, or -1 otherwise. */
-extern int sepol_genbools(void *data, size_t len, char *boolpath);
+extern int sepol_genbools(void *data, size_t len, const char *boolpath);
 
 /* Given an existing binary policy (starting at 'data', with length 'len')
    and boolean settings specified by the parallel arrays ('names', 'values')
diff --git a/libsepol/man/man3/sepol_genbools.3 b/libsepol/man/man3/sepol_genbools.3
index ca5b5a63bb8c..dcfb69da4eb9 100644
--- a/libsepol/man/man3/sepol_genbools.3
+++ b/libsepol/man/man3/sepol_genbools.3
@@ -4,7 +4,7 @@ sepol_genbools \- Rewrite a binary policy with different boolean settings
 .SH "SYNOPSIS"
 .B #include <sepol/sepol.h>
 .sp
-.BI "int sepol_genbools(void *" data ", size_t "len ", char *" boolpath );
+.BI "int sepol_genbools(void *" data ", size_t "len ", const char *" boolpath );
 .br
 .BI "int sepol_genbools_array(void *" data ", size_t " len ", char **" names ", int *" values ", int " nel );
 
diff --git a/libsepol/src/genbools.c b/libsepol/src/genbools.c
index d79433531f76..d4a2df62d315 100644
--- a/libsepol/src/genbools.c
+++ b/libsepol/src/genbools.c
@@ -157,7 +157,7 @@ static int load_booleans(struct policydb *policydb, const char *path,
 	return errors ? -1 : 0;
 }
 
-int sepol_genbools(void *data, size_t len, char *booleans)
+int sepol_genbools(void *data, size_t len, const char *booleans)
 {
 	struct policydb policydb;
 	struct policy_file pf;
-- 
2.12.0

[PATCH 4/7] libsepol: fix use-after-free in sepol_user_clone()

[Nicolas Iooss]

When sepol_user_add_role() fails to allocate memory for role_cp but
succeeds in reallocating user->roles memory, it frees this reallocated
memory, thus leaving user->roles referencing a free memory block. When
sepol_user_clone() calls sepol_user_free(new_user) because the
allocation failure made sepol_user_add_role() fail, the following code
is executed:

    for (i = 0; i < user->num_roles; i++)
        free(user->roles[i]);
    free(user->roles);

As user->roles has been freed, this code frees pointers which may be
invalid and then tries to free user->roles again.

Fix this flaw by returning right after strdup() failed in
sepol_user_add_role().

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

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

diff --git a/libsepol/src/user_record.c b/libsepol/src/user_record.c
index e7e2fc20fe36..ed5b048203d2 100644
--- a/libsepol/src/user_record.c
+++ b/libsepol/src/user_record.c
@@ -178,16 +178,18 @@ int sepol_user_add_role(sepol_handle_t * handle,
 {
 
 	char *role_cp;
-	char **roles_realloc;
+	char **roles_realloc = NULL;
 
 	if (sepol_user_has_role(user, role))
 		return STATUS_SUCCESS;
 
 	role_cp = strdup(role);
+	if (!role_cp)
+		goto omem;
+
 	roles_realloc = realloc(user->roles,
 				sizeof(char *) * (user->num_roles + 1));
-
-	if (!role_cp || !roles_realloc)
+	if (!roles_realloc)
 		goto omem;
 
 	user->num_roles++;
-- 
2.12.0

[PATCH 5/7] libsemanage: do not close uninitialized file descriptors

[Nicolas Iooss]

When pipe() fails in semanage_pipe_data(), this function closes all file
descriptors in variables output_fd, err_fd and input_fd even when they
have not been initialized. Fix this by initializing the file descriptors
to -1.

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

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

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index c23494bb4270..568732355f54 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -705,9 +705,9 @@ static int read_from_pipe_to_data(semanage_handle_t *sh, size_t initial_len, int
 
 static int semanage_pipe_data(semanage_handle_t *sh, char *path, char *in_data, size_t in_data_len, char **out_data, size_t *out_data_len, char **err_data, size_t *err_data_len)
 {
-	int input_fd[2];
-	int output_fd[2];
-	int err_fd[2];
+	int input_fd[2] = {-1, -1};
+	int output_fd[2] = {-1, -1};
+	int err_fd[2] = {-1, -1};
 	pid_t pid;
 	char *data_read = NULL;
 	char *err_data_read = NULL;
-- 
2.12.0

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

[Nicolas Iooss]

If "names = calloc(num_modinfos, sizeof(*names))" fails in
semanage_get_cil_paths(), the function tries to frees items in array
"names" even though it is NULL. Avoid this by returning directly.

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

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

diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 47ec93185e06..bf1a448384b9 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -1012,8 +1012,7 @@ int semanage_get_cil_paths(semanage_handle_t * sh,
 	names = calloc(num_modinfos, sizeof(*names));
 	if (names == NULL) {
 		ERR(sh, "Error allocating space for filenames.");
-		status = -1;
-		goto cleanup;
+		return -1;
 	}
 
 	for (i = 0; i < num_modinfos; i++) {
-- 
2.12.0

[PATCH 7/7] libsemanage: genhomedircon: fix possible double-free

[Nicolas Iooss]

When write_contexts() frees variables context and new_context_str after
a line has been successfully emitted, these variables are not reset to
NULL. This leads the function to free them again if an error occurs when
processing the next line. Fix this by always resetting these variables
at the beginning of the loop.

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

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

diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
index 465dd8829403..e8c95ee46130 100644
--- a/libsemanage/src/genhomedircon.c
+++ b/libsemanage/src/genhomedircon.c
@@ -607,10 +607,12 @@ static int write_contexts(genhomedircon_settings_t *s, FILE *out,
 			  const genhomedircon_user_entry_t *user)
 {
 	char *line, *temp;
-	sepol_context_t *context = NULL;
-	char *new_context_str = NULL;
+	sepol_context_t *context;
+	char *new_context_str;
 
 	for (; tpl; tpl = tpl->next) {
+		context = NULL;
+		new_context_str = NULL;
 		line = replace_all(tpl->data, repl);
 		if (!line) {
 			goto fail;
-- 
2.12.0

Re: [PATCH 1/7] libsepol: do not dereference a NULL pointer when stack_init() fails

[James Carter]

On 03/28/2017 05:41 PM, Nicolas Iooss wrote:
> In cond_expr_to_cil() when stack_init(&stack) fails, stack is set to
> NULL and the execution flow jumps to label "exit". This triggers a call
> to stack_pop(stack) which dereferences a NULL pointer in "if (stack->pos
> == -1)".
>
> This issue has been found using clang's static analyzer.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>

I applied these seven patches.

Thanks,
Jim

> ---
>  libsepol/src/module_to_cil.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> index 308ada4f1381..5c98c29bcf13 100644
> --- a/libsepol/src/module_to_cil.c
> +++ b/libsepol/src/module_to_cil.c
> @@ -1363,11 +1363,12 @@ exit:
>  	free(new_val);
>  	free(val1);
>  	free(val2);
> -	while ((val1 = stack_pop(stack)) != NULL) {
> -		free(val1);
> +	if (stack != NULL) {
> +		while ((val1 = stack_pop(stack)) != NULL) {
> +			free(val1);
> +		}
> +		stack_destroy(&stack);
>  	}
> -	stack_destroy(&stack);
> -
>  	return rc;
>  }
>
>


-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency