| Message ID | 20190918210450.8692-1-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] libselinux: ensure strlen() is not called on NULL | expand |
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 > >
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
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 >
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
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(-)