Request for Leak

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Request for Leak

Christian Biere
Hi,

I've submitted the following bug report against NetBSD a few months
ago. I've glance at OpenBSD's source and I believe it suffers from the
same bug.  Otherwise sorry for wasting your time. Maybe someone could
verify this using the test case provided with the report.

I believe I also found a fix for NetBSD (see below). If OpenBSD is
really affected, I think the equivalent fix can be applied i.e., in
the PRU_SEND case, unp_dispose(control) should be called if an error
occurs to undo the effect of unp_internalize().

----- Forwarded message from Christian Biere <[hidden email]> -----

>Submitter-Id: net
>Originator: Christian Biere
>Confidential: no
>Synopsis: SCM_RIGHTS can leak file descriptor resources
>Severity: serious
>Priority: medium
>Category: kern
>Class: sw-bug
>Release: NetBSD 3.99.15
>Environment:
System: NetBSD cyclonus 3.99.15 NetBSD 3.99.15 (STARSCREAM) #1: Sun Jan 22 18:59:43 CET 2006 bin@cyclonus:/o/NetBSD/obj/sys/arch/i386/compile/STARSCREAM i386
Architecture: i386
Machine: i386
>Description:
When passing a file descriptor of a socket using SCM_RIGHTS over a
unix domain socket (i.e., AF_LOCAL, SOCK_DGRAM) to a non-existing
socket sendmsg() fails with errno = ENOENT. Even though the sent file
descriptor is unconditionally closed after sendmsg(), the associated
socket is never released, not even after terminating the sending
process and removing its unix domain socket. If this happens
repeatedly, the system limit for file descriptors gets hit. If the
system limit is sufficiently large this may also consume all available
UDP/TCP port numbers.  The non-released socket is not accounted
against the process limit.

>How-To-Repeat:

$ cat > grab.c <<EOF && cc -W -Wall -Wformat=2 -o grab grab.c && ./grab
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/un.h>
#include <netinet/in.h>
#include <errno.h>
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>
#include <stdio.h>
#include <getopt.h>
#include <fcntl.h>
#include <assert.h>

#define ARRAY_LEN(x) (sizeof (x) / sizeof (x)[0])

static int
set_socket_address(struct sockaddr_un *sun, const char *path)
{
  static const struct sockaddr_un zero_sun;

  assert(sun);
  assert(path);
 
  *sun = zero_sun;
  if (strlen(path) >= sizeof sun->sun_path) {
    fprintf(stderr, "sockpath is too long\n");
    return -1;
  }
  strncpy(sun->sun_path, path, sizeof sun->sun_path);
  sun->sun_len = SUN_LEN(sun);
  return 0;
}

static int
clear_old_socket(const char *path)
{
  struct stat sb;

  if (0 == lstat(path, &sb)) {
    if (S_IFSOCK != (sb.st_mode & S_IFMT)) {
      fprintf(stderr, "existing \"%s\" is not a socket\n", path);
      return -1;
    }
    if (getuid() != sb.st_uid) {
      fprintf(stderr, "existing \"%s\" has different owner\n", path);
      return -1;
    }
    if (0 != unlink(path)) {
      perror("unlink()");
      /* Ignore */
    }
  } else if (ENOENT != errno)  {
    perror("lstat()");
    return -1;
  }

  return 0;
}

static int
create_new_socket(const char *path)
{
  int fd;

  fd = socket(PF_LOCAL, SOCK_DGRAM, 0);
  if (-1 == fd) {
    perror("socket(PF_LOCAL, SOCK_DGRAM, 0)");
    return -1;
  }

  {
    int enable = 1;
   
    if (0 != setsockopt(fd, 0, LOCAL_CREDS, &enable, sizeof enable)) {
      perror("setsockopt(fd, 0, LOCAL_CREDS, enable, sizeof enable)");
      return -1;
    }
  }

  {
    struct sockaddr_un sun;

    if (0 != set_socket_address(&sun, path))
      return -1;

    if (0 != bind(fd, (void *) &sun, sizeof sun)) {
      perror("bind()");
      return -1;
    }
  }

  return fd;
}

