Al Viro reported a flaw in sendmsg(). "When we copy 32bit ->msg_control contents to kernel, we walk the same userland data twice without sanity checks on the second pass. Moreover, if original looks small enough, we end up copying to on-stack array." This is a kernel buffer overflow and therefore could lead to privilege escalation. This affects 2.6 head, and from brief inspection the code in linux 2.6.9 is similar and therefore RHEL4 is affected. Marking as embargoed for now, needs reporting to vendor-sec and kernel@security soon.
Created attachment 117865 [details] Patch and comments from Al Viro
Notified security, vendor-sec. Embargo set for one week, 20050907:12
The RHEL4 patch appears to cause portmap to fail on ppc64. recvmsg(3, {msg_name(16)={sa_family=AF_INET, sin_port=htons(963), sin_addr=inet_addr("127.0.0.1")}, msg_iov(1)=[{"s\37\273a\0\0\0\0\0\0\0\2\0\1\206\240\0\0\0\2\0\0\0\1\0"..., 8800}], msg_controllen=24, {cmsg_len=24, cmsg_level=SOL_IP, cmsg_type=, ...}, msg_flags=0x80000000}, 0) = 56 sendmsg(3, {msg_name(16)={sa_family=AF_INET, sin_port=htons(963), sin_addr=inet_addr("127.0.0.1")}, msg_iov(1)=[{"s\37\273a\0\0\0\1\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 28}], msg_controllen=24, {cmsg_len=24, cmsg_level=SOL_IP, cmsg_type=, ...}, msg_flags=0x80000000}, 0) = -1 EINVAL (Invalid argument)
tmp = ((ucmlen - CMSG_COMPAT_ALIGN(sizeof(*ucmsg))) + CMSG_ALIGN(sizeof(struct cmsghdr))); + if ((char *)kcmsg_base + kcmlen - (char *)kcmsg < CMSG_ALIGN(tmp)) + goto Einval; This is what's causing portmap to die. The test fails: c00000001e32bc40 + 28 - c00000001e32bc40 < 32 tmp is 28 (24-12 + 16) Should we really be comparing against CMSG_ALIGN(tmp) there instead of just (tmp)? Or is it the initial calculation loop that's wrong, and should be rounding up with CMSG_ALIGN() each time it does 'kcmlen += tmp'? S'far too late on a Friday night to be working this out :)
Created attachment 118407 [details] Updated patch, correcting CMSG_LEN() usage.
ITYM "didn't add missing CMSG_ALIGN() in the first loop". ACK. What happens here is that we had another bug in the same function - size of kernel buffer had been underestimated in the first loop since we didn't adjust for alignment. Another source of overflow, that is. Original patch didn't fix that bug, but with proper checks for overflow in place in the second loop we started to see -EINVAL instead of silently overflowing the damn thing. Adding CMSG_ALIGN() in the first loop closes that one for good. And
grrr.... And we need it both in 2.4 and 2.6. One note: I would assign ->cmsg_len the original value of tmp, not the aligned-up one. _Probably_ OK either way for all existing uses, but we are better off with length of payload remaining the same. So in the second loop do comparison with CMSG_ALIGN(tmp), then assign tmp to ->cmsg_len, then align tmp up.
I think cmsg_len should be the aligned one, because that is what the application would have padded it out to were it native.
ACK, then
Actually, Al, I think I'm wrong. We should set the cmsg_len to the pre-aligned calculation. I am pretty sure this is why David W. sees that filure in sys_sendmsg().
Created attachment 118435 [details] Yeah, this one appears to work. Thanks.
Created attachment 118437 [details] Incremental (for easier DaveM review) patch to do same for RHEL3.
Looks great David, I'll take the RHEL3 side from here.
Created attachment 118445 [details] Test case for sendmsg() CMSG usage. This is a test program which specifically tests the code path being modified by this bug fix. Since this code path is used only for 32-bit applications running under a 64-bit kernel, make sure that the test program is compiled into a 32-bit binary for proper testing.
Test case passes on RHEL3 and RHEL4 ppc64 with the currently proposed patches.
sending updated patches to security and vendor-sec
Sigh... One more turd: in the second loop we must do the following: 1) check that we won't overflow 2) check that CMSG_ALIGN(tmp) >= CMSG_ALIGN(sizeof cmsg_header)) 3) _then_ set ->cmsg_len 4) then align tmp up. Otherwise we can still be tricked into overflow - one last header can be written past the end of buffer, if kcmsg is at the end of buffer (or just before it) and tmp is small enough (less than sizeof).
Actually, it's not quite that bad (due to previous checks we know that tmp will be large enough). However, we really want to check for overflow before setting ->cmsg_len - otherwise we can get one word written just past the end of buffer here.
Created attachment 118475 [details] incremental to net/compat.c
Created attachment 118486 [details] Final (hopefully) patch resulting from comment #32
Removing embargo, public today.
An advisory has been issued which should help the problem described in this bug report. This report is therefore being closed with a resolution of ERRATA. For more information on the solution and/or where to find the updated files, please follow the link below. You may reopen this bug report if the solution does not work for you. http://rhn.redhat.com/errata/RHSA-2005-514.html