netdev - Re: [Security] TIPC security issues

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20101028195107.GA884@windriver.com>
Date:	Thu, 28 Oct 2010 15:51:07 -0400
From:	Paul Gortmaker <paul.gortmaker@...driver.com>
To:	David Miller <davem@...emloft.net>
Cc:	torvalds@...ux-foundation.org, drosenberg@...curity.com,
	jon.maloy@...csson.com, allan.stephens@...driver.com,
	netdev@...r.kernel.org, security@...nel.org
Subject: Re: [Security] TIPC security issues

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

> From: Linus Torvalds <torvalds@...ux-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@...driver.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@...curity.com>
Signed-off-by: Allan Stephens <Allan.Stephens@...driver.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@...driver.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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html