SELinux Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/1] libselinux: ensure strlen() is not called on NULL
@ 2019-09-18 21:04 Nicolas Iooss
  2019-09-19 16:57 ` Stephen Smalley
  0 siblings, 1 reply; 4+ messages in thread
From: Nicolas Iooss @ 2019-09-18 21:04 UTC (permalink / raw)
  To: selinux

When compile_regex() calls regex_prepare_data() and this function fails
in the following condition:

    *regex = regex_data_create();
    if (!(*regex))
        return -1;

... error_data has been zero-ed and compile_regex() calls:

    regex_format_error(&error_data,
        regex_error_format_buffer,
        sizeof(regex_error_format_buffer));

This leads to a call to strlen(error_data->error_buffer), where
error_data->error_buffer is NULL.

Avoid this by checking that error_data->error_buffer is not NULL before
calling strlen().

This issue has been found using clang's static analyzer:
https://337-118970575-gh.circle-artifacts.com/0/output-scan-build/2019-09-01-181851-6152-1/report-0b122b.html#EndPath

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

diff --git a/libselinux/src/regex.c b/libselinux/src/regex.c
index a6fcbbfec1f3..f967efe4a32f 100644
--- a/libselinux/src/regex.c
+++ b/libselinux/src/regex.c
@@ -546,7 +546,7 @@ void regex_format_error(struct regex_error_data const *error_data, char *buffer,
 	if (rc < 0)
 		abort();
 
-	if ((size_t)rc < strlen(error_data->error_buffer))
+	if (error_data->error_buffer && (size_t)rc < strlen(error_data->error_buffer))
 		goto truncated;
 #endif
 
-- 
2.22.0


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

* Re: [PATCH 1/1] libselinux: ensure strlen() is not called on NULL
  2019-09-18 21:04 [PATCH 1/1] libselinux: ensure strlen() is not called on NULL Nicolas Iooss
@ 2019-09-19 16:57 ` Stephen Smalley
  2019-09-19 20:51   ` Nicolas Iooss
  0 siblings, 1 reply; 4+ messages in thread
From: Stephen Smalley @ 2019-09-19 16:57 UTC (permalink / raw)
  To: Nicolas Iooss, selinux

On 9/18/19 5:04 PM, Nicolas Iooss wrote:
> When compile_regex() calls regex_prepare_data() and this function fails
> in the following condition:
> 
>      *regex = regex_data_create();
>      if (!(*regex))
>          return -1;
> 
> ... error_data has been zero-ed and compile_regex() calls:
> 
>      regex_format_error(&error_data,
>          regex_error_format_buffer,
>          sizeof(regex_error_format_buffer));
> 
> This leads to a call to strlen(error_data->error_buffer), where
> error_data->error_buffer is NULL.
> 
> Avoid this by checking that error_data->error_buffer is not NULL before
> calling strlen().

It seems like regex_format_error() should just return immediately if 
!error_data->error_code (#ifdef USE_PCRE2) or !error_data->error_buffer 
(#ifndef USE_PCRE2), since there is no back-end error message to get and 
report in that situation.

> 
> This issue has been found using clang's static analyzer:
> https://337-118970575-gh.circle-artifacts.com/0/output-scan-build/2019-09-01-181851-6152-1/report-0b122b.html#EndPath
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>   libselinux/src/regex.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libselinux/src/regex.c b/libselinux/src/regex.c
> index a6fcbbfec1f3..f967efe4a32f 100644
> --- a/libselinux/src/regex.c
> +++ b/libselinux/src/regex.c
> @@ -546,7 +546,7 @@ void regex_format_error(struct regex_error_data const *error_data, char *buffer,
>   	if (rc < 0)
>   		abort();
>   
> -	if ((size_t)rc < strlen(error_data->error_buffer))
> +	if (error_data->error_buffer && (size_t)rc < strlen(error_data->error_buffer))
>   		goto truncated;
>   #endif
>   
> 


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

* Re: [PATCH 1/1] libselinux: ensure strlen() is not called on NULL
  2019-09-19 16:57 ` Stephen Smalley
@ 2019-09-19 20:51   ` Nicolas Iooss
  2019-09-20 12:12     ` Stephen Smalley
  0 siblings, 1 reply; 4+ messages in thread
From: Nicolas Iooss @ 2019-09-19 20:51 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: SElinux list

On Thu, Sep 19, 2019 at 6:57 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On 9/18/19 5:04 PM, Nicolas Iooss wrote:
> > When compile_regex() calls regex_prepare_data() and this function fails
> > in the following condition:
> >
> >      *regex = regex_data_create();
> >      if (!(*regex))
> >          return -1;
> >
> > ... error_data has been zero-ed and compile_regex() calls:
> >
> >      regex_format_error(&error_data,
> >          regex_error_format_buffer,
> >          sizeof(regex_error_format_buffer));
> >
> > This leads to a call to strlen(error_data->error_buffer), where
> > error_data->error_buffer is NULL.
> >
> > Avoid this by checking that error_data->error_buffer is not NULL before
> > calling strlen().
>
> It seems like regex_format_error() should just return immediately if
> !error_data->error_code (#ifdef USE_PCRE2) or !error_data->error_buffer
> (#ifndef USE_PCRE2), since there is no back-end error message to get and
> report in that situation.

I agree. I will modify the patch.

By the way, while reading function regex_format_error() more
precisely, something seems strange in:

pos += rc;
if (pos >= buf_size)
    goto truncated;
if (error_data->error_offset > 0) {
    /* ... */
}
pos += rc;

As rc is not reset to zero, its value is added twice to pos. Is this a
bug, or am I misunderstanding something?

Thanks,
Nicolas


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

* Re: [PATCH 1/1] libselinux: ensure strlen() is not called on NULL
  2019-09-19 20:51   ` Nicolas Iooss
@ 2019-09-20 12:12     ` Stephen Smalley
  0 siblings, 0 replies; 4+ messages in thread
From: Stephen Smalley @ 2019-09-20 12:12 UTC (permalink / raw)
  To: Nicolas Iooss; +Cc: SElinux list

On 9/19/19 4:51 PM, Nicolas Iooss wrote:
> On Thu, Sep 19, 2019 at 6:57 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>
>> On 9/18/19 5:04 PM, Nicolas Iooss wrote:
>>> When compile_regex() calls regex_prepare_data() and this function fails
>>> in the following condition:
>>>
>>>       *regex = regex_data_create();
>>>       if (!(*regex))
>>>           return -1;
>>>
>>> ... error_data has been zero-ed and compile_regex() calls:
>>>
>>>       regex_format_error(&error_data,
>>>           regex_error_format_buffer,
>>>           sizeof(regex_error_format_buffer));
>>>
>>> This leads to a call to strlen(error_data->error_buffer), where
>>> error_data->error_buffer is NULL.
>>>
>>> Avoid this by checking that error_data->error_buffer is not NULL before
>>> calling strlen().
>>
>> It seems like regex_format_error() should just return immediately if
>> !error_data->error_code (#ifdef USE_PCRE2) or !error_data->error_buffer
>> (#ifndef USE_PCRE2), since there is no back-end error message to get and
>> report in that situation.
> 
> I agree. I will modify the patch.
> 
> By the way, while reading function regex_format_error() more
> precisely, something seems strange in:
> 
> pos += rc;
> if (pos >= buf_size)
>      goto truncated;
> if (error_data->error_offset > 0) {
>      /* ... */
> }
> pos += rc;
> 
> As rc is not reset to zero, its value is added twice to pos. Is this a
> bug, or am I misunderstanding something?

I think you are correct - it is a bug.

> 
> Thanks,
> Nicolas
> 


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

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-18 21:04 [PATCH 1/1] libselinux: ensure strlen() is not called on NULL Nicolas Iooss
2019-09-19 16:57 ` Stephen Smalley
2019-09-19 20:51   ` Nicolas Iooss
2019-09-20 12:12     ` Stephen Smalley

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
	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.git