| Message ID | 20210216141446.171306-1-plautrba@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] libselinux: fix segfault in add_xattr_entry() | expand |
On Tue, Feb 16, 2021 at 3:16 PM Petr Lautrbach <plautrba@redhat.com> wrote: > > When selabel_get_digests_all_partial_matches(), resp > get_digests_all_partial_matches() doesn't find a match, > calculated_digest is not initialized and followup memcmp() could > segfault. Given that calculated_digest and xattr_digest are already > compared in get_digests_all_partial_matches() and the function returns > true or false based on this comparison, it's not neccessary to compare (minor typo: necessary with only one C) > these values again. > > Fixes: > # restorecon_xattr -d -v tmp > specfiles SHA1 digest: afc752f47d489f3e82ac1da8fd247a2e1a6af5f8 > calculated using the following specfile(s): > /etc/selinux/targeted/contexts/files/file_contexts.subs_dist > /etc/selinux/targeted/contexts/files/file_contexts.subs > /etc/selinux/targeted/contexts/files/file_contexts.bin > /etc/selinux/targeted/contexts/files/file_contexts.homedirs.bin > /etc/selinux/targeted/contexts/files/file_contexts.local.bin > > Segmentation fault (core dumped) > > Signed-off-by: Petr Lautrbach <plautrba@redhat.com> Thanks! I wanted to reproduce the issue on an Arch Linux test VM and it was slightly more complex. Here is what I did: cd /root mkdir tmp restorecon -D -Rv tmp # create security.sehash attribute restorecon_xattr -d -v tmp # this segfaults in the memcmp() Both your patches look good. Nevertheless there is some inconsistencies in the "coding style" used in your patches: > --- > libselinux/src/selinux_restorecon.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c > index 6993be6fda17..4bca29b9de78 100644 > --- a/libselinux/src/selinux_restorecon.c > +++ b/libselinux/src/selinux_restorecon.c > @@ -297,6 +297,7 @@ static int add_xattr_entry(const char *directory, bool delete_nonmatch, > char *sha1_buf = NULL; > size_t i, digest_len = 0; > int rc, digest_result; > + bool match; > struct dir_xattr *new_entry; > uint8_t *xattr_digest = NULL; > uint8_t *calculated_digest = NULL; > @@ -306,7 +307,7 @@ static int add_xattr_entry(const char *directory, bool delete_nonmatch, > return -1; > } > > - selabel_get_digests_all_partial_matches(fc_sehandle, directory, > + match = selabel_get_digests_all_partial_matches(fc_sehandle, directory, > &calculated_digest, > &xattr_digest, &digest_len); Here: the parameters need to be indented with the new indentation. > > @@ -326,11 +327,10 @@ static int add_xattr_entry(const char *directory, bool delete_nonmatch, > for (i = 0; i < digest_len; i++) > sprintf((&sha1_buf[i * 2]), "%02x", xattr_digest[i]); > > - rc = memcmp(calculated_digest, xattr_digest, digest_len); > - digest_result = rc ? NOMATCH : MATCH; > + digest_result = match ? MATCH : NOMATCH; > > - if ((delete_nonmatch && rc != 0) || delete_all) { > - digest_result = rc ? DELETED_NOMATCH : DELETED_MATCH; > + if ((delete_nonmatch && ! match) || delete_all) { Here: the space between "!" and "match" is unexpected. > + digest_result = match ? DELETED_MATCH : DELETED_NOMATCH; > rc = removexattr(directory, RESTORECON_PARTIAL_MATCH_DIGEST); > if (rc) { > selinux_log(SELINUX_ERROR, > -- > 2.30.1 ... and in the second patch, the indentation of the parameters of the new fprintf(stderr,...) does not match the one used in other places of the file. Anyway these are minor issues, and ignoring them, for both patches: Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org> Thanks, Nicolas
Nicolas Iooss <nicolas.iooss@m4x.org> writes: > On Tue, Feb 16, 2021 at 3:16 PM Petr Lautrbach <plautrba@redhat.com> wrote: >> >> When selabel_get_digests_all_partial_matches(), resp >> get_digests_all_partial_matches() doesn't find a match, >> calculated_digest is not initialized and followup memcmp() could >> segfault. Given that calculated_digest and xattr_digest are already >> compared in get_digests_all_partial_matches() and the function returns >> true or false based on this comparison, it's not neccessary to compare > > (minor typo: necessary with only one C) > >> these values again. >> >> Fixes: >> # restorecon_xattr -d -v tmp >> specfiles SHA1 digest: afc752f47d489f3e82ac1da8fd247a2e1a6af5f8 >> calculated using the following specfile(s): >> /etc/selinux/targeted/contexts/files/file_contexts.subs_dist >> /etc/selinux/targeted/contexts/files/file_contexts.subs >> /etc/selinux/targeted/contexts/files/file_contexts.bin >> /etc/selinux/targeted/contexts/files/file_contexts.homedirs.bin >> /etc/selinux/targeted/contexts/files/file_contexts.local.bin >> >> Segmentation fault (core dumped) >> >> Signed-off-by: Petr Lautrbach <plautrba@redhat.com> > > Thanks! I wanted to reproduce the issue on an Arch Linux test VM and > it was slightly more complex. Here is what I did: > > cd /root > mkdir tmp > restorecon -D -Rv tmp # create security.sehash attribute > restorecon_xattr -d -v tmp # this segfaults in the memcmp() > > Both your patches look good. Nevertheless there is some > inconsistencies in the "coding style" used in your patches: > >> --- >> libselinux/src/selinux_restorecon.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c >> index 6993be6fda17..4bca29b9de78 100644 >> --- a/libselinux/src/selinux_restorecon.c >> +++ b/libselinux/src/selinux_restorecon.c >> @@ -297,6 +297,7 @@ static int add_xattr_entry(const char *directory, bool delete_nonmatch, >> char *sha1_buf = NULL; >> size_t i, digest_len = 0; >> int rc, digest_result; >> + bool match; >> struct dir_xattr *new_entry; >> uint8_t *xattr_digest = NULL; >> uint8_t *calculated_digest = NULL; >> @@ -306,7 +307,7 @@ static int add_xattr_entry(const char *directory, bool delete_nonmatch, >> return -1; >> } >> >> - selabel_get_digests_all_partial_matches(fc_sehandle, directory, >> + match = selabel_get_digests_all_partial_matches(fc_sehandle, directory, >> &calculated_digest, >> &xattr_digest, &digest_len); > > Here: the parameters need to be indented with the new indentation. > >> >> @@ -326,11 +327,10 @@ static int add_xattr_entry(const char *directory, bool delete_nonmatch, >> for (i = 0; i < digest_len; i++) >> sprintf((&sha1_buf[i * 2]), "%02x", xattr_digest[i]); >> >> - rc = memcmp(calculated_digest, xattr_digest, digest_len); >> - digest_result = rc ? NOMATCH : MATCH; >> + digest_result = match ? MATCH : NOMATCH; >> >> - if ((delete_nonmatch && rc != 0) || delete_all) { >> - digest_result = rc ? DELETED_NOMATCH : DELETED_MATCH; >> + if ((delete_nonmatch && ! match) || delete_all) { > > Here: the space between "!" and "match" is unexpected. > >> + digest_result = match ? DELETED_MATCH : DELETED_NOMATCH; >> rc = removexattr(directory, RESTORECON_PARTIAL_MATCH_DIGEST); >> if (rc) { >> selinux_log(SELINUX_ERROR, >> -- >> 2.30.1 > > ... and in the second patch, the indentation of the parameters of the > new fprintf(stderr,...) does not match the one used in other places of > the file. > > Anyway these are minor issues, and ignoring them, for both patches: > Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org> > Merged with suggested improvements. Thanks!
diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c index 6993be6fda17..4bca29b9de78 100644 --- a/libselinux/src/selinux_restorecon.c +++ b/libselinux/src/selinux_restorecon.c @@ -297,6 +297,7 @@ static int add_xattr_entry(const char *directory, bool delete_nonmatch, char *sha1_buf = NULL; size_t i, digest_len = 0; int rc, digest_result; + bool match; struct dir_xattr *new_entry; uint8_t *xattr_digest = NULL; uint8_t *calculated_digest = NULL; @@ -306,7 +307,7 @@ static int add_xattr_entry(const char *directory, bool delete_nonmatch, return -1; } - selabel_get_digests_all_partial_matches(fc_sehandle, directory, + match = selabel_get_digests_all_partial_matches(fc_sehandle, directory, &calculated_digest, &xattr_digest, &digest_len); @@ -326,11 +327,10 @@ static int add_xattr_entry(const char *directory, bool delete_nonmatch, for (i = 0; i < digest_len; i++) sprintf((&sha1_buf[i * 2]), "%02x", xattr_digest[i]); - rc = memcmp(calculated_digest, xattr_digest, digest_len); - digest_result = rc ? NOMATCH : MATCH; + digest_result = match ? MATCH : NOMATCH; - if ((delete_nonmatch && rc != 0) || delete_all) { - digest_result = rc ? DELETED_NOMATCH : DELETED_MATCH; + if ((delete_nonmatch && ! match) || delete_all) { + digest_result = match ? DELETED_MATCH : DELETED_NOMATCH; rc = removexattr(directory, RESTORECON_PARTIAL_MATCH_DIGEST); if (rc) { selinux_log(SELINUX_ERROR,
When selabel_get_digests_all_partial_matches(), resp get_digests_all_partial_matches() doesn't find a match, calculated_digest is not initialized and followup memcmp() could segfault. Given that calculated_digest and xattr_digest are already compared in get_digests_all_partial_matches() and the function returns true or false based on this comparison, it's not neccessary to compare these values again. Fixes: # restorecon_xattr -d -v tmp specfiles SHA1 digest: afc752f47d489f3e82ac1da8fd247a2e1a6af5f8 calculated using the following specfile(s): /etc/selinux/targeted/contexts/files/file_contexts.subs_dist /etc/selinux/targeted/contexts/files/file_contexts.subs /etc/selinux/targeted/contexts/files/file_contexts.bin /etc/selinux/targeted/contexts/files/file_contexts.homedirs.bin /etc/selinux/targeted/contexts/files/file_contexts.local.bin Segmentation fault (core dumped) Signed-off-by: Petr Lautrbach <plautrba@redhat.com> --- libselinux/src/selinux_restorecon.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)