netdev - Re: [PATCH] net: Fix security_socket

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 3 Aug 2011 23:29:57 +1000
From:	Anton Blanchard <anton@...ba.org>
To:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc:	davem@...emloft.net, eparis@...isplace.org, casey@...aufler-ca.com,
	mjt@....msk.ru, netdev@...r.kernel.org,
	linux-security-module@...r.kernel.org
Subject: Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

Hi,

> > I much prefer to make the error handling more correct, rather than
> > making sendmmsg() have fundamentally different semantics depending
> > upon the underlying LSM.
> 
> Well, the way how sendmmsg() returns error code is tricky. But
> recvmmsg() has been doing in this way for a while. So, for symmetry
> reason, maybe sendmmsg() should do as with recvmmsg() since it is too
> late to change recvmmsg()'s way.
> 
> So, programmers should be warned (in the man pages) that they should
> always call getsockopt(SO_ERROR) (in order to clear the error code)
> if sendmmsg() or recvmmsg() returned less than requested.

As you suggest, I wanted to mirror how recvmmsg returns errors. But I
now agree with Dave, we should not return an error if we managed to send
any datagrams.

Perhaps we need to modify recvmmsg to do the same?

> By the way, don't we want integer overflow check and/or
> cond_resched() here? I don't know whether there is an arch where
> userspace can allocate (1 << BITS_PER_INT) * sizeof(struct msghdr)
> bytes using malloc() and kernel can allocate huge memory for the
> socket buffer.
> 
> #include <stdio.h>
> int main(int argc, char *argv[])
> {
>         int datagrams = 0;
>         unsigned int vlen = 4294967290U;
>         while (datagrams < vlen)
>                 datagrams++;
>         printf("%u\n", datagrams);
>         return 0;
> }
> 
> I think this program (on x86_32) will print an IS_ERR() value upon
> success.

Good catch. I wonder if we can do something similar to read/write where
we just truncate the length. What value should we use? One option is to
reuse UIO_MAXIOV (1024).

The following patch is compiled tested only so far.

Anton
--

[PATCH] net: Cap number of elements for recvmmsg and sendmmsg 

To limit the amount of time we can spend in recvmmsg and sendmmsg,
cap the number of elements to UIO_MAXIOV (currently 1024). 
       
Signed-off-by: Anton Blanchard <anton@...ba.org>
Cc: <stable@...nel.org>
---

diff --git a/net/socket.c b/net/socket.c
index b1cbbcd..ad345b1 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1999,6 +1999,9 @@ int __sys_sendmmsg(int fd, struct mmsghdr __user *mmsg, unsigned int vlen,
 	struct compat_mmsghdr __user *compat_entry;
 	struct msghdr msg_sys;
 
+	if (vlen > UIO_MAXIOV)
+		vlen = UIO_MAXIOV;
+
 	datagrams = 0;
 
 	sock = sockfd_lookup_light(fd, &err, &fput_needed);
@@ -2199,6 +2202,9 @@ int __sys_recvmmsg(int fd, struct mmsghdr __user *mmsg, unsigned int vlen,
 	struct msghdr msg_sys;
 	struct timespec end_time;
 
+	if (vlen > UIO_MAXIOV)
+		vlen = UIO_MAXIOV;
+
 	if (timeout &&
 	    poll_select_set_timeout(&end_time, timeout->tv_sec,
 				    timeout->tv_nsec))
--
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