SELinux Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/9] Fix issues found by static analyzers
@ 2019-09-01 18:06 Nicolas Iooss
  2019-09-01 18:06 ` [PATCH 1/9] semodule-utils: fix comparison with argc Nicolas Iooss
                   ` (9 more replies)
  0 siblings, 10 replies; 15+ messages in thread
From: Nicolas Iooss @ 2019-09-01 18:06 UTC (permalink / raw)
  To: selinux

Hi,

In August I ran several free static analyzers in order to find new
issues in the userspace code. I used:

* clang's static analyzer (with scripts/run-scan-build)
* https://lgtm.com/ (I wrote a simple configuration file to analyze the
  C code, the results are available on
  https://lgtm.com/projects/g/fishilico/selinux-for-lgtm/ )
* Facebook's Infer (https://fbinfer.com/, I wrote a script to invoke it
  like clang's static analyzer)

Each analyzer gave many results, with several true bugs in them. Here
are patches that fix some of them. I will post the configuration files
and scripts I used separately.

Thanks,
Nicolas

Nicolas Iooss (9):
  semodule-utils: fix comparison with argc
  libsepol/cil: help static analyzers by aborting when an allocation
    fails
  libsepol: do not dereference a failed allocated pointer
  libsepol: do not dereference scope if it can be NULL
  libsepol: reset *p to NULL if sepol_module_package_create fails
  libsepol/cil: do not dereference perm_value_to_cil when it has not
    been allocated
  python/chcat: remove unnecessary assignment
  python/sepolicy: remove unnecessary pass statement
  libsepol/tests: do not dereference a NULL pointer

 libsepol/cil/src/cil_binary.c                        | 10 ++++++----
 libsepol/cil/src/cil_mem.c                           |  5 +++++
 libsepol/src/avrule_block.c                          |  3 ++-
 libsepol/src/kernel_to_cil.c                         |  8 +++++---
 libsepol/src/kernel_to_conf.c                        |  8 +++++---
 libsepol/src/module.c                                |  4 +++-
 libsepol/tests/test-common.c                         | 11 +++++++----
 python/chcat/chcat                                   |  1 -
 python/sepolicy/sepolicy/__init__.py                 |  1 -
 semodule-utils/semodule_package/semodule_unpackage.c |  2 +-
 10 files changed, 34 insertions(+), 19 deletions(-)

-- 
2.22.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/9] semodule-utils: fix comparison with argc
  2019-09-01 18:06 [PATCH 0/9] Fix issues found by static analyzers Nicolas Iooss
@ 2019-09-01 18:06 ` Nicolas Iooss
  2019-09-01 18:06 ` [PATCH 2/9] libsepol/cil: help static analyzers by aborting when an allocation fails Nicolas Iooss
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Nicolas Iooss @ 2019-09-01 18:06 UTC (permalink / raw)
  To: selinux

In order for argv[3] to be used, argc needs to be at least 4, not 3.

This bug was found using lgtm.com analyzer:
https://lgtm.com/projects/g/fishilico/selinux-for-lgtm/snapshot/8c1b2658f80392ff8b3532c6bd5d0cefac8afb30/files/semodule-utils/semodule_package/semodule_unpackage.c?sort=name&dir=ASC&mode=heatmap#xb1ce80b43260d34c:1

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

diff --git a/semodule-utils/semodule_package/semodule_unpackage.c b/semodule-utils/semodule_package/semodule_unpackage.c
index c9124c0f5ac1..b8c4fbcec434 100644
--- a/semodule-utils/semodule_package/semodule_unpackage.c
+++ b/semodule-utils/semodule_package/semodule_unpackage.c
@@ -55,7 +55,7 @@ int main(int argc, char **argv)
 
 	ppfile = argv[1];
 	modfile = argv[2];
-	if (argc >= 3)
+	if (argc >= 4)
 		fcfile = argv[3];
 
 	if (file_to_policy_file(ppfile, &in, "r"))
-- 
2.22.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/9] libsepol/cil: help static analyzers by aborting when an allocation fails
  2019-09-01 18:06 [PATCH 0/9] Fix issues found by static analyzers Nicolas Iooss
  2019-09-01 18:06 ` [PATCH 1/9] semodule-utils: fix comparison with argc Nicolas Iooss
@ 2019-09-01 18:06 ` Nicolas Iooss
  2019-09-10 18:48   ` [Non-DoD Source] " jwcart2
  2019-09-01 18:06 ` [PATCH 3/9] libsepol: do not dereference a failed allocated pointer Nicolas Iooss
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 15+ messages in thread
From: Nicolas Iooss @ 2019-09-01 18:06 UTC (permalink / raw)
  To: selinux

