Patchwork avoid buffer overflow in sunrpc clnt_create (BZ #22542)

login
register
mail settings
Submitter Martin Sebor
Date Dec. 3, 2017, 11:09 p.m.
Message ID <f6a8c32f-f524-9ebb-03bc-4484f8a80a16@gmail.com>
Download mbox | patch
Permalink /patch/24701/
State New
Headers show

Comments

Martin Sebor - Dec. 3, 2017, 11:09 p.m.
Annotating sockaddr_un::sun_path with attribute nonstring as
suggested by Carlos in [1] triggers a warning for call to strlen()
with the array as an argument in the clntunix_create() function.

The only caller of the function in Glibc, clnt_create(), calls
strcpy() to copy the string passed to it as the hostname argument
to the sun_path array member of a local struct sockadd_un object.
This causes a buffer overflow when the string is longer than
the size of the sun_path member array.

The attached patch tries to fix both of these problems.

It annotates the sun_path array member of struct sockaddr_un
with attribute nonstring.

It then changes clntunix_create() to use strnlen() instead of
strlen() to read as most as many characters from the sun_path
array as it has elements.

Finally, the patch changes the clnt_create() function to reject
hostnames longer than sizeof (sun_path) to avoid the overflow.

I considered changing clnt_create() to dynamically allocate
a larger object than sizeof (struct sockaddr_un) to fit the
whole hostname but decided to keep it simple.  Longer names
aren't supported now (they cause a buffer overflow) and since
no one complained, it seems good enough.  However, if it is
considered important to handle longer hostnames, it can be
changed.

Given the approach I chose in clnt_create(), using strnlen()
in clntunix_create() isn't really necessary to avoid reading
past the end of sun_path because it's nul-terminated, but it
is necessary to avoid the warning.

If it's decided that clnt_create() should handle overlong
hostnames then clntunix_create() will need to change back
to use either strlen(sun_path) and the warning will need
to be suppressed by some other means, or to use memchr().

Thanks
Martin

[1] https://sourceware.org/ml/libc-alpha/2017-11/msg00932.html
Paul Eggert - Dec. 4, 2017, 1:36 a.m.
>  struct sockaddr_un
>    {
>      __SOCKADDR_COMMON (sun_);
> -    char sun_path[108];		/* Path name.  */
> +    char sun_path[108]
> +      __attribute_nonstring__;	/* Path name.  */
>    };

This says "sun_path uses strncpy format", but....

> +      if (strlen (hostname) >= sizeof sun.sun_path)
> +	{
> +	  struct rpc_createerr *ce = &get_rpc_createerr ();
> +	  ce->cf_stat = RPC_UNKNOWNHOST;
> +	  ce->cf_error.re_errno = EINVAL;
> +	  return NULL;
> +	}

... this says "sun_path uses ordinary string format", which isn't consistent.

I suggest that sun_path should use ordinary string format, since that's what 
people expect. In other words, do not add __attribute_nonstring__ or change 
clntunix_create, but instead just add the strlen check to clnt_create.

You might also consider using "__strnlen (hostname, sizeof sun.sun_path)" 
instead of "strlen (hostname)" to avoid bad asymptotic behavior if HOSTNAME is long.
Dmitry Levin - Dec. 4, 2017, 1:55 a.m.
On Sun, Dec 03, 2017 at 05:36:50PM -0800, Paul Eggert wrote:
[...]
> I suggest that sun_path should use ordinary string format, since that's what 
> people expect.

Do people really expect that?  Assuming that people are aware
of linux kernel behaviour, why would they expect that?
Paul Eggert - Dec. 4, 2017, 8:04 a.m.
Dmitry V. Levin wrote:
> Do people really expect that?  Assuming that people are aware
> of linux kernel behaviour, why would they expect that?

These days, it's because strncpy format is obsolete and is not something 
programmers are ordinarily aware of. When in doubt (which there seems to be 
here), glibc should use null-terminated strings rather than strncpy format.

Patch

2017-12-03  Martin Sebor  <msebor@redhat.com>

	[BZ #22542]
	* socket/sys/un.h (struct sockaddr_un): Declare sun_path attribute
	nonstring.
	* sunrpc/clnt_gen.c (clnt_create): Avoid buffer overflow.
	* sunrpc/clnt_unix.c (clntunix_create): Use strnlen instead of strlen.
	* sunrpc/tst-unix-name-too-long.c: New test.
	* sunrpc/Makefile (tests): Add tst-unix-name-too-long.

diff --git a/socket/sys/un.h b/socket/sys/un.h
index fc82f8c..f4e0715 100644
--- a/socket/sys/un.h
+++ b/socket/sys/un.h
@@ -29,7 +29,8 @@  __BEGIN_DECLS
 struct sockaddr_un
   {
     __SOCKADDR_COMMON (sun_);
-    char sun_path[108];		/* Path name.  */
+    char sun_path[108]
+      __attribute_nonstring__;	/* Path name.  */
   };
 
 
diff --git a/sunrpc/Makefile b/sunrpc/Makefile
index f1b8323..431634c 100644
--- a/sunrpc/Makefile
+++ b/sunrpc/Makefile
@@ -95,7 +95,7 @@  others += rpcgen
 endif
 
 tests = tst-xdrmem tst-xdrmem2 test-rpcent tst-udp-error tst-udp-timeout \
-  tst-udp-nonblocking
+  tst-udp-nonblocking tst-unix-name-too-long
 xtests := tst-getmyaddr
 
 ifeq ($(have-thread-library),yes)
@@ -166,6 +166,7 @@  $(objpfx)tst-xdrmem2: $(common-objpfx)linkobj/libc.so
 $(objpfx)tst-udp-error: $(common-objpfx)linkobj/libc.so
 $(objpfx)tst-svc_register: \
   $(common-objpfx)linkobj/libc.so $(shared-thread-library)
+$(objpfx)tst-unix-name-too-long: $(common-objpfx)linkobj/libc.so
 
 $(objpfx)rpcgen: $(addprefix $(objpfx),$(rpcgen-objs))
 
@@ -250,3 +251,4 @@  $(objpfx)tst-udp-timeout: $(common-objpfx)linkobj/libc.so
 $(objpfx)tst-udp-nonblocking: $(common-objpfx)linkobj/libc.so
 $(objpfx)tst-udp-garbage: \
   $(common-objpfx)linkobj/libc.so $(shared-thread-library)
+$(objpfx)/tst-unix-name-too-long: $(common-objpfx)linkobj/libc.so
diff --git a/sunrpc/clnt_gen.c b/sunrpc/clnt_gen.c
index 13ced89..8d354ec 100644
--- a/sunrpc/clnt_gen.c
+++ b/sunrpc/clnt_gen.c
@@ -59,6 +59,14 @@  clnt_create (const char *hostname, u_long prog, u_long vers,
     {
       memset ((char *)&sun, 0, sizeof (sun));
       sun.sun_family = AF_UNIX;
+      if (strlen (hostname) >= sizeof sun.sun_path)
+	{
+	  struct rpc_createerr *ce = &get_rpc_createerr ();
+	  ce->cf_stat = RPC_UNKNOWNHOST;
+	  ce->cf_error.re_errno = EINVAL;
+	  return NULL;
+	}
+
       strcpy (sun.sun_path, hostname);
       sock = RPC_ANYSOCK;
       client = clntunix_create (&sun, prog, vers, &sock, 0, 0);
diff --git a/sunrpc/clnt_unix.c b/sunrpc/clnt_unix.c
index 33a02cc..9bb24c5 100644
--- a/sunrpc/clnt_unix.c
+++ b/sunrpc/clnt_unix.c
@@ -134,7 +134,8 @@  clntunix_create (struct sockaddr_un *raddr, u_long prog, u_long vers,
   if (*sockp < 0)
     {
       *sockp = __socket (AF_UNIX, SOCK_STREAM, 0);
-      len = strlen (raddr->sun_path) + sizeof (raddr->sun_family) + 1;
+      len = (__strnlen (raddr->sun_path, sizeof (raddr->sun_path))
+	     + sizeof (raddr->sun_family) + 1);
       if (*sockp < 0
 	  || __connect (*sockp, (struct sockaddr *) raddr, len) < 0)
 	{
diff --git a/sunrpc/tst-unix-name-too-long.c b/sunrpc/tst-unix-name-too-long.c
new file mode 100644
index 0000000..82df7ef
--- /dev/null
+++ b/sunrpc/tst-unix-name-too-long.c
@@ -0,0 +1,44 @@ 
+/* Test to verify that overlong hostname is rejected by clnt_create()
+   and doesn't cause a buffer overflow (bug  22542).
+
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <rpc/clnt.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+#include <string.h>
+
+
+static int
+do_test (void)
+{
+  /* Create an arbitrary hostname that's longer than fits in sun_path.  */
+  char name [sizeof ((struct sockaddr_un*)0)->sun_path * 2];
+  memset (name, 'x', sizeof name - 1);
+  name [sizeof name - 1] = '\0';
+
+  CLIENT *clnt = clnt_create (name, 0, 0, "unix");
+
+  if (clnt)
+    clnt_destroy (clnt);
+
+  return clnt == 0 && errno == EINVAL;
+}
+
+#include <support/test-driver.c>