* 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly
@ 2000-11-03 20:45 ` Philippe Troin
2000-11-03 22:18 ` David S. Miller
0 siblings, 1 reply; 10+ messages in thread
From: Philippe Troin @ 2000-11-03 20:45 UTC (permalink / raw)
To: linux-kernel; +Cc: Alan Cox
[-- Attachment #1: Type: text/plain, Size: 391 bytes --]
I found this in all 2.2.x kernels, and it might possibly be present in
2.4.x too...
When receiving file descriptors via recvmsg(), scm_detach_fds() in
net/core/scm.c can overflow user space data at msg_control if
msg_controllen is less than sizeof(struct cmsghdr).
This is a security problem.
Attached is a patch to fix the problem and a little program to
demonstrate the problem.
Phil.
[-- Attachment #2: linux-2.2.17-8-scmrights.patch --]
[-- Type: application/octet-stream, Size: 707 bytes --]
diff -ruN linux.orig/net/core/scm.c linux/net/core/scm.c
--- linux.orig/net/core/scm.c Thu Apr 22 19:45:19 1999
+++ linux/net/core/scm.c Sun Oct 29 15:13:14 2000
@@ -216,6 +216,13 @@
int *cmfptr;
int err = 0, i;
+ /* Handle the case when we don't even have enough room for the header.
+ Fixed Oct 29 2000 by Philippe Troin <phil@fifi.org> */
+ if (msg->msg_controllen < sizeof(struct cmsghdr)) {
+ msg->msg_flags |= MSG_CTRUNC;
+ goto out;
+ }
+
if (fdnum < fdmax)
fdmax = fdnum;
@@ -258,6 +265,7 @@
* All of the files that fit in the message have had their
* usage counts incremented, so we just free the list.
*/
+out:
__scm_destroy(scm);
}
[-- Attachment #3: check-anc.c --]
[-- Type: application/octet-stream, Size: 3452 bytes --]
/*
* Check that fd does not get created when the ancillary data buffer
* gets truncated...
* Philippe Troin <phil@fifi.org>
* This snippet is licensed under the GNU General Public License.
*/
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <sys/socket.h>
#include <sys/un.h>
#include <sys/uio.h>
void
child_proc(int sock)
{
struct msghdr msg;
int zero;
int devnull;
char ancdata[CMSG_SPACE(sizeof(devnull))];
struct iovec iov[1];
struct cmsghdr *cmsg;
/**/
zero = 0;
if ((devnull = open("/dev/null", O_RDWR))<0)
perror("child: open /dev/null"), exit(1);
msg.msg_name = NULL;
msg.msg_namelen = 0;
msg.msg_iov = iov;
msg.msg_iovlen = 1;
msg.msg_control = ancdata;
msg.msg_controllen = sizeof(ancdata);
msg.msg_flags = 0;
iov[0].iov_base = &zero;
iov[0].iov_len = sizeof(zero);
cmsg = CMSG_FIRSTHDR(&msg);
cmsg->cmsg_len = CMSG_LEN(sizeof(devnull));
cmsg->cmsg_level = SOL_SOCKET;
cmsg->cmsg_type = SCM_RIGHTS;
*((int*)CMSG_DATA(cmsg)) = devnull;
if (sendmsg(sock, &msg, 0)<0)
perror("child: sendmsg"), exit(1);
}
void
parent_proc(int sock)
{
struct msghdr msg;
int data;
const char padvalue[]="f00bar";
union {
struct {
char ancdata[2];
char padding[6];
} a;
char safety[CMSG_SPACE(sizeof(int))];
} ancs;
struct iovec iov[1];
/**/
if (sock != 3)
fprintf(stderr, "expected parent socket at fd 3, got %d\n", sock),
exit(1);
if (close(4)>=0 || errno != EBADF)
fprintf(stderr, "already got a descriptor at fd 4, why ?\n"),
exit(1);
msg.msg_name = NULL;
msg.msg_namelen = 0;
msg.msg_iov = iov;
msg.msg_iovlen = 1;
msg.msg_control = ancs.a.ancdata;
msg.msg_controllen = sizeof(ancs.a.ancdata);
msg.msg_flags = 0;
iov[0].iov_base = &data;
iov[0].iov_len = sizeof(data);
memset(&ancs, 0, sizeof(ancs));
memcpy(ancs.a.padding, padvalue, strlen(padvalue));
fprintf(stderr, "calling recvmsg with controllen = %d\n",
msg.msg_controllen);
if (recvmsg(sock, &msg, 0)<0)
perror("parent: recvmsg"), exit(1);
fprintf(stderr, "recvmsg returned with controllen = %d\n",
msg.msg_controllen);
if (memcmp(ancs.a.padding, padvalue, strlen(padvalue)) != 0)
fprintf(stderr, "recvmsg smashed the stack !\n");
if (close(4)>=0 || errno != EBADF)
fprintf(stderr, "fd 4 was passed, even it had no room to be put in !\n");
}
int
main(int argc, char *argv[])
{
int fds[2];
pid_t childpid;
/**/
if (socketpair(PF_UNIX, SOCK_DGRAM, 0, fds)<0)
perror("socketpair"), exit(1);
if ((childpid=fork())<0)
perror("fork"), exit(1);
if (childpid==0)
{
/* Child */
if (close(fds[0])<0)
perror("child: close"), exit(1);
child_proc(fds[1]);
}
else
{
/* Parent */
int status;
/**/
if (close(fds[1])<0)
perror("parent: close"), exit(1);
parent_proc(fds[0]);
if (waitpid(childpid, &status, 0)<0)
perror("parent: waitpid"), exit(1);
if (status != 0)
fprintf(stderr, "child exited with status %d\n", status), exit(1);
}
exit(0);
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly
2000-11-03 20:45 ` Philippe Troin
@ 2000-11-03 22:18 ` David S. Miller
2000-11-04 0:17 ` Philippe Troin
2000-11-04 0:38 ` David S. Miller
0 siblings, 2 replies; 10+ messages in thread
From: David S. Miller @ 2000-11-03 22:18 UTC (permalink / raw)
To: phil; +Cc: linux-kernel, alan
The real bug is in the setting of MSG_TRUNC (which is the only side
effect of your change). So the better fix is:
--- net/core/scm.c.~1~ Tue Jun 15 09:19:30 1999
+++ net/core/scm.c Fri Nov 3 14:18:06 2000
@@ -251,7 +251,7 @@
msg->msg_controllen -= cmlen;
}
}
- if (i < fdnum)
+ if (i < fdnum || (fdnum && fdmax <= 0))
msg->msg_flags |= MSG_CTRUNC;
/*
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly
2000-11-03 22:18 ` David S. Miller
@ 2000-11-04 0:17 ` Philippe Troin
2000-11-04 0:38 ` David S. Miller
1 sibling, 0 replies; 10+ messages in thread
From: Philippe Troin @ 2000-11-04 0:17 UTC (permalink / raw)
To: David S. Miller; +Cc: linux-kernel, alan
"David S. Miller" <davem@redhat.com> writes:
> The real bug is in the setting of MSG_TRUNC (which is the only side
> effect of your change). So the better fix is:
>
> --- net/core/scm.c.~1~ Tue Jun 15 09:19:30 1999
> +++ net/core/scm.c Fri Nov 3 14:18:06 2000
> @@ -251,7 +251,7 @@
> msg->msg_controllen -= cmlen;
> }
> }
> - if (i < fdnum)
> + if (i < fdnum || (fdnum && fdmax <= 0))
> msg->msg_flags |= MSG_CTRUNC;
>
> /*
Mmmh, no, if fdmax <= 0 (which happens when msg_controllen <
sizeof(struct cmsghdr)), then alls fds are passed, eventually
clobbering past ((char*)(msg_control)+m_controllen).
Run the little test case if you're not convinced...
I stand by my patch :-)
Phil.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly
2000-11-03 22:18 ` David S. Miller
2000-11-04 0:17 ` Philippe Troin
@ 2000-11-04 0:38 ` David S. Miller
2000-11-04 3:53 ` Philippe Troin
2000-11-04 4:51 ` David S. Miller
1 sibling, 2 replies; 10+ messages in thread
From: David S. Miller @ 2000-11-04 0:38 UTC (permalink / raw)
To: phil; +Cc: linux-kernel, alan
From: Philippe Troin <phil@fifi.org>
Date: 03 Nov 2000 16:17:53 -0800
Mmmh, no, if fdmax <= 0 (which happens when msg_controllen <
sizeof(struct cmsghdr)), then alls fds are passed, eventually
clobbering past ((char*)(msg_control)+m_controllen).
Run the little test case if you're not convinced...
I stand by my patch :-)
If fdmax <= 0, no iterations of the "for (i=0" loop will run.
'i' will therefore be left equal to zero. Therefore the next
bit of code writing in the SOL_SOCKET/SCM_RIGHTS/etc. values
will not run.
Next comes the test I modified, which will set MSG_CTRUNC.
Next scm_destroy(scm) is called which frees the list (this has to get
called and is why I say your patch wasn't correct).
So where in this code are all the fds passed to the user in this case?
I don't care what it actually does, I want to be shown why because as
far as I see it doesn't do what you say it does.
Later,
David S. Miller
davem@redhat.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly
2000-11-04 0:38 ` David S. Miller
@ 2000-11-04 3:53 ` Philippe Troin
2000-11-04 4:51 ` David S. Miller
1 sibling, 0 replies; 10+ messages in thread
From: Philippe Troin @ 2000-11-04 3:53 UTC (permalink / raw)
To: David S. Miller; +Cc: linux-kernel, alan
"David S. Miller" <davem@redhat.com> writes:
> From: Philippe Troin <phil@fifi.org>
> Date: 03 Nov 2000 16:17:53 -0800
>
> Mmmh, no, if fdmax <= 0 (which happens when msg_controllen <
> sizeof(struct cmsghdr)), then alls fds are passed, eventually
> clobbering past ((char*)(msg_control)+m_controllen).
>
> Run the little test case if you're not convinced...
> I stand by my patch :-)
>
> If fdmax <= 0, no iterations of the "for (i=0" loop will run.
> 'i' will therefore be left equal to zero. Therefore the next
> bit of code writing in the SOL_SOCKET/SCM_RIGHTS/etc. values
> will not run.
>
> Next comes the test I modified, which will set MSG_CTRUNC.
>
> Next scm_destroy(scm) is called which frees the list (this has to get
> called and is why I say your patch wasn't correct).
>
> So where in this code are all the fds passed to the user in this case?
> I don't care what it actually does, I want to be shown why because as
> far as I see it doesn't do what you say it does.
Well, you should have ran my little test case...
No really :-)
All your explanations make sense.
When I re-read the code in scm.c, I had trouble figuring out why it
did not work before my patch and why it worked after...
Here it is:
int fdmax = (msg->msg_controllen - sizeof(struct cmsghdr))/sizeof(int);
But, msg->msg_controllen is of type __kernel_size_t, which is unsigned
int (on i386).
Which means that if msg_controllen < sizeof(struct cmsghdr), then
fdmax is somewhere around 0x40000000, courtesy of the int->unsigned
int C promotion...
Ooops...
Yes I agree, mixing signed and unsigned arithmetic is evil... Doesn't
gcc have a flag for unsafe signed/unsigned mixtures ?
Would you consider this patch (or a variant) for inclusion ?
Phil.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly
2000-11-04 0:38 ` David S. Miller
2000-11-04 3:53 ` Philippe Troin
@ 2000-11-04 4:51 ` David S. Miller
2000-11-05 2:40 ` Philippe Troin
2000-11-06 3:32 ` David S. Miller
1 sibling, 2 replies; 10+ messages in thread
From: David S. Miller @ 2000-11-04 4:51 UTC (permalink / raw)
To: phil; +Cc: linux-kernel, alan
From: Philippe Troin <phil@fifi.org>
Date: 03 Nov 2000 19:53:04 -0800
Yes I agree, mixing signed and unsigned arithmetic is evil... Doesn't
gcc have a flag for unsafe signed/unsigned mixtures ?
Would you consider this patch (or a variant) for inclusion ?
I would accept a patch which made the code set fdmax <= 0 when
(msg->msg_controllen < (sizeof(struct cmsghdr) + sizeof(int)))
because it is the sole reason this bug exists at all.
Later,
David S. Miller
davem@redhat.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly
2000-11-04 4:51 ` David S. Miller
@ 2000-11-05 2:40 ` Philippe Troin
2000-11-06 3:32 ` David S. Miller
1 sibling, 0 replies; 10+ messages in thread
From: Philippe Troin @ 2000-11-05 2:40 UTC (permalink / raw)
To: David S. Miller; +Cc: linux-kernel, alan
[-- Attachment #1: Type: text/plain, Size: 550 bytes --]
"David S. Miller" <davem@redhat.com> writes:
> From: Philippe Troin <phil@fifi.org>
> Date: 03 Nov 2000 19:53:04 -0800
>
> Yes I agree, mixing signed and unsigned arithmetic is evil... Doesn't
> gcc have a flag for unsafe signed/unsigned mixtures ?
>
> Would you consider this patch (or a variant) for inclusion ?
>
> I would accept a patch which made the code set fdmax <= 0 when
> (msg->msg_controllen < (sizeof(struct cmsghdr) + sizeof(int)))
> because it is the sole reason this bug exists at all.
How about this one ?
Phil.
[-- Attachment #2: linux-2.2.17-scmrights.patch --]
[-- Type: application/octet-stream, Size: 800 bytes --]
diff -ruN linux.orig/net/core/scm.c linux/net/core/scm.c
--- linux.orig/net/core/scm.c Thu Apr 22 19:45:19 1999
+++ linux/net/core/scm.c Sat Nov 4 17:19:34 2000
@@ -210,12 +210,16 @@
{
struct cmsghdr *cm = (struct cmsghdr*)msg->msg_control;
- int fdmax = (msg->msg_controllen - sizeof(struct cmsghdr))/sizeof(int);
+ int fdmax = 0;
int fdnum = scm->fp->count;
struct file **fp = scm->fp->fp;
int *cmfptr;
int err = 0, i;
+ if (msg->msg_controllen > sizeof(struct cmsghdr))
+ fdmax = (msg->msg_controllen - sizeof(struct cmsghdr))
+ /sizeof(int);
+
if (fdnum < fdmax)
fdmax = fdnum;
@@ -251,7 +255,7 @@
msg->msg_controllen -= cmlen;
}
}
- if (i < fdnum)
+ if (i < fdnum || (fdnum && fdmax == 0))
msg->msg_flags |= MSG_CTRUNC;
/*
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly
2000-11-04 4:51 ` David S. Miller
2000-11-05 2:40 ` Philippe Troin
@ 2000-11-06 3:32 ` David S. Miller
2000-11-06 3:57 ` David S. Miller
1 sibling, 1 reply; 10+ messages in thread
From: David S. Miller @ 2000-11-06 3:32 UTC (permalink / raw)
To: phil; +Cc: linux-kernel, alan
From: Philippe Troin <phil@fifi.org>
Date: 04 Nov 2000 18:40:30 -0800
How about this one ?
Yep, this works, applied.
Please send plain-ASCII patches in the future though :-(
Later,
David S. Miller
davem@redhat.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly
2000-11-06 3:32 ` David S. Miller
@ 2000-11-06 3:57 ` David S. Miller
0 siblings, 0 replies; 10+ messages in thread
From: David S. Miller @ 2000-11-06 3:57 UTC (permalink / raw)
To: phil; +Cc: linux-kernel, alan
From: Philippe Troin <phil@fifi.org>
Date: 05 Nov 2000 20:07:07 -0800
"David S. Miller" <davem@redhat.com> writes:
> Yep, this works, applied.
I assume this will go in 2.2.18 right ? Alan ?
It has been sent to Alan already for inclusion.
Did 2.4.x exhibit the same problem ?
Yes, patch queued for when Linus makes a the next pre-patch.
Later,
David S. Miller
davem@redhat.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly
2000-11-06 3:32 ` David S. Miller
@ 2000-11-06 4:07 Philippe Troin
2000-11-03 20:45 ` Philippe Troin
0 siblings, 1 reply; 10+ messages in thread
From: Philippe Troin @ 2000-11-06 4:07 UTC (permalink / raw)
To: David S. Miller; +Cc: linux-kernel, alan
"David S. Miller" <davem@redhat.com> writes:
> Yep, this works, applied.
I assume this will go in 2.2.18 right ? Alan ?
Did 2.4.x exhibit the same problem ?
> Please send plain-ASCII patches in the future though :-(
Sorry, I'll remember that...
Phil.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2000-11-06 4:12 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2000-11-06 4:07 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly Philippe Troin
2000-11-03 20:45 ` Philippe Troin
2000-11-03 22:18 ` David S. Miller
2000-11-04 0:17 ` Philippe Troin
2000-11-04 0:38 ` David S. Miller
2000-11-04 3:53 ` Philippe Troin
2000-11-04 4:51 ` David S. Miller
2000-11-05 2:40 ` Philippe Troin
2000-11-06 3:32 ` David S. Miller
2000-11-06 3:57 ` David S. Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).