When allocating memory with cil_* helpers, if malloc/calloc/realloc/...
failed, (*cil_mem_error_handler)() is called. Implementations of this
function are expected not to return to the caller, and the default one
calls exit(1) to ensure this. In order for static analyzers to find out
that cil_malloc/cil_realloc/... never returns a NULL pointer when
failing to allocate some memory, introduce a call to abort().

This decreases the number of false positive warnings about null pointer
dereferences reported by Infer static analyzer.

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

diff --git a/libsepol/cil/src/cil_mem.c b/libsepol/cil/src/cil_mem.c
index 12c59be21914..885431d8a8fd 100644
--- a/libsepol/cil/src/cil_mem.c
+++ b/libsepol/cil/src/cil_mem.c
@@ -55,6 +55,7 @@ void *cil_malloc(size_t size)
 			return NULL;
 		}
 		(*cil_mem_error_handler)();
+		abort();
 	}
 
 	return mem;
@@ -65,6 +66,7 @@ void *cil_calloc(size_t num_elements, size_t element_size)
 	void *mem = calloc(num_elements, element_size);
 	if (mem == NULL){
 		(*cil_mem_error_handler)();
+		abort();
 	}
 
 	return mem;
@@ -78,6 +80,7 @@ void *cil_realloc(void *ptr, size_t size)
 			return NULL;
 		}
 		(*cil_mem_error_handler)();
+		abort();
 	}
 
 	return mem;
@@ -95,6 +98,7 @@ char *cil_strdup(const char *str)
 	mem = strdup(str);
 	if (mem == NULL) {
 		(*cil_mem_error_handler)();
+		abort();
 	}
 
 	return mem;
@@ -111,6 +115,7 @@ __attribute__ ((format (printf, 2, 3))) int cil_asprintf(char **strp, const char
 
 	if (rc == -1) {
 		(*cil_mem_error_handler)();
+		abort();
 	}
 
 	return rc;
-- 
2.22.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 3/9] libsepol: do not dereference a failed allocated pointer
  2019-09-01 18:06 [PATCH 0/9] Fix issues found by static analyzers Nicolas Iooss
  2019-09-01 18:06 ` [PATCH 1/9] semodule-utils: fix comparison with argc Nicolas Iooss
  2019-09-01 18:06 ` [PATCH 2/9] libsepol/cil: help static analyzers by aborting when an allocation fails Nicolas Iooss
