From owner-svn-src-head@freebsd.org Mon May 15 17:14:54 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id D27FBD6ECC5; Mon, 15 May 2017 17:14:54 +0000 (UTC) (envelope-from kib@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id ADDCB10A6; Mon, 15 May 2017 17:14:54 +0000 (UTC) (envelope-from kib@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id v4FHErT0025525; Mon, 15 May 2017 17:14:53 GMT (envelope-from kib@FreeBSD.org) Received: (from kib@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id v4FHEr2m025524; Mon, 15 May 2017 17:14:53 GMT (envelope-from kib@FreeBSD.org) Message-Id: <201705151714.v4FHEr2m025524@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: kib set sender to kib@FreeBSD.org using -f From: Konstantin Belousov Date: Mon, 15 May 2017 17:14:53 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r318298 - head/lib/libc/stdlib X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 May 2017 17:14:54 -0000 Author: kib Date: Mon May 15 17:14:53 2017 New Revision: 318298 URL: https://svnweb.freebsd.org/changeset/base/318298 Log: Fix several buffer overflows in realpath(3). - The statement "left_len -= s - left;" does not take the slash into account if one was found. This results in the invariant "left[left_len] == '\0'" being violated (and possible buffer overflows). The patch replaces the variable "s" with a size_t "next_token_len" for more clarity. - "slen" from readlink(2) can be 0 when encountering empty symlinks. Then, further down, "symlink[slen - 1]" underflows the buffer. When slen == 0, realpath(3) should probably return ENOENT (http://austingroupbugs.net/view.php?id=825, https://lwn.net/Articles/551224/). Some other minor issues: - The condition "resolved_len >= PATH_MAX" cannot be true. - Similarly, "s - left >= sizeof(next_token)" cannot be true, as long as "sizeof(next_token) >= sizeof(left)". - Return ENAMETOOLONG when a resolved symlink from readlink(2) is too long for the symlink buffer (instead of just truncating it). - "resolved_len > 1" below the call to readlink(2) is always true as "strlcat(resolved, next_token, PATH_MAX);" always results in a string of length > 1. Also, "resolved[resolved_len - 1] = '\0';" is not needed; there can never be a trailing slash here. - The truncation check for "strlcat(symlink, left, sizeof(symlink));" should be against "sizeof(symlink)" (the third argument to strlcat) instead of "sizeof(left)". Submitted by: Jan Kokemц╪ller PR: 219154 MFC after: 2 weeks Modified: head/lib/libc/stdlib/realpath.c Modified: head/lib/libc/stdlib/realpath.c ============================================================================== --- head/lib/libc/stdlib/realpath.c Mon May 15 16:53:02 2017 (r318297) +++ head/lib/libc/stdlib/realpath.c Mon May 15 17:14:53 2017 (r318298) @@ -51,10 +51,11 @@ char * realpath(const char * __restrict path, char * __restrict resolved) { struct stat sb; - char *p, *q, *s; - size_t left_len, resolved_len; + char *p, *q; + size_t left_len, resolved_len, next_token_len; unsigned symlinks; - int m, slen; + int m; + ssize_t slen; char left[PATH_MAX], next_token[PATH_MAX], symlink[PATH_MAX]; if (path == NULL) { @@ -109,18 +110,19 @@ realpath(const char * __restrict path, c * and its length. */ p = strchr(left, '/'); - s = p ? p : left + left_len; - if (s - left >= sizeof(next_token)) { - if (m) - free(resolved); - errno = ENAMETOOLONG; - return (NULL); + + next_token_len = p ? p - left : left_len; + memcpy(next_token, left, next_token_len); + next_token[next_token_len] = '\0'; + + if (p != NULL) { + left_len -= next_token_len + 1; + memmove(left, p + 1, left_len + 1); + } else { + left[0] = '\0'; + left_len = 0; } - memcpy(next_token, left, s - left); - next_token[s - left] = '\0'; - left_len -= s - left; - if (p != NULL) - memmove(left, s + 1, left_len + 1); + if (resolved[resolved_len - 1] != '/') { if (resolved_len + 1 >= PATH_MAX) { if (m) @@ -173,19 +175,25 @@ realpath(const char * __restrict path, c errno = ELOOP; return (NULL); } - slen = readlink(resolved, symlink, sizeof(symlink) - 1); - if (slen < 0) { + slen = readlink(resolved, symlink, sizeof(symlink)); + if (slen <= 0 || slen >= sizeof(symlink)) { if (m) free(resolved); + if (slen < 0) { + /* keep errno from readlink(2) call */ + } else if (slen == 0) { + errno = ENOENT; + } else { + errno = ENAMETOOLONG; + } return (NULL); } symlink[slen] = '\0'; if (symlink[0] == '/') { resolved[1] = 0; resolved_len = 1; - } else if (resolved_len > 1) { + } else { /* Strip the last path component. */ - resolved[resolved_len - 1] = '\0'; q = strrchr(resolved, '/') + 1; *q = '\0'; resolved_len = q - resolved; @@ -209,7 +217,7 @@ realpath(const char * __restrict path, c } left_len = strlcat(symlink, left, sizeof(symlink)); - if (left_len >= sizeof(left)) { + if (left_len >= sizeof(symlink)) { if (m) free(resolved); errno = ENAMETOOLONG;