integrity fixes

PATCH Superseded Dmitry Kasatkin 5 participants 8 messages Oct 23, 2014 – Oct 24, 2014

Subsystems (3)

Security (LSM) Integrity measurement architecture (IMA) Extended Verification Module (EVM)

lore

Share & tools

lore

Dmitry Kasatkin

author

2014-10-23 13:49 UTC #0 lore raw

cover integrity fixes

Hi,

Here is couple of patches to fix bugs reported by Jan Kara to prevent kernel oopses when setxattr() is called without xattr values.

- Dmitry

Dmitry Kasatkin (2):
ima: check xattr value length in ima_inode_setxattr()
evm: check xattr value length in evm_inode_setxattr()

 security/integrity/evm/evm_main.c     | 9 ++++++---
 security/integrity/ima/ima_appraise.c | 2 ++
 2 files changed, 8 insertions(+), 3 deletions(-)

Signature · 1 line

1.9.1

Dmitry Kasatkin

author

2014-10-23 13:49 UTC #1 lore raw

PATCH 1/2 ima: check xattr value length in ima_inode_setxattr()

ima_inode_setxattr() can be called with no value. Function does not check the length so that following command can be used to produce kernel oops: setfattr -n security.ima FOO. This patch fixes it.

[  261.562522] BUG: unable to handle kernel NULL pointer dereference at           (null)
[  261.564109] IP: [<ffffffff812af272>] ima_inode_setxattr+0x3e/0x5a
[  261.564109] PGD 3112f067 PUD 42965067 PMD 0
[  261.564109] Oops: 0000 [#1] SMP
[  261.564109] Modules linked in: bridge stp llc evdev serio_raw i2c_piix4 button fuse
[  261.564109] CPU: 0 PID: 3299 Comm: setxattr Not tainted 3.16.0-kds+ #2924
[  261.564109] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[  261.564109] task: ffff8800428c2430 ti: ffff880042be0000 task.ti: ffff880042be0000
[  261.564109] RIP: 0010:[<ffffffff812af272>]  [<ffffffff812af272>] ima_inode_setxattr+0x3e/0x5a
[  261.564109] RSP: 0018:ffff880042be3d50  EFLAGS: 00010246
[  261.564109] RAX: 0000000000000001 RBX: 0000000000000000 RCX: 0000000000000015
[  261.564109] RDX: 0000001500000000 RSI: 0000000000000000 RDI: ffff8800375cc600
[  261.564109] RBP: ffff880042be3d68 R08: 0000000000000000 R09: 00000000004d6256
[  261.564109] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88002149ba00
[  261.564109] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[  261.564109] FS:  00007f6c1e219740(0000) GS:ffff88005da00000(0000) knlGS:0000000000000000
[  261.564109] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  261.564109] CR2: 0000000000000000 CR3: 000000003b35a000 CR4: 00000000000006f0
[  261.564109] Stack:
[  261.564109]  ffff88002149ba00 ffff880042be3df8 0000000000000000 ffff880042be3d98
[  261.564109]  ffffffff812a101b ffff88002149ba00 ffff880042be3df8 0000000000000000
[  261.564109]  0000000000000000 ffff880042be3de0 ffffffff8116d08a ffff880042be3dc8
[  261.564109] Call Trace:
[  261.564109]  [<ffffffff812a101b>] security_inode_setxattr+0x48/0x6a
[  261.564109]  [<ffffffff8116d08a>] vfs_setxattr+0x6b/0x9f
[  261.564109]  [<ffffffff8116d1e0>] setxattr+0x122/0x16c
[  261.564109]  [<ffffffff811687e8>] ? mnt_want_write+0x21/0x45
[  261.564109]  [<ffffffff8114d011>] ? __sb_start_write+0x10f/0x143
[  261.564109]  [<ffffffff811687e8>] ? mnt_want_write+0x21/0x45
[  261.564109]  [<ffffffff811687c0>] ? __mnt_want_write+0x48/0x4f
[  261.564109]  [<ffffffff8116d3e6>] SyS_setxattr+0x6e/0xb0
[  261.564109]  [<ffffffff81529da9>] system_call_fastpath+0x16/0x1b
[  261.564109] Code: 48 89 f7 48 c7 c6 58 36 81 81 53 31 db e8 73 27 04 00 85 c0 75 28 bf 15 00 00 00 e8 8a a5 d9 ff 84 c0 75 05 83 cb ff eb 15 31 f6 <41> 80 7d 00 03 49 8b 7c 24 68 40 0f 94 c6 e8 e1 f9 ff ff 89 d8
[  261.564109] RIP  [<ffffffff812af272>] ima_inode_setxattr+0x3e/0x5a
[  261.564109]  RSP <ffff880042be3d50>
[  261.564109] CR2: 0000000000000000
[  261.599998] ---[ end trace 39a89a3fc267e652 ]---