@ 2019-09-01 18:06 ` Nicolas Iooss
  2019-09-10 18:52   ` [Non-DoD Source] " jwcart2
  2019-09-01 18:06 ` [PATCH 4/9] libsepol: do not dereference scope if it can be NULL Nicolas Iooss
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 15+ messages in thread
From: Nicolas Iooss @ 2019-09-01 18:06 UTC (permalink / raw)
  To: selinux

When strs_stack_init(&stack) fails to allocate memory and stack is still
NULL, it should not be dereferenced with strs_stack_pop(stack).

This issue has been found using Infer static analyzer.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/src/kernel_to_cil.c  | 8 +++++---
 libsepol/src/kernel_to_conf.c | 8 +++++---
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
index 320af37b2bc8..9fcc254707ba 100644
--- a/libsepol/src/kernel_to_cil.c
+++ b/libsepol/src/kernel_to_cil.c
@@ -108,10 +108,12 @@ static char *cond_expr_to_str(struct policydb *pdb, struct cond_expr *expr)
 	return str;
 
 exit:
-	while ((new_val = strs_stack_pop(stack)) != NULL) {
-		free(new_val);
+	if (stack) {
+		while ((new_val = strs_stack_pop(stack)) != NULL) {
+			free(new_val);
+		}
+		strs_stack_destroy(&stack);
 	}
-	strs_stack_destroy(&stack);
 
 	return NULL;
 }
diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
index 930bafabdd4b..2c8da49a11ab 100644
--- a/libsepol/src/kernel_to_conf.c
+++ b/libsepol/src/kernel_to_conf.c
@@ -106,10 +106,12 @@ static char *cond_expr_to_str(struct policydb *pdb, struct cond_expr *expr)
 	return str;
 
 exit:
-	while ((new_val = strs_stack_pop(stack)) != NULL) {
-		free(new_val);
+	if (stack != NULL) {
+		while ((new_val = strs_stack_pop(stack)) != NULL) {
+			free(new_val);
+		}
+		strs_stack_destroy(&stack);
 	}
-	strs_stack_destroy(&stack);
 
 	return NULL;
 }
-- 
2.22.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 4/9] libsepol: do not dereference scope if it can be NULL
  2019-09-01 18:06 [PATCH 0/9] Fix issues found by static analyzers Nicolas Iooss
                   ` (2 preceding siblings ...)
  2019-09-01 18:06 ` [PATCH 3/9] libsepol: do not dereference a failed allocated pointer Nicolas Iooss
@ 2019-09-01 18:06 ` Nicolas Iooss
  2019-09-01 18:06 ` [PATCH 5/9] libsepol: reset *p to NULL if sepol_module_package_create fails Nicolas Iooss
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Nicolas Iooss @ 2019-09-01 18:06 UTC (permalink / raw)
  To: selinux

Doing this looks wrong:

    len = scope->decl_ids_len;
    if (scope == NULL) {
        /* ... */

Move the dereferencing of scope after the NULL check.

This issue has been found using Infer static analyzer.

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

diff --git a/libsepol/src/avrule_block.c b/libsepol/src/avrule_block.c
index 5a873af4a864..a9832d0d118f 100644
--- a/libsepol/src/avrule_block.c
+++ b/libsepol/src/avrule_block.c
@@ -157,7 +157,7 @@ int is_id_enabled(char *id, policydb_t * p, int symbol_table)
 	scope_datum_t *scope =
 	    (scope_datum_t *) hashtab_search(p->scope[symbol_table].table, id);
 	avrule_decl_t *decl;
-	uint32_t len = scope->decl_ids_len;
+	uint32_t len;
 
 	if (scope == NULL) {
 		return 0;
@@ -166,6 +166,7 @@ int is_id_enabled(char *id, policydb_t * p, int symbol_table)
 		return 0;
 	}
 
+	len = scope->decl_ids_len;
 	if (len < 1) {
 		return 0;
 	}
-- 
2.22.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 5/9] libsepol: reset *p to NULL if sepol_module_package_create fails
  2019-09-01 18:06 [PATCH 0/9] Fix issues found by static analyzers Nicolas Iooss
                   ` (3 preceding siblings ...)
  2019-09-01 18:06 ` [PATCH 4/9] libsepol: do not dereference scope if it can be NULL Nicolas Iooss
@ 2019-09-01 18:06 ` Nicolas Iooss
  2019-09-01 18:06 ` [PATCH 6/9] libsepol/cil: do not dereference perm_value_to_cil when it has not been allocated Nicolas Iooss
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Nicolas Iooss @ 2019-09-01 18:06 UTC (permalink / raw)
  To: selinux

semodule-utils/semodule_link/semodule_link.c contains:

    static sepol_module_package_t *load_module(char *filename)
    {
        /* ... */
        if (sepol_module_package_create(&p)) {
            /* ... */
            goto bad;

    /* ... */
    bad:
        sepol_module_package_free(p);

When sepol_module_package_create() fails while having successfully
allocated p, it currently frees p without setting it back to NULL. This
causes a use-after-free in load_module().

Prevent this use-after-free by setting sepol_module_package_create's
argument back to NULL when an error happens.

This issue has been found using Infer static analyzer.

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

diff --git a/libsepol/src/module.c b/libsepol/src/module.c
index 219355f30d27..3b8a0a59ca68 100644
--- a/libsepol/src/module.c
+++ b/libsepol/src/module.c
@@ -124,8 +124,10 @@ int sepol_module_package_create(sepol_module_package_t ** p)
 		return -1;
 
 	rc = module_package_init(*p);
-	if (rc < 0)
+	if (rc < 0) {
 		free(*p);
+		*p = NULL;
+	}
 
 	return rc;
 }
-- 
2.22.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 6/9] libsepol/cil: do not dereference perm_value_to_cil when it has not been allocated
  2019-09-01 18:06 [PATCH 0/9] Fix issues found by static analyzers Nicolas Iooss
                   ` (4 preceding siblings ...)
  2019-09-01 18:06 ` [PATCH 5/9] libsepol: reset *p to NULL if sepol_module_package_create fails Nicolas Iooss
@ 2019-09-01 18:06 ` Nicolas Iooss
  2019-09-01 18:06 ` [PATCH 7/9] python/chcat: remove unnecessary assignment Nicolas Iooss
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Nicolas Iooss @ 2019-09-01 18:06 UTC (permalink / raw)
  To: selinux

When one of the first allocations of cil_binary_create_allocated_pdb()
fails, the exit label dereferences the items of array perm_value_to_cil
even though it could be still NULL.

This issue has been found using clang's static analyzer:
https://327-118970575-gh.circle-artifacts.com/0/output-scan-build/2019-08-05-203459-6149-1/report-febf85.html#EndPath

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

diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
index 77ffc36f20ff..c882d5b74226 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -5043,11 +5043,13 @@ exit:
 	hashtab_destroy(avrulex_ioctl_table);
 	free(type_value_to_cil);
 	free(class_value_to_cil);
-	/* Range is because libsepol values start at 1. */
-	for (i=1; i < db->num_classes+1; i++) {
-		free(perm_value_to_cil[i]);
+	if (perm_value_to_cil != NULL) {
+		/* Range is because libsepol values start at 1. */
+		for (i=1; i < db->num_classes+1; i++) {
+			free(perm_value_to_cil[i]);
+		}
+		free(perm_value_to_cil);
 	}
-	free(perm_value_to_cil);
 	cil_list_destroy(&neverallows, CIL_FALSE);
 
 	return rc;
-- 
2.22.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 7/9] python/chcat: remove unnecessary assignment
  2019-09-01 18:06 [PATCH 0/9] Fix issues found by static analyzers Nicolas Iooss
                   ` (5 preceding siblings ...)
  2019-09-01 18:06 ` [PATCH 6/9] libsepol/cil: do not dereference perm_value_to_cil when it has not been allocated Nicolas Iooss
@ 2019-09-01 18:06 ` Nicolas Iooss
  2019-09-01 18:06 ` [PATCH 8/9] python/sepolicy: remove unnecessary pass statement Nicolas Iooss
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Nicolas Iooss @ 2019-09-01 18:06 UTC (permalink / raw)
  To: selinux

chcat_add() defines variable cmd twice before calling
subprocess.check_call(cmd, ...). Remove the first definition.

This bug was found using lgtm.com analyzer:
https://lgtm.com/projects/g/SELinuxProject/selinux/snapshot/eac5e661ca7300800000496fe13985286af70c6d/files/python/chcat/chcat?sort=name&dir=ASC&mode=heatmap#L118

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 python/chcat/chcat | 1 -
 1 file changed, 1 deletion(-)

diff --git a/python/chcat/chcat b/python/chcat/chcat
index ba398684376d..fdd2e46ee3f9 100755
--- a/python/chcat/chcat
+++ b/python/chcat/chcat
@@ -115,7 +115,6 @@ def chcat_add(orig, newcat, objects, login_ind):
     errors = 0
     sensitivity = newcat[0]
     cat = newcat[1]
-    cmd = 'chcon -l %s' % sensitivity
     for f in objects:
         (rc, c) = selinux.getfilecon(f)
         con = c.split(":")[3:]
-- 
2.22.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 8/9] python/sepolicy: remove unnecessary pass statement
  2019-09-01 18:06 [PATCH 0/9] Fix issues found by static analyzers Nicolas Iooss
                   ` (6 preceding siblings ...)
  2019-09-01 18:06 ` [PATCH 7/9] python/chcat: remove unnecessary assignment Nicolas Iooss
@ 2019-09-01 18:06 ` Nicolas Iooss
  2019-09-01 18:06 ` [PATCH 9/9] libsepol/tests: do not dereference a NULL pointer Nicolas Iooss
  2019-09-16 16:46 ` [Non-DoD Source] [PATCH 0/9] Fix issues found by static analyzers jwcart2
  9 siblings, 0 replies; 15+ messages in thread
