Re: [PATCH 1/2] ima: check xattr value length in ima_inode_setxattr()
On Fri, 2014-10-24 at 13:55 +1100, James Morris wrote: > On Thu, 23 Oct 2014, 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. > > 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 > > > -- > James Morris > > > -- > To unsubscribe from this list: send the line "unsubscribe > linux-security-module" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ima: check xattr value length in ima_inode_setxattr()
On Thu, 23 Oct 2014, 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. I'd like to see more review/acks on this before sending it to Linus. Mimi? -- James Morris -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ima: check xattr value length in ima_inode_setxattr()
On Thu 23-10-14 18:59:07, Dmitry Kasatkin wrote: > On 23 October 2014 18:40, Jan Kara wrote: > > On Thu 23-10-14 16:47:17, 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. > >> > > .. > >> > >> Reported-by: Jan Kara > >> Signed-off-by: Dmitry Kasatkin > >> --- > >> security/integrity/ima/ima_appraise.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/security/integrity/ima/ima_appraise.c > >> b/security/integrity/ima/ima_appraise.c > >> index 5b845af..f07aacd 100644 > >> --- a/security/integrity/ima/ima_appraise.c > >> +++ b/security/integrity/ima/ima_appraise.c > >> @@ -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. I see. Thanks for explanation. Honza > >> ima_reset_appraise_flags(dentry->d_inode, > >>(xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0); > >> result = 0; > >> -- > >> 1.9.1 > >> > > -- > > Jan Kara > > SUSE Labs, CR > > > > -- > Thanks, > Dmitry -- Jan Kara SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ima: check xattr value length in ima_inode_setxattr()
On 23 October 2014 18:40, Jan Kara wrote: > On Thu 23-10-14 16:47:17, 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. >> > .. >> >> Reported-by: Jan Kara >> Signed-off-by: Dmitry Kasatkin >> --- >> security/integrity/ima/ima_appraise.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/security/integrity/ima/ima_appraise.c >> b/security/integrity/ima/ima_appraise.c >> index 5b845af..f07aacd 100644 >> --- a/security/integrity/ima/ima_appraise.c >> +++ b/security/integrity/ima/ima_appraise.c >> @@ -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 > > Honza >> ima_reset_appraise_flags(dentry->d_inode, >>(xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0); >> result = 0; >> -- >> 1.9.1 >> > -- > Jan Kara > SUSE Labs, CR -- Thanks, Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ima: check xattr value length in ima_inode_setxattr()
On Thu 23-10-14 16:47:17, 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. > .. > > Reported-by: Jan Kara > Signed-off-by: Dmitry Kasatkin > --- > security/integrity/ima/ima_appraise.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/security/integrity/ima/ima_appraise.c > b/security/integrity/ima/ima_appraise.c > index 5b845af..f07aacd 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -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 > ima_reset_appraise_flags(dentry->d_inode, >(xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0); > result = 0; > -- > 1.9.1 > -- Jan Kara SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[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: [] ima_inode_setxattr+0x3e/0x5a [ 261.564109] PGD 3112f067 PUD 42965067 PMD 0 [ 261.564109] Oops: [#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: 8800428c2430 ti: 880042be task.ti: 880042be [ 261.564109] RIP: 0010:[] [] ima_inode_setxattr+0x3e/0x5a [ 261.564109] RSP: 0018:880042be3d50 EFLAGS: 00010246 [ 261.564109] RAX: 0001 RBX: RCX: 0015 [ 261.564109] RDX: 0015 RSI: RDI: 8800375cc600 [ 261.564109] RBP: 880042be3d68 R08: R09: 004d6256 [ 261.564109] R10: R11: R12: 88002149ba00 [ 261.564109] R13: R14: R15: [ 261.564109] FS: 7f6c1e219740() GS:88005da0() knlGS: [ 261.564109] CS: 0010 DS: ES: CR0: 80050033 [ 261.564109] CR2: CR3: 3b35a000 CR4: 06f0 [ 261.564109] Stack: [ 261.564109] 88002149ba00 880042be3df8 880042be3d98 [ 261.564109] 812a101b 88002149ba00 880042be3df8 [ 261.564109] 880042be3de0 8116d08a 880042be3dc8 [ 261.564109] Call Trace: [ 261.564109] [] security_inode_setxattr+0x48/0x6a [ 261.564109] [] vfs_setxattr+0x6b/0x9f [ 261.564109] [] setxattr+0x122/0x16c [ 261.564109] [] ? mnt_want_write+0x21/0x45 [ 261.564109] [] ? __sb_start_write+0x10f/0x143 [ 261.564109] [] ? mnt_want_write+0x21/0x45 [ 261.564109] [] ? __mnt_want_write+0x48/0x4f [ 261.564109] [] SyS_setxattr+0x6e/0xb0 [ 261.564109] [] 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 [] ima_inode_setxattr+0x3e/0x5a [ 261.564109] RSP [ 261.564109] CR2: [ 261.58] ---[ end trace 39a89a3fc267e652 ]--- Reported-by: Jan Kara Signed-off-by: Dmitry Kasatkin --- security/integrity/ima/ima_appraise.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 5b845af..f07aacd 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -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; -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[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: [812af272] ima_inode_setxattr+0x3e/0x5a [ 261.564109] PGD 3112f067 PUD 42965067 PMD 0 [ 261.564109] Oops: [#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: 8800428c2430 ti: 880042be task.ti: 880042be [ 261.564109] RIP: 0010:[812af272] [812af272] ima_inode_setxattr+0x3e/0x5a [ 261.564109] RSP: 0018:880042be3d50 EFLAGS: 00010246 [ 261.564109] RAX: 0001 RBX: RCX: 0015 [ 261.564109] RDX: 0015 RSI: RDI: 8800375cc600 [ 261.564109] RBP: 880042be3d68 R08: R09: 004d6256 [ 261.564109] R10: R11: R12: 88002149ba00 [ 261.564109] R13: R14: R15: [ 261.564109] FS: 7f6c1e219740() GS:88005da0() knlGS: [ 261.564109] CS: 0010 DS: ES: CR0: 80050033 [ 261.564109] CR2: CR3: 3b35a000 CR4: 06f0 [ 261.564109] Stack: [ 261.564109] 88002149ba00 880042be3df8 880042be3d98 [ 261.564109] 812a101b 88002149ba00 880042be3df8 [ 261.564109] 880042be3de0 8116d08a 880042be3dc8 [ 261.564109] Call Trace: [ 261.564109] [812a101b] security_inode_setxattr+0x48/0x6a [ 261.564109] [8116d08a] vfs_setxattr+0x6b/0x9f [ 261.564109] [8116d1e0] setxattr+0x122/0x16c [ 261.564109] [811687e8] ? mnt_want_write+0x21/0x45 [ 261.564109] [8114d011] ? __sb_start_write+0x10f/0x143 [ 261.564109] [811687e8] ? mnt_want_write+0x21/0x45 [ 261.564109] [811687c0] ? __mnt_want_write+0x48/0x4f [ 261.564109] [8116d3e6] SyS_setxattr+0x6e/0xb0 [ 261.564109] [81529da9] 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 [812af272] ima_inode_setxattr+0x3e/0x5a [ 261.564109] RSP 880042be3d50 [ 261.564109] CR2: [ 261.58] ---[ end trace 39a89a3fc267e652 ]--- Reported-by: Jan Kara j...@suse.cz Signed-off-by: Dmitry Kasatkin d.kasat...@samsung.com --- security/integrity/ima/ima_appraise.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 5b845af..f07aacd 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -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; -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ima: check xattr value length in ima_inode_setxattr()
On Thu 23-10-14 16:47:17, 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. .. Reported-by: Jan Kara j...@suse.cz Signed-off-by: Dmitry Kasatkin d.kasat...@samsung.com --- security/integrity/ima/ima_appraise.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 5b845af..f07aacd 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -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 ima_reset_appraise_flags(dentry-d_inode, (xvalue-type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0); result = 0; -- 1.9.1 -- Jan Kara j...@suse.cz SUSE Labs, CR -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ima: check xattr value length in ima_inode_setxattr()
On 23 October 2014 18:40, Jan Kara j...@suse.cz wrote: On Thu 23-10-14 16:47:17, 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. .. Reported-by: Jan Kara j...@suse.cz Signed-off-by: Dmitry Kasatkin d.kasat...@samsung.com --- security/integrity/ima/ima_appraise.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 5b845af..f07aacd 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -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 Honza ima_reset_appraise_flags(dentry-d_inode, (xvalue-type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0); result = 0; -- 1.9.1 -- Jan Kara j...@suse.cz SUSE Labs, CR -- Thanks, Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ima: check xattr value length in ima_inode_setxattr()
On Thu 23-10-14 18:59:07, Dmitry Kasatkin wrote: On 23 October 2014 18:40, Jan Kara j...@suse.cz wrote: On Thu 23-10-14 16:47:17, 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. .. Reported-by: Jan Kara j...@suse.cz Signed-off-by: Dmitry Kasatkin d.kasat...@samsung.com --- security/integrity/ima/ima_appraise.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 5b845af..f07aacd 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -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. I see. Thanks for explanation. Honza ima_reset_appraise_flags(dentry-d_inode, (xvalue-type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0); result = 0; -- 1.9.1 -- Jan Kara j...@suse.cz SUSE Labs, CR -- Thanks, Dmitry -- Jan Kara j...@suse.cz SUSE Labs, CR -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ima: check xattr value length in ima_inode_setxattr()
On Thu, 23 Oct 2014, 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. I'd like to see more review/acks on this before sending it to Linus. Mimi? -- James Morris jmor...@namei.org -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ima: check xattr value length in ima_inode_setxattr()
On Fri, 2014-10-24 at 13:55 +1100, James Morris wrote: On Thu, 23 Oct 2014, 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. 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 -- James Morris jmor...@namei.org -- To unsubscribe from this list: send the line unsubscribe linux-security-module in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/