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>
Thanks,
Nicolas