From: Nicolas Iooss @ 2019-09-01 18:06 UTC (permalink / raw)
  To: selinux

This issue has been found using lgtm.com analyzer:
https://lgtm.com/projects/g/SELinuxProject/selinux/snapshot/eac5e661ca7300800000496fe13985286af70c6d/files/python/sepolicy/sepolicy/__init__.py?sort=name&dir=ASC&mode=heatmap#x9f8225117f52fb01:1

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 python/sepolicy/sepolicy/__init__.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/python/sepolicy/sepolicy/__init__.py b/python/sepolicy/sepolicy/__init__.py
index 6aed31bddf1e..e4540977d042 100644
--- a/python/sepolicy/sepolicy/__init__.py
+++ b/python/sepolicy/sepolicy/__init__.py
@@ -539,7 +539,6 @@ def find_file(reg):
             path += "/"
     except IndexError:
         print("try failed got an IndexError")
-        pass
 
     try:
         pat = re.compile(r"%s$" % reg)
-- 
2.22.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 9/9] libsepol/tests: do not dereference a NULL pointer
  2019-09-01 18:06 [PATCH 0/9] Fix issues found by static analyzers Nicolas Iooss
                   ` (7 preceding siblings ...)
  2019-09-01 18:06 ` [PATCH 8/9] python/sepolicy: remove unnecessary pass statement Nicolas Iooss