static int
send_msg(const int fd, const char * const dst_path,
  const struct msghdr * const msg_ptr)
{
  struct msghdr msg;
  struct sockaddr_un sun;

  assert(-1 != fd);
  assert(dst_path);
  assert(msg_ptr);
 
  if (set_socket_address(&sun, dst_path))
    return -1;

  msg = *msg_ptr;
  msg.msg_name = &sun;
  msg.msg_namelen = sizeof sun;
 
  if ((ssize_t) -1 == sendmsg(fd, &msg, 0)) {
    perror("sendmsg()");
    return -1;
  }

  return 0;
}

static int
send_descriptors(const int fd, const char * const dst_path,
  const int * const fd_array, const size_t num_fds)
{
  static const struct cmsghdr zero_cmsg;
  static const struct msghdr zero_msg;
  static struct iovec iov[1];
  struct msghdr msg;
  struct cmsghdr *cmsg;
  size_t data_size;

  assert(-1 != fd);
  assert(dst_path);
  assert(fd_array);

  data_size = num_fds * sizeof fd_array[0];

  cmsg = malloc(CMSG_SPACE(data_size));
  if (!cmsg) {
    perror("malloc()");
    return -1;
  }

  *cmsg = zero_cmsg;
  cmsg->cmsg_len = CMSG_LEN(data_size);
  cmsg->cmsg_level = SOL_SOCKET;
  cmsg->cmsg_type = SCM_RIGHTS;

  memcpy((char *) cmsg + CMSG_LEN(0), fd_array, data_size);

  msg = zero_msg;
  msg.msg_iov = iov;
  msg.msg_iovlen = ARRAY_LEN(iov);
  msg.msg_control = cmsg;
  msg.msg_controllen = CMSG_LEN(data_size);

  return send_msg(fd, dst_path, &msg);
}

static int
send_socket(const int master_fd, const char * const dst_path, in_port_t port)
{
  static const struct sockaddr_in zero_sin;
  struct sockaddr_in sin;
  int fd;

  sin = zero_sin;
  sin.sin_family = AF_INET;
  sin.sin_addr.s_addr = htonl(INADDR_ANY);
  sin.sin_port = htons(port);

  fd = socket(AF_INET, SOCK_DGRAM, 0);
  if (-1 == fd) {
    perror("socket(AF_INET, SOCK_DGRAM, 0)");
    return -1;
  }

  if (0 != bind(fd, (void *) &sin, sizeof sin)) {
    perror("handle_request: bind()");
    close(fd);
    return -1;
  }

  send_descriptors(master_fd, dst_path, &fd, 1);
  close(fd);

  return 0;
}

static int
run_server(const char *path)
{
  int fd;
  unsigned port;

  if (0 != clear_old_socket(path))
    return -1;
 
  fd = create_new_socket(path);
  if (-1 == fd)
    return -1;
 
  for (port = 1024; port < 65536; port++)
    send_socket(fd, "no such socket", port);

  if (0 != clear_old_socket(path))
    return -1;

  return 0;
}

static const char server_path[] = "/tmp/server";

int
main(void)
{
  if (0 != run_server(server_path))
    exit(EXIT_FAILURE);

  return 0;
}

/* vi: set ai et ts=2 sts=2 sw=2 cindent: */
EOF

>Fix:

----- End forwarded message -----

----- Forwarded message from Christian Biere <[hidden email]> -----

Subject: Re: kern/32842: SCM_RIGHTS can leak file descriptor resources

Christian Biere wrote:
> >Synopsis: SCM_RIGHTS can leak file descriptor resources
> When passing a file descriptor of a socket using SCM_RIGHTS over a
> unix domain socket (i.e., AF_LOCAL, SOCK_DGRAM) to a non-existing
> socket sendmsg() fails with errno = ENOENT. Even though the sent file
> descriptor is unconditionally closed after sendmsg(), the associated
> socket is never released, not even after terminating the sending
> process and removing its unix domain socket.

The following patch seems to fix this leak. I found the hint
leading to this patch in an archived FreeBSD mailing list
discussion from 1999:

