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 <assert.h> #include <errno.h> #include <stdlib.h> #include <string.h> @@ -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) 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 +190,8 @@ realpath(const char *path, char *resolved) 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;