@ 2019-09-01 18:06 ` Nicolas Iooss
  2019-09-16 16:46 ` [Non-DoD Source] [PATCH 0/9] Fix issues found by static analyzers jwcart2
  9 siblings, 0 replies; 15+ messages in thread
From: Nicolas Iooss @ 2019-09-01 18:06 UTC (permalink / raw)
  To: selinux

In test_attr_types, the pointer decl is allowed to be NULL in the
beginning, but is dereferenced to produce a helpful message right before
a CU_ASSERT_FATAL. Make this derefence not happen if the pointer is
NULL.

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

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/tests/test-common.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/libsepol/tests/test-common.c b/libsepol/tests/test-common.c
index e6619ed1d152..1d902880fe2e 100644
--- a/libsepol/tests/test-common.c
+++ b/libsepol/tests/test-common.c
@@ -228,13 +228,16 @@ void test_attr_types(policydb_t * p, const char *id, avrule_decl_t * decl, const
 	unsigned int i;
 	type_datum_t *attr;
 
-	if (decl)
+	if (decl) {
 		attr = hashtab_search(decl->p_types.table, id);
-	else
+		if (attr == NULL)
+			printf("could not find attr %s in decl %d\n", id, decl->decl_id);
+	} else {
 		attr = hashtab_search(p->p_types.table, id);
+		if (attr == NULL)
+			printf("could not find attr %s in policy\n", id);
+	}
 
-	if (attr == NULL)
-		printf("could not find attr %s in decl %d\n", id, decl->decl_id);
 	CU_ASSERT_FATAL(attr != NULL);
 	CU_ASSERT(attr->flavor == TYPE_ATTRIB);
 	CU_ASSERT(attr->primary == 1);
-- 
2.22.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Non-DoD Source] [PATCH 2/9] libsepol/cil: help static analyzers by aborting when an allocation fails
  2019-09-01 18:06 ` [PATCH 2/9] libsepol/cil: help static analyzers by aborting when an allocation fails Nicolas Iooss
@ 2019-09-10 18:48   ` " jwcart2
  0 siblings, 0 replies; 15+ messages in thread
From: jwcart2 @ 2019-09-10 18:48 UTC (permalink / raw)
  To: Nicolas Iooss, selinux

On 9/1/19 2:06 PM, Nicolas Iooss wrote:
> When allocating memory with cil_* helpers, if malloc/calloc/realloc/...
> failed, (*cil_mem_error_handler)() is called. Implementations of this
> function are expected not to return to the caller, and the default one
> calls exit(1) to ensure this. In order for static analyzers to find out
> that cil_malloc/cil_realloc/... never returns a NULL pointer when
> failing to allocate some memory, introduce a call to abort().
> 
> This decreases the number of false positive warnings about null pointer
> dereferences reported by Infer static analyzer.
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>   libsepol/cil/src/cil_mem.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/libsepol/cil/src/cil_mem.c b/libsepol/cil/src/cil_mem.c
> index 12c59be21914..885431d8a8fd 100644
> --- a/libsepol/cil/src/cil_mem.c
> +++ b/libsepol/cil/src/cil_mem.c
> @@ -55,6 +55,7 @@ void *cil_malloc(size_t size)
>   			return NULL;
>   		}
>   		(*cil_mem_error_handler)();
> +		abort();
>   	}
> 

I am going to have to think about this one.

I think the real answer might be to rip out the (*cil_mem_error_handler)() 
statements and replace them with what is in the default handler. I notice now 
that even though there is a cil_set_mem_error_handler() function, it is not used 
anywhere.

