[PATCH 3/3] libsemanage: silence clang static analyzer report

Discussion:

Nicolas Iooss

2018-03-05 22:58:20 UTC

clang's static analyzer reports an out-of-bound array access in
semanage_user_roles() when num_roles is zero, with the following
statement:

strcpy(roles,roles_arr[0]);

When num_roles is zero, roles_arr[0] is not uninitialized and roles is
the result of malloc(0) so this strcpy is dangerous. Make
semanage_user_roles() return an empty string instead.

Signed-off-by: Nicolas Iooss <***@m4x.org>
---
libsemanage/src/seusers_local.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/libsemanage/src/seusers_local.c b/libsemanage/src/seusers_local.c
index 42c3a8b662c2..413ebdddeb34 100644
--- a/libsemanage/src/seusers_local.c
+++ b/libsemanage/src/seusers_local.c
@@ -35,12 +35,16 @@ static char *semanage_user_roles(semanage_handle_t * handle, const char *sename)
for (i = 0; i<num_roles; i++) {
size += (strlen(roles_arr[i]) + 1);
}
- roles = malloc(size);
- if (roles) {
- strcpy(roles,roles_arr[0]);
- for (i = 1; i<num_roles; i++) {
- strcat(roles,",");
- strcat(roles,roles_arr[i]);
+ if (num_roles == 0) {
+ roles = strdup("");
+ } else {
+ roles = malloc(size);
+ if (roles) {
+ strcpy(roles,roles_arr[0]);
+ for (i = 1; i<num_roles; i++) {
+ strcat(roles,",");
+ strcat(roles,roles_arr[i]);
+ }
}
}
}

--
2.16.0

Nicolas Iooss

2018-03-05 22:58:19 UTC

Permalink

cil_tree_print_expr() calls cil_expr_to_string() in order to compute a
string expression into expr_str. If this function fails, expr_str is
left unitialized but its value is dereferenced with:

cil_log(CIL_INFO, "%s)", expr_str);

Prevent such an issue by checking cil_expr_to_string()'s return value
before using expr_str.

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

Signed-off-by: Nicolas Iooss <***@m4x.org>
---
libsepol/cil/src/cil_tree.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c
index d36401b41dba..b394a9d849df 100644
--- a/libsepol/cil/src/cil_tree.c
+++ b/libsepol/cil/src/cil_tree.c
@@ -503,15 +503,19 @@ exit:
void cil_tree_print_expr(struct cil_list *datum_expr, struct cil_list *str_expr)
{
char *expr_str;
+ int rc;

cil_log(CIL_INFO, "(");

if (datum_expr != NULL) {
- cil_expr_to_string(datum_expr, &expr_str);
+ rc = cil_expr_to_string(datum_expr, &expr_str);
} else {
- cil_expr_to_string(str_expr, &expr_str);
+ rc = cil_expr_to_string(str_expr, &expr_str);
+ }
+ if (rc < 0) {
+ cil_log(CIL_INFO, "ERROR)");
+ return;
}
-
cil_log(CIL_INFO, "%s)", expr_str);
free(expr_str);
}

--
2.16.0

Stephen Smalley

2018-03-06 21:29:57 UTC

Permalink

Post by Nicolas Iooss
cil_tree_print_expr() calls cil_expr_to_string() in order to compute a
string expression into expr_str. If this function fails, expr_str is
cil_log(CIL_INFO, "%s)", expr_str);
Prevent such an issue by checking cil_expr_to_string()'s return value
before using expr_str.
This issue has been found with clang's static analyzer.
---
libsepol/cil/src/cil_tree.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c
index d36401b41dba..b394a9d849df 100644
--- a/libsepol/cil/src/cil_tree.c
+++ b/libsepol/cil/src/cil_tree.c
void cil_tree_print_expr(struct cil_list *datum_expr, struct cil_list *str_expr)
{
char *expr_str;
+ int rc;
cil_log(CIL_INFO, "(");
if (datum_expr != NULL) {
- cil_expr_to_string(datum_expr, &expr_str);
+ rc = cil_expr_to_string(datum_expr, &expr_str);
} else {
- cil_expr_to_string(str_expr, &expr_str);
+ rc = cil_expr_to_string(str_expr, &expr_str);
+ }
+ if (rc < 0) {
+ cil_log(CIL_INFO, "ERROR)");
+ return;

Wondering if we should abort or return an error to the caller instead of just logging an error and returning?

Post by Nicolas Iooss
}
-
cil_log(CIL_INFO, "%s)", expr_str);
free(expr_str);
}

Nicolas Iooss

2018-03-08 20:42:32 UTC

Permalink

Post by Stephen Smalley

Wondering if we should abort or return an error to the caller instead of just logging an error and returning?

I believe this depends on the usage of cil_tree_print_expr(). I have
not found any caller of cil_tree_print_* outside of
libsepol/cil/src/cil_tree.c so I guess this interface is only used by
developers for debugging purpose when playing with libsepol's CIL
objects (being able to quickly print an object in a human-readable
form is a very valuable feature). Am I right? The current interface of
ignoring all errors and logging as much as possible without aborting
seems consistent with such a use-case.

If there is an interest in propagating errors found by
cil_tree_print_expr() back to its callers, I can spend some time to
clean up the code. Otherwise I will send other patches (once they are
in a suitable state) to fix some issues that clang's static analyzer
find.

Thanks,
Nicolas

Stephen Smalley

2018-03-08 21:28:46 UTC

Permalink

Post by Nicolas Iooss

Post by Stephen Smalley

Wondering if we should abort or return an error to the caller instead of just logging an error and returning?

I believe this depends on the usage of cil_tree_print_expr(). I have
not found any caller of cil_tree_print_* outside of
libsepol/cil/src/cil_tree.c so I guess this interface is only used by
developers for debugging purpose when playing with libsepol's CIL
objects (being able to quickly print an object in a human-readable
form is a very valuable feature). Am I right? The current interface of
ignoring all errors and logging as much as possible without aborting
seems consistent with such a use-case.
If there is an interest in propagating errors found by
cil_tree_print_expr() back to its callers, I can spend some time to
clean up the code. Otherwise I will send other patches (once they are
in a suitable state) to fix some issues that clang's static analyzer
find.

You are correct, and James agreed with your approach, which is why I merged the patch as is.

jwcart2

2018-03-07 14:53:40 UTC

Permalink

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

Stephen Smalley

2018-03-06 21:29:20 UTC

Permalink

In sepol_ibendport_key_create(), if sepol_ibendport_alloc_ibdev_name()
fails to allocate tmp_key->ibdev_name, sepol_ibendport_key_free() is
called to free the memory associated with tmp_key, which results in
free() being called on uninitialized tmp_key->ibdev_name.
This issue is reported by clang's static analyzer with the following
ibendport_record.c:115:2: warning: 1st function call argument is an
uninitialized value
free(key->ibdev_name);
^~~~~~~~~~~~~~~~~~~~~
---
libsepol/src/ibendport_record.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/libsepol/src/ibendport_record.c b/libsepol/src/ibendport_record.c
index 912aeb536fd9..8285a9d6ffcf 100644
--- a/libsepol/src/ibendport_record.c
+++ b/libsepol/src/ibendport_record.c
@@ -62,8 +62,10 @@ int sepol_ibendport_key_create(sepol_handle_t *handle,
goto omem;
}
- if (sepol_ibendport_alloc_ibdev_name(handle, &tmp_key->ibdev_name) < 0)
+ if (sepol_ibendport_alloc_ibdev_name(handle, &tmp_key->ibdev_name) < 0) {
+ tmp_key->ibdev_name = NULL;
goto err;
+ }
strncpy(tmp_key->ibdev_name, ibdev_name, IB_DEVICE_NAME_MAX);
tmp_key->port = port;

Stephen Smalley

2018-03-06 21:31:49 UTC

Permalink

Post by Nicolas Iooss
clang's static analyzer reports an out-of-bound array access in
semanage_user_roles() when num_roles is zero, with the following
strcpy(roles,roles_arr[0]);
When num_roles is zero, roles_arr[0] is not uninitialized and roles is
the result of malloc(0) so this strcpy is dangerous. Make
semanage_user_roles() return an empty string instead.
---
libsemanage/src/seusers_local.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/libsemanage/src/seusers_local.c b/libsemanage/src/seusers_local.c
index 42c3a8b662c2..413ebdddeb34 100644
--- a/libsemanage/src/seusers_local.c
+++ b/libsemanage/src/seusers_local.c
@@ -35,12 +35,16 @@ static char *semanage_user_roles(semanage_handle_t * handle, const char *sename)
for (i = 0; i<num_roles; i++) {
size += (strlen(roles_arr[i]) + 1);
}
- roles = malloc(size);
- if (roles) {
- strcpy(roles,roles_arr[0]);
- for (i = 1; i<num_roles; i++) {
- strcat(roles,",");
- strcat(roles,roles_arr[i]);
+ if (num_roles == 0) {
+ roles = strdup("");
+ } else {
+ roles = malloc(size);
+ if (roles) {
+ strcpy(roles,roles_arr[0]);
+ for (i = 1; i<num_roles; i++) {
+ strcat(roles,",");
+ strcat(roles,roles_arr[i]);
+ }
}
}
}

jwcart2

2018-03-07 14:57:06 UTC

Permalink

It seems to be that a better way to fix this is to modify
sepol_ibendport_alloc_ibdev_name(), so that the result of the calloc is assigned
to *ibdev_name. That way if the calloc fails, tmp_key->ibdev_name will be set to
NULL automatically.

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

Stephen Smalley

2018-03-08 19:49:38 UTC

Permalink

Applied patches 2 and 3; patch 1 will be obsoleted by James' patch when it is merged, unless there are objections to it.

Post by Nicolas Iooss
---
libsemanage/src/seusers_local.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/libsemanage/src/seusers_local.c b/libsemanage/src/seusers_local.c
index 42c3a8b662c2..413ebdddeb34 100644
--- a/libsemanage/src/seusers_local.c
+++ b/libsemanage/src/seusers_local.c
@@ -35,12 +35,16 @@ static char *semanage_user_roles(semanage_handle_t * handle, const char *sename)
for (i = 0; i<num_roles; i++) {
size += (strlen(roles_arr[i]) + 1);
}
- roles = malloc(size);
- if (roles) {
- strcpy(roles,roles_arr[0]);
- for (i = 1; i<num_roles; i++) {
- strcat(roles,",");
- strcat(roles,roles_arr[i]);
+ if (num_roles == 0) {
+ roles = strdup("");
+ } else {
+ roles = malloc(size);
+ if (roles) {
+ strcpy(roles,roles_arr[0]);
+ for (i = 1; i<num_roles; i++) {
+ strcat(roles,",");
+ strcat(roles,roles_arr[i]);
+ }
}
}
}