| Message ID | 20131002212720.GA30492@elgon.mountain |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Dan Carpenter <dan.carpenter@oracle.com> Date: Thu, 3 Oct 2013 00:27:20 +0300 > We need to cap ->msg_namelen or it leads to a buffer overflow when we > to the memcpy() in __audit_sockaddr(). It requires CAP_AUDIT_CONTROL to > exploit this bug. > > The call tree is: > ___sys_recvmsg() > move_addr_to_user() > audit_sockaddr() > __audit_sockaddr() > > Reported-by: Jüri Aedla <juri.aedla@gmail.com> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > v2: The limit check to the compat code was missing as pointed out by > Ben Hutchings. Applied and queued up for -stable, thanks Dan. -- 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
Dan Carpenter <dan.carpenter@oracle.com> wrote: > --- a/net/socket.c > +++ b/net/socket.c > @@ -1964,6 +1964,16 @@ struct used_address { > unsigned int name_len; > }; > > +static int copy_msghdr_from_user(struct msghdr *kmsg, > + struct msghdr __user *umsg) > +{ > + if (copy_from_user(kmsg, umsg, sizeof(struct msghdr))) > + return -EFAULT; > + if (kmsg->msg_namelen > sizeof(struct sockaddr_storage)) > + return -EINVAL; > + return 0; Crap, this seems to break Ruby trunk :x https://bugs.ruby-lang.org/issues/9124 I'm inclined to think Ruby is wrong to use a gigantic buffer, but this may also break some other existing userspace code. I'm not sure what the best option since breaking userspace (even buggy userspace?) is not taken lightly. Is there a different way to fix this in the kernel? Note: this doesn't affect a stable release of Ruby, yet. (Not a Ruby developer myself, just occasional contributor). -- 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
On Wed, Nov 27, 2013 at 11:32:18AM +0000, Eric Wong wrote: > Dan Carpenter <dan.carpenter@oracle.com> wrote: > > --- a/net/socket.c > > +++ b/net/socket.c > > @@ -1964,6 +1964,16 @@ struct used_address { > > unsigned int name_len; > > }; > > > > +static int copy_msghdr_from_user(struct msghdr *kmsg, > > + struct msghdr __user *umsg) > > +{ > > + if (copy_from_user(kmsg, umsg, sizeof(struct msghdr))) > > + return -EFAULT; > > + if (kmsg->msg_namelen > sizeof(struct sockaddr_storage)) > > + return -EINVAL; > > + return 0; > > Crap, this seems to break Ruby trunk :x > https://bugs.ruby-lang.org/issues/9124 > > I'm inclined to think Ruby is wrong to use a gigantic buffer, but this > may also break some other existing userspace code. I'm not sure what > the best option since breaking userspace (even buggy userspace?) is not > taken lightly. > > Is there a different way to fix this in the kernel? > > Note: this doesn't affect a stable release of Ruby, yet. We have to clamp msg_namelen to max sizeof(struct sockaddr_storage). The sendmsg handler will check msg_namelen again and error out correctly if the size of msg_name is too short. Greetings, Hannes -- 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
On Wed, Nov 27, 2013 at 3:51 AM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > > We have to clamp msg_namelen to max sizeof(struct sockaddr_storage). > The sendmsg handler will check msg_namelen again and error out correctly if > the size of msg_name is too short. Yeah, clamping sounds like the right thing to do at least for receiving. For sending, you should say "we can't send packets that big due to memory constraints" (of, for the case of a sockaddr, "to an address this big"), but for receiving the size of the user space buffer is kind of irrelevant - if the user gives a bigger buffer than necessary, who cares? We just need to make sure that the kernel doesn't then allocate silly-big temporary buffers internally. There seems to be a patch floating around to clamp things already. Linus -- 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
On Wed, Nov 27, 2013 at 12:24:41PM -0800, Linus Torvalds wrote: > On Wed, Nov 27, 2013 at 3:51 AM, Hannes Frederic Sowa > <hannes@stressinduktion.org> wrote: > > > > We have to clamp msg_namelen to max sizeof(struct sockaddr_storage). > > The sendmsg handler will check msg_namelen again and error out correctly if > > the size of msg_name is too short. > > Yeah, clamping sounds like the right thing to do at least for > receiving. For sending, you should say "we can't send packets that big > due to memory constraints" (of, for the case of a sockaddr, "to an > address this big"), but for receiving the size of the user space > buffer is kind of irrelevant - if the user gives a bigger buffer than > necessary, who cares? We just need to make sure that the kernel > doesn't then allocate silly-big temporary buffers internally. > > There seems to be a patch floating around to clamp things already. Data buffers are clamed fine already. The sockaddr buffers are currently always 128 bytes (== sizeof(struct sockaddr_storage)) in size and are allocated on the stack of the recvmsg/sendmsg syscall handlers. We normally don't have high stack usage on recvmsg calls but it could be worth trying to optimize that on sendmsg, I agree. I have not seen a patch which tries to do so but maybe I haven't looked far enough back in the mailing list archives. Clamping on sending seems necessary to not break exisiting applications. I guess those programs expect the kernel to know which namelen the protocol expects and only use that part of the provided sockaddr buffer (an example is the mentioned ruby bug in this thread which seems to pass down the size of a union for all possile sockaddrs: <https://bugs.ruby-lang.org/issues/9124#note-2>). I recently noticed a some more subtile annoyance in that code: We don't know the anticipated sockaddr size before calling the recvmsg handler. Hence it is currently possible that we dequeue a packet from the socket receiving queue and later error out and drop the packet because the user provided a socket address buffer which is too small. IMHO we should catch that before dequeueing the packet. Either we can export the address size via the per-protocol-structures or we have to start passing the user provided buffer sizes down the stack (currently all recvmsg handlers expect to always have 128 bytes for storing addresses). I'll look into this. Greetings, Hannes -- 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
diff --git a/net/socket.c b/net/socket.c index ebed4b6..c226ace 100644 --- a/net/socket.c +++ b/net/socket.c @@ -1964,6 +1964,16 @@ struct used_address { unsigned int name_len; }; +static int copy_msghdr_from_user(struct msghdr *kmsg, + struct msghdr __user *umsg) +{ + if (copy_from_user(kmsg, umsg, sizeof(struct msghdr))) + return -EFAULT; + if (kmsg->msg_namelen > sizeof(struct sockaddr_storage)) + return -EINVAL; + return 0; +} + static int ___sys_sendmsg(struct socket *sock, struct msghdr __user *msg, struct msghdr *msg_sys, unsigned int flags, struct used_address *used_address) @@ -1982,8 +1992,11 @@ static int ___sys_sendmsg(struct socket *sock, struct msghdr __user *msg, if (MSG_CMSG_COMPAT & flags) { if (get_compat_msghdr(msg_sys, msg_compat)) return -EFAULT; - } else if (copy_from_user(msg_sys, msg, sizeof(struct msghdr))) - return -EFAULT; + } else { + err = copy_msghdr_from_user(msg_sys, msg); + if (err) + return err; + } if (msg_sys->msg_iovlen > UIO_FASTIOV) { err = -EMSGSIZE; @@ -2191,8 +2204,11 @@ static int ___sys_recvmsg(struct socket *sock, struct msghdr __user *msg, if (MSG_CMSG_COMPAT & flags) { if (get_compat_msghdr(msg_sys, msg_compat)) return -EFAULT; - } else if (copy_from_user(msg_sys, msg, sizeof(struct msghdr))) - return -EFAULT; + } else { + err = copy_msghdr_from_user(msg_sys, msg); + if (err) + return err; + } if (msg_sys->msg_iovlen > UIO_FASTIOV) { err = -EMSGSIZE; diff --git a/net/compat.c b/net/compat.c index f0a1ba6..8903258 100644 --- a/net/compat.c +++ b/net/compat.c @@ -71,6 +71,8 @@ int get_compat_msghdr(struct msghdr *kmsg, struct compat_msghdr __user *umsg) __get_user(kmsg->msg_controllen, &umsg->msg_controllen) || __get_user(kmsg->msg_flags, &umsg->msg_flags)) return -EFAULT; + if (kmsg->msg_namelen > sizeof(struct sockaddr_storage)) + return -EINVAL; kmsg->msg_name = compat_ptr(tmp1); kmsg->msg_iov = compat_ptr(tmp2); kmsg->msg_control = compat_ptr(tmp3);
We need to cap ->msg_namelen or it leads to a buffer overflow when we to the memcpy() in __audit_sockaddr(). It requires CAP_AUDIT_CONTROL to exploit this bug. The call tree is: ___sys_recvmsg() move_addr_to_user() audit_sockaddr() __audit_sockaddr() Reported-by: Jüri Aedla <juri.aedla@gmail.com> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- v2: The limit check to the compat code was missing as pointed out by Ben Hutchings. -- 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