>   	return mem;
> @@ -65,6 +66,7 @@ void *cil_calloc(size_t num_elements, size_t element_size)
>   	void *mem = calloc(num_elements, element_size);
>   	if (mem == NULL){
>   		(*cil_mem_error_handler)();
> +		abort();
>   	}
>   
>   	return mem;
> @@ -78,6 +80,7 @@ void *cil_realloc(void *ptr, size_t size)
>   			return NULL;
>   		}
>   		(*cil_mem_error_handler)();
> +		abort();
>   	}
>   
>   	return mem;
> @@ -95,6 +98,7 @@ char *cil_strdup(const char *str)
>   	mem = strdup(str);
>   	if (mem == NULL) {
>   		(*cil_mem_error_handler)();
> +		abort();
>   	}
>   
>   	return mem;
> @@ -111,6 +115,7 @@ __attribute__ ((format (printf, 2, 3))) int cil_asprintf(char **strp, const char
>   
>   	if (rc == -1) {
>   		(*cil_mem_error_handler)();
> +		abort();
>   	}
>   
>   	return rc;
> 


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Non-DoD Source] [PATCH 3/9] libsepol: do not dereference a failed allocated pointer
  2019-09-01 18:06 ` [PATCH 3/9] libsepol: do not dereference a failed allocated pointer Nicolas Iooss
@ 2019-09-10 18:52   ` " jwcart2
  2019-09-10 20:11     ` jwcart2
  0 siblings, 1 reply; 15+ messages in thread
From: jwcart2 @ 2019-09-10 18:52 UTC (permalink / raw)
  To: Nicolas Iooss, selinux

On 9/1/19 2:06 PM, Nicolas Iooss wrote:
> When strs_stack_init(&stack) fails to allocate memory and stack is still
> NULL, it should not be dereferenced with strs_stack_pop(stack).
> 
> This issue has been found using Infer static analyzer.
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>   libsepol/src/kernel_to_cil.c  | 8 +++++---
>   libsepol/src/kernel_to_conf.c | 8 +++++---
>   2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
> index 320af37b2bc8..9fcc254707ba 100644
> --- a/libsepol/src/kernel_to_cil.c
> +++ b/libsepol/src/kernel_to_cil.c
> @@ -108,10 +108,12 @@ static char *cond_expr_to_str(struct policydb *pdb, struct cond_expr *expr)
>   	return str;
>   
>   exit:
> -	while ((new_val = strs_stack_pop(stack)) != NULL) {
> -		free(new_val);
> +	if (stack) {
> +		while ((new_val = strs_stack_pop(stack)) != NULL) {
> +			free(new_val);
> +		}
> +		strs_stack_destroy(&stack);
>   	}
> -	strs_stack_destroy(&stack);
>   
>   	return NULL;
>   }

constraint_expr_to_str() has the same problem.

> diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
> index 930bafabdd4b..2c8da49a11ab 100644
> --- a/libsepol/src/kernel_to_conf.c
> +++ b/libsepol/src/kernel_to_conf.c
> @@ -106,10 +106,12 @@ static char *cond_expr_to_str(struct policydb *pdb, struct cond_expr *expr)
>   	return str;
>   
>   exit:
> -	while ((new_val = strs_stack_pop(stack)) != NULL) {
> -		free(new_val);
> +	if (stack != NULL) {
> +		while ((new_val = strs_stack_pop(stack)) != NULL) {
> +			free(new_val);
> +		}
> +		strs_stack_destroy(&stack);
>   	}
> -	strs_stack_destroy(&stack);
>   
>   	return NULL;
>   }
> 

Same comment here. constraint_expr_to_str() also needs to be fixed.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Non-DoD Source] [PATCH 3/9] libsepol: do not dereference a failed allocated pointer
  2019-09-10 18:52   ` [Non-DoD Source] " jwcart2
