* [PATCH v2 0/2] integrity fixes @ 2014-10-24 7:07 Dmitry Kasatkin 2014-10-24 7:07 ` [PATCH v2 1/2] ima: check xattr value length in ima_inode_setxattr() Dmitry Kasatkin 2014-10-24 7:07 ` [PATCH v2 2/2] evm: check xattr value length in evm_inode_setxattr() Dmitry Kasatkin 0 siblings, 2 replies; 7+ messages in thread From: Dmitry Kasatkin @ 2014-10-24 7:07 UTC (permalink / raw) To: zohar, linux-security-module, linux-ima-devel Cc: linux-kernel, jack, jmorris, dmitry.kasatkin, Dmitry Kasatkin Hi, At first I did not notice Mimi's email with suggestion to allow setting hash only in fix mode. Here is a next set with checking validity of xattr type and allowing setting hash only in permissive modes such as fix and log. - 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 | 11 ++++++++--- security/integrity/ima/ima_appraise.c | 13 +++++++++++-- 2 files changed, 19 insertions(+), 5 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] ima: check xattr value length in ima_inode_setxattr() 2014-10-24 7:07 [PATCH v2 0/2] integrity fixes Dmitry Kasatkin @ 2014-10-24 7:07 ` Dmitry Kasatkin 2014-10-24 14:18 ` Mimi Zohar 2014-10-24 7:07 ` [PATCH v2 2/2] evm: check xattr value length in evm_inode_setxattr() Dmitry Kasatkin 1 sibling, 1 reply; 7+ messages in thread From: Dmitry Kasatkin @ 2014-10-24 7:07 UTC (permalink / raw) To: zohar, linux-security-module, linux-ima-devel Cc: linux-kernel, jack, jmorris, dmitry.kasatkin, 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. Changes in v2: * testing validity of xattr type * allow setting hash only in fix or log mode (Mimi) [ 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 <jack@suse.cz> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> --- security/integrity/ima/ima_appraise.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 9226854..e302cbf 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -378,8 +378,17 @@ 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) { - ima_reset_appraise_flags(dentry->d_inode, - (xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0); + bool digsig; + + if (!xattr_value_len || + (xvalue->type != IMA_XATTR_DIGEST && + xvalue->type != IMA_XATTR_DIGEST_NG && + xvalue->type != EVM_IMA_XATTR_DIGSIG)) + return -EINVAL; + digsig = (xvalue->type == EVM_IMA_XATTR_DIGSIG); + if (!digsig && (ima_appraise & IMA_APPRAISE_ENFORCE)) + return -EPERM; + ima_reset_appraise_flags(dentry->d_inode, digsig); result = 0; } return result; -- 1.9.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] ima: check xattr value length in ima_inode_setxattr() 2014-10-24 7:07 ` [PATCH v2 1/2] ima: check xattr value length in ima_inode_setxattr() Dmitry Kasatkin @ 2014-10-24 14:18 ` Mimi Zohar 2014-10-24 15:00 ` Dmitry Kasatkin 0 siblings, 1 reply; 7+ messages in thread From: Mimi Zohar @ 2014-10-24 14:18 UTC (permalink / raw) To: Dmitry Kasatkin Cc: linux-security-module, linux-ima-devel, linux-kernel, jack, jmorris, dmitry.kasatkin On Fri, 2014-10-24 at 10:07 +0300, Dmitry Kasatkin wrote: > 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. > > Changes in v2: > * testing validity of xattr type > * allow setting hash only in fix or log mode (Mimi) I only mentioned "fix" mode, not "log" mode (explanation below). > > [ 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 <jack@suse.cz> > Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> > --- > security/integrity/ima/ima_appraise.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > index 9226854..e302cbf 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -378,8 +378,17 @@ 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) { > - ima_reset_appraise_flags(dentry->d_inode, > - (xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0); > + bool digsig; > + > + if (!xattr_value_len || > + (xvalue->type != IMA_XATTR_DIGEST && > + xvalue->type != IMA_XATTR_DIGEST_NG && > + xvalue->type != EVM_IMA_XATTR_DIGSIG)) "xvalue->type" is an enumerated type. Testing each possible value seems kind of a brittle method for vetting the value. I suggest testing the existing last value or, better yet, define a last value, so if someone adds or changes the order, nothing breaks. > + return -EINVAL; > + digsig = (xvalue->type == EVM_IMA_XATTR_DIGSIG); > + if (!digsig && (ima_appraise & IMA_APPRAISE_ENFORCE)) > + return -EPERM; According to the new ima_appraise "log" mode, commit "2faa6ef ima: provide 'ima_appraise=log' kernel option", "log" mode permits normal execution without "fixing" anything. Normal execution, here, prevents writing the extended attribute. Mimi > + ima_reset_appraise_flags(dentry->d_inode, digsig); > result = 0; > } > return result; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] ima: check xattr value length in ima_inode_setxattr() 2014-10-24 14:18 ` Mimi Zohar @ 2014-10-24 15:00 ` Dmitry Kasatkin 2014-10-24 15:08 ` Dmitry Kasatkin 0 siblings, 1 reply; 7+ messages in thread From: Dmitry Kasatkin @ 2014-10-24 15:00 UTC (permalink / raw) To: Mimi Zohar Cc: linux-security-module, linux-ima-devel, linux-kernel, jack, jmorris, dmitry.kasatkin On 24/10/14 17:18, Mimi Zohar wrote: > On Fri, 2014-10-24 at 10:07 +0300, Dmitry Kasatkin wrote: >> 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. >> >> Changes in v2: >> * testing validity of xattr type >> * allow setting hash only in fix or log mode (Mimi) > I only mentioned "fix" mode, not "log" mode (explanation below). > We need it in log mode as well (explanation bellow) >> [ 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 <jack@suse.cz> >> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> >> --- >> security/integrity/ima/ima_appraise.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c >> index 9226854..e302cbf 100644 >> --- a/security/integrity/ima/ima_appraise.c >> +++ b/security/integrity/ima/ima_appraise.c >> @@ -378,8 +378,17 @@ 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) { >> - ima_reset_appraise_flags(dentry->d_inode, >> - (xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0); >> + bool digsig; >> + >> + if (!xattr_value_len || >> + (xvalue->type != IMA_XATTR_DIGEST && >> + xvalue->type != IMA_XATTR_DIGEST_NG && >> + xvalue->type != EVM_IMA_XATTR_DIGSIG)) > "xvalue->type" is an enumerated type. Testing each possible value seems > kind of a brittle method for vetting the value. I suggest testing the > existing last value or, better yet, define a last value, so if someone > adds or changes the order, nothing breaks. I was considering to define _LAST value, but we have EVM_XATTR_HMAC in the middle... In fact I was expecting to get some feedback about it, because in reality it is just a sanity check. It does not prevent DoS because it is possible to set correctly formatted but wrong value and force DoS. > >> + return -EINVAL; >> + digsig = (xvalue->type == EVM_IMA_XATTR_DIGSIG); >> + if (!digsig && (ima_appraise & IMA_APPRAISE_ENFORCE)) >> + return -EPERM; > According to the new ima_appraise "log" mode, commit "2faa6ef ima: > provide 'ima_appraise=log' kernel option", "log" mode permits normal > execution without "fixing" anything. Normal execution, here, prevents > writing the extended attribute. 'log' mode is also special mode for system developing and debugging. It is beneficial to be able to 'label' target object with correct value... - Dmitry > Mimi > >> + ima_reset_appraise_flags(dentry->d_inode, digsig); >> result = 0; >> } >> return result; > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] ima: check xattr value length in ima_inode_setxattr() 2014-10-24 15:00 ` Dmitry Kasatkin @ 2014-10-24 15:08 ` Dmitry Kasatkin 2014-10-24 16:00 ` Mimi Zohar 0 siblings, 1 reply; 7+ messages in thread From: Dmitry Kasatkin @ 2014-10-24 15:08 UTC (permalink / raw) To: Mimi Zohar Cc: linux-security-module, linux-ima-devel, linux-kernel, jack, jmorris, dmitry.kasatkin On 24/10/14 18:00, Dmitry Kasatkin wrote: > On 24/10/14 17:18, Mimi Zohar wrote: >> On Fri, 2014-10-24 at 10:07 +0300, Dmitry Kasatkin wrote: >>> 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. >>> >>> Changes in v2: >>> * testing validity of xattr type >>> * allow setting hash only in fix or log mode (Mimi) >> I only mentioned "fix" mode, not "log" mode (explanation below). >> > We need it in log mode as well (explanation bellow) >>> [ 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 <jack@suse.cz> >>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> >>> --- >>> security/integrity/ima/ima_appraise.c | 13 +++++++++++-- >>> 1 file changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c >>> index 9226854..e302cbf 100644 >>> --- a/security/integrity/ima/ima_appraise.c >>> +++ b/security/integrity/ima/ima_appraise.c >>> @@ -378,8 +378,17 @@ 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) { >>> - ima_reset_appraise_flags(dentry->d_inode, >>> - (xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0); >>> + bool digsig; >>> + >>> + if (!xattr_value_len || >>> + (xvalue->type != IMA_XATTR_DIGEST && >>> + xvalue->type != IMA_XATTR_DIGEST_NG && >>> + xvalue->type != EVM_IMA_XATTR_DIGSIG)) >> "xvalue->type" is an enumerated type. Testing each possible value seems >> kind of a brittle method for vetting the value. I suggest testing the >> existing last value or, better yet, define a last value, so if someone >> adds or changes the order, nothing breaks. > I was considering to define _LAST value, but we have EVM_XATTR_HMAC in > the middle... > In fact I was expecting to get some feedback about it, because in > reality it is just a sanity check. > It does not prevent DoS because it is possible to set correctly > formatted but wrong value and force DoS. > Forgot to ask. If possibility to set HMAC type is fine with you I can define _LAST.. Thanks. >>> + return -EINVAL; >>> + digsig = (xvalue->type == EVM_IMA_XATTR_DIGSIG); >>> + if (!digsig && (ima_appraise & IMA_APPRAISE_ENFORCE)) >>> + return -EPERM; >> According to the new ima_appraise "log" mode, commit "2faa6ef ima: >> provide 'ima_appraise=log' kernel option", "log" mode permits normal >> execution without "fixing" anything. Normal execution, here, prevents >> writing the extended attribute. > 'log' mode is also special mode for system developing and debugging. > It is beneficial to be able to 'label' target object with correct value... > > - Dmitry > > >> Mimi >> >>> + ima_reset_appraise_flags(dentry->d_inode, digsig); >>> result = 0; >>> } >>> return result; >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] ima: check xattr value length in ima_inode_setxattr() 2014-10-24 15:08 ` Dmitry Kasatkin @ 2014-10-24 16:00 ` Mimi Zohar 0 siblings, 0 replies; 7+ messages in thread From: Mimi Zohar @ 2014-10-24 16:00 UTC (permalink / raw) To: Dmitry Kasatkin Cc: linux-security-module, linux-ima-devel, linux-kernel, jack, jmorris, dmitry.kasatkin On Fri, 2014-10-24 at 18:08 +0300, Dmitry Kasatkin wrote: > On 24/10/14 18:00, Dmitry Kasatkin wrote: > > On 24/10/14 17:18, Mimi Zohar wrote: > >> On Fri, 2014-10-24 at 10:07 +0300, Dmitry Kasatkin wrote: > >>> 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. > >>> > >>> Changes in v2: > >>> * testing validity of xattr type > >>> * allow setting hash only in fix or log mode (Mimi) > >> I only mentioned "fix" mode, not "log" mode (explanation below). > >> > > We need it in log mode as well (explanation bellow) > >>> [ 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 <jack@suse.cz> > >>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> > >>> --- > >>> security/integrity/ima/ima_appraise.c | 13 +++++++++++-- > >>> 1 file changed, 11 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > >>> index 9226854..e302cbf 100644 > >>> --- a/security/integrity/ima/ima_appraise.c > >>> +++ b/security/integrity/ima/ima_appraise.c > >>> @@ -378,8 +378,17 @@ 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) { > >>> - ima_reset_appraise_flags(dentry->d_inode, > >>> - (xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0); > >>> + bool digsig; > >>> + > >>> + if (!xattr_value_len || > >>> + (xvalue->type != IMA_XATTR_DIGEST && > >>> + xvalue->type != IMA_XATTR_DIGEST_NG && > >>> + xvalue->type != EVM_IMA_XATTR_DIGSIG)) > >> "xvalue->type" is an enumerated type. Testing each possible value seems > >> kind of a brittle method for vetting the value. I suggest testing the > >> existing last value or, better yet, define a last value, so if someone > >> adds or changes the order, nothing breaks. > > I was considering to define _LAST value, but we have EVM_XATTR_HMAC in > > the middle... > > In fact I was expecting to get some feedback about it, because in > > reality it is just a sanity check. > > It does not prevent DoS because it is possible to set correctly > > formatted but wrong value and force DoS. This patch prevents the oops and hash values from being written. Verifying the signature would require access to the public key, which isn't necessarily loaded on the keyring. For now, I think this is fine. Future patches, as described in Dave's LSS 2014 talk, will address this issue. http://kernsec.org/wiki/index.php/Linux_Security_Summit_2014/Abstracts/Safford > > Forgot to ask. If possibility to set HMAC type is fine with you I can > define _LAST.. Setting anything other than a digital signature is prevented by this patch, except in "fix" or "log" mode. That should be fine. > > Thanks. > > >>> + return -EINVAL; > >>> + digsig = (xvalue->type == EVM_IMA_XATTR_DIGSIG); > >>> + if (!digsig && (ima_appraise & IMA_APPRAISE_ENFORCE)) > >>> + return -EPERM; > >> According to the new ima_appraise "log" mode, commit "2faa6ef ima: > >> provide 'ima_appraise=log' kernel option", "log" mode permits normal > >> execution without "fixing" anything. Normal execution, here, prevents > >> writing the extended attribute. > > 'log' mode is also special mode for system developing and debugging. > > It is beneficial to be able to 'label' target object with correct value... Ok. After re-reading the patch description, "without fixing it" refers to fixing existing labels, as opposed to directly labeling the filesystem. Mimi ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] evm: check xattr value length in evm_inode_setxattr() 2014-10-24 7:07 [PATCH v2 0/2] integrity fixes Dmitry Kasatkin 2014-10-24 7:07 ` [PATCH v2 1/2] ima: check xattr value length in ima_inode_setxattr() Dmitry Kasatkin @ 2014-10-24 7:07 ` Dmitry Kasatkin 1 sibling, 0 replies; 7+ messages in thread From: Dmitry Kasatkin @ 2014-10-24 7:07 UTC (permalink / raw) To: zohar, linux-security-module, linux-ima-devel Cc: linux-kernel, jack, jmorris, dmitry.kasatkin, 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. Changes in v2: * testing for validity of xattr type [ 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 <jack@suse.cz> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> --- security/integrity/evm/evm_main.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index b392fe6..1384e4b 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -324,9 +324,14 @@ 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; + if (xattr_data->type != EVM_IMA_XATTR_DIGSIG) + return -EINVAL; + } return evm_protect_xattr(dentry, xattr_name, xattr_value, xattr_value_len); } -- 1.9.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-10-24 16:05 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-24 7:07 [PATCH v2 0/2] integrity fixes Dmitry Kasatkin 2014-10-24 7:07 ` [PATCH v2 1/2] ima: check xattr value length in ima_inode_setxattr() Dmitry Kasatkin 2014-10-24 14:18 ` Mimi Zohar 2014-10-24 15:00 ` Dmitry Kasatkin 2014-10-24 15:08 ` Dmitry Kasatkin 2014-10-24 16:00 ` Mimi Zohar 2014-10-24 7:07 ` [PATCH v2 2/2] evm: check xattr value length in evm_inode_setxattr() Dmitry Kasatkin
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.