http://groups.google.com/group/muc.lists.freebsd.security/browse_thread/thread/6273768d4d8bfc6c/00db9d3ac832084b?lnk=st&q=&rnum=1

The patch might be incomplete but I cannot reproduce the leak
using my provided test case with this patch applied anymore.

Index: uipc_usrreq.c
===================================================================
RCS file: /cvsroot/src/sys/kern/uipc_usrreq.c,v
retrieving revision 1.92
diff -u -r1.92 uipc_usrreq.c
--- uipc_usrreq.c 23 Jul 2006 22:06:12 -0000 1.92
+++ uipc_usrreq.c 27 Jul 2006 00:22:41 -0000
@@ -326,6 +326,7 @@
  error = unp_connect(so, nam, l);
  if (error) {
  die:
+ unp_dispose(control);
  m_freem(control);
  m_freem(m);
  break;


--
Christian

Reply | Threaded
Open this post in threaded view
|

Re: Request for Leak

Markus Friedl-2
On Thu, Jul 27, 2006 at 04:37:26AM +0200, Christian Biere wrote:

> Hi,
>
> I've submitted the following bug report against NetBSD a few months
> ago. I've glance at OpenBSD's source and I believe it suffers from the
> same bug.  Otherwise sorry for wasting your time. Maybe someone could
> verify this using the test case provided with the report.
>
> I believe I also found a fix for NetBSD (see below). If OpenBSD is
> really affected, I think the equivalent fix can be applied i.e., in
> the PRU_SEND case, unp_dispose(control) should be called if an error
> occurs to undo the effect of unp_internalize().

i think this is necessary. could you please test this?


Index: uipc_usrreq.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v
retrieving revision 1.31
diff -u -p -u -r1.31 uipc_usrreq.c
--- uipc_usrreq.c 27 Feb 2006 23:38:11 -0000 1.31
+++ uipc_usrreq.c 27 Jul 2006 12:21:49 -0000
@@ -240,6 +240,9 @@ uipc_usrreq(struct socket *so, int req,
  default:
  panic("uipc 4");
  }
+ /* we need to undo unp_internalize in case of errors */
+ if (control && error)
+ unp_dispose(control);
  break;
 
  case PRU_ABORT:

Reply | Threaded
Open this post in threaded view
|

Re: Request for Leak

Christian Biere
Markus Friedl wrote:
> On Thu, Jul 27, 2006 at 04:37:26AM +0200, Christian Biere wrote:
> > I've submitted the following bug report against NetBSD a few months
> > ago. I've glance at OpenBSD's source and I believe it suffers from the
> > same bug.  Otherwise sorry for wasting your time. Maybe someone could
> > verify this using the test case provided with the report.

> i think this is necessary. could you please test this?
[...]

I didn't mention it explicitely but I myself don't use OpenBSD. So,
please, someone else test this. Thanks.

--
Christian

Reply | Threaded
Open this post in threaded view
|

Re: Request for Leak

Christian Biere
In reply to this post by Markus Friedl-2
Markus Friedl wrote:
> On Thu, Jul 27, 2006 at 04:37:26AM +0200, Christian Biere wrote:
> > I've submitted the following bug report against NetBSD a few months
> > ago. I've glance at OpenBSD's source and I believe it suffers from the
> > same bug.  Otherwise sorry for wasting your time. Maybe someone could
> > verify this using the test case provided with the report.

> > I believe I also found a fix for NetBSD (see below). If OpenBSD is
> > really affected, I think the equivalent fix can be applied i.e., in
> > the PRU_SEND case, unp_dispose(control) should be called if an error
> > occurs to undo the effect of unp_internalize().

> i think this is necessary. could you please test this?
[patch with proposed fix]

I've noticed that this patch hasn't been applied to CVS yet. Also,
there were no further comments regarding this potential issue. So,
is this bug in OpenBSD or not? I believe it's not very useful
if I submit a bug report as I'm not an OpenBSD user. If this is
the wrong place to ask, where's the right one?

--
Christian