Nicolas Iooss <nicolas.iooss@xxxxxxx> writes:
> On Tue, Feb 16, 2021 at 3:16 PM Petr Lautrbach <plautrba@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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@xxxxxxx>
>
Merged with suggested improvements. Thanks!
