These changes fix a bug in xattr handling, where the evm and ima
inode_setxattr() functions do not check for empty xattrs being passed from
userspace (leading to user-triggerable null pointer dereferences).
Please pull.
The following changes since commit 9f76628da20f96a179ca62b504886f99ecc29223:
Merge branch 'for-3.18' of git://linux-nfs.org/~bfields/linux (2014-10-28 13:32:06 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git for-linus
Dmitry Kasatkin (2):
ima: check xattr value length and type in the ima_inode_setxattr()
evm: check xattr value length and type in evm_inode_setxattr()
James Morris (1):
Merge branch 'for-linus' of git://git.kernel.org/.../zohar/linux-integrity into for-linus
security/integrity/evm/evm_main.c | 9 ++++++---
security/integrity/ima/ima_appraise.c | 2 ++
security/integrity/integrity.h | 1 +
3 files changed, 9 insertions(+), 3 deletions(-)
On Tue, Oct 28, 2014 at 8:55 PM, James Morris <[email protected]> wrote:
> These changes fix a bug in xattr handling, where the evm and ima
> inode_setxattr() functions do not check for empty xattrs being passed from
> userspace (leading to user-triggerable null pointer dereferences).
>
> Please pull.
>
>
> The following changes since commit 9f76628da20f96a179ca62b504886f99ecc29223:
>
> Merge branch 'for-3.18' of git://linux-nfs.org/~bfields/linux (2014-10-28 13:32:06 -0700)
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git for-linus
>
> Dmitry Kasatkin (2):
> ima: check xattr value length and type in the ima_inode_setxattr()
I haven't read this one, but:
> evm: check xattr value length and type in evm_inode_setxattr()
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_IMA_XATTR_DIGSIG)
+ return -EPERM;
+ }
Huh? (Sorry about severe whitespace damage.)
Shouldn't there be something like if (xattr_value_len < sizeof(struct
evm_ima_xattr_data)) return -EINVAL?
--Andy
>
> James Morris (1):
> Merge branch 'for-linus' of git://git.kernel.org/.../zohar/linux-integrity into for-linus
>
> security/integrity/evm/evm_main.c | 9 ++++++---
> security/integrity/ima/ima_appraise.c | 2 ++
> security/integrity/integrity.h | 1 +
> 3 files changed, 9 insertions(+), 3 deletions(-)
--
Andy Lutomirski
AMA Capital Management, LLC
On Tue, 2014-10-28 at 22:08 -0700, Andy Lutomirski wrote:
> On Tue, Oct 28, 2014 at 8:55 PM, James Morris <[email protected]> wrote:
> > These changes fix a bug in xattr handling, where the evm and ima
> > inode_setxattr() functions do not check for empty xattrs being passed from
> > userspace (leading to user-triggerable null pointer dereferences).
> >
> > Please pull.
> >
> >
> > The following changes since commit 9f76628da20f96a179ca62b504886f99ecc29223:
> >
> > Merge branch 'for-3.18' of git://linux-nfs.org/~bfields/linux (2014-10-28 13:32:06 -0700)
> >
> > are available in the git repository at:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git for-linus
> >
> > Dmitry Kasatkin (2):
> > ima: check xattr value length and type in the ima_inode_setxattr()
>
> I haven't read this one, but:
>
> > evm: check xattr value length and type in evm_inode_setxattr()
>
> 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_IMA_XATTR_DIGSIG)
> + return -EPERM;
> + }
>
> Huh? (Sorry about severe whitespace damage.)
>
> Shouldn't there be something like if (xattr_value_len < sizeof(struct
> evm_ima_xattr_data)) return -EINVAL?
Prior to commit 2fb1c9a "evm: prohibit userspace writing 'security.evm'
HMAC value", a process with CAP_SYS_ADMIN could write either an HMAC or
signature. As the HMAC key should only be known to the kernel, only
signatures are now allowed. Instead of "struct evm_ima_xattr_data", the
code should reflect this change and use "struct signature_v2_hdr".
We'll clean up this code for the next release. For now, this patch
prevents the oops.
thanks,
Mimi
On Oct 29, 2014 6:00 AM, "Mimi Zohar" <[email protected]> wrote:
>
> On Tue, 2014-10-28 at 22:08 -0700, Andy Lutomirski wrote:
> > On Tue, Oct 28, 2014 at 8:55 PM, James Morris <[email protected]> wrote:
> > > These changes fix a bug in xattr handling, where the evm and ima
> > > inode_setxattr() functions do not check for empty xattrs being passed from
> > > userspace (leading to user-triggerable null pointer dereferences).
> > >
> > > Please pull.
> > >
> > >
> > > The following changes since commit 9f76628da20f96a179ca62b504886f99ecc29223:
> > >
> > > Merge branch 'for-3.18' of git://linux-nfs.org/~bfields/linux (2014-10-28 13:32:06 -0700)
> > >
> > > are available in the git repository at:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git for-linus
> > >
> > > Dmitry Kasatkin (2):
> > > ima: check xattr value length and type in the ima_inode_setxattr()
> >
> > I haven't read this one, but:
> >
> > > evm: check xattr value length and type in evm_inode_setxattr()
> >
> > 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_IMA_XATTR_DIGSIG)
> > + return -EPERM;
> > + }
> >
> > Huh? (Sorry about severe whitespace damage.)
> >
> > Shouldn't there be something like if (xattr_value_len < sizeof(struct
> > evm_ima_xattr_data)) return -EINVAL?
>
> Prior to commit 2fb1c9a "evm: prohibit userspace writing 'security.evm'
> HMAC value", a process with CAP_SYS_ADMIN could write either an HMAC or
> signature. As the HMAC key should only be known to the kernel, only
> signatures are now allowed. Instead of "struct evm_ima_xattr_data", the
> code should reflect this change and use "struct signature_v2_hdr".
> We'll clean up this code for the next release. For now, this patch
> prevents the oops.
>
I have no idea what the semantics are. All I'm saying is that it
looks like the code still accesses memory past the end of the buffer.
The buffer isn't a null pointer, so the symptom is different, but it
may still be a security bug.
--Andy
> thanks,
>
> Mimi
>
On Wed, 2014-10-29 at 09:23 -0700, Andy Lutomirski wrote:
> On Oct 29, 2014 6:00 AM, "Mimi Zohar" <[email protected]> wrote:
> >
> > On Tue, 2014-10-28 at 22:08 -0700, Andy Lutomirski wrote:
> > > On Tue, Oct 28, 2014 at 8:55 PM, James Morris <[email protected]> wrote:
> > > > These changes fix a bug in xattr handling, where the evm and ima
> > > > inode_setxattr() functions do not check for empty xattrs being passed from
> > > > userspace (leading to user-triggerable null pointer dereferences).
> > > >
> > > > Please pull.
> > > >
> > > >
> > > > The following changes since commit 9f76628da20f96a179ca62b504886f99ecc29223:
> > > >
> > > > Merge branch 'for-3.18' of git://linux-nfs.org/~bfields/linux (2014-10-28 13:32:06 -0700)
> > > >
> > > > are available in the git repository at:
> > > >
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git for-linus
> > > >
> > > > Dmitry Kasatkin (2):
> > > > ima: check xattr value length and type in the ima_inode_setxattr()
> > >
> > > I haven't read this one, but:
> > >
> > > > evm: check xattr value length and type in evm_inode_setxattr()
> > >
> > > 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_IMA_XATTR_DIGSIG)
> > > + return -EPERM;
> > > + }
> > >
> > > Huh? (Sorry about severe whitespace damage.)
> > >
> > > Shouldn't there be something like if (xattr_value_len < sizeof(struct
> > > evm_ima_xattr_data)) return -EINVAL?
> >
> > Prior to commit 2fb1c9a "evm: prohibit userspace writing 'security.evm'
> > HMAC value", a process with CAP_SYS_ADMIN could write either an HMAC or
> > signature. As the HMAC key should only be known to the kernel, only
> > signatures are now allowed. Instead of "struct evm_ima_xattr_data", the
> > code should reflect this change and use "struct signature_v2_hdr".
> > We'll clean up this code for the next release. For now, this patch
> > prevents the oops.
> >
>
> I have no idea what the semantics are. All I'm saying is that it
> looks like the code still accesses memory past the end of the buffer.
> The buffer isn't a null pointer, so the symptom is different, but it
> may still be a security bug.
There's no accessing of data here, just writing the data out as an
extended attribute, which requires CAP_SYS_ADMIN privilege.
Mimi
On Wed, Oct 29, 2014 at 09:23:45AM -0700, Andy Lutomirski wrote:
> I have no idea what the semantics are. All I'm saying is that it
> looks like the code still accesses memory past the end of the buffer.
> The buffer isn't a null pointer, so the symptom is different, but it
> may still be a security bug.
>
> --Andy
It only reads one byte into the struct "xattr_data->type" so checking
for non-zero is sufficient and the patch is fine.
I fixed that exact same bug in lustre last week where the xattr size is
not zero but it's less than the size of the struct. So this seems like
maybe it could be a common anti-pattern though.
regards,
dan carpenter
On Wed, Oct 29, 2014 at 11:36 AM, Dan Carpenter
<[email protected]> wrote:
> On Wed, Oct 29, 2014 at 09:23:45AM -0700, Andy Lutomirski wrote:
>> I have no idea what the semantics are. All I'm saying is that it
>> looks like the code still accesses memory past the end of the buffer.
>> The buffer isn't a null pointer, so the symptom is different, but it
>> may still be a security bug.
>>
>> --Andy
>
> It only reads one byte into the struct "xattr_data->type" so checking
> for non-zero is sufficient and the patch is fine.
Indeed. Still... eww. I don't like code that, upon local inspection,
is apparently wrong, even though it's coincidentally correct due to
some other far away condition.
--Andy
>
> I fixed that exact same bug in lustre last week where the xattr size is
> not zero but it's less than the size of the struct. So this seems like
> maybe it could be a common anti-pattern though.
>
> regards,
> dan carpenter
>
--
Andy Lutomirski
AMA Capital Management, LLC
On Wed, 2014-10-29 at 11:51 -0700, Andy Lutomirski wrote:
> On Wed, Oct 29, 2014 at 11:36 AM, Dan Carpenter
> <[email protected]> wrote:
> > On Wed, Oct 29, 2014 at 09:23:45AM -0700, Andy Lutomirski wrote:
> >> I have no idea what the semantics are. All I'm saying is that it
> >> looks like the code still accesses memory past the end of the buffer.
> >> The buffer isn't a null pointer, so the symptom is different, but it
> >> may still be a security bug.
> >>
> >> --Andy
> >
> > It only reads one byte into the struct "xattr_data->type" so checking
> > for non-zero is sufficient and the patch is fine.
>
> Indeed. Still... eww. I don't like code that, upon local inspection,
> is apparently wrong, even though it's coincidentally correct due to
> some other far away condition.
No, the code may be incomplete, but definitely not wrong.
Mimi
On Oct 29, 2014 1:20 PM, "Mimi Zohar" <[email protected]> wrote:
>
> On Wed, 2014-10-29 at 11:51 -0700, Andy Lutomirski wrote:
> > On Wed, Oct 29, 2014 at 11:36 AM, Dan Carpenter
> > <[email protected]> wrote:
> > > On Wed, Oct 29, 2014 at 09:23:45AM -0700, Andy Lutomirski wrote:
> > >> I have no idea what the semantics are. All I'm saying is that it
> > >> looks like the code still accesses memory past the end of the buffer.
> > >> The buffer isn't a null pointer, so the symptom is different, but it
> > >> may still be a security bug.
> > >>
> > >> --Andy
> > >
> > > It only reads one byte into the struct "xattr_data->type" so checking
> > > for non-zero is sufficient and the patch is fine.
> >
> > Indeed. Still... eww. I don't like code that, upon local inspection,
> > is apparently wrong, even though it's coincidentally correct due to
> > some other far away condition.
>
> No, the code may be incomplete, but definitely not wrong.
I said "apparently wrong" instead of "wrong" for a reason :)
>
> Mimi
>
On 29 October 2014 23:22, Andy Lutomirski <[email protected]> wrote:
> On Oct 29, 2014 1:20 PM, "Mimi Zohar" <[email protected]> wrote:
>>
>> On Wed, 2014-10-29 at 11:51 -0700, Andy Lutomirski wrote:
>> > On Wed, Oct 29, 2014 at 11:36 AM, Dan Carpenter
>> > <[email protected]> wrote:
>> > > On Wed, Oct 29, 2014 at 09:23:45AM -0700, Andy Lutomirski wrote:
>> > >> I have no idea what the semantics are. All I'm saying is that it
>> > >> looks like the code still accesses memory past the end of the buffer.
>> > >> The buffer isn't a null pointer, so the symptom is different, but it
>> > >> may still be a security bug.
>> > >>
>> > >> --Andy
>> > >
>> > > It only reads one byte into the struct "xattr_data->type" so checking
>> > > for non-zero is sufficient and the patch is fine.
>> >
>> > Indeed. Still... eww. I don't like code that, upon local inspection,
>> > is apparently wrong, even though it's coincidentally correct due to
>> > some other far away condition.
>>
>> No, the code may be incomplete, but definitely not wrong.
>
> I said "apparently wrong" instead of "wrong" for a reason :)
I see there is a long discussion about this patch.
Actually using something like this "if (xattr_value_len <
sizeof(struct evm_ima_xattr_data))" or using sizeof (struct
signature_v2_hdr)
in this function does not give too much.
xattr value is variable length value and having that statement false
absolutely does not mean that the value is correct.
It can be even a random garbage of the correct size.
This particular function checks the first byte only so the test is good enough.
- Dmitry
>
>>
>> Mimi
>>
> --
> 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
--
Thanks,
Dmitry
On Wed, Oct 29, 2014 at 3:23 PM, Dmitry Kasatkin
<[email protected]> wrote:
> On 29 October 2014 23:22, Andy Lutomirski <[email protected]> wrote:
>> On Oct 29, 2014 1:20 PM, "Mimi Zohar" <[email protected]> wrote:
>>>
>>> On Wed, 2014-10-29 at 11:51 -0700, Andy Lutomirski wrote:
>>> > On Wed, Oct 29, 2014 at 11:36 AM, Dan Carpenter
>>> > <[email protected]> wrote:
>>> > > On Wed, Oct 29, 2014 at 09:23:45AM -0700, Andy Lutomirski wrote:
>>> > >> I have no idea what the semantics are. All I'm saying is that it
>>> > >> looks like the code still accesses memory past the end of the buffer.
>>> > >> The buffer isn't a null pointer, so the symptom is different, but it
>>> > >> may still be a security bug.
>>> > >>
>>> > >> --Andy
>>> > >
>>> > > It only reads one byte into the struct "xattr_data->type" so checking
>>> > > for non-zero is sufficient and the patch is fine.
>>> >
>>> > Indeed. Still... eww. I don't like code that, upon local inspection,
>>> > is apparently wrong, even though it's coincidentally correct due to
>>> > some other far away condition.
>>>
>>> No, the code may be incomplete, but definitely not wrong.
>>
>> I said "apparently wrong" instead of "wrong" for a reason :)
>
> I see there is a long discussion about this patch.
>
> Actually using something like this "if (xattr_value_len <
> sizeof(struct evm_ima_xattr_data))" or using sizeof (struct
> signature_v2_hdr)
> in this function does not give too much.
> xattr value is variable length value and having that statement false
> absolutely does not mean that the value is correct.
> It can be even a random garbage of the correct size.
> This particular function checks the first byte only so the test is good enough.
>
My point is that there's no possible way to tell that only the first
byte is read just by reading the function.
--Andy
> - Dmitry
>
>>
>>>
>>> Mimi
>>>
>> --
>> 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
>
>
>
> --
> Thanks,
> Dmitry
--
Andy Lutomirski
AMA Capital Management, LLC
On Wed, Oct 29, 2014 at 09:36:12PM +0300, Dan Carpenter wrote:
> I fixed that exact same bug in lustre last week where the xattr size is
> not zero but it's less than the size of the struct. So this seems like
> maybe it could be a common anti-pattern though.
It must not be very common. I wrote a Smatch script which finds both
the lustre and the ima bugs but it doesn't find anything else major.
Apparently parsing vmcores is buggy, for example and I reported a couple
other small bugs to other lists.
fs/proc/vmcore.c:547 update_note_header_size_elf64() warn: is 'notes_section' large enough for 'struct elf64_note'?
fs/proc/vmcore.c:733 update_note_header_size_elf32() warn: is 'notes_section' large enough for 'struct elf32_note'?
regards,
dan carpenter