@ 2019-09-10 20:11     ` jwcart2
  0 siblings, 0 replies; 15+ messages in thread
From: jwcart2 @ 2019-09-10 20:11 UTC (permalink / raw)
  To: Nicolas Iooss, selinux

On 9/10/19 2:52 PM, jwcart2 wrote:
> On 9/1/19 2:06 PM, Nicolas Iooss wrote:
>> When strs_stack_init(&stack) fails to allocate memory and stack is still
>> NULL, it should not be dereferenced with strs_stack_pop(stack).
>>
>> This issue has been found using Infer static analyzer.
>>
>> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>> ---
>>   libsepol/src/kernel_to_cil.c  | 8 +++++---
>>   libsepol/src/kernel_to_conf.c | 8 +++++---
>>   2 files changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
>> index 320af37b2bc8..9fcc254707ba 100644
>> --- a/libsepol/src/kernel_to_cil.c
>> +++ b/libsepol/src/kernel_to_cil.c
>> @@ -108,10 +108,12 @@ static char *cond_expr_to_str(struct policydb *pdb, 
>> struct cond_expr *expr)
>>       return str;
>>   exit:
>> -    while ((new_val = strs_stack_pop(stack)) != NULL) {
>> -        free(new_val);
>> +    if (stack) {
>> +        while ((new_val = strs_stack_pop(stack)) != NULL) {
>> +            free(new_val);
>> +        }
>> +        strs_stack_destroy(&stack);
>>       }
>> -    strs_stack_destroy(&stack);
>>       return NULL;
>>   }
> 
> constraint_expr_to_str() has the same problem.
> 
>> diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
>> index 930bafabdd4b..2c8da49a11ab 100644
>> --- a/libsepol/src/kernel_to_conf.c
>> +++ b/libsepol/src/kernel_to_conf.c
>> @@ -106,10 +106,12 @@ static char *cond_expr_to_str(struct policydb *pdb, 
>> struct cond_expr *expr)
>>       return str;
>>   exit:
>> -    while ((new_val = strs_stack_pop(stack)) != NULL) {
>> -        free(new_val);
>> +    if (stack != NULL) {

I forgot to mention that you should just use "if (stack) {" like above so the 
syntax is consistent.

Thanks,
Jim

>> +        while ((new_val = strs_stack_pop(stack)) != NULL) {
>> +            free(new_val);
>> +        }
>> +        strs_stack_destroy(&stack);
>>       }
>> -    strs_stack_destroy(&stack);
>>       return NULL;
>>   }
>>
> 
> Same comment here. constraint_expr_to_str() also needs to be fixed.
> 


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Non-DoD Source] [PATCH 0/9] Fix issues found by static analyzers
  2019-09-01 18:06 [PATCH 0/9] Fix issues found by static analyzers Nicolas Iooss
                   ` (8 preceding siblings ...)
  2019-09-01 18:06 ` [PATCH 9/9] libsepol/tests: do not dereference a NULL pointer Nicolas Iooss
@ 2019-09-16 16:46 ` jwcart2
  2019-09-17 15:01   ` jwcart2
  9 siblings, 1 reply; 15+ messages in thread
From: jwcart2 @ 2019-09-16 16:46 UTC (permalink / raw)
  To: Nicolas Iooss, selinux

On 9/1/19 2:06 PM, Nicolas Iooss wrote:
> Hi,
> 
> In August I ran several free static analyzers in order to find new
> issues in the userspace code. I used:
> 
> * clang's static analyzer (with scripts/run-scan-build)
> * https://lgtm.com/ (I wrote a simple configuration file to analyze the
>    C code, the results are available on
>    https://lgtm.com/projects/g/fishilico/selinux-for-lgtm/ )
> * Facebook's Infer (https://fbinfer.com/, I wrote a script to invoke it
>    like clang's static analyzer)
> 
> Each analyzer gave many results, with several true bugs in them. Here
> are patches that fix some of them. I will post the configuration files
> and scripts I used separately.
> 
> Thanks,
> Nicolas
> 
> Nicolas Iooss (9):
>    semodule-utils: fix comparison with argc
>    libsepol/cil: help static analyzers by aborting when an allocation
>      fails
>    libsepol: do not dereference a failed allocated pointer
>    libsepol: do not dereference scope if it can be NULL
>    libsepol: reset *p to NULL if sepol_module_package_create fails
>    libsepol/cil: do not dereference perm_value_to_cil when it has not
>      been allocated
>    python/chcat: remove unnecessary assignment
>    python/sepolicy: remove unnecessary pass statement
>    libsepol/tests: do not dereference a NULL pointer
> 
>   libsepol/cil/src/cil_binary.c                        | 10 ++++++----
>   libsepol/cil/src/cil_mem.c                           |  5 +++++
>   libsepol/src/avrule_block.c                          |  3 ++-
>   libsepol/src/kernel_to_cil.c                         |  8 +++++---
>   libsepol/src/kernel_to_conf.c                        |  8 +++++---
>   libsepol/src/module.c                                |  4 +++-
>   libsepol/tests/test-common.c                         | 11 +++++++----
>   python/chcat/chcat                                   |  1 -
>   python/sepolicy/sepolicy/__init__.py                 |  1 -
>   semodule-utils/semodule_package/semodule_unpackage.c |  2 +-
>   10 files changed, 34 insertions(+), 19 deletions(-)
> 

I forgot to mention this when I commented on patches 2 and 3, but patches 1 and 
4-9 all look good to me.

I plan on merging these, my take on your patch 2, and your updated patch 3 
tomorrow, unless there are any objections.

Jim


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Non-DoD Source] [PATCH 0/9] Fix issues found by static analyzers
  2019-09-16 16:46 ` [Non-DoD Source] [PATCH 0/9] Fix issues found by static analyzers jwcart2
