Re: [PATCH 1/2] libselinux: fix segfault in add_xattr_entry()

[Petr Lautrbach <plautrba@redhat.com>]

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!