Re: realpath(3): some buffer overflows and conformance issues
Here's a diff for libc and ld.so without the asserts. - todd Index: lib/libc/stdlib/realpath.c === RCS file: /cvs/src/lib/libc/stdlib/realpath.c,v retrieving revision 1.21 diff -u -p -u -r1.21 realpath.c --- lib/libc/stdlib/realpath.c 28 Aug 2016 04:08:59 - 1.21 +++ lib/libc/stdlib/realpath.c 22 Dec 2017 22:13:24 - @@ -45,12 +45,19 @@ char * realpath(const char *path, char *resolved) { - char *p, *q, *s; - size_t left_len, resolved_len; + const char *p; + char *q; + size_t left_len, resolved_len, next_token_len; unsigned symlinks; - int serrno, slen, mem_allocated; + int serrno, mem_allocated; + ssize_t slen; char left[PATH_MAX], next_token[PATH_MAX], symlink[PATH_MAX]; + if (path == NULL) { + errno = EINVAL; + return (NULL); + } + if (path[0] == '\0') { errno = ENOENT; return (NULL); @@ -85,7 +92,7 @@ realpath(const char *path, char *resolve resolved_len = strlen(resolved); left_len = strlcpy(left, path, sizeof(left)); } - if (left_len >= sizeof(left) || resolved_len >= PATH_MAX) { + if (left_len >= sizeof(left)) { errno = ENAMETOOLONG; goto err; } @@ -99,16 +106,19 @@ realpath(const char *path, char *resolve * and its length. */ p = strchr(left, '/'); - s = p ? p : left + left_len; - if (s - left >= sizeof(next_token)) { - errno = ENAMETOOLONG; - goto err; + + next_token_len = p ? (size_t) (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) { errno = ENAMETOOLONG; @@ -136,16 +146,17 @@ realpath(const char *path, char *resolve } /* -* Append the next path component and lstat() it. If -* lstat() fails we still can return successfully if -* there are no more path components left. +* Append the next path component and readlink() it. If +* readlink() fails we still can return successfully if +* it exists but isn't a symlink, or if there are no more +* path components left. */ resolved_len = strlcat(resolved, next_token, PATH_MAX); if (resolved_len >= PATH_MAX) { errno = ENAMETOOLONG; goto err; } - slen = readlink(resolved, symlink, sizeof(symlink) - 1); + slen = readlink(resolved, symlink, sizeof(symlink)); if (slen < 0) { switch (errno) { case EINVAL: @@ -160,6 +171,12 @@ realpath(const char *path, char *resolve default: goto err; } + } else if (slen == 0) { + errno = EINVAL; + goto err; + } else if (slen == sizeof(symlink)) { + errno = ENAMETOOLONG; + goto err; } else { if (symlinks++ > SYMLOOP_MAX) { errno = ELOOP; @@ -170,9 +187,8 @@ realpath(const char *path, char *resolve 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; Index: libexec/ld.so/dl_realpath.c === RCS file: /cvs/src/libexec/ld.so/dl_realpath.c,v retrieving revision 1.5 diff -u -p -u -r1.5 dl_realpath.c ---
Re: realpath(3): some buffer overflows and conformance issues
On Fri, 22 Dec 2017 12:08:58 +0100, =?UTF-8?Q?Jan_Kokem=c3=bcller?= wrote: > I've found some buffer overflows in realpath(3). They are limited to > just two bytes though (one after the 'left' buffer and one before > 'symlink'), so the impact is minimal. > > Similar bugs in FreeBSD: > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=219154 > > > Here is a list of issues: > > - 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/). > > - return ENOENT if path is NULL (POSIX requires this) > > - 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. > > > Here is a patch. I must admit that I haven't tested this on OpenBSD but > compiled the OpenBSD version on FreeBSD. I have left in some asserts to > describe loop invariants. Those can be removed I guess. I've just > noticed the realpath copy in 'libexec/ld.so/dl_realpath.c' that should > be fixed similarly. The patch looks correct to me. We can either remove the asserts() or simply define NDEBUG before including assert.h. Anyone else want to OK this? - todd
realpath(3): some buffer overflows and conformance issues
Hi, I've found some buffer overflows in realpath(3). They are limited to just two bytes though (one after the 'left' buffer and one before 'symlink'), so the impact is minimal. Similar bugs in FreeBSD: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=219154 Here is a list of issues: - 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/). - return ENOENT if path is NULL (POSIX requires this) - 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. Here is a patch. I must admit that I haven't tested this on OpenBSD but compiled the OpenBSD version on FreeBSD. I have left in some asserts to describe loop invariants. Those can be removed I guess. I've just noticed the realpath copy in 'libexec/ld.so/dl_realpath.c' that should be fixed similarly. diff --git lib/libc/stdlib/realpath.c lib/libc/stdlib/realpath.c index c691252..31f7120 100644 --- lib/libc/stdlib/realpath.c +++ lib/libc/stdlib/realpath.c @@ -27,6 +27,7 @@ * SUCH DAMAGE. */ +#include #include #include #include @@ -45,12 +46,18 @@ char * realpath(const char *path, char *resolved) { - 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 serrno, slen, mem_allocated; + int serrno, mem_allocated; + ssize_t slen; char left[PATH_MAX], next_token[PATH_MAX], symlink[PATH_MAX]; + if (path == NULL) { + errno = EINVAL; + return (NULL); + } + if (path[0] == '\0') { errno = ENOENT; return (NULL); @@ -85,7 +92,7 @@ realpath(const char *path, char *resolved) resolved_len = strlen(resolved); left_len = strlcpy(left, path, sizeof(left)); } - if (left_len >= sizeof(left) || resolved_len >= PATH_MAX) { + if (left_len >= sizeof(left)) { errno = ENAMETOOLONG; goto err; } @@ -94,21 +101,28 @@ realpath(const char *path, char *resolved) * Iterate over path components in `left'. */ while (left_len != 0) { + assert(left[left_len] == '\0'); + /* * Extract the next path component and adjust `left' * and its length. */ p = strchr(left, '/'); - s = p ? p : left + left_len; - if (s - left >= sizeof(next_token)) { - errno = ENAMETOOLONG; - goto err; + + assert(sizeof(next_token) >= sizeof(left)); + + next_token_len = p ? (size_t) (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) { errno = ENAMETOOLONG; @@ -145,7 +159,7 @@ realpath(const char *path, char *resolved) errno = ENAMETOOLONG; goto err; } - slen = readlink(resolved, symlink, sizeof(symlink) - 1); + slen = readlink(resolved, symlink, sizeof(symlink)); if (slen < 0) { switch (errno) { case EINVAL: @@ -160,6 +174,12 @@ realpath(const char *path, char *resolved)