@ 2019-09-17 15:01   ` jwcart2
  0 siblings, 0 replies; 15+ messages in thread
From: jwcart2 @ 2019-09-17 15:01 UTC (permalink / raw)
  To: Nicolas Iooss, selinux

On 9/16/19 12:46 PM, jwcart2 wrote:
> On 9/1/19 2:06 PM, Nicolas Iooss wrote:
>> Hi,
>>
>> In August I ran several free static analyzers in order to find new
>> issues in the userspace code. I used:
>>
>> * clang's static analyzer (with scripts/run-scan-build)
>> * https://lgtm.com/ (I wrote a simple configuration file to analyze the
>>    C code, the results are available on
>>    https://lgtm.com/projects/g/fishilico/selinux-for-lgtm/ )
>> * Facebook's Infer (https://fbinfer.com/, I wrote a script to invoke it
>>    like clang's static analyzer)
>>
>> Each analyzer gave many results, with several true bugs in them. Here
>> are patches that fix some of them. I will post the configuration files
>> and scripts I used separately.
>>
>> Thanks,
>> Nicolas
>>
>> Nicolas Iooss (9):
>>    semodule-utils: fix comparison with argc
>>    libsepol/cil: help static analyzers by aborting when an allocation
>>      fails
>>    libsepol: do not dereference a failed allocated pointer
>>    libsepol: do not dereference scope if it can be NULL
>>    libsepol: reset *p to NULL if sepol_module_package_create fails
>>    libsepol/cil: do not dereference perm_value_to_cil when it has not
>>      been allocated
>>    python/chcat: remove unnecessary assignment
>>    python/sepolicy: remove unnecessary pass statement
>>    libsepol/tests: do not dereference a NULL pointer
>>
>>   libsepol/cil/src/cil_binary.c                        | 10 ++++++----
>>   libsepol/cil/src/cil_mem.c                           |  5 +++++
>>   libsepol/src/avrule_block.c                          |  3 ++-
>>   libsepol/src/kernel_to_cil.c                         |  8 +++++---
>>   libsepol/src/kernel_to_conf.c                        |  8 +++++---
>>   libsepol/src/module.c                                |  4 +++-
>>   libsepol/tests/test-common.c                         | 11 +++++++----
>>   python/chcat/chcat                                   |  1 -
>>   python/sepolicy/sepolicy/__init__.py                 |  1 -
>>   semodule-utils/semodule_package/semodule_unpackage.c |  2 +-
>>   10 files changed, 34 insertions(+), 19 deletions(-)
>>
> 
> I forgot to mention this when I commented on patches 2 and 3, but patches 1 and 
> 4-9 all look good to me.
> 
> I plan on merging these, my take on your patch 2, and your updated patch 3 
> tomorrow, unless there are any objections.
> 

I have indeed merged your patches 1 and 4-9 along with my take on your patch 2 
and your updated patch 3.

Thanks,
Jim

> Jim
> 
> 


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-01 18:06 [PATCH 0/9] Fix issues found by static analyzers Nicolas Iooss
2019-09-01 18:06 ` [PATCH 1/9] semodule-utils: fix comparison with argc Nicolas Iooss
2019-09-01 18:06 ` [PATCH 2/9] libsepol/cil: help static analyzers by aborting when an allocation fails Nicolas Iooss
2019-09-10 18:48   ` [Non-DoD Source] " jwcart2
2019-09-01 18:06 ` [PATCH 3/9] libsepol: do not dereference a failed allocated pointer Nicolas Iooss
2019-09-10 18:52   ` [Non-DoD Source] " jwcart2
2019-09-10 20:11     ` jwcart2
2019-09-01 18:06 ` [PATCH 4/9] libsepol: do not dereference scope if it can be NULL Nicolas Iooss
2019-09-01 18:06 ` [PATCH 5/9] libsepol: reset *p to NULL if sepol_module_package_create fails Nicolas Iooss
2019-09-01 18:06 ` [PATCH 6/9] libsepol/cil: do not dereference perm_value_to_cil when it has not been allocated Nicolas Iooss
2019-09-01 18:06 ` [PATCH 7/9] python/chcat: remove unnecessary assignment Nicolas Iooss
2019-09-01 18:06 ` [PATCH 8/9] python/sepolicy: remove unnecessary pass statement Nicolas Iooss
2019-09-01 18:06 ` [PATCH 9/9] libsepol/tests: do not dereference a NULL pointer Nicolas Iooss
2019-09-16 16:46 ` [Non-DoD Source] [PATCH 0/9] Fix issues found by static analyzers jwcart2
2019-09-17 15:01   ` jwcart2

SELinux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/selinux/0 selinux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 selinux selinux/ https://lore.kernel.org/selinux \
		selinux@vger.kernel.org selinux@archiver.kernel.org
	public-inbox-index selinux

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.selinux


AGPL code for this site: git clone https://public-inbox.org/ public-inbox