* [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 related [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 related [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
* [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 related [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
* [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 related [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 related [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 related [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 related [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 related [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 related [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