Created attachment 182421 [details] fixes for realpath(3) There are some buffer overflows in realpath(3). The attached patch fixes them: - 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)".
I found those bugs with LLVM libFuzzer [1]. The fuzz target here [2] should generate some interesting inputs fairly quickly (when using on the unpatched realpath(3)). This fuzzer results in 100% code coverage according to clangs coverage report. [1]: http://llvm.org/docs/LibFuzzer.html [2]: https://github.com/jiixyj/realpath-fuzzer
Overall this looks fine. Could you add the tests to our test suite for (most of all) situations you found ?
I've added some tests to a local copy of 'contrib/netbsd-tests/lib/libc/gen/t_realpath.c' [1]. Would a new file in 'lib/libc/tests/gen' be a better place? realpath_buffer_overflow: This tests for the buffer overflow of the "left" array. But this test only fails with ASAN enabled ("-fsanitize=address"). realpath_empty_symlink: This tests for empty symlink behavior. Currently, "/tmp/empty_symlink/aaa" resolves to "/tmp/aaa", but the right behavior is to fail and return ENOENT. When ASAN is enabled this test crashes. [1]: https://github.com/jiixyj/realpath-fuzzer/commit/eb1c51adbdc2fe56b216f22b8f46cffd6a032c67
(In reply to Jan Kokemüller from comment #3) Yes, the new test file is better, we prefer to not modify third-party code to ease future imports.
A commit references this bug: Author: kib Date: Mon May 15 17:14:53 UTC 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 <jan.kokemueller@gmail.com> PR: 219154 MFC after: 2 weeks Changes: head/lib/libc/stdlib/realpath.c
(In reply to commit-hook from comment #5) I committed the patch almost as is, the bits I omitted are asserts. Generally, library must not kill the application. If you consider it is more appropriate, checks might be made into function errors, but I do not see much point. Still waiting for the tests.
Created attachment 182684 [details] More tests for realpath(3) Here is a patch that adds 'lib/libc/tests/gen/realpath2_test.c'. The first test triggers the out of bounds read of the 'left' array. It only fails when realpath.c is compiled with '-fsanitize=address' so I'm not sure how useful this test is. I didn't manage to read more than one byte beyond the buffer or trigger some visible faulty behavior. The other test checks for ENOENT when running into an empty symlink. This matches NetBSD's realpath(3) semantics. Previously, empty symlinks were treated like ".".
A commit references this bug: Author: kib Date: Thu May 18 13:49:53 UTC 2017 New revision: 318450 URL: https://svnweb.freebsd.org/changeset/base/318450 Log: Add tests for some cases in r318298. The first test triggers the out of bounds read of the 'left' array. It only fails when realpath.c is compiled with '-fsanitize=address'. The other test checks for ENOENT when running into an empty symlink. This matches NetBSD's realpath(3) semantics. Previously, empty symlinks were treated like ".". Submitted by: Jan Kokem??ller <jan.kokemueller@gmail.com> PR: 219154 MFC after: 2 weeks Changes: head/lib/libc/tests/gen/Makefile head/lib/libc/tests/gen/realpath2_test.c
A commit references this bug: Author: kib Date: Mon May 29 12:52:13 UTC 2017 New revision: 319126 URL: https://svnweb.freebsd.org/changeset/base/319126 Log: MFC r318298: Fix several buffer overflows in realpath(3), and other minor issues. PR: 219154 Changes: _U stable/11/ stable/11/lib/libc/stdlib/realpath.c
A commit references this bug: Author: kib Date: Mon May 29 12:58:31 UTC 2017 New revision: 319129 URL: https://svnweb.freebsd.org/changeset/base/319129 Log: MFC r318298: Fix several buffer overflows in realpath(3), and other minor issues. PR: 219154 Changes: _U stable/10/ stable/10/lib/libc/stdlib/realpath.c
Assign to committer resolving
Track branches MFC'd to
A commit references this bug: Author: kib Date: Thu Jun 1 13:20:47 UTC 2017 New revision: 319418 URL: https://svnweb.freebsd.org/changeset/base/319418 Log: MFC r318450: Add tests for some cases in r318298. PR: 219154 Changes: _U stable/11/ stable/11/lib/libc/tests/gen/Makefile stable/11/lib/libc/tests/gen/realpath2_test.c
A commit references this bug: Author: kib Date: Thu Jun 1 13:22:05 UTC 2017 New revision: 319419 URL: https://svnweb.freebsd.org/changeset/base/319419 Log: MFC r318450: Add tests for some cases in r318298. PR: 219154 Changes: _U stable/10/ stable/10/lib/libc/tests/gen/Makefile stable/10/lib/libc/tests/gen/realpath2_test.c
MFCed Thu Jun 1 13:22:05 UTC 2017.
MARKED AS SPAM