[GIT PULL] Fix for Integrity subsystem null pointer deref

[GIT PULL] Fix for Integrity subsystem null pointer deref

[James Morris]

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(-)

Re: [GIT PULL] Fix for Integrity subsystem null pointer deref

[Andy Lutomirski]

On Tue, Oct 28, 2014 at 8:55 PM, James Morris <jmorris@namei.org> 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

Re: [GIT PULL] Fix for Integrity subsystem null pointer deref

[Mimi Zohar]

On Tue, 2014-10-28 at 22:08 -0700, Andy Lutomirski wrote: 
> On Tue, Oct 28, 2014 at 8:55 PM, James Morris <jmorris@namei.org> 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

Re: [GIT PULL] Fix for Integrity subsystem null pointer deref

[Andy Lutomirski]

On Oct 29, 2014 6:00 AM, "Mimi Zohar" <zohar@linux.vnet.ibm.com> wrote:
>
> On Tue, 2014-10-28 at 22:08 -0700, Andy Lutomirski wrote:
> > On Tue, Oct 28, 2014 at 8:55 PM, James Morris <jmorris@namei.org> 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
>

Re: [GIT PULL] Fix for Integrity subsystem null pointer deref

[Mimi Zohar]

On Wed, 2014-10-29 at 09:23 -0700, Andy Lutomirski wrote: 
> On Oct 29, 2014 6:00 AM, "Mimi Zohar" <zohar@linux.vnet.ibm.com> wrote:
> >
> > On Tue, 2014-10-28 at 22:08 -0700, Andy Lutomirski wrote:
> > > On Tue, Oct 28, 2014 at 8:55 PM, James Morris <jmorris@namei.org> 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

Re: [GIT PULL] Fix for Integrity subsystem null pointer deref

[Dan Carpenter]

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

Re: [GIT PULL] Fix for Integrity subsystem null pointer deref

[Andy Lutomirski]

On Wed, Oct 29, 2014 at 11:36 AM, Dan Carpenter
<dan.carpenter@oracle.com> 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

Re: [GIT PULL] Fix for Integrity subsystem null pointer deref

[Mimi Zohar]

On Wed, 2014-10-29 at 11:51 -0700, Andy Lutomirski wrote: 
> On Wed, Oct 29, 2014 at 11:36 AM, Dan Carpenter
> <dan.carpenter@oracle.com> 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

Re: [GIT PULL] Fix for Integrity subsystem null pointer deref

[Andy Lutomirski]

On Oct 29, 2014 1:20 PM, "Mimi Zohar" <zohar@linux.vnet.ibm.com> wrote:
>
> On Wed, 2014-10-29 at 11:51 -0700, Andy Lutomirski wrote:
> > On Wed, Oct 29, 2014 at 11:36 AM, Dan Carpenter
> > <dan.carpenter@oracle.com> 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
>

Re: [GIT PULL] Fix for Integrity subsystem null pointer deref

[Dmitry Kasatkin]

On 29 October 2014 23:22, Andy Lutomirski <luto@amacapital.net> wrote:
> On Oct 29, 2014 1:20 PM, "Mimi Zohar" <zohar@linux.vnet.ibm.com> wrote:
>>
>> On Wed, 2014-10-29 at 11:51 -0700, Andy Lutomirski wrote:
>> > On Wed, Oct 29, 2014 at 11:36 AM, Dan Carpenter
>> > <dan.carpenter@oracle.com> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,
Dmitry

Re: [GIT PULL] Fix for Integrity subsystem null pointer deref

[Andy Lutomirski]

On Wed, Oct 29, 2014 at 3:23 PM, Dmitry Kasatkin
<dmitry.kasatkin@gmail.com> wrote:
> On 29 October 2014 23:22, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Oct 29, 2014 1:20 PM, "Mimi Zohar" <zohar@linux.vnet.ibm.com> wrote:
>>>
>>> On Wed, 2014-10-29 at 11:51 -0700, Andy Lutomirski wrote:
>>> > On Wed, Oct 29, 2014 at 11:36 AM, Dan Carpenter
>>> > <dan.carpenter@oracle.com> 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 majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Thanks,
> Dmitry



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [GIT PULL] Fix for Integrity subsystem null pointer deref

[Dan Carpenter]

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