[PATCH 1/3] libsepol: initialize tmp_key->ibdev_name if its allocation failed

[PATCH 1/3] libsepol: initialize tmp_key->ibdev_name if its allocation failed

[Nicolas Iooss]

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
message:

    ibendport_record.c:115:2: warning: 1st function call argument is an
    uninitialized value
            free(key->ibdev_name);
            ^~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 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;
-- 
2.16.0

[PATCH 2/3] libsepol: cil: show an error when cil_expr_to_string() fails

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

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

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

Re: [PATCH 1/3] libsepol: initialize tmp_key->ibdev_name if its allocation failed

[Stephen Smalley]

On 03/05/2018 05:58 PM, Nicolas Iooss wrote:
> 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
> message:
> 
>     ibendport_record.c:115:2: warning: 1st function call argument is an
>     uninitialized value
>             free(key->ibdev_name);
>             ^~~~~~~~~~~~~~~~~~~~~
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

> ---
>  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;
> 

Re: [PATCH 2/3] libsepol: cil: show an error when cil_expr_to_string() fails

[Stephen Smalley]

On 03/05/2018 05:58 PM, Nicolas Iooss wrote:
> 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 <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;

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

>  	}
> -
>  	cil_log(CIL_INFO, "%s)", expr_str);
>  	free(expr_str);
>  }
> 

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

[Stephen Smalley]

On 03/05/2018 05:58 PM, Nicolas Iooss wrote:
> 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 <nicolas.iooss@m4x.org>

Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

> ---
>  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]);
> +						}
>  					}
>  				}
>  			}
> 

Re: [PATCH 2/3] libsepol: cil: show an error when cil_expr_to_string() fails

[jwcart2]

On 03/05/2018 05:58 PM, Nicolas Iooss wrote:
> 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 <nicolas.iooss@m4x.org>

Acked-by: James Carter <jwcart2@tycho.nsa.gov>

> ---
>   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);
>   }
> 


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

Re: [PATCH 1/3] libsepol: initialize tmp_key->ibdev_name if its allocation failed

[jwcart2]

On 03/05/2018 05:58 PM, Nicolas Iooss wrote:
> 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
> message:
> 
>      ibendport_record.c:115:2: warning: 1st function call argument is an
>      uninitialized value
>              free(key->ibdev_name);
>              ^~~~~~~~~~~~~~~~~~~~~
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>   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;
> 

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 <jwcart2@tycho.nsa.gov>
National Security Agency

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

[Stephen Smalley]

On 03/05/2018 05:58 PM, Nicolas Iooss wrote:
> 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.

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

> 
> Signed-off-by: Nicolas Iooss <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]);
> +						}
>  					}
>  				}
>  			}
> 

Re: [PATCH 2/3] libsepol: cil: show an error when cil_expr_to_string() fails

[Nicolas Iooss]

On Tue, Mar 6, 2018 at 10:29 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 03/05/2018 05:58 PM, Nicolas Iooss wrote:
>> 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 <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;
>
> 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

Re: Re: [PATCH 2/3] libsepol: cil: show an error when cil_expr_to_string() fails

[Stephen Smalley]

On 03/08/2018 03:42 PM, Nicolas Iooss wrote:
> On Tue, Mar 6, 2018 at 10:29 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 03/05/2018 05:58 PM, Nicolas Iooss wrote:
>>> 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 <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;
>>
>> 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.