[PATCH] avoid buffer overflow in sunrpc clnt_create (BZ #22542)

[Martin Sebor <msebor at gmail dot com>]

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

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>