TIPC security issues

TIPC security issues

[Dan Rosenberg]

The tipc_msg_build() function in net/tipc/msg.c is written in such a way
as to create a highly exploitable kernel heap overflow that would allow
a local user to escalate privileges to root by issuing maliciously
crafted sendmsg() calls.  At a minimum, the following issues should be
fixed:

1. The tipc_msg_calc_data_size() function is almost totally broken.  It
sums together size_t values (iov_lens), but returns an integer.  Two
things can go wrong - the total value can wrap around, or on 64-bit
platforms, iov_len values greater than UINT_MAX will be truncated.

2. The comparison of dsz to TIPC_MAX_USER_MSG_SIZE is signed, so
negative (large unsigned) values will pass this check.

3. The comparison of sz to max_size is also signed.

As a result of these issues, it's possible to cause the allocation of a
small heap buffer and the subsequent copying of a carefully controlled
larger amount of data into that buffer.

I haven't found a Linux distribution that defines a module alias for
TIPC (even though most compile it as a module), so an administrator will
have had to explicitly load the TIPC module for a system to be
vulnerable.

-Dan

Re: [Security] TIPC security issues

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1555 bytes --]

On Thu, Oct 21, 2010 at 4:45 PM, Dan Rosenberg <drosenberg@vsecurity.com> wrote:
>
> 1. The tipc_msg_calc_data_size() function is almost totally broken.  It
> sums together size_t values (iov_lens), but returns an integer.  Two
> things can go wrong - the total value can wrap around, or on 64-bit
> platforms, iov_len values greater than UINT_MAX will be truncated.

Hmm. I actually think that this is a problem with "verify_iovec()". We
should just make that one stricter, I think.

We already (long ago) decided that POSIX.1 compatibility be damned,
and that reading and writing more than 2GB in a single system call is
bogus: so normal write calls will actually limit size_t arguments to
MAX_INT, exactly so that various filesystems don't have to worry about
overflow and can keep length arguments in an "int".

And we probably should just do the same in verify[_compat]_iovec(). We
do 99% of the necessary work there already - adding a few simple
checks would get us there all the way. And then silly things like this
wouldn't matter.

Something simple like:
  if an iovec has a total length that is > MAX_INT, then limit that
entry to MAX_INT, and clear out the rest

Something like the appended UNTESTED patch. NOTE! it actually makes
"verify_iovec()" *change* the iovec if it grows too big.

Also note that not only is it untested, it also doesn't do this for
the verify_compat_iovec() function, so this is really more of a "look,
we could do this" patch - to get discussion started.

                       Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 993 bytes --]

 net/core/iovec.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/net/core/iovec.c b/net/core/iovec.c
index e6b133b..b489fd6 100644
--- a/net/core/iovec.c
+++ b/net/core/iovec.c
@@ -38,7 +38,7 @@
 long verify_iovec(struct msghdr *m, struct iovec *iov, struct sockaddr *address, int mode)
 {
 	int size, ct;
-	long err;
+	size_t err;
 
 	if (m->msg_namelen) {
 		if (mode == VERIFY_READ) {
@@ -60,14 +60,13 @@ long verify_iovec(struct msghdr *m, struct iovec *iov, struct sockaddr *address,
 	err = 0;
 
 	for (ct = 0; ct < m->msg_iovlen; ct++) {
-		err += iov[ct].iov_len;
-		/*
-		 * Goal is not to verify user data, but to prevent returning
-		 * negative value, which is interpreted as errno.
-		 * Overflow is still possible, but it is harmless.
-		 */
-		if (err < 0)
-			return -EMSGSIZE;
+		size_t len = iov[ct].iov_len;
+
+		if (len > INT_MAX - err) {
+			len = INT_MAX - err;
+			iov[ct].iov_len = len;
+		}
+		err += len;
 	}
 
 	return err;

RE: TIPC security issues

[Jon Maloy]

Acked. We clearly have a couple of things to fix here.
We'll try to get it done asap.

///jon



> -----Original Message-----
> From: Dan Rosenberg [mailto:drosenberg@vsecurity.com] 
> Sent: October-21-10 19:46
> To: Jon Maloy; allan.stephens@windriver.com
> Cc: security@kernel.org; netdev@vger.kernel.org
> Subject: TIPC security issues
> 
> The tipc_msg_build() function in net/tipc/msg.c is written in 
> such a way as to create a highly exploitable kernel heap 
> overflow that would allow a local user to escalate privileges 
> to root by issuing maliciously crafted sendmsg() calls.  At a 
> minimum, the following issues should be
> fixed:
> 
> 1. The tipc_msg_calc_data_size() function is almost totally 
> broken.  It sums together size_t values (iov_lens), but 
> returns an integer.  Two things can go wrong - the total 
> value can wrap around, or on 64-bit platforms, iov_len values 
> greater than UINT_MAX will be truncated.
> 
> 2. The comparison of dsz to TIPC_MAX_USER_MSG_SIZE is signed, 
> so negative (large unsigned) values will pass this check.
> 
> 3. The comparison of sz to max_size is also signed.
> 
> As a result of these issues, it's possible to cause the 
> allocation of a small heap buffer and the subsequent copying 
> of a carefully controlled larger amount of data into that buffer.
> 
> I haven't found a Linux distribution that defines a module 
> alias for TIPC (even though most compile it as a module), so 
> an administrator will have had to explicitly load the TIPC 
> module for a system to be vulnerable.
> 
> -Dan
> 
> 

Re: [Security] TIPC security issues

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 21 Oct 2010 17:31:12 -0700

> We already (long ago) decided that POSIX.1 compatibility be damned,
> and that reading and writing more than 2GB in a single system call is
> bogus: so normal write calls will actually limit size_t arguments to
> MAX_INT, exactly so that various filesystems don't have to worry about
> overflow and can keep length arguments in an "int".

Maybe the filesystem paths are this way, but the bulk of the socket
paths properly use size_t when touching anything even related
to an I/O length.

I know that TCP can do a >= 4GB write just fine right now.

In fact if you look I recently removed the last obstacle to this based
upon a bug report from a user trying to do a 4GB write (which ended up
getting truncated to zero):

commit 01db403cf99f739f86903314a489fb420e0e254f
Author: David S. Miller <davem@davemloft.net>
Date:   Mon Sep 27 20:24:54 2010 -0700

    tcp: Fix >4GB writes on 64-bit.
    
    Fixes kernel bugzilla #16603
    
    tcp_sendmsg() truncates iov_len to an 'int' which a 4GB write to write
    zero bytes, for example.
    
    There is also the problem higher up of how verify_iovec() works.  It
    wants to prevent the total length from looking like an error return
    value.
    
    However it does this using 'int', but syscalls return 'long' (and
    thus signed 64-bit on 64-bit machines).  So it could trigger
    false-positives on 64-bit as written.  So fix it to use 'long'.
    
    Reported-by: Olaf Bonorden <bono@onlinehome.de>
    Reported-by: Daniel Büse <dbuese@gmx.de>
    Reported-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Anyways, my point is that not only is the socket layer entirely ready
for this, it is also the case that while 2GB may seem big today in
most places, some day it might not be.  :-)

Re: [Security] TIPC security issues

[Linus Torvalds]

On Sun, Oct 24, 2010 at 7:14 PM, David Miller <davem@davemloft.net> wrote:
>
> Maybe the filesystem paths are this way, but the bulk of the socket
> paths properly use size_t when touching anything even related
> to an I/O length.

Umm. "Bulk" is not "all".

Which is the whole point. Most filesystems have no trouble either. But
when a mistake is a security issue, that's not enough.

> I know that TCP can do a >= 4GB write just fine right now.

Again - totally irrelevant. Plus anybody who relies on doing 4GB
writes in one go would be broken _anyway_.

In other words, what you argue for has zero upsides, and it has
downsides. As shown by the fact that TIPC was buggy.

> In fact if you look I recently removed the last obstacle to this based
> upon a bug report from a user trying to do a 4GB write (which ended up
> getting truncated to zero):

.. and if you looked at my suggested patch, you would have seen that
it would have avoided that, and still worked fine (exactly because it
doesn't truncate anything).

David - the issue is _security_. The way to fix security problems is
not to say "most things handle this correctly". The way to avoid them
is to have several layers of handling things correctly, so that even
when one turns out to be broken, the others still protect it.

                        Linus

Re: [Security] TIPC security issues

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sun, 24 Oct 2010 20:42:39 -0700

> David - the issue is _security_. The way to fix security problems is
> not to say "most things handle this correctly". The way to avoid them
> is to have several layers of handling things correctly, so that even
> when one turns out to be broken, the others still protect it.

Fair enough.

Re: [Security] TIPC security issues

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 21 Oct 2010 17:31:12 -0700

> Something like the appended UNTESTED patch. NOTE! it actually makes
> "verify_iovec()" *change* the iovec if it grows too big.

Ok I thought a bit about this patch.

How we can behave here depends upon the socket type.

For example, for a stream socket such as TCP a partial write is OK and
we could truncate the iovec like this.  That's fine.

But for a datagram socket, we have to have a one-to-one correspondance
between write() calls and packets on the wire.  So we'd either need to
accept the entire write() length or fail it with an error.

verify_iovec() (currently) doesn't have the socket type information
available, so it's not able to key off of that right now.

I agree that cutting off these cases at a high level would be the
thing to do long term, but right now verify_iovec() isn't positioned
such that we can do it just yet.

For now I'm going to look into specifically fixing the TIPC case and
also think longer term about another way to address this at a high
level.

Re: [Security] TIPC security issues

[Linus Torvalds]

On Wed, Oct 27, 2010 at 10:29 AM, David Miller <davem@davemloft.net> wrote:
>
> But for a datagram socket, we have to have a one-to-one correspondance
> between write() calls and packets on the wire.  So we'd either need to
> accept the entire write() length or fail it with an error.

I disagree. We had that exact issue with regular file read/write: in
theory, POSIX says that you should never do a partial write to a
regular file.

And the thing is, WE SIMPLY DON'T CARE. If somebody does a 2GB+ IO,
they damn well need to accept that it's not going to be some atomic
single event. It doesn't matter _how_ much actual real memory you
have, it's just stupid to even care about that situation. It's not
something any real app actually can reasonably ever expect to work, so
rather than say "we have to do it right or error out", you should just
see it as a "it's a stupid situation, we can do whatever the hell we
want, because anybody who cares is a f*cking moron that we don't care
about".

If you _really_ care deeply, then some packet-oriented protocol can
just have its own private packet size limit (which would be way less
than 2GB), and then just look at the total size and say "oh, the total
size is bigger than my limit, so I'll just error out". Then, the fact
that verify_iovec() may have truncated the message to 2GB-1 doesn't
matter at all.

(Practically speaking, I bet all packet-oriented protocols already
have a limit that is enforced by simply allocation patterns, so I
don't think it's actually a problem even now)

                   Linus

Re: [Security] TIPC security issues

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 27 Oct 2010 10:37:46 -0700

> If you _really_ care deeply, then some packet-oriented protocol can
> just have its own private packet size limit (which would be way less
> than 2GB), and then just look at the total size and say "oh, the total
> size is bigger than my limit, so I'll just error out". Then, the fact
> that verify_iovec() may have truncated the message to 2GB-1 doesn't
> matter at all.
> 
> (Practically speaking, I bet all packet-oriented protocols already
> have a limit that is enforced by simply allocation patterns, so I
> don't think it's actually a problem even now)

This is, as it turns out, effectively what the TIPC socket layer
already does.

Most of the send calls that propagate down to this code adding up the
iov_len lengths gets passed a maximum packet size.

Anyways, here is what I came up with to kill this specific bug in
TIPC:

>From 39dc17049a5ed989bab8997945a048ffddf48387 Mon Sep 17 00:00:00 2001
From: David S. Miller <davem@davemloft.net>
Date: Wed, 27 Oct 2010 10:46:59 -0700
Subject: [PATCH] tipc: Fix iov_len handling in message send path.

Use size_t to add together iov_len's from the iovec, error
out with -EMSGSIZE if total is greater than INT_MAX.

Reported-by: Dan Rosenberg <drosenberg@vsecurity.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/tipc/msg.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index ecb532f..b29eb8b 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -76,11 +76,15 @@ void tipc_msg_init(struct tipc_msg *m, u32 user, u32 type,
 
 int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 num_sect)
 {
-	int dsz = 0;
+	size_t dsz = 0;
 	int i;
 
 	for (i = 0; i < num_sect; i++)
 		dsz += msg_sect[i].iov_len;
+
+	if (dsz > INT_MAX)
+		return -EMSGSIZE;
+
 	return dsz;
 }
 
@@ -93,12 +97,15 @@ int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 num_sect)
  */
 
 int tipc_msg_build(struct tipc_msg *hdr,
-			    struct iovec const *msg_sect, u32 num_sect,
-			    int max_size, int usrmem, struct sk_buff** buf)
+		      struct iovec const *msg_sect, u32 num_sect,
+		      int max_size, int usrmem, struct sk_buff** buf)
 {
 	int dsz, sz, hsz, pos, res, cnt;
 
 	dsz = tipc_msg_calc_data_size(msg_sect, num_sect);
+	if (dsz < 0)
+		return dsz;
+
 	if (unlikely(dsz > TIPC_MAX_USER_MSG_SIZE)) {
 		*buf = NULL;
 		return -EINVAL;
-- 
1.7.3.2

Re: [Security] TIPC security issues

[Dan Rosenberg]

The proposed fix is a start, but it's not sufficient to completely fix
the problem.  What if the total of the iovecs wraps around back to 0?
The total size will be returned as a small number, but large amounts of
data will be copied into the allocated buffer since the individual
iovecs can have arbitrary sizes.

-Dan

On Wed, 2010-10-27 at 10:50 -0700, David Miller wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Wed, 27 Oct 2010 10:37:46 -0700
> 
> > If you _really_ care deeply, then some packet-oriented protocol can
> > just have its own private packet size limit (which would be way less
> > than 2GB), and then just look at the total size and say "oh, the total
> > size is bigger than my limit, so I'll just error out". Then, the fact
> > that verify_iovec() may have truncated the message to 2GB-1 doesn't
> > matter at all.
> > 
> > (Practically speaking, I bet all packet-oriented protocols already
> > have a limit that is enforced by simply allocation patterns, so I
> > don't think it's actually a problem even now)
> 
> This is, as it turns out, effectively what the TIPC socket layer
> already does.
> 
> Most of the send calls that propagate down to this code adding up the
> iov_len lengths gets passed a maximum packet size.
> 
> Anyways, here is what I came up with to kill this specific bug in
> TIPC:
> 
> From 39dc17049a5ed989bab8997945a048ffddf48387 Mon Sep 17 00:00:00 2001
> From: David S. Miller <davem@davemloft.net>
> Date: Wed, 27 Oct 2010 10:46:59 -0700
> Subject: [PATCH] tipc: Fix iov_len handling in message send path.
> 
> Use size_t to add together iov_len's from the iovec, error
> out with -EMSGSIZE if total is greater than INT_MAX.
> 
> Reported-by: Dan Rosenberg <drosenberg@vsecurity.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
>  net/tipc/msg.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index ecb532f..b29eb8b 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -76,11 +76,15 @@ void tipc_msg_init(struct tipc_msg *m, u32 user, u32 type,
>  
>  int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 num_sect)
>  {
> -	int dsz = 0;
> +	size_t dsz = 0;
>  	int i;
>  
>  	for (i = 0; i < num_sect; i++)
>  		dsz += msg_sect[i].iov_len;
> +
> +	if (dsz > INT_MAX)
> +		return -EMSGSIZE;
> +
>  	return dsz;
>  }
>  
> @@ -93,12 +97,15 @@ int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 num_sect)
>   */
>  
>  int tipc_msg_build(struct tipc_msg *hdr,
> -			    struct iovec const *msg_sect, u32 num_sect,
> -			    int max_size, int usrmem, struct sk_buff** buf)
> +		      struct iovec const *msg_sect, u32 num_sect,
> +		      int max_size, int usrmem, struct sk_buff** buf)
>  {
>  	int dsz, sz, hsz, pos, res, cnt;
>  
>  	dsz = tipc_msg_calc_data_size(msg_sect, num_sect);
> +	if (dsz < 0)
> +		return dsz;
> +
>  	if (unlikely(dsz > TIPC_MAX_USER_MSG_SIZE)) {
>  		*buf = NULL;
>  		return -EINVAL;
> -- 
> 1.7.3.2

Re: [Security] TIPC security issues

[Paul Gortmaker]

On Wed, Oct 27, 2010 at 1:50 PM, David Miller <davem@davemloft.net> wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Wed, 27 Oct 2010 10:37:46 -0700
>
>> If you _really_ care deeply, then some packet-oriented protocol can
>> just have its own private packet size limit (which would be way less
>> than 2GB), and then just look at the total size and say "oh, the total
>> size is bigger than my limit, so I'll just error out". Then, the fact
>> that verify_iovec() may have truncated the message to 2GB-1 doesn't
>> matter at all.
>>
>> (Practically speaking, I bet all packet-oriented protocols already
>> have a limit that is enforced by simply allocation patterns, so I
>> don't think it's actually a problem even now)
>
> This is, as it turns out, effectively what the TIPC socket layer
> already does.
>
> Most of the send calls that propagate down to this code adding up the
> iov_len lengths gets passed a maximum packet size.
>
> Anyways, here is what I came up with to kill this specific bug in
> TIPC:
>
> From 39dc17049a5ed989bab8997945a048ffddf48387 Mon Sep 17 00:00:00 2001
> From: David S. Miller <davem@davemloft.net>
> Date: Wed, 27 Oct 2010 10:46:59 -0700
> Subject: [PATCH] tipc: Fix iov_len handling in message send path.
>
> Use size_t to add together iov_len's from the iovec, error
> out with -EMSGSIZE if total is greater than INT_MAX.
>
> Reported-by: Dan Rosenberg <drosenberg@vsecurity.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
>  net/tipc/msg.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index ecb532f..b29eb8b 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -76,11 +76,15 @@ void tipc_msg_init(struct tipc_msg *m, u32 user, u32 type,
>
>  int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 num_sect)
>  {
> -       int dsz = 0;
> +       size_t dsz = 0;
>        int i;
>
>        for (i = 0; i < num_sect; i++)
>                dsz += msg_sect[i].iov_len;
> +
> +       if (dsz > INT_MAX)
> +               return -EMSGSIZE;
> +

I've got some patches from Al that I'm pre-reviewing/testing
before spamming everyone with them (will send within 24h, if
that is OK).   Anyway, in the above, does it make sense to
check for the overflow incrementally, i.e. something like this?

-       for (i = 0; i < num_sect; i++)
-               dsz += msg_sect[i].iov_len;
+       for (i = 0; i < num_sect; i++) {
+               len = msg_sect[i].iov_len;
+               if (len > LONG_MAX - dsz)
+                       return LONG_MAX;
+               dsz += len;
+       }

(where len and dsz are both size_t).

Thanks,
Paul.

>        return dsz;
>  }
>
> @@ -93,12 +97,15 @@ int tipc_msg_calc_data_size(struct iovec const *msg_sect, u32 num_sect)
>  */
>
>  int tipc_msg_build(struct tipc_msg *hdr,
> -                           struct iovec const *msg_sect, u32 num_sect,
> -                           int max_size, int usrmem, struct sk_buff** buf)
> +                     struct iovec const *msg_sect, u32 num_sect,
> +                     int max_size, int usrmem, struct sk_buff** buf)
>  {
>        int dsz, sz, hsz, pos, res, cnt;
>
>        dsz = tipc_msg_calc_data_size(msg_sect, num_sect);
> +       if (dsz < 0)
> +               return dsz;
> +
>        if (unlikely(dsz > TIPC_MAX_USER_MSG_SIZE)) {
>                *buf = NULL;
>                return -EINVAL;
> --
> 1.7.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [Security] TIPC security issues

[David Miller]

From: Dan Rosenberg <drosenberg@vsecurity.com>
Date: Wed, 27 Oct 2010 14:26:19 -0400

> The proposed fix is a start, but it's not sufficient to completely fix
> the problem.  What if the total of the iovecs wraps around back to 0?
> The total size will be returned as a small number, but large amounts of
> data will be copied into the allocated buffer since the individual
> iovecs can have arbitrary sizes.

The calculated length total is what should be used by the calling function
to decide how much to copy.

Sorry, I assumed the TIPC doing was sane like the rest of the networking.
:-(

I'll fix this up.

Re: [Security] TIPC security issues

[David Miller]

From: Paul Gortmaker <paul.gortmaker@windriver.com>
Date: Wed, 27 Oct 2010 14:27:13 -0400

> I've got some patches from Al that I'm pre-reviewing/testing
> before spamming everyone with them (will send within 24h, if
> that is OK).   Anyway, in the above, does it make sense to
> check for the overflow incrementally, i.e. something like this?

Can you please not work on this in private?  That's the only reason
we have duplicated work here.

Re: [Security] TIPC security issues

[Linus Torvalds]

On Wed, Oct 27, 2010 at 11:26 AM, Dan Rosenberg
<drosenberg@vsecurity.com> wrote:
> The proposed fix is a start, but it's not sufficient to completely fix
> the problem.  What if the total of the iovecs wraps around back to 0?
> The total size will be returned as a small number, but large amounts of
> data will be copied into the allocated buffer since the individual
> iovecs can have arbitrary sizes.

That's why I suggested just fixing iovec_verify() to do this all. It
already walks the thing, and while it allows the overflow right now
(and only wants to make sure the end result is positive to separate it
out from error numbers), that was always a ugly thing to do.

And the thing is, once you disallow overflow, you really MUST NOT
return an error - you need to instead cap the max, the way I also did
in my patch.

Why? Because for a streaming thing, it's entirely possible that
somebody tries to write out a whole mmap'ed file in one go, for
example. Returning an error because somebody tries to write 8GB in one
single system call is wrong as it would result in the application
reporting an IO error - but saying "I will write out part of it" is
fine, and then it's up to the user to loop over it (it already needs
to do that for other reasons for partial IO).

So doing this in verify_iovec() (and verify_compat_iovec - which I
didn't do in my RFC patch) really does fix everything, and means that
the individual socket types never have to worry about the subtle cases
of overflow in any type.

                        Linus

Re: [Security] TIPC security issues

[Paul Gortmaker]

On 10-10-27 02:35 PM, David Miller wrote:
> From: Paul Gortmaker<paul.gortmaker@windriver.com>
> Date: Wed, 27 Oct 2010 14:27:13 -0400
> 
>> I've got some patches from Al that I'm pre-reviewing/testing
>> before spamming everyone with them (will send within 24h, if
>> that is OK).   Anyway, in the above, does it make sense to
>> check for the overflow incrementally, i.e. something like this?
> 
> Can you please not work on this in private?  That's the only reason
> we have duplicated work here.

Sorry -- my intent was to not waste people's time with stuff
that might have obvious issues.  I'll send what I have now
and we can go from there.

P.

Re: [Security] TIPC security issues

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 27 Oct 2010 11:51:19 -0700

> So doing this in verify_iovec() (and verify_compat_iovec - which I
> didn't do in my RFC patch) really does fix everything, and means that
> the individual socket types never have to worry about the subtle cases
> of overflow in any type.

I completely agree about capping things in verify_iovec().

And it was completely erroneous of me to change verify_iovec() to
return 'long' instead of 'int', it should have stayed at 'int' with a
cap.

Because the protocols don't even have a way to return something larger
than an 'int' as a return value due to the signature of the recvmsg()
and sendmsg() socket ops.

Re: [Security] TIPC security issues

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1950 bytes --]

Heh. We apparently have _another_ iovec overflow in networking. This time rds.

Reported by Thomas Pollet <thomas.pollet@gmail.com>: look at
net/rds/rdma.c around line 490. It doesn't use the regular iovec code,
instead it cooks its own, and has a few problems with overflow.

It gathers the number of pages into an "unsigned int", and for each
entry in its own rds_iovec it will check that the size is < UINT_MAX,
and then generate the number of pages for that entry. With the whole
"unaligned address adds one" logic, it means that each entry can get
(UINT_MAX >> PAGE_SHIFT)+1 pages.

And how many entries can we have? Apparently that is capped to
UINT_MAX too. So add all those up, and they can easily overflow the
unsigned int page counter.

So this time fixing verify_iovec() doesn't help, because rds just
cooks its own, and this is using a totally different interface: it
seems to hook into sendmsg, but it looks like it uses the ancillary
data objects and passes in its own magical iovec rather than use any
"normal" iovec thing. I don't know the code, I may be totally off.

Attached is a half-arsed patch. I say "half-arsed" because I think it
fixes one thing, but I haven't looked at any other use. And quite
frankly, even the one thing it fixes is totally broken: the iovec is
left in user space, so there are all those crazy race-conditions where
another thread in user space can _change_ the rds_iovec after we have
counted the pages, and now the page count is totally wrong.

So the code is just an unmitigated disaster from any standpoint. My
patch - if it works - makes it slightly better. But I'd suggest
disabling RDS in any sane setup.

This was the same subsystem that totally didn't check the user
addresses at all, after all. So there are probably tons of other bugs
lurking.

Btw: patch is totally untested. I haven't even compiled it. So take it
with a pinch of salt.

                                                 Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1306 bytes --]

From: Linus Torvalds <torvalds@linux-foundation.org>
Subject: net: fix rds_iovec page count overflow

As reported by Thomas Pollet, the rdma page counting can overflow.  We
get the rdma sizes in 64-bit unsigned entities, but then limit it to
UINT_MAX bytes and shift them down to pages (so with a possible "+1" for
an unaligned address). 

So each individual page count fits comfortably in an 'unsigned int' (not
even close to overflowing into signed), but as they are added up, they
might end up resulting in a signed return value. Which would be wrong.

Catch the case of tot_pages turning negative, and return the appropriate
error code. 

Reported-by: Thomas Pollet <thomas.pollet@gmail.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 net/rds/rdma.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/net/rds/rdma.c b/net/rds/rdma.c
index 1a41deb..0df02c8 100644
--- a/net/rds/rdma.c
+++ b/net/rds/rdma.c
@@ -502,6 +502,13 @@ static int rds_rdma_pages(struct rds_rdma_args *args)
 			return -EINVAL;
 
 		tot_pages += nr_pages;
+
+		/*
+		 * nr_pages for one entry is limited to (UINT_MAX>>PAGE_SHIFT)+1,
+		 * so tot_pages cannot overflow without first going negative.
+		 */
+		if ((int)tot_pages < 0)
+			return -EINVAL;
 	}
 
 	return tot_pages;

Re: [Security] TIPC security issues

[Andy Grover]

On 10/28/2010 08:32 AM, Linus Torvalds wrote:
> Heh. We apparently have _another_ iovec overflow in networking. This time rds.
>
> Reported by Thomas Pollet<thomas.pollet@gmail.com>: look at
> net/rds/rdma.c around line 490. It doesn't use the regular iovec code,
> instead it cooks its own, and has a few problems with overflow.
>
> It gathers the number of pages into an "unsigned int", and for each
> entry in its own rds_iovec it will check that the size is<  UINT_MAX,
> and then generate the number of pages for that entry. With the whole
> "unaligned address adds one" logic, it means that each entry can get
> (UINT_MAX>>  PAGE_SHIFT)+1 pages.

FWIW both the signed issue and not checking the iovec changed were 
correct in 2.6.36, and only added in ff87e97.

> And how many entries can we have? Apparently that is capped to
> UINT_MAX too. So add all those up, and they can easily overflow the
> unsigned int page counter.
>
> So this time fixing verify_iovec() doesn't help, because rds just
> cooks its own, and this is using a totally different interface: it
> seems to hook into sendmsg, but it looks like it uses the ancillary
> data objects and passes in its own magical iovec rather than use any
> "normal" iovec thing. I don't know the code, I may be totally off.

Yes that's right, it's to map a memory region that will be the target of 
an RDMA operation. I don't know why struct rds_iovec was used instead of 
struct iovec, but I think we're stuck, since it's part of our socket API.

I'll send DaveM patches to fix those two immediately-identified problems 
today, and we'll take a good long look at the rest of the code for 
further issues.

Regards -- Andy

Re: [Security] TIPC security issues

[David Miller]

From: Andy Grover <andy.grover@oracle.com>
Date: Thu, 28 Oct 2010 11:45:04 -0700

> Yes that's right, it's to map a memory region that will be the target
> of an RDMA operation. I don't know why struct rds_iovec was used
> instead of struct iovec, but I think we're stuck, since it's part of
> our socket API.
> 
> I'll send DaveM patches to fix those two immediately-identified
> problems today, and we'll take a good long look at the rest of the
> code for further issues.

FWIW, I would strongly suggest that you copy the iovecs into the
kernel before parsing them like sys_sendmsg() and sys_recvmsg() do in
net/socket.c as part of these fixes.

Re: [Security] TIPC security issues

[Paul Gortmaker]

[Re: [Security] TIPC security issues] On 27/10/2010 (Wed 10:50) David Miller wrote:

> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Wed, 27 Oct 2010 10:37:46 -0700
> 
> > If you _really_ care deeply, then some packet-oriented protocol can
> > just have its own private packet size limit (which would be way less
> > than 2GB), and then just look at the total size and say "oh, the total
> > size is bigger than my limit, so I'll just error out". Then, the fact
> > that verify_iovec() may have truncated the message to 2GB-1 doesn't
> > matter at all.
> > 
> > (Practically speaking, I bet all packet-oriented protocols already
> > have a limit that is enforced by simply allocation patterns, so I
> > don't think it's actually a problem even now)
> 
> This is, as it turns out, effectively what the TIPC socket layer
> already does.
> 
> Most of the send calls that propagate down to this code adding up the
> iov_len lengths gets passed a maximum packet size.
> 

In keeping with this idea, perhaps this is a better solution for getting
an immediate fix to the tipc part of this issue than the previous
patches I'd sent?  I can see some immediate advantages to this:

   -it adds checks that arguably should have been there since day
    one, since it is always best to check for garbage input ASAP.

   -it is a much smaller change, and thus easier to review and have
    confidence in

   -by being smaller and clearer, it lends itself better for being
    directly cherry picked onto the -stable release(s).

We'll still need to clean up the mishmash of variable types being
used in the tipc internals, but at least we can then do that in
a development cycle, and we won't have to inflict those bigger
cleanup changesets back onto GregKH.

Paul.

----

>From 3fb200c1b27cf5cde668888ab85cffb1e9c6314f Mon Sep 17 00:00:00 2001
From: Allan Stephens <Allan.Stephens@windriver.com>
Date: Thu, 28 Oct 2010 07:58:24 -0400
Subject: [PATCH] tipc: Fix security hole exploitable by excessive send requests

Add checks to TIPC's socket send routines to promptly detect and
abort attempts to send more than 66,000 bytes in a single TIPC
message, or more than 2**31-1 bytes in a single TIPC byte stream
request.  This prevents excessively large size_t based inputs from
reaching internal tipc routines that currently use int values where
they risk being truncated or incorrectly wrapped.

The three checks are added to send_msg() send_packet() and
send_stream() -- all of which are entered via proto_ops .sendmsg, which
in turn already checked for msg_iovlen > UIO_MAXIOV [in net/socket.c],
so there is no need to repeat that specific test in these new checks.

Reported-by: Dan Rosenberg <drosenberg@vsecurity.com>
Signed-off-by: Allan Stephens <Allan.Stephens@windriver.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 include/linux/tipc.h |    2 +-
 net/tipc/socket.c    |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/include/linux/tipc.h b/include/linux/tipc.h
index d10614b..1fd2889 100644
--- a/include/linux/tipc.h
+++ b/include/linux/tipc.h
@@ -101,7 +101,7 @@ static inline unsigned int tipc_node(__u32 addr)
  * Limiting values for messages
  */
 
-#define TIPC_MAX_USER_MSG_SIZE	66000
+#define TIPC_MAX_USER_MSG_SIZE	66000U
 
 /*
  * Message importance levels
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 33217fc..3562cf9 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -542,6 +542,8 @@ static int send_msg(struct kiocb *iocb, struct socket *sock,
 	if (unlikely((m->msg_namelen < sizeof(*dest)) ||
 		     (dest->family != AF_TIPC)))
 		return -EINVAL;
+	if (total_len > TIPC_MAX_USER_MSG_SIZE)
+		return -EMSGSIZE;
 
 	if (iocb)
 		lock_sock(sk);
@@ -649,6 +651,9 @@ static int send_packet(struct kiocb *iocb, struct socket *sock,
 	if (unlikely(dest))
 		return send_msg(iocb, sock, m, total_len);
 
+	if (total_len > TIPC_MAX_USER_MSG_SIZE)
+		return -EMSGSIZE;
+
 	if (iocb)
 		lock_sock(sk);
 
@@ -733,6 +738,11 @@ static int send_stream(struct kiocb *iocb, struct socket *sock,
 		goto exit;
 	}
 
+	if (total_len > (unsigned)INT_MAX) {
+		res = -EMSGSIZE;
+		goto exit;
+	}
+
 	/*
 	 * Send each iovec entry using one or more messages
 	 *
-- 
1.7.3.1