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

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

[Petr Lautrbach]

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(-)

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,
-- 
2.30.1

[PATCH 2/2] policycoreutils: Resolve path in restorecon_xattr

[Petr Lautrbach]

Resolve pathname before selinux_restorecon_xattr() to prevent problems
with 'No Match' when relative path is used.

Fixes:
    # restorecon_xattr -v tmp
    ...
    tmp Digest: f9cd2da7141068bd2c08bc02fa471db63ac7d44c No Match

    # restorecon_xattr -v `pwd`/tmp
    ...
    /root/tmp Digest: f9cd2da7141068bd2c08bc02fa471db63ac7d44c Match

Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---
 policycoreutils/setfiles/restorecon_xattr.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/policycoreutils/setfiles/restorecon_xattr.c b/policycoreutils/setfiles/restorecon_xattr.c
index 59b1f748b8c5..56f6f9d0e043 100644
--- a/policycoreutils/setfiles/restorecon_xattr.c
+++ b/policycoreutils/setfiles/restorecon_xattr.c
@@ -38,7 +38,7 @@ int main(int argc, char **argv)
 	unsigned int xattr_flags = 0, delete_digest = 0, recurse = 0;
 	unsigned int delete_all_digests = 0, ignore_mounts = 0;
 	bool display_digest = false;
-	char *sha1_buf, **specfiles, *fc_file = NULL;
+	char *sha1_buf, **specfiles, *fc_file = NULL, *pathname = NULL;
 	unsigned char *fc_digest = NULL;
 	size_t i, fc_digest_len = 0, num_specfiles;
 
@@ -163,7 +163,16 @@ int main(int argc, char **argv)
 	xattr_flags = delete_digest | delete_all_digests |
 		      ignore_mounts | recurse;
 
-	if (selinux_restorecon_xattr(argv[optind], xattr_flags, &xattr_list)) {
+	pathname = realpath(argv[optind], NULL);
+	if (!pathname) {
+		fprintf(stderr,
+			    "restorecon_xattr: realpath(%s) failed: %s\n",
+			    argv[optind], strerror(errno));
+		rc = -1;
+		goto out;
+	}
+
+	if (selinux_restorecon_xattr(pathname, xattr_flags, &xattr_list)) {
 		fprintf(stderr,
 			"Error selinux_restorecon_xattr: %s\n",
 			strerror(errno));
@@ -215,6 +224,7 @@ int main(int argc, char **argv)
 
 	rc = 0;
 out:
+	free(pathname);
 	selabel_close(hnd);
 	restore_finish();
 	return rc;
-- 
2.30.1

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

[Nicolas Iooss]

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

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

[Petr Lautrbach]

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!