Reported-by Jan Kara <[email protected]>

Signed-off-by Dmitry Kasatkin <[email protected]>

---
 security/integrity/ima/ima_appraise.c | 2 ++
 1 file changed, 2 insertions(+)

security/integrity/ima/ima_appraise.c +2 -0

index 5b845af..f07aacd 100644
@@ -378,6 +378,8 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 	result = ima_protect_xattr(dentry, xattr_name, xattr_value,
 				   xattr_value_len);
 	if (result == 1) {
+		if (!xattr_value_len)
+			return -EINVAL;
 		ima_reset_appraise_flags(dentry->d_inode,
 			 (xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0);
 		result = 0;

Signature · 1 line

1.9.1

Jan Kara

2014-10-23 15:40 UTC #2 lore raw

Re: [PATCH 1/2] ima: check xattr value length in ima_inode_setxattr()

Quoted 4 lines · Dmitry Kasatkin

ima_inode_setxattr() can be called with no value. Function does not check the length so that following command can be used to produce kernel oops: setfattr -n security.ima FOO. This patch fixes it.

Quoted 12 lines

Reported-by Jan Kara <[email protected]>
Signed-off-by Dmitry Kasatkin <[email protected]>
---
 security/integrity/ima/ima_appraise.c | 2 ++
 1 file changed, 2 insertions(+)
security/integrity/ima/ima_appraise.c +0 -0
index 5b845af..f07aacd 100644

security/integrity/ima/ima_appraise.c int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name, · line 382

Quoted 5 lines

security/integrity/ima/ima_appraise.c +2 -0

@@ -378,6 +378,8 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 	result = ima_protect_xattr(dentry, xattr_name, xattr_value,
 				   xattr_value_len);
 	if (result == 1) {
+		if (!xattr_value_len)
+			return -EINVAL;

Wouldn't it be safer to return EINVAL whenever xattr_value_len !=
sizeof(evm_ima_xattr_data)?

Honza

Quoted 6 lines

security/integrity/ima/ima_appraise.c +0 -0

 		ima_reset_appraise_flags(dentry->d_inode,
 			 (xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0);
 		result = 0;

Signature · 1 line

1.9.1

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Dmitry Kasatkin

maintainer

2014-10-23 15:59 UTC #3 lore raw

Re: [PATCH 1/2] ima: check xattr value length in ima_inode_setxattr()

Quoted 13 lines · Jan Kara

Quoted 4 lines · Dmitry Kasatkin
ima_inode_setxattr() can be called with no value. Function does not check the length so that following command can be used to produce kernel oops: setfattr -n security.ima FOO. This patch fixes it.
..
Quoted 7 lines
Reported-by Jan Kara <[email protected]>
Signed-off-by Dmitry Kasatkin <[email protected]>
---
 security/integrity/ima/ima_appraise.c | 2 ++
 1 file changed, 2 insertions(+)

Quoted 12 lines

security/integrity/ima/ima_appraise.c int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name, · line 382
Quoted 10 lines
security/integrity/ima/ima_appraise.c +2 -0
index 5b845af..f07aacd 100644
@@ -378,6 +378,8 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
      result = ima_protect_xattr(dentry, xattr_name, xattr_value,
                                 xattr_value_len);
      if (result == 1) {
+             if (!xattr_value_len)
+                     return -EINVAL;
Wouldn't it be safer to return EINVAL whenever xattr_value_len !=
sizeof(evm_ima_xattr_data)?

In this function we only use first byte to identify attribute type. sizeof(evm_ima_xattr_data) is SHA1_DIGEST_SIZE + 1.
But IMA may use any other algorithm where digest size is different.

- Dmitry

Quoted 11 lines

Honza

Quoted 6 lines

security/integrity/ima/ima_appraise.c +0 -1

              ima_reset_appraise_flags(dentry->d_inode,
                       (xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0);
              result = 0;
--

1.9.1

--
Jan Kara <[email protected]>
SUSE Labs, CR

Signature · 2 lines

Thanks,
Dmitry

Jan Kara

2014-10-23 16:04 UTC #4 lore raw

Re: [PATCH 1/2] ima: check xattr value length in ima_inode_setxattr()

Quoted 27 lines · Dmitry Kasatkin

Quoted 13 lines · Jan Kara
Quoted 4 lines · Dmitry Kasatkin
ima_inode_setxattr() can be called with no value. Function does not check the length so that following command can be used to produce kernel oops: setfattr -n security.ima FOO. This patch fixes it.
..
Quoted 7 lines
Reported-by Jan Kara <[email protected]>
Signed-off-by Dmitry Kasatkin <[email protected]>
---
 security/integrity/ima/ima_appraise.c | 2 ++
 1 file changed, 2 insertions(+)
Quoted 12 lines
security/integrity/ima/ima_appraise.c int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name, · line 382
Quoted 10 lines
security/integrity/ima/ima_appraise.c +2 -0
index 5b845af..f07aacd 100644
@@ -378,6 +378,8 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
      result = ima_protect_xattr(dentry, xattr_name, xattr_value,
                                 xattr_value_len);
      if (result == 1) {
+             if (!xattr_value_len)
+                     return -EINVAL;
Wouldn't it be safer to return EINVAL whenever xattr_value_len !=
sizeof(evm_ima_xattr_data)?

Quoted 3 lines

In this function we only use first byte to identify attribute type. sizeof(evm_ima_xattr_data) is SHA1_DIGEST_SIZE + 1.
But IMA may use any other algorithm where digest size is different.

I see. Thanks for explanation.

Honza

Quoted 15 lines

Quoted 9 lines
Quoted 6 lines
security/integrity/ima/ima_appraise.c +0 -1
              ima_reset_appraise_flags(dentry->d_inode,
                       (xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0);
              result = 0;
--
1.9.1
--
Jan Kara <[email protected]>
SUSE Labs, CR
Signature · 2 lines
Thanks,
Dmitry

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

James Morris

maintainer

2014-10-24 02:55 UTC #5 lore raw

Re: [PATCH 1/2] ima: check xattr value length in ima_inode_setxattr()

In reply to Dmitry Kasatkin

Quoted 3 lines · Dmitry Kasatkin

ima_inode_setxattr() can be called with no value. Function does not check the length so that following command can be used to produce kernel oops: setfattr -n security.ima FOO. This patch fixes it.

I'd like to see more review/acks on this before sending it to Linus.

Mimi?

Signature · 2 lines

James Morris
<[email protected]>

Mimi Zohar

2014-10-24 03:52 UTC #6 lore raw

Re: [PATCH 1/2] ima: check xattr value length in ima_inode_setxattr()

Quoted 9 lines · James Morris

Quoted 3 lines · Dmitry Kasatkin
ima_inode_setxattr() can be called with no value. Function does not check the length so that following command can be used to produce kernel oops: setfattr -n security.ima FOO. This patch fixes it.
I'd like to see more review/acks on this before sending it to Linus.
Mimi?

This fixes the oops, but a more complete solution would at least test the first byte, verifying that it is valid. As previously described
http://sourceforge.net/p/linux-ima/mailman/message/32958242/ I think we can do better than this.

Mimi

Quoted 11 lines

Signature · 8 lines

-- 
James Morris
<[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dmitry Kasatkin

author

2014-10-23 13:49 UTC #7 lore raw

PATCH 2/2 evm: check xattr value length in evm_inode_setxattr()

In reply to Dmitry Kasatkin

evm_inode_setxattr() can be called with no value. Function does not check the length so that following command can be used to produce kernel oops: setfattr -n security.evm FOO. This patch fixes it.

[ 1106.396921] BUG: unable to handle kernel NULL pointer dereference at           (null)
[ 1106.398192] IP: [<ffffffff812af7b8>] evm_inode_setxattr+0x2a/0x48
[ 1106.399244] PGD 29048067 PUD 290d7067 PMD 0
[ 1106.399953] Oops: 0000 [#1] SMP
[ 1106.400020] Modules linked in: bridge stp llc evdev serio_raw i2c_piix4 button fuse
[ 1106.400020] CPU: 0 PID: 3635 Comm: setxattr Not tainted 3.16.0-kds+ #2936
[ 1106.400020] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 1106.400020] task: ffff8800291a0000 ti: ffff88002917c000 task.ti: ffff88002917c000
[ 1106.400020] RIP: 0010:[<ffffffff812af7b8>]  [<ffffffff812af7b8>] evm_inode_setxattr+0x2a/0x48
[ 1106.400020] RSP: 0018:ffff88002917fd50  EFLAGS: 00010246
[ 1106.400020] RAX: 0000000000000000 RBX: ffff88002917fdf8 RCX: 0000000000000000
[ 1106.400020] RDX: 0000000000000000 RSI: ffffffff818136d3 RDI: ffff88002917fdf8
[ 1106.400020] RBP: ffff88002917fd68 R08: 0000000000000000 R09: 00000000003ec1df
[ 1106.400020] R10: 0000000000000000 R11: 0000000000000000 R12: ffff8800438a0a00
[ 1106.400020] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[ 1106.400020] FS:  00007f7dfa7d7740(0000) GS:ffff88005da00000(0000) knlGS:0000000000000000
[ 1106.400020] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1106.400020] CR2: 0000000000000000 CR3: 000000003763e000 CR4: 00000000000006f0
[ 1106.400020] Stack:
[ 1106.400020]  ffff8800438a0a00 ffff88002917fdf8 0000000000000000 ffff88002917fd98
[ 1106.400020]  ffffffff812a1030 ffff8800438a0a00 ffff88002917fdf8 0000000000000000
[ 1106.400020]  0000000000000000 ffff88002917fde0 ffffffff8116d08a ffff88002917fdc8
[ 1106.400020] Call Trace:
[ 1106.400020]  [<ffffffff812a1030>] security_inode_setxattr+0x5d/0x6a
[ 1106.400020]  [<ffffffff8116d08a>] vfs_setxattr+0x6b/0x9f
[ 1106.400020]  [<ffffffff8116d1e0>] setxattr+0x122/0x16c
[ 1106.400020]  [<ffffffff811687e8>] ? mnt_want_write+0x21/0x45
[ 1106.400020]  [<ffffffff8114d011>] ? __sb_start_write+0x10f/0x143
[ 1106.400020]  [<ffffffff811687e8>] ? mnt_want_write+0x21/0x45
[ 1106.400020]  [<ffffffff811687c0>] ? __mnt_want_write+0x48/0x4f
[ 1106.400020]  [<ffffffff8116d3e6>] SyS_setxattr+0x6e/0xb0
[ 1106.400020]  [<ffffffff81529da9>] system_call_fastpath+0x16/0x1b
[ 1106.400020] Code: c3 0f 1f 44 00 00 55 48 89 e5 41 55 49 89 d5 41 54 49 89 fc 53 48 89 f3 48 c7 c6 d3 36 81 81 48 89 df e8 18 22 04 00 85 c0 75 07 <41> 80 7d 00 02 74 0d 48 89 de 4c 89 e7 e8 5a fe ff ff eb 03 83
[ 1106.400020] RIP  [<ffffffff812af7b8>] evm_inode_setxattr+0x2a/0x48
[ 1106.400020]  RSP <ffff88002917fd50>
[ 1106.400020] CR2: 0000000000000000
[ 1106.428061] ---[ end trace ae08331628ba3050 ]---

Reported-by Jan Kara <[email protected]>

Signed-off-by Dmitry Kasatkin <[email protected]>

---
 security/integrity/evm/evm_main.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

security/integrity/evm/evm_main.c +6 -3

index b392fe6..5ca72a4 100644
@@ -324,9 +324,12 @@ int evm_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 {
 	const struct evm_ima_xattr_data *xattr_data = xattr_value;
 
-	if ((strcmp(xattr_name, XATTR_NAME_EVM) == 0)
-	    && (xattr_data->type == EVM_XATTR_HMAC))
-		return -EPERM;
+	if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) {
+		if (!xattr_value_len)
+			return -EINVAL;
+		if (xattr_data->type == EVM_XATTR_HMAC)
+			return -EPERM;
+	}
 	return evm_protect_xattr(dentry, xattr_name, xattr_value,
 				 xattr_value_len);
 }

Signature · 1 line

1.9.1