vfs_getxattr_alloc() problem

DISCUSSION Roberto Sassu 2 participants 6 messages Apr 21, 2020 – Apr 28, 2020 Keys-trusted

lore

Share & tools

lore

Roberto Sassu

author

2020-04-21 10:58 UTC #0 lore raw

Hi Mimi

I found a problem in the calculation of the EVM digest.

If an xattr is in the security domain, vfs_getxattr() calls xattr_getsecurity(), which is implemented by LSMs. vfs_getxattr_alloc() instead calls directly the filesystem function to read xattrs.

The problem arises for example when you have a file with a portable signature on the correct SELinux label (with \0) and you set security.selinux manually:

setfattr -n security.selinux -v "system_u:object_r:bin_t:s0" cat

Although the length passed is 26 bytes (without \0), you get:

# attr -l cat
Attribute "selinux" has a 27 byte value for cat

which includes \0.

From user space, evmctl does not complain (the signature is ok) because it calculates the EVM digest with \0, but EVM verification fails (because it calculates the digest without \0).

Should this problem be fixed?

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Mimi Zohar

maintainer

2020-04-23 23:51 UTC #1 lore raw

[Cc'ing Matthew]

Hi Roberto,

Quoted 19 lines · Roberto Sassu

Hi Mimi
I found a problem in the calculation of the EVM digest.
If an xattr is in the security domain, vfs_getxattr() calls xattr_getsecurity(), which is implemented by LSMs. vfs_getxattr_alloc() instead calls directly the filesystem function to read xattrs.
The problem arises for example when you have a file with a portable signature on the correct SELinux label (with \0) and you set security.selinux manually:
setfattr -n security.selinux -v "system_u:object_r:bin_t:s0" cat
Although the length passed is 26 bytes (without \0), you get:
# attr -l cat
Attribute "selinux" has a 27 byte value for cat

Quoted 7 lines

which includes \0.
From user space, evmctl does not complain (the signature is ok) because it calculates the EVM digest with \0, but EVM verification fails (because it calculates the digest without \0).
Should this problem be fixed?

I don't seem to be having any problems verifying the EVM immutable & portable signatures. To test, I've copied a properly labeled file twice, once with the "--preserve=xattr" and once without it. I signed the properly labeled file with the EVM immutable & portable signature. On the other file, I first set the selinux label before signing it. If there was a problem manually writing the SELinux label, the security.evm labels would be different, which they aren't.

Matthew, are you able to reproduce Roberto's problem?

Mimi

Roberto Sassu

author

2020-04-24 14:32 UTC #2 lore raw

Quoted 42 lines

-----Original Message-----
From: Mimi Zohar [mailto:[email protected]]
Sent: Friday, April 24, 2020 1:52 AM
To: Roberto Sassu <[email protected]>; Matthew Garrett <[email protected]>
Cc: [email protected]; Silviu Vlasceanu <[email protected]>
Subject: Re: vfs_getxattr_alloc() problem
[Cc'ing Matthew]
Hi Roberto,
Quoted 10 lines · Roberto Sassu
Hi Mimi
I found a problem in the calculation of the EVM digest.
If an xattr is in the security domain, vfs_getxattr() calls xattr_getsecurity(), which is implemented by LSMs. vfs_getxattr_alloc() instead calls directly the filesystem function to read xattrs.
The problem arises for example when you have a file with a portable signature on the correct SELinux label (with \0) and you set
security.selinux
Quoted 9 lines
manually:
setfattr -n security.selinux -v "system_u:object_r:bin_t:s0" cat
Although the length passed is 26 bytes (without \0), you get:
# attr -l cat
Attribute "selinux" has a 27 byte value for cat
Quoted 7 lines
which includes \0.
From user space, evmctl does not complain (the signature is ok) because it calculates the EVM digest with \0, but EVM verification fails (because it calculates the digest without \0).
Should this problem be fixed?

Quoted 7 lines

I don't seem to be having any problems verifying the EVM immutable & portable signatures. To test, I've copied a properly labeled file twice, once with the "--preserve=xattr" and once without it. I signed the properly labeled file with the EVM immutable & portable signature. On the other file, I first set the selinux label before signing it. If there was a problem manually writing the SELinux label, the security.evm labels would be different, which they aren't.

[root@vm demo]# ls -lZ /bin/cat
-rwxr-xr-x. 1 root root system_u:object_r:bin_t:s0 85520 Apr 24 16:20 /bin/cat [root@vm demo]# evmctl sign -o -a sha256 --imahash --key $PWD/signing_key.pem /bin/cat -v -v hash(sha256): 0404d3d78d8249317ed50056ec7d04da382488f36a6127f4e9161792d97f13e10bc6
name: security.selinux, size: 27 73797374656d5f753a6f626a6563745f723a62696e5f743a733000
no xattr: security.SMACK64
no xattr: security.apparmor
name: security.ima, size: 34 0404d3d78d8249317ed50056ec7d04da382488f36a6127f4e9161792d97f13e10bc6
no xattr: security.capability
calc_evm_hash:532 hmac_misc (24): 0000000000000000000000000000000000000000ed810000 hash(sha256): 331e36ce1b32374a22e12df28b58d79536c0ee97ba01451bd60343191c073b55 calc_keyid_v2:735 keyid: aecec286
keyid: aecec286
evm/ima signature: 520 bytes
...
[root@vm demo]# cat
^C
[root@vm demo]# setfattr -n security.selinux -v "system_u:object_r:bin_t:s0" /bin/cat [root@vm demo]# evmctl verify -o -a sha256 --imahash /bin/cat -v -v calc_keyid_v2:735 keyid: aecec286
keyid: aecec286
key 1: aecec286 /etc/keys/x509_evm.der
name: security.selinux, size: 27 73797374656d5f753a6f626a6563745f723a62696e5f743a733000
no xattr: security.SMACK64
no xattr: security.apparmor
name: security.ima, size: 34 0404d3d78d8249317ed50056ec7d04da382488f36a6127f4e9161792d97f13e10bc6
no xattr: security.capability
calc_evm_hash:532 hmac_misc (24): 0000000000000000000000000000000000000000ed810000 hash(sha256): 331e36ce1b32374a22e12df28b58d79536c0ee97ba01451bd60343191c073b55 /bin/cat: verification is OK
[root@vm demo]# cat
-bash: /usr/bin/cat: Permission denied
[root@vm demo]#

It fails because the actual xattr in the filesystem is:

name: security.selinux, size: 26 73797374656d5f753a6f626a6563745f723a62696e5f743a7330

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Mimi Zohar

maintainer

2020-04-27 18:55 UTC #3 lore raw

Quoted 51 lines · Roberto Sassu

Quoted 31 lines
Hi Roberto,
Quoted 10 lines · Roberto Sassu
Hi Mimi
I found a problem in the calculation of the EVM digest.
If an xattr is in the security domain, vfs_getxattr() calls xattr_getsecurity(), which is implemented by LSMs. vfs_getxattr_alloc() instead calls directly the filesystem function to read xattrs.
The problem arises for example when you have a file with a portable signature on the correct SELinux label (with \0) and you set
security.selinux
Quoted 9 lines
manually:
setfattr -n security.selinux -v "system_u:object_r:bin_t:s0" cat
Although the length passed is 26 bytes (without \0), you get:
# attr -l cat
Attribute "selinux" has a 27 byte value for cat
Quoted 7 lines
which includes \0.
From user space, evmctl does not complain (the signature is ok) because it calculates the EVM digest with \0, but EVM verification fails (because it calculates the digest without \0).
Should this problem be fixed?
Quoted 7 lines
I don't seem to be having any problems verifying the EVM immutable & portable signatures. To test, I've copied a properly labeled file twice, once with the "--preserve=xattr" and once without it. I signed the properly labeled file with the EVM immutable & portable signature. On the other file, I first set the selinux label before signing it. If there was a problem manually writing the SELinux label, the security.evm labels would be different, which they aren't.
[root@vm demo]# ls -lZ /bin/cat
-rwxr-xr-x. 1 root root system_u:object_r:bin_t:s0 85520 Apr 24 16:20 /bin/cat [root@vm demo]# evmctl sign -o -a sha256 --imahash --key $PWD/signing_key.pem /bin/cat -v -v hash(sha256): 0404d3d78d8249317ed50056ec7d04da382488f36a6127f4e9161792d97f13e10bc6
name: security.selinux, size: 27 73797374656d5f753a6f626a6563745f723a62696e5f743a733000
no xattr: security.SMACK64
no xattr: security.apparmor
name: security.ima, size: 34 0404d3d78d8249317ed50056ec7d04da382488f36a6127f4e9161792d97f13e10bc6
no xattr: security.capability
calc_evm_hash:532 hmac_misc (24): 0000000000000000000000000000000000000000ed810000

Quoted 8 lines

hash(sha256): 331e36ce1b32374a22e12df28b58d79536c0ee97ba01451bd60343191c073b55 calc_keyid_v2:735 keyid: aecec286
keyid: aecec286
evm/ima signature: 520 bytes
...
[root@vm demo]# cat
^C
[root@vm demo]# setfattr -n security.selinux -v "system_u:object_r:bin_t:s0" /bin/cat

In the past, when I looked at writing the same SELinux label, there was some performance improvement that only updated the label if the label actually changed. Unless things have changed since, I don't think the same selinux label is rewritten.

Quoted 18 lines

[root@vm demo]# evmctl verify -o -a sha256 --imahash /bin/cat -v -v calc_keyid_v2:735 keyid: aecec286
keyid: aecec286
key 1: aecec286 /etc/keys/x509_evm.der
name: security.selinux, size: 27 73797374656d5f753a6f626a6563745f723a62696e5f743a733000
no xattr: security.SMACK64
no xattr: security.apparmor
name: security.ima, size: 34 0404d3d78d8249317ed50056ec7d04da382488f36a6127f4e9161792d97f13e10bc6 no xattr: security.capability
calc_evm_hash:532 hmac_misc (24): 0000000000000000000000000000000000000000ed810000 hash(sha256): 331e36ce1b32374a22e12df28b58d79536c0ee97ba01451bd60343191c073b55 /bin/cat: verification is OK
[root@vm demo]# cat
-bash: /usr/bin/cat: Permission denied
[root@vm demo]#

Quoted 4 lines

It fails because the actual xattr in the filesystem is:
name: security.selinux, size: 26 73797374656d5f753a6f626a6563745f723a62696e5f743a7330

Looking at security/selinux/hooks.c: I'm seeing a comment selinux_inode_setxattr() that says:

/* We strip a nul only if it is at the end, otherwise the
* context contains a nul and we should audit that */

Mimi

Mimi Zohar

maintainer

2020-04-27 19:20 UTC #4 lore raw

Quoted 86 lines · Mimi Zohar

Quoted 51 lines · Roberto Sassu
Quoted 31 lines
Hi Roberto,
Quoted 10 lines · Roberto Sassu
Hi Mimi
I found a problem in the calculation of the EVM digest.
If an xattr is in the security domain, vfs_getxattr() calls xattr_getsecurity(), which is implemented by LSMs. vfs_getxattr_alloc() instead calls directly the filesystem function to read xattrs.
The problem arises for example when you have a file with a portable signature on the correct SELinux label (with \0) and you set
security.selinux
Quoted 9 lines
manually:
setfattr -n security.selinux -v "system_u:object_r:bin_t:s0" cat
Although the length passed is 26 bytes (without \0), you get:
# attr -l cat
Attribute "selinux" has a 27 byte value for cat
Quoted 7 lines
which includes \0.
From user space, evmctl does not complain (the signature is ok) because it calculates the EVM digest with \0, but EVM verification fails (because it calculates the digest without \0).
Should this problem be fixed?
Quoted 7 lines
I don't seem to be having any problems verifying the EVM immutable & portable signatures. To test, I've copied a properly labeled file twice, once with the "--preserve=xattr" and once without it. I signed the properly labeled file with the EVM immutable & portable signature. On the other file, I first set the selinux label before signing it. If there was a problem manually writing the SELinux label, the security.evm labels would be different, which they aren't.
[root@vm demo]# ls -lZ /bin/cat
-rwxr-xr-x. 1 root root system_u:object_r:bin_t:s0 85520 Apr 24 16:20 /bin/cat [root@vm demo]# evmctl sign -o -a sha256 --imahash --key $PWD/signing_key.pem /bin/cat -v -v hash(sha256): 0404d3d78d8249317ed50056ec7d04da382488f36a6127f4e9161792d97f13e10bc6
name: security.selinux, size: 27 73797374656d5f753a6f626a6563745f723a62696e5f743a733000
no xattr: security.SMACK64
no xattr: security.apparmor
name: security.ima, size: 34 0404d3d78d8249317ed50056ec7d04da382488f36a6127f4e9161792d97f13e10bc6
no xattr: security.capability
calc_evm_hash:532 hmac_misc (24): 0000000000000000000000000000000000000000ed810000
Quoted 8 lines
hash(sha256): 331e36ce1b32374a22e12df28b58d79536c0ee97ba01451bd60343191c073b55 calc_keyid_v2:735 keyid: aecec286
keyid: aecec286
evm/ima signature: 520 bytes
...
[root@vm demo]# cat
^C
[root@vm demo]# setfattr -n security.selinux -v "system_u:object_r:bin_t:s0" /bin/cat
In the past, when I looked at writing the same SELinux label, there was some performance improvement that only updated the label if the label actually changed. Unless things have changed since, I don't think the same selinux label is rewritten.
Quoted 20 lines
[root@vm demo]# evmctl verify -o -a sha256 --imahash /bin/cat -v -v calc_keyid_v2:735 keyid: aecec286
keyid: aecec286
key 1: aecec286 /etc/keys/x509_evm.der
name: security.selinux, size: 27 73797374656d5f753a6f626a6563745f723a62696e5f743a733000
no xattr: security.SMACK64
no xattr: security.apparmor
name: security.ima, size: 34 0404d3d78d8249317ed50056ec7d04da382488f36a6127f4e9161792d97f13e10bc6 no xattr: security.capability
calc_evm_hash:532 hmac_misc (24): 0000000000000000000000000000000000000000ed810000 hash(sha256): 331e36ce1b32374a22e12df28b58d79536c0ee97ba01451bd60343191c073b55 /bin/cat: verification is OK
[root@vm demo]# cat
-bash: /usr/bin/cat: Permission denied
[root@vm demo]#
It fails because the actual xattr in the filesystem is:

Quoted 8 lines

Quoted 2 lines
name: security.selinux, size: 26 73797374656d5f753a6f626a6563745f723a62696e5f743a7330
Looking at security/selinux/hooks.c: I'm seeing a comment selinux_inode_setxattr() that says:
/* We strip a nul only if it is at the end, otherwise the
* context contains a nul and we should audit that */

strace shows setxattr is writing 26 bytes:

setxattr("/bin/cat-test", "security.selinux",
"system_u:object_r:bin_t:s0", 26, 0) = 0

Mimi

Roberto Sassu

author

2020-04-28 07:02 UTC #5 lore raw

Quoted 120 lines

-----Original Message-----
From: Mimi Zohar [mailto:[email protected]]
Sent: Monday, April 27, 2020 9:20 PM
To: Roberto Sassu <[email protected]>; Matthew Garrett <[email protected]>
Cc: [email protected]; Silviu Vlasceanu <[email protected]>
Subject: Re: vfs_getxattr_alloc() problem
Quoted 9 lines · Mimi Zohar
Quoted 8 lines · Roberto Sassu
Quoted 8 lines
Hi Roberto,
Quoted 5 lines · Roberto Sassu
Hi Mimi
I found a problem in the calculation of the EVM digest.
If an xattr is in the security domain, vfs_getxattr() calls
xattr_getsecurity(),
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
which is implemented by LSMs. vfs_getxattr_alloc() instead calls
directly
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
the filesystem function to read xattrs.
The problem arises for example when you have a file with a portable signature on the correct SELinux label (with \0) and you set
security.selinux
Quoted 4 lines
manually:
setfattr -n security.selinux -v "system_u:object_r:bin_t:s0" cat
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Although the length passed is 26 bytes (without \0), you get:
# attr -l cat
Attribute "selinux" has a 27 byte value for cat
which includes \0.
From user space, evmctl does not complain (the signature is ok)
because
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
it calculates the EVM digest with \0, but EVM verification fails
(because it
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
calculates the digest without \0).
Should this problem be fixed?
I don't seem to be having any problems verifying the EVM immutable
&
Quoted 9 lines
Quoted 9 lines
Quoted 6 lines
portable signatures. To test, I've copied a properly labeled file twice, once with the "--preserve=xattr" and once without it. I signed the properly labeled file with the EVM immutable & portable signature. On the other file, I first set the selinux label before signing it. If there was a problem manually writing the SELinux label, the security.evm labels would be different, which they aren't.
[root@vm demo]# ls -lZ /bin/cat
-rwxr-xr-x. 1 root root system_u:object_r:bin_t:s0 85520 Apr 24 16:20
/bin/cat
Quoted 1 line
Quoted 1 line
[root@vm demo]# evmctl sign -o -a sha256 --imahash --key
$PWD/signing_key.pem /bin/cat -v -v
Quoted 1 line
Quoted 1 line
hash(sha256):
0404d3d78d8249317ed50056ec7d04da382488f36a6127f4e9161792d97f13e10 bc6
Quoted 6 lines
Quoted 6 lines
name: security.selinux, size: 27 73797374656d5f753a6f626a6563745f723a62696e5f743a733000 no xattr: security.SMACK64
no xattr: security.apparmor
name: security.ima, size: 34
0404d3d78d8249317ed50056ec7d04da382488f36a6127f4e9161792d97f13e10 bc6
Quoted 2 lines
Quoted 2 lines
no xattr: security.capability
calc_evm_hash:532 hmac_misc (24):
0000000000000000000000000000000000000000ed810000
Quoted 1 line
Quoted 1 line
hash(sha256):
331e36ce1b32374a22e12df28b58d79536c0ee97ba01451bd60343191c073b55
Quoted 7 lines
Quoted 7 lines
calc_keyid_v2:735 keyid: aecec286
keyid: aecec286
evm/ima signature: 520 bytes
...
[root@vm demo]# cat
^C
[root@vm demo]# setfattr -n security.selinux -v
"system_u:object_r:bin_t:s0" /bin/cat
Quoted 16 lines
In the past, when I looked at writing the same SELinux label, there was some performance improvement that only updated the label if the label actually changed. Unless things have changed since, I don't think the same selinux label is rewritten.
Quoted 10 lines
[root@vm demo]# evmctl verify -o -a sha256 --imahash /bin/cat -v -v calc_keyid_v2:735 keyid: aecec286
keyid: aecec286
key 1: aecec286 /etc/keys/x509_evm.der
name: security.selinux, size: 27 73797374656d5f753a6f626a6563745f723a62696e5f743a733000 no xattr: security.SMACK64
no xattr: security.apparmor
name: security.ima, size: 34
0404d3d78d8249317ed50056ec7d04da382488f36a6127f4e9161792d97f13e10 bc6
Quoted 2 lines
Quoted 2 lines
no xattr: security.capability
calc_evm_hash:532 hmac_misc (24):
0000000000000000000000000000000000000000ed810000
Quoted 1 line
Quoted 1 line
hash(sha256):
331e36ce1b32374a22e12df28b58d79536c0ee97ba01451bd60343191c073b55
Quoted 13 lines
Quoted 9 lines
/bin/cat: verification is OK
[root@vm demo]# cat
-bash: /usr/bin/cat: Permission denied
[root@vm demo]#
It fails because the actual xattr in the filesystem is:
name: security.selinux, size: 26 73797374656d5f753a6f626a6563745f723a62696e5f743a7330
Looking at security/selinux/hooks.c: I'm seeing a comment selinux_inode_setxattr() that says:

Quoted 7 lines

Quoted 2 lines
/* We strip a nul only if it is at the end, otherwise the
* context contains a nul and we should audit that */
strace shows setxattr is writing 26 bytes:
setxattr("/bin/cat-test", "security.selinux",
"system_u:object_r:bin_t:s0", 26, 0) = 0

Yes, but SELinux fixes the label and adds \0:

static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
const void *value, size_t size, int flags)
{
[...]
rc = security_context_to_sid(&selinux_state, value, size, &newsid,
GFP_KERNEL);

static int security_context_to_sid_core(struct selinux_state *state,
const char *scontext, u32 scontext_len, u32 *sid, u32 def_sid, gfp_t gfp_flags, int force)
{
[...]

        /* Copy the string to allow changes and ensure a NUL terminator */
        scontext2 = kmemdup_nul(scontext, scontext_len, gfp_flags);

When evmctl reads from user space, the kernel does:

ssize_t
vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size) {
[...]

        if (!strncmp(name, XATTR_SECURITY_PREFIX,
                                XATTR_SECURITY_PREFIX_LEN)) {
                const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
                int ret = xattr_getsecurity(inode, suffix, value, size);

static int selinux_inode_getsecurity(struct inode *inode, const char *name, void **buffer, bool alloc) {
[...]

                error = security_sid_to_context(&selinux_state, isec->sid,
                                                &context, &size);

evmctl gets the string of 27 bytes with the \0, which is not the content stored in the filesystem but the SELinux translation of the sid.

Instead EVM takes a different path:

ssize_t
vfs_getxattr_alloc(struct dentry *dentry, const char *name, char **xattr_value,
size_t xattr_size, gfp_t flags)
{
[...]

        error = handler->get(handler, dentry, inode, name, NULL, 0);

so, it gets 26 bytes, not 27. This is why permission is denied.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli