Skip to content

Commit 681c27c

Browse files
cypharcyphar
committed
syscall: remove unneeded path_has_dotdot_component checks
This code was added in commit 30656c5 ("syscall: add symlink-race-safe do_*_at() wrappers and harden secure_relative_open") but the justification given is really not particularly convincing. The stated justification is that RESOLVE_BENEATH does not block some ".." escapes, which is categorically false (if it were true, that would be a Linux kernel bug and the kernel actually has an extra safety check at the end of lookup that would render this kind of breakout practically impossible). Reading the code, the actual reason appears to be to provide some consistency between the RESOLVE_BENEATH and fallback per-component O_NOFOLLOW lookups. However, the O_NOFOLLOW resolver can never have full parity with the RESOLVE_BENEATH resolvers because RESOLVE_BENEATH permit symlinks and symlinks can contain ".." components. It seems more prudent to just allow ".." for modern systems and reject it for older ones. If truly necessary, there are even ways to support ".." in a cross-platform way so this could even be seen as a "not-implemented-yet" error rather than a permanent API contract. For the really-old systems case (with no O_NOFOLLOW or AT_DIRFD) then just as we have to accept symlinks (which might contain "..") we might as well accept bare ".." components. Rejecting ".." for that case is just pure security theatre. Fixes: 30656c5 ("syscall: add symlink-race-safe do_*_at() wrappers and harden secure_relative_open") Signed-off-by: Aleksa Sarai <aleksa@amutable.com>
1 parent a2e5edb commit 681c27c

2 files changed

Lines changed: 110 additions & 117 deletions

File tree

‎syscall.c‎

Lines changed: 26 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -1677,34 +1677,8 @@ int do_open_nofollow(const char *pathname, int flags)
16771677
flag name, picked up by the same #ifdef;
16781678
flag value differs from FreeBSD)
16791679
Other systems fall back to the per-component O_NOFOLLOW walk below.
1680-
1681-
The relpath must also not contain any ../ elements in the path.
16821680
*/
16831681

1684-
/* Returns 1 if path has any "/"-separated component that is exactly
1685-
* "..", 0 otherwise. Used by secure_relative_open's front-door
1686-
* validation to reject inputs that the per-component walk fallback
1687-
* would otherwise resolve through ".." -- e.g. bare "..", "foo/..",
1688-
* "subdir/.." -- which RESOLVE_BENEATH-equivalent kernels reject in
1689-
* the kernel but the per-component fallback (NetBSD/OpenBSD/Solaris/
1690-
* Cygwin/pre-5.6 Linux) does not. */
1691-
static int path_has_dotdot_component(const char *path)
1692-
{
1693-
const char *p = path;
1694-
1695-
while (*p) {
1696-
const char *q;
1697-
if (*p == '/') { p++; continue; }
1698-
q = p;
1699-
while (*q && *q != '/')
1700-
q++;
1701-
if (q - p == 2 && p[0] == '.' && p[1] == '.')
1702-
return 1;
1703-
p = q;
1704-
}
1705-
return 0;
1706-
}
1707-
17081682
#if defined(__linux__) && defined(HAVE_OPENAT2)
17091683
static int secure_relative_open_linux(const char *basedir, const char *relpath, int flags, mode_t mode)
17101684
{
@@ -1787,23 +1761,6 @@ int secure_relative_open(const char *basedir, const char *relpath, int flags, mo
17871761
errno = EINVAL;
17881762
return -1;
17891763
}
1790-
/* Reject any path with a literal ".." component (bare "..",
1791-
* "../foo", "foo/..", "foo/../bar", "subdir/.."). The previous
1792-
* substring-based check caught only "../" prefix and "/../"
1793-
* substring; bare ".." and trailing "/.." escape on the per-
1794-
* component walk fallback used by NetBSD/OpenBSD/Solaris/Cygwin
1795-
* and pre-5.6 Linux. RESOLVE_BENEATH on Linux/FreeBSD/macOS
1796-
* catches some of these in-kernel with EXDEV, but the front
1797-
* door must reject them consistently with EINVAL across all
1798-
* platforms so callers can rely on the validation. */
1799-
if (path_has_dotdot_component(relpath)) {
1800-
errno = EINVAL;
1801-
return -1;
1802-
}
1803-
if (basedir && basedir[0] != '/' && path_has_dotdot_component(basedir)) {
1804-
errno = EINVAL;
1805-
return -1;
1806-
}
18071764

18081765
#if defined(__linux__) && defined(HAVE_OPENAT2)
18091766
/* openat2(2) can fail with -EAGAIN if the path contains a ".." component
@@ -1833,56 +1790,61 @@ int secure_relative_open(const char *basedir, const char *relpath, int flags, mo
18331790
pathjoin(fullpath, sizeof fullpath, basedir, relpath);
18341791
return open(fullpath, flags, mode);
18351792
#else
1836-
int dirfd = AT_FDCWD;
1793+
int dirfd = AT_FDCWD, retfd = -1;
1794+
char *path_copy = NULL;
18371795
if (basedir != NULL) {
18381796
if (basedir[0] == '/') {
18391797
/* Absolute basedir: operator-trusted, plain openat. */
18401798
dirfd = openat(AT_FDCWD, basedir, O_RDONLY | O_DIRECTORY);
1841-
if (dirfd == -1) {
1799+
if (dirfd == -1)
18421800
return -1;
1843-
}
18441801
} else {
1845-
/* Relative basedir: walk it component-by-component
1846-
* with O_NOFOLLOW. This is the per-component
1847-
* RESOLVE_BENEATH equivalent for platforms without
1848-
* kernel-supported confinement, and matches the
1849-
* relpath walk below. Symlinks in basedir are
1850-
* rejected outright on this fallback path; the
1851-
* Linux openat2 / O_RESOLVE_BENEATH paths above
1852-
* still allow within-tree symlinks. */
1802+
/* Relative basedir: walk it component-by-component with
1803+
* O_NOFOLLOW. This is the per-component RESOLVE_BENEATH equivalent
1804+
* for platforms without kernel-supported confinement, and matches
1805+
* the relpath walk below. Symlinks and ".." components in basedir
1806+
* are rejected outright on this fallback path; the Linux openat2 /
1807+
* O_RESOLVE_BENEATH paths above still allow them as long as they
1808+
* stay within the tree.
1809+
* */
18531810
char *bcopy = my_strdup(basedir, __FILE__, __LINE__);
18541811
if (!bcopy)
18551812
return -1;
18561813
for (const char *part = strtok(bcopy, "/");
18571814
part != NULL;
18581815
part = strtok(NULL, "/"))
18591816
{
1817+
if (!strcmp(part, "..")) {
1818+
free(bcopy);
1819+
errno = EXDEV; // emulate RESOLVE_BENEATH
1820+
goto cleanup;
1821+
}
18601822
int next_fd = openat(dirfd, part, O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
18611823
if (next_fd == -1) {
1862-
int save_errno = errno;
1863-
if (dirfd != AT_FDCWD) close(dirfd);
1824+
int save_errno = errno; // free only saves errno on glibc >= 2.33
18641825
free(bcopy);
18651826
errno = save_errno;
1866-
return -1;
1827+
goto cleanup;
18671828
}
18681829
if (dirfd != AT_FDCWD) close(dirfd);
18691830
dirfd = next_fd;
18701831
}
18711832
free(bcopy);
18721833
}
18731834
}
1874-
int retfd = -1;
18751835

1876-
char *path_copy = my_strdup(relpath, __FILE__, __LINE__);
1877-
if (!path_copy) {
1878-
if (dirfd != AT_FDCWD) close(dirfd);
1879-
return -1;
1880-
}
1881-
1836+
path_copy = my_strdup(relpath, __FILE__, __LINE__);
1837+
if (!path_copy)
1838+
goto cleanup;
1839+
18821840
for (const char *part = strtok(path_copy, "/");
18831841
part != NULL;
18841842
part = strtok(NULL, "/"))
18851843
{
1844+
if (!strcmp(part, "..")) {
1845+
errno = EXDEV; // emulate RESOLVE_BENEATH
1846+
goto cleanup;
1847+
}
18861848
int next_fd = openat(dirfd, part, O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
18871849
if (next_fd == -1 && errno == ENOTDIR) {
18881850
if (strtok(NULL, "/") != NULL) {

‎t_secure_relpath.c‎

Lines changed: 84 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,14 @@
2626

2727
#include "rsync.h"
2828

29+
#include <stdbool.h>
2930
#include <sys/stat.h>
3031

32+
#ifdef __linux__
33+
#include <sys/syscall.h>
34+
#include <linux/openat2.h>
35+
#endif
36+
3137
int dry_run = 0;
3238
int am_root = 0;
3339
int am_sender = 0;
@@ -41,62 +47,80 @@ short info_levels[COUNT_INFO], debug_levels[COUNT_DEBUG];
4147

4248
static int errs = 0;
4349

44-
static void check_relpath(const char *relpath)
50+
/* Probe the running kernel for the RESOLVE_BENEATH-equivalent confinement
51+
* that secure_relative_open() prefers over the per-component O_NOFOLLOW
52+
* walk. Returns 1 if either openat2(RESOLVE_BENEATH) on Linux 5.6+ or
53+
* openat(O_RESOLVE_BENEATH) on FreeBSD 13+ / macOS 15+ is honoured by
54+
* the running kernel, 0 otherwise. The probe opens "." (a directory
55+
* the helper has just chdir'd into) so it can't fail for any reason
56+
* other than the kernel rejecting the requested confinement flag. */
57+
static int kernel_resolve_beneath_supported(void)
4558
{
4659
int fd;
47-
int saved_errno;
48-
49-
errno = 0;
50-
fd = secure_relative_open(NULL, relpath, O_RDONLY | O_DIRECTORY, 0);
51-
saved_errno = errno;
52-
60+
#if defined __linux__
61+
struct open_how how;
62+
memset(&how, 0, sizeof how);
63+
how.flags = O_RDONLY | O_DIRECTORY;
64+
how.resolve = RESOLVE_BENEATH | RESOLVE_NO_MAGICLINKS;
65+
fd = syscall(SYS_openat2, AT_FDCWD, ".", &how, sizeof how);
5366
if (fd >= 0) {
54-
fprintf(stderr,
55-
"FAIL [relpath=%-12s]: returned valid fd %d (escape) -- expected -1 EINVAL\n",
56-
relpath, fd);
5767
close(fd);
58-
errs++;
59-
return;
68+
return 1;
6069
}
61-
62-
if (saved_errno != EINVAL) {
63-
fprintf(stderr,
64-
"FAIL [relpath=%-12s]: rejected but errno=%d (%s), expected EINVAL\n",
65-
relpath, saved_errno, strerror(saved_errno));
66-
errs++;
67-
return;
70+
/* O_RESOLVE_BENEATH is not defined on Linux, and even if it were, Linux's
71+
* openat(2) does not return -EINVAL for unknown flag bits and so if
72+
* O_RESOLVE_BENEATH happened to get defined somehow, the following
73+
* fallback would always return success. */
74+
#elif defined O_RESOLVE_BENEATH
75+
fd = openat(AT_FDCWD, ".", O_RDONLY | O_DIRECTORY | O_RESOLVE_BENEATH);
76+
if (fd >= 0) {
77+
close(fd);
78+
return 1;
6879
}
80+
#endif
81+
return 0;
82+
}
6983

70-
fprintf(stderr, "OK [relpath=%-12s]: rejected with EINVAL\n", relpath);
84+
static void touch(const char *pathname, int mode)
85+
{
86+
int fd = open(pathname, O_CREAT|O_RDWR, mode);
87+
if (fd < 0) {
88+
fprintf(stderr, "could not create file %s: %m\n", pathname);
89+
abort();
90+
}
91+
close(fd);
7192
}
7293

73-
static void check_basedir(const char *basedir)
94+
static void check_relative_open(const char *basedir, const char *relpath,
95+
int want_errno)
7496
{
7597
int fd;
7698
int saved_errno;
7799

78100
errno = 0;
79-
fd = secure_relative_open(basedir, "ok", O_RDONLY | O_DIRECTORY, 0);
101+
fd = secure_relative_open(basedir, relpath, O_RDONLY | O_DIRECTORY, 0);
80102
saved_errno = errno;
81103

82104
if (fd >= 0) {
83-
fprintf(stderr,
84-
"FAIL [basedir=%-12s]: returned valid fd %d -- expected -1 EINVAL\n",
85-
basedir, fd);
86105
close(fd);
87-
errs++;
88-
return;
89-
}
90-
91-
if (saved_errno != EINVAL) {
106+
if (want_errno != 0) {
107+
fprintf(stderr,
108+
"FAIL [basedir=%-12s relpath=%-12s]: returned valid fd %d (escape) -- expected -1 errno=%d (%s)\n",
109+
basedir, relpath, fd, want_errno, strerror(want_errno));
110+
errs++;
111+
return;
112+
}
113+
} else if (saved_errno != want_errno) {
92114
fprintf(stderr,
93-
"FAIL [basedir=%-12s]: rejected but errno=%d (%s), expected EINVAL\n",
94-
basedir, saved_errno, strerror(saved_errno));
115+
"FAIL [basedir=%-12s relpath=%-12s]: rejected but errno=%d (%s), expected errno=%d (%s)\n",
116+
basedir, relpath, saved_errno, strerror(saved_errno), want_errno, strerror(want_errno));
95117
errs++;
96118
return;
97119
}
98120

99-
fprintf(stderr, "OK [basedir=%-12s]: rejected with EINVAL\n", basedir);
121+
fprintf(stderr,
122+
"OK [basedir=%-12s relpath=%-12s]: rejected with errno %d (%s)\n",
123+
basedir, relpath, saved_errno, strerror(saved_errno));
100124
}
101125

102126
int main(int argc, char **argv)
@@ -110,6 +134,11 @@ int main(int argc, char **argv)
110134
return 2;
111135
}
112136

137+
/* On systems with no RESOLVE_BENEATH, ".." at any point is an error. */
138+
int contained_dotdot_errno = 0;
139+
if (!kernel_resolve_beneath_supported())
140+
contained_dotdot_errno = EXDEV;
141+
113142
/* secure_relative_open's daemon-only confinement protections only
114143
* fire when am_daemon && !am_chrooted (the threat model is the
115144
* daemon-no-chroot deployment), but the front-door input
@@ -119,31 +148,33 @@ int main(int argc, char **argv)
119148
am_chrooted = 0;
120149

121150
mkdir("subdir", 0755);
122-
123-
/* Each of these relpaths must be rejected with EINVAL at the
124-
* secure_relative_open() front door. ".." is the actual one-level
125-
* escape; the others ("subdir/..", "subdir/../subdir") resolve
126-
* back to the start dir on systems that allow them, but we still
127-
* reject them as defence-in-depth: a path containing a ".." token
128-
* is suspicious and the caller should normalise before passing
129-
* it in. The "../foo" / "foo/../bar" / "/foo" / "/" cases are
130-
* regression checks for the existing checks. */
131-
check_relpath("..");
132-
check_relpath("../foo");
133-
check_relpath("subdir/..");
134-
check_relpath("subdir/../subdir");
135-
check_relpath("foo/../bar");
136-
check_relpath("/foo");
137-
check_relpath("/");
151+
touch("subdir/ok", 0644);
152+
mkdir("foo", 0755);
153+
touch("foo/ok", 0644);
154+
mkdir("bar", 0755);
155+
touch("bar/ok", 0644);
156+
157+
/* ".." that jumps outside of the root is rejected with EXDEV. */
158+
check_relative_open(NULL, "..", EXDEV);
159+
check_relative_open(NULL, "../foo", EXDEV);
160+
/* Absolute paths for relpath are rejected with EINVAL. */
161+
check_relative_open(NULL, "/foo", EINVAL);
162+
check_relative_open(NULL, "/", EINVAL);
163+
/* If RESOLVE_BENEATH is supported, ".." components that are contained
164+
* within the root are permitted. On older systems, they are rejected with
165+
* EXDEV. */
166+
check_relative_open(NULL, "subdir/..", contained_dotdot_errno);
167+
check_relative_open(NULL, "subdir/../subdir", contained_dotdot_errno);
168+
check_relative_open(NULL, "foo/../bar", contained_dotdot_errno);
138169

139170
/* Same checks against basedir (which the codex Finding 2 fix
140171
* routes through the same RESOLVE_BENEATH-equivalent). Absolute
141172
* basedirs are operator-trusted and intentionally not validated
142173
* here. */
143-
check_basedir("..");
144-
check_basedir("../subdir");
145-
check_basedir("subdir/..");
146-
check_basedir("foo/../bar");
174+
check_relative_open("..", ".", EXDEV);
175+
check_relative_open("../subdir", ".", EXDEV);
176+
check_relative_open("subdir/..", ".", contained_dotdot_errno);
177+
check_relative_open("foo/../bar", ".", contained_dotdot_errno);
147178

148179
if (errs)
149180
fprintf(stderr, "\n%d failure(s)\n", errs);

0 commit comments

Comments
 (0)