SELinux Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 1/2] libselinux: ensure strlen() is not called on NULL
@ 2019-09-20  5:59 Nicolas Iooss
  2019-09-20  5:59 ` [PATCH v2 2/2] libselinux: do not add rc to pos twice Nicolas Iooss
  2019-09-20 14:22 ` [PATCH v2 1/2] libselinux: ensure strlen() is not called on NULL Stephen Smalley
  0 siblings, 2 replies; 5+ messages in thread
From: Nicolas Iooss @ 2019-09-20  5:59 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
trying to format it.

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 | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/libselinux/src/regex.c b/libselinux/src/regex.c
index a6fcbbfec1f3..c835dd1b0e5d 100644
--- a/libselinux/src/regex.c
+++ b/libselinux/src/regex.c
@@ -519,6 +519,29 @@ void regex_format_error(struct regex_error_data const *error_data, char *buffer,
 	if (pos >= buf_size)
 		goto truncated;
 
+	/* Return early if there is no error to format */
+#ifdef USE_PCRE2
+	if (!error_data->error_code) {
+		rc = snprintf(buffer + pos, buf_size - pos, "no error code");
+		if (rc < 0)
+			abort();
+		pos += rc;
+		if (pos >= buf_size)
+			goto truncated;
+		return;
+	}
+#else
+	if (!error_data->error_buffer) {
+		rc = snprintf(buffer + pos, buf_size - pos, "empty error");
+		if (rc < 0)
+			abort();
+		pos += rc;
+		if (pos >= buf_size)
+			goto truncated;
+		return;
+	}
+#endif
+
 	if (error_data->error_offset > 0) {
 #ifdef USE_PCRE2
 		rc = snprintf(buffer + pos, buf_size - pos, "At offset %zu: ",
-- 
2.22.0


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

* [PATCH v2 2/2] libselinux: do not add rc to pos twice
  2019-09-20  5:59 [PATCH v2 1/2] libselinux: ensure strlen() is not called on NULL Nicolas Iooss
@ 2019-09-20  5:59 ` Nicolas Iooss
  2019-09-20 14:22   ` Stephen Smalley
  2019-09-23 13:54   ` Stephen Smalley
  2019-09-20 14:22 ` [PATCH v2 1/2] libselinux: ensure strlen() is not called on NULL Stephen Smalley
  1 sibling, 2 replies; 5+ messages in thread
From: Nicolas Iooss @ 2019-09-20  5:59 UTC (permalink / raw)
  To: selinux

In regex_format_error(), when error_data->error_offset is zero, rc is
not updated and should not be added to pos again.

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

diff --git a/libselinux/src/regex.c b/libselinux/src/regex.c
index c835dd1b0e5d..770bc3ea1310 100644
--- a/libselinux/src/regex.c
+++ b/libselinux/src/regex.c
@@ -552,10 +552,10 @@ void regex_format_error(struct regex_error_data const *error_data, char *buffer,
 #endif
 		if (rc < 0)
 			abort();
+		pos += rc;
+		if (pos >= buf_size)
+			goto truncated;
 	}
-	pos += rc;
-	if (pos >= buf_size)
-		goto truncated;
 
 #ifdef USE_PCRE2
 	rc = pcre2_get_error_message(error_data->error_code,
-- 
2.22.0


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

* Re: [PATCH v2 1/2] libselinux: ensure strlen() is not called on NULL
  2019-09-20  5:59 [PATCH v2 1/2] libselinux: ensure strlen() is not called on NULL Nicolas Iooss
  2019-09-20  5:59 ` [PATCH v2 2/2] libselinux: do not add rc to pos twice Nicolas Iooss
@ 2019-09-20 14:22 ` Stephen Smalley
  1 sibling, 0 replies; 5+ messages in thread
From: Stephen Smalley @ 2019-09-20 14:22 UTC (permalink / raw)
  To: Nicolas Iooss, selinux

On 9/20/19 1:59 AM, 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
> trying to format it.
> 
> 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>

Personally, I would have just used a single error message for both cases 
(and only wrapped the condition itself with #ifdef...#else...#endif), 
but I'm ok with this as well.

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

> ---
>   libselinux/src/regex.c | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
> 
> diff --git a/libselinux/src/regex.c b/libselinux/src/regex.c
> index a6fcbbfec1f3..c835dd1b0e5d 100644
> --- a/libselinux/src/regex.c
> +++ b/libselinux/src/regex.c
> @@ -519,6 +519,29 @@ void regex_format_error(struct regex_error_data const *error_data, char *buffer,
>   	if (pos >= buf_size)
>   		goto truncated;
>   
> +	/* Return early if there is no error to format */
> +#ifdef USE_PCRE2
> +	if (!error_data->error_code) {
> +		rc = snprintf(buffer + pos, buf_size - pos, "no error code");
> +		if (rc < 0)
> +			abort();
> +		pos += rc;
> +		if (pos >= buf_size)
> +			goto truncated;
> +		return;
> +	}
> +#else
> +	if (!error_data->error_buffer) {
> +		rc = snprintf(buffer + pos, buf_size - pos, "empty error");
> +		if (rc < 0)
> +			abort();
> +		pos += rc;
> +		if (pos >= buf_size)
> +			goto truncated;
> +		return;
> +	}
> +#endif
> +
>   	if (error_data->error_offset > 0) {
>   #ifdef USE_PCRE2
>   		rc = snprintf(buffer + pos, buf_size - pos, "At offset %zu: ",
> 


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

* Re: [PATCH v2 2/2] libselinux: do not add rc to pos twice
  2019-09-20  5:59 ` [PATCH v2 2/2] libselinux: do not add rc to pos twice Nicolas Iooss
@ 2019-09-20 14:22   ` Stephen Smalley
  2019-09-23 13:54   ` Stephen Smalley
  1 sibling, 0 replies; 5+ messages in thread
From: Stephen Smalley @ 2019-09-20 14:22 UTC (permalink / raw)
  To: Nicolas Iooss, selinux

On 9/20/19 1:59 AM, Nicolas Iooss wrote:
> In regex_format_error(), when error_data->error_offset is zero, rc is
> not updated and should not be added to pos again.
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>

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

> ---
>   libselinux/src/regex.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libselinux/src/regex.c b/libselinux/src/regex.c
> index c835dd1b0e5d..770bc3ea1310 100644
> --- a/libselinux/src/regex.c
> +++ b/libselinux/src/regex.c
> @@ -552,10 +552,10 @@ void regex_format_error(struct regex_error_data const *error_data, char *buffer,
>   #endif
>   		if (rc < 0)
>   			abort();
> +		pos += rc;
> +		if (pos >= buf_size)
> +			goto truncated;
>   	}
> -	pos += rc;
> -	if (pos >= buf_size)
> -		goto truncated;
>   
>   #ifdef USE_PCRE2
>   	rc = pcre2_get_error_message(error_data->error_code,
> 


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

* Re: [PATCH v2 2/2] libselinux: do not add rc to pos twice
  2019-09-20  5:59 ` [PATCH v2 2/2] libselinux: do not add rc to pos twice Nicolas Iooss
  2019-09-20 14:22   ` Stephen Smalley
@ 2019-09-23 13:54   ` Stephen Smalley
  1 sibling, 0 replies; 5+ messages in thread
From: Stephen Smalley @ 2019-09-23 13:54 UTC (permalink / raw)
  To: Nicolas Iooss, selinux

On 9/20/19 1:59 AM, Nicolas Iooss wrote:
> In regex_format_error(), when error_data->error_offset is zero, rc is
> not updated and should not be added to pos again.

Thanks, both patches applied.

> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>   libselinux/src/regex.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libselinux/src/regex.c b/libselinux/src/regex.c
> index c835dd1b0e5d..770bc3ea1310 100644
> --- a/libselinux/src/regex.c
> +++ b/libselinux/src/regex.c
> @@ -552,10 +552,10 @@ void regex_format_error(struct regex_error_data const *error_data, char *buffer,
>   #endif
>   		if (rc < 0)
>   			abort();
> +		pos += rc;
> +		if (pos >= buf_size)
> +			goto truncated;
>   	}
> -	pos += rc;
> -	if (pos >= buf_size)
> -		goto truncated;
>   
>   #ifdef USE_PCRE2
>   	rc = pcre2_get_error_message(error_data->error_code,
> 


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

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-20  5:59 [PATCH v2 1/2] libselinux: ensure strlen() is not called on NULL Nicolas Iooss
2019-09-20  5:59 ` [PATCH v2 2/2] libselinux: do not add rc to pos twice Nicolas Iooss
2019-09-20 14:22   ` Stephen Smalley
2019-09-23 13:54   ` Stephen Smalley
2019-09-20 14:22 ` [PATCH v2 1/2] libselinux: ensure strlen() is not called on NULL 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 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