Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly

Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly

[Philippe Troin]

"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/

2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly

[Philippe Troin]

[-- 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);
}

Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly

[David S. Miller]

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/

Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly

[Philippe Troin]

"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/

Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly

[David S. Miller]

   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/

Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly

[Philippe Troin]

"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/

Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly

[David S. Miller]

   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/

Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly

[Philippe Troin]

[-- 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;
 
 	/*

Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly

[David S. Miller]

   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/

Re: 2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly

[David S. Miller]

   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/