* [PATCH 1/2] libselinux: fix segfault in add_xattr_entry()
@ 2021-02-16 14:14 Petr Lautrbach
2021-02-16 14:14 ` [PATCH 2/2] policycoreutils: Resolve path in restorecon_xattr Petr Lautrbach
2021-02-21 19:01 ` [PATCH 1/2] libselinux: fix segfault in add_xattr_entry() Nicolas Iooss
0 siblings, 2 replies; 4+ messages in thread
From: Petr Lautrbach @ 2021-02-16 14:14 UTC (permalink / raw)
To: selinux; +Cc: 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
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 2/2] policycoreutils: Resolve path in restorecon_xattr
2021-02-16 14:14 [PATCH 1/2] libselinux: fix segfault in add_xattr_entry() Petr Lautrbach
@ 2021-02-16 14:14 ` Petr Lautrbach
2021-02-21 19:01 ` [PATCH 1/2] libselinux: fix segfault in add_xattr_entry() Nicolas Iooss
1 sibling, 0 replies; 4+ messages in thread
From: Petr Lautrbach @ 2021-02-16 14:14 UTC (permalink / raw)
To: selinux; +Cc: 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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] libselinux: fix segfault in add_xattr_entry()
2021-02-16 14:14 [PATCH 1/2] libselinux: fix segfault in add_xattr_entry() Petr Lautrbach
2021-02-16 14:14 ` [PATCH 2/2] policycoreutils: Resolve path in restorecon_xattr Petr Lautrbach
@ 2021-02-21 19:01 ` Nicolas Iooss
2021-02-24 10:58 ` Petr Lautrbach
1 sibling, 1 reply; 4+ messages in thread
From: Nicolas Iooss @ 2021-02-21 19:01 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: SElinux list
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] libselinux: fix segfault in add_xattr_entry()
2021-02-21 19:01 ` [PATCH 1/2] libselinux: fix segfault in add_xattr_entry() Nicolas Iooss
@ 2021-02-24 10:58 ` Petr Lautrbach
0 siblings, 0 replies; 4+ messages in thread
From: Petr Lautrbach @ 2021-02-24 10:58 UTC (permalink / raw)
To: SElinux list; +Cc: Nicolas Iooss
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!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-02-24 11:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-16 14:14 [PATCH 1/2] libselinux: fix segfault in add_xattr_entry() Petr Lautrbach
2021-02-16 14:14 ` [PATCH 2/2] policycoreutils: Resolve path in restorecon_xattr Petr Lautrbach
2021-02-21 19:01 ` [PATCH 1/2] libselinux: fix segfault in add_xattr_entry() Nicolas Iooss
2021-02-24 10:58 ` Petr Lautrbach
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.