From patchwork Sun Dec 3 23:09:34 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Sebor X-Patchwork-Id: 24701 Received: (qmail 85264 invoked by alias); 3 Dec 2017 23:09:43 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 85255 invoked by uid 89); 3 Dec 2017 23:09:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.0 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=2503, prog X-HELO: mail-ot0-f175.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:message-id:date:user-agent :mime-version; bh=9DtlDKCkE+olTR8FdeizmODAPwcp7LY+cvGg3m59W8Y=; b=oj+dyQME+x3roNPufEngIUquBKTvRWk8tS0NbllnSnxhj1euOdRauW+HWP4F4FYGPb C9BHGboO39DC7N++TnR1tsxRb/raSumOpiuszcJenqUHc2Yg7EAm2nJiYAb1PMNseWly hOeHGmzUckrMLRWSwY2HlrFlHd7SEZMEeL3tW2trL+iLxekFT3MNIhmSEhRyzOCYDjfK 9js04Bj+OA1EjFmITBNLdXXjc2KdUBugzqPWH2gUnzOsojm2krBlcCFgbarIUZdFYSKp bS+3AIyYjItGZPCnvlff7EhlTM00jH5t4VJkZ2Wba+Zv2tIhfBDhzCVAHMKk82MTtbvJ LT8A== X-Gm-Message-State: AJaThX74LQu1G+HHMe+WuBXTTL87d3IZKo+eNGBrmnCHpX2IL3DH7qWx /NmERfsI/kEVOri8t59iGzwjpA== X-Google-Smtp-Source: AGs4zMYmwFTcFyYvHM8fyxkWRnVzrvRmoDd8M/z9oIheRmVQjSaSbvqN/0fqe1HueTvPMUHN/xpzRw== X-Received: by 10.157.43.244 with SMTP id u107mr14365749ota.121.1512342578578; Sun, 03 Dec 2017 15:09:38 -0800 (PST) From: Martin Sebor Subject: [PATCH] avoid buffer overflow in sunrpc clnt_create (BZ #22542) To: Carlos O'Donell , GNU C Library Message-ID: Date: Sun, 3 Dec 2017 16:09:34 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 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 [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 + . */ + +#include +#include +#include +#include +#include + + +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