diff --git a/Makefile.in b/Makefile.in index 481f3ece..5c2b9a5a 100644 --- a/Makefile.in +++ b/Makefile.in @@ -58,12 +58,12 @@ TLS_OBJ = tls.o syscall.o util2.o t_stub.o lib/compat.o lib/snprintf.o lib/perms # Programs we must have to run the test cases CHECK_PROGS = rsync$(EXEEXT) tls$(EXEEXT) getgroups$(EXEEXT) getfsdev$(EXEEXT) \ testrun$(EXEEXT) trimslash$(EXEEXT) t_unsafe$(EXEEXT) t_chmod_secure$(EXEEXT) \ - wildtest$(EXEEXT) simdtest$(EXEEXT) + t_secure_relpath$(EXEEXT) wildtest$(EXEEXT) simdtest$(EXEEXT) CHECK_SYMLINKS = testsuite/chown-fake.test testsuite/devices-fake.test testsuite/xattrs-hlink.test # Objects for CHECK_PROGS to clean -CHECK_OBJS=tls.o testrun.o getgroups.o getfsdev.o t_stub.o t_unsafe.o t_chmod_secure.o trimslash.o wildtest.o +CHECK_OBJS=tls.o testrun.o getgroups.o getfsdev.o t_stub.o t_unsafe.o t_chmod_secure.o t_secure_relpath.o trimslash.o wildtest.o # note that the -I. is needed to handle config.h when using VPATH .c.o: @@ -183,6 +183,10 @@ T_CHMOD_SECURE_OBJ = t_chmod_secure.o syscall.o util1.o util2.o t_stub.o lib/com t_chmod_secure$(EXEEXT): $(T_CHMOD_SECURE_OBJ) $(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(T_CHMOD_SECURE_OBJ) $(LIBS) +T_SECURE_RELPATH_OBJ = t_secure_relpath.o syscall.o util1.o util2.o t_stub.o lib/compat.o lib/snprintf.o lib/wildmatch.o lib/permstring.o +t_secure_relpath$(EXEEXT): $(T_SECURE_RELPATH_OBJ) + $(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(T_SECURE_RELPATH_OBJ) $(LIBS) + .PHONY: conf conf: configure.sh config.h.in diff --git a/backup.c b/backup.c index 686cb297..ae8cb49e 100644 --- a/backup.c +++ b/backup.c @@ -39,7 +39,7 @@ static int validate_backup_dir(void) { STRUCT_STAT st; - if (do_lstat(backup_dir_buf, &st) < 0) { + if (do_lstat_at(backup_dir_buf, &st) < 0) { if (errno == ENOENT) return 0; rsyserr(FERROR, errno, "backup lstat %s failed", backup_dir_buf); @@ -98,7 +98,7 @@ static BOOL copy_valid_path(const char *fname) for ( ; b; name = b + 1, b = strchr(name, '/')) { *b = '\0'; - while (do_mkdir(backup_dir_buf, ACCESSPERMS) < 0) { + while (do_mkdir_at(backup_dir_buf, ACCESSPERMS) < 0) { if (errno == EEXIST) { val = validate_backup_dir(); if (val > 0) @@ -197,7 +197,7 @@ static inline int link_or_rename(const char *from, const char *to, if (IS_SPECIAL(stp->st_mode) || IS_DEVICE(stp->st_mode)) return 0; /* Use copy code. */ #endif - if (do_link(from, to) == 0) { + if (do_link_at(from, to) == 0) { if (DEBUG_GTE(BACKUP, 1)) rprintf(FINFO, "make_backup: HLINK %s successful.\n", from); return 2; @@ -207,7 +207,7 @@ static inline int link_or_rename(const char *from, const char *to, return 0; } #endif - if (do_rename(from, to) == 0) { + if (do_rename_at(from, to) == 0) { if (stp->st_nlink > 1 && !S_ISDIR(stp->st_mode)) { /* If someone has hard-linked the file into the backup * dir, rename() might return success but do nothing! */ @@ -246,7 +246,7 @@ int make_backup(const char *fname, BOOL prefer_rename) goto success; if (errno == EEXIST || errno == EISDIR) { STRUCT_STAT bakst; - if (do_lstat(buf, &bakst) == 0) { + if (do_lstat_at(buf, &bakst) == 0) { int flags = get_del_for_flag(bakst.st_mode) | DEL_FOR_BACKUP | DEL_RECURSE; if (delete_item(buf, bakst.st_mode, flags) != 0) return 0; @@ -277,7 +277,7 @@ int make_backup(const char *fname, BOOL prefer_rename) /* Check to see if this is a device file, or link */ if ((am_root && preserve_devices && IS_DEVICE(file->mode)) || (preserve_specials && IS_SPECIAL(file->mode))) { - if (do_mknod(buf, file->mode, sx.st.st_rdev) < 0) + if (do_mknod_at(buf, file->mode, sx.st.st_rdev) < 0) rsyserr(FERROR, errno, "mknod %s failed", full_fname(buf)); else if (DEBUG_GTE(BACKUP, 1)) rprintf(FINFO, "make_backup: DEVICE %s successful.\n", fname); @@ -294,7 +294,7 @@ int make_backup(const char *fname, BOOL prefer_rename) } ret = 2; } else { - if (do_symlink(sl, buf) < 0) + if (do_symlink_at(sl, buf) < 0) rsyserr(FERROR, errno, "link %s -> \"%s\"", full_fname(buf), sl); else if (DEBUG_GTE(BACKUP, 1)) rprintf(FINFO, "make_backup: SYMLINK %s successful.\n", fname); diff --git a/cleanup.c b/cleanup.c index 40d26baa..0493fbbb 100644 --- a/cleanup.c +++ b/cleanup.c @@ -198,7 +198,7 @@ NORETURN void _exit_cleanup(int code, const char *file, int line) switch_step++; if (cleanup_fname) - do_unlink(cleanup_fname); + do_unlink_at(cleanup_fname); if (exit_code) kill_all(SIGUSR1); if (cleanup_pid && cleanup_pid == getpid()) { diff --git a/delete.c b/delete.c index 3a625610..3a82bc4c 100644 --- a/delete.c +++ b/delete.c @@ -160,7 +160,7 @@ enum delret delete_item(char *fbuf, uint16 mode, uint16 flags) if (S_ISDIR(mode)) { what = "rmdir"; - ok = do_rmdir(fbuf) == 0; + ok = do_rmdir_at(fbuf) == 0; } else { if (make_backups > 0 && !(flags & DEL_FOR_BACKUP) && (backup_dir || !is_backup_file(fbuf))) { what = "make_backup"; diff --git a/generator.c b/generator.c index c3ace1c2..a6bce20b 100644 --- a/generator.c +++ b/generator.c @@ -984,7 +984,7 @@ static int try_dests_reg(struct file_struct *file, char *fname, int ndx, if (find_exact_for_existing) { if (alt_dest_type == LINK_DEST && real_st.st_dev == sxp->st.st_dev && real_st.st_ino == sxp->st.st_ino) return -1; - if (do_unlink(fname) < 0 && errno != ENOENT) + if (do_unlink_at(fname) < 0 && errno != ENOENT) goto got_nothing_for_ya; } #ifdef SUPPORT_HARD_LINKS @@ -1112,7 +1112,7 @@ static int try_dests_non(struct file_struct *file, char *fname, int ndx, && !IS_SPECIAL(file->mode) && !IS_DEVICE(file->mode) #endif && !S_ISDIR(file->mode)) { - if (do_link(cmpbuf, fname) < 0) { + if (do_link_at(cmpbuf, fname) < 0) { rsyserr(FERROR_XFER, errno, "failed to hard-link %s with %s", cmpbuf, fname); @@ -1315,7 +1315,7 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx, } } if (relative_paths && !implied_dirs && file->mode != 0 - && do_stat(dn, &sx.st) < 0) { + && do_stat_at(dn, &sx.st) < 0) { if (dry_run) goto parent_is_dry_missing; if (make_path(fname, MKP_DROP_NAME | MKP_SKIP_SLASH) < 0) { @@ -1427,7 +1427,7 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx, && (stype == FT_DIR || delete_item(fname, sx.st.st_mode, del_opts | DEL_FOR_DIR) != 0)) goto cleanup; /* Any errors get reported later. */ - if (do_mkdir(fname, (file->mode|added_perms) & 0700) == 0) + if (do_mkdir_at(fname, (file->mode|added_perms) & 0700) == 0) file->flags |= FLAG_DIR_CREATED; goto cleanup; } @@ -1469,10 +1469,10 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx, itemize(fnamecmp, file, ndx, statret, &sx, statret ? ITEM_LOCAL_CHANGE : 0, 0, NULL); } - if (real_ret != 0 && do_mkdir(fname,file->mode|added_perms) < 0 && errno != EEXIST) { + if (real_ret != 0 && do_mkdir_at(fname,file->mode|added_perms) < 0 && errno != EEXIST) { if (!relative_paths || errno != ENOENT || make_path(fname, MKP_DROP_NAME | MKP_SKIP_SLASH) < 0 - || (do_mkdir(fname, file->mode|added_perms) < 0 && errno != EEXIST)) { + || (do_mkdir_at(fname, file->mode|added_perms) < 0 && errno != EEXIST)) { rsyserr(FERROR_XFER, errno, "recv_generator: mkdir %s failed", full_fname(fname)); @@ -1808,7 +1808,7 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx, ; else if (quick_check_ok(FT_REG, fnamecmp, file, &sx.st)) { if (partialptr) { - do_unlink(partialptr); + do_unlink_at(partialptr); handle_partial_dir(partialptr, PDIR_DELETE); } set_file_attrs(fname, file, &sx, NULL, maybe_ATTRS_REPORT | maybe_ATTRS_ACCURATE_TIME); @@ -2016,7 +2016,7 @@ int atomic_create(struct file_struct *file, char *fname, const char *slnk, const if (slnk) { #ifdef SUPPORT_LINKS - if (do_symlink(slnk, create_name) < 0) { + if (do_symlink_at(slnk, create_name) < 0) { rsyserr(FERROR_XFER, errno, "symlink %s -> \"%s\" failed", full_fname(create_name), slnk); return 0; @@ -2032,7 +2032,7 @@ int atomic_create(struct file_struct *file, char *fname, const char *slnk, const return 0; #endif } else { - if (do_mknod(create_name, file->mode, rdev) < 0) { + if (do_mknod_at(create_name, file->mode, rdev) < 0) { rsyserr(FERROR_XFER, errno, "mknod %s failed", full_fname(create_name)); return 0; @@ -2040,10 +2040,14 @@ int atomic_create(struct file_struct *file, char *fname, const char *slnk, const } if (!skip_atomic) { - if (do_rename(tmpname, fname) < 0) { + if (do_rename_at(tmpname, fname) < 0) { + char *full_tmpname = strdup(full_fname(tmpname)); + if (full_tmpname == NULL) + out_of_memory("atomic_create"); rsyserr(FERROR_XFER, errno, "rename %s -> \"%s\" failed", - full_fname(tmpname), full_fname(fname)); - do_unlink(tmpname); + full_tmpname, full_fname(fname)); + free(full_tmpname); + do_unlink_at(tmpname); return 0; } } diff --git a/hlink.c b/hlink.c index 2c14407a..eb36730f 100644 --- a/hlink.c +++ b/hlink.c @@ -454,7 +454,7 @@ int hard_link_check(struct file_struct *file, int ndx, char *fname, int hard_link_one(struct file_struct *file, const char *fname, const char *oldname, int terse) { - if (do_link(oldname, fname) < 0) { + if (do_link_at(oldname, fname) < 0) { enum logcode code; if (terse) { if (!INFO_GTE(NAME, 1)) diff --git a/receiver.c b/receiver.c index cbe18196..8f5b51dd 100644 --- a/receiver.c +++ b/receiver.c @@ -442,7 +442,7 @@ static void handle_delayed_updates(char *local_name) } /* We don't use robust_rename() here because the * partial-dir must be on the same drive. */ - if (do_rename(partialptr, fname) < 0) { + if (do_rename_at(partialptr, fname) < 0) { rsyserr(FERROR_XFER, errno, "rename failed for %s (from %s)", full_fname(fname), partialptr); @@ -926,7 +926,7 @@ int recv_files(int f_in, int f_out, char *local_name) recv_ok = -1; else if (fnamecmp == partialptr) { if (!one_inplace) - do_unlink(partialptr); + do_unlink_at(partialptr); handle_partial_dir(partialptr, PDIR_DELETE); } } else if (keep_partial && partialptr && (!one_inplace || delay_updates)) { @@ -935,7 +935,7 @@ int recv_files(int f_in, int f_out, char *local_name) "Unable to create partial-dir for %s -- discarding %s.\n", local_name ? local_name : f_name(file, NULL), recv_ok ? "completed file" : "partial file"); - do_unlink(fnametmp); + do_unlink_at(fnametmp); recv_ok = -1; } else if (!finish_transfer(partialptr, fnametmp, fnamecmp, NULL, file, recv_ok, !partial_dir)) @@ -946,7 +946,7 @@ int recv_files(int f_in, int f_out, char *local_name) } else partialptr = NULL; } else if (!one_inplace) - do_unlink(fnametmp); + do_unlink_at(fnametmp); cleanup_disable(); diff --git a/rsync.c b/rsync.c index cc46a2f9..1d2ae82a 100644 --- a/rsync.c +++ b/rsync.c @@ -547,7 +547,7 @@ int set_file_attrs(const char *fname, struct file_struct *file, stat_x *sxp, if (am_root >= 0) { uid_t uid = change_uid ? (uid_t)F_OWNER(file) : sxp->st.st_uid; gid_t gid = change_gid ? (gid_t)F_GROUP(file) : sxp->st.st_gid; - if (do_lchown(fname, uid, gid) != 0) { + if (do_lchown_at(fname, uid, gid) != 0) { /* We shouldn't have attempted to change uid * or gid unless have the privilege. */ rsyserr(FERROR_XFER, errno, "%s %s failed", @@ -758,7 +758,7 @@ int finish_transfer(const char *fname, const char *fnametmp, full_fname(fnametmp), fname); if (!partialptr || (ret == -2 && temp_copy_name) || robust_rename(fnametmp, partialptr, NULL, file->mode) < 0) - do_unlink(fnametmp); + do_unlink_at(fnametmp); return 0; } if (ret == 0) { @@ -774,7 +774,7 @@ int finish_transfer(const char *fname, const char *fnametmp, ok_to_set_time ? ATTRS_ACCURATE_TIME : ATTRS_SKIP_MTIME | ATTRS_SKIP_ATIME | ATTRS_SKIP_CRTIME); if (temp_copy_name) { - if (do_rename(fnametmp, fname) < 0) { + if (do_rename_at(fnametmp, fname) < 0) { rsyserr(FERROR_XFER, errno, "rename %s -> \"%s\"", full_fname(fnametmp), fname); return 0; diff --git a/syscall.c b/syscall.c index 167aae0e..2cff0b38 100644 --- a/syscall.c +++ b/syscall.c @@ -93,6 +93,63 @@ int do_unlink(const char *path) return unlink(path); } +/* + Symlink-race-safe variant of do_unlink() for receiver-side use. See + the comment on do_chmod_at() for the threat model. unlink() resolves + parent components, so a parent-symlink swap can delete an outside + file under the daemon's authority. Defence: open the parent of path + under secure_relative_open() and use unlinkat() (flags=0) against + that dirfd. + + Falls through to do_unlink() for the same dry-run / non-daemon / + chrooted / no-parent / absolute-path cases as the other wrappers. +*/ +int do_unlink_at(const char *path) +{ +#ifdef AT_FDCWD + extern int am_daemon, am_chrooted; + char dirpath[MAXPATHLEN]; + const char *bname; + const char *slash; + int dfd, ret, e; + size_t dlen; + + if (dry_run) return 0; + RETURN_ERROR_IF_RO_OR_LO; + + if (!am_daemon || am_chrooted) + return unlink(path); + + if (!path || !*path || *path == '/') + return unlink(path); + + slash = strrchr(path, '/'); + if (!slash) + return unlink(path); + + dlen = slash - path; + if (dlen >= sizeof dirpath) { + errno = ENAMETOOLONG; + return -1; + } + memcpy(dirpath, path, dlen); + dirpath[dlen] = '\0'; + bname = slash + 1; + + dfd = secure_relative_open(NULL, dirpath, O_RDONLY | O_DIRECTORY, 0); + if (dfd < 0) + return -1; + + ret = unlinkat(dfd, bname, 0); + e = errno; + close(dfd); + errno = e; + return ret; +#else + return do_unlink(path); +#endif +} + #ifdef SUPPORT_LINKS int do_symlink(const char *lnk, const char *path) { @@ -117,6 +174,70 @@ int do_symlink(const char *lnk, const char *path) return symlink(lnk, path); } +/* + Symlink-race-safe variant of do_symlink() for receiver-side use. See + the comment on do_chmod_at() for the threat model. Only the parent + directory of `path` needs protection -- symlinkat() does not resolve + the final component (it creates it). Defence: open parent of `path` + under secure_relative_open() and call symlinkat() against that + dirfd. The link target string `lnk` is stored verbatim and not + resolved at creation time, so it doesn't need scrutiny here. + + Falls through to do_symlink() for the --fake-super (am_root < 0) + path -- that code path opens `path` with do_open() which has its + own (separate) symlink-race exposure tracked elsewhere. +*/ +int do_symlink_at(const char *lnk, const char *path) +{ +#ifdef AT_FDCWD + extern int am_daemon, am_chrooted; + char dirpath[MAXPATHLEN]; + const char *bname; + const char *slash; + int dfd, ret, e; + size_t dlen; + + if (dry_run) return 0; + RETURN_ERROR_IF_RO_OR_LO; + + if (!am_daemon || am_chrooted) + return do_symlink(lnk, path); + +#if defined NO_SYMLINK_XATTRS || defined NO_SYMLINK_USER_XATTRS + if (am_root < 0) + return do_symlink(lnk, path); +#endif + + if (!path || !*path || *path == '/') + return do_symlink(lnk, path); + + slash = strrchr(path, '/'); + if (!slash) + return do_symlink(lnk, path); + + dlen = slash - path; + if (dlen >= sizeof dirpath) { + errno = ENAMETOOLONG; + return -1; + } + memcpy(dirpath, path, dlen); + dirpath[dlen] = '\0'; + bname = slash + 1; + + dfd = secure_relative_open(NULL, dirpath, O_RDONLY | O_DIRECTORY, 0); + if (dfd < 0) + return -1; + + ret = symlinkat(lnk, dfd, bname); + e = errno; + close(dfd); + errno = e; + return ret; +#else + return do_symlink(lnk, path); +#endif +} + #if defined NO_SYMLINK_XATTRS || defined NO_SYMLINK_USER_XATTRS ssize_t do_readlink(const char *path, char *buf, size_t bufsiz) { @@ -153,6 +274,106 @@ int do_link(const char *old_path, const char *new_path) return link(old_path, new_path); #endif } + +/* + Symlink-race-safe variant of do_link() for receiver-side use. See + the comment on do_chmod_at() for the threat model. link() resolves + parent components of *both* old_path and new_path, so a parent- + symlink swap on either side can plant the new hard link outside + the module, or hard-link an outside file into the module (read + disclosure). + + Defence: open each parent under secure_relative_open() and use + linkat() between the two dirfds, reusing one when the parents + match. flags=0 matches the existing do_link() (don't follow a + symbolic-link old_path). Only available on systems with linkat(); + pre-AT_FDCWD systems fall through to do_link(). +*/ +int do_link_at(const char *old_path, const char *new_path) +{ +#if defined AT_FDCWD && defined HAVE_LINKAT + extern int am_daemon, am_chrooted; + char old_dirpath[MAXPATHLEN], new_dirpath[MAXPATHLEN]; + const char *old_bname, *new_bname; + const char *old_slash, *new_slash; + int old_dfd = AT_FDCWD, new_dfd = AT_FDCWD; + BOOL old_owns = False, new_owns = False; + int ret, e; + size_t old_dlen = 0, new_dlen = 0; + + if (dry_run) return 0; + RETURN_ERROR_IF_RO_OR_LO; + + if (!am_daemon || am_chrooted) + return do_link(old_path, new_path); + + if (!old_path || !*old_path || *old_path == '/' + || !new_path || !*new_path || *new_path == '/') + return do_link(old_path, new_path); + + old_slash = strrchr(old_path, '/'); + new_slash = strrchr(new_path, '/'); + + /* Resolve each path's parent dir independently. A path without a + * slash lives in CWD (AT_FDCWD), no parent open required. A path + * with a slash needs secure_relative_open to confine its parent + * resolution -- otherwise a parent symlink (e.g. --link-dest=cd + * where cd -> /outside) lets the kernel-level linkat(AT_FDCWD, + * "cd/target.txt", ...) escape the module. */ + if (old_slash) { + old_dlen = old_slash - old_path; + if (old_dlen >= sizeof old_dirpath) { errno = ENAMETOOLONG; return -1; } + memcpy(old_dirpath, old_path, old_dlen); + old_dirpath[old_dlen] = '\0'; + old_bname = old_slash + 1; + old_dfd = secure_relative_open(NULL, old_dirpath, O_RDONLY | O_DIRECTORY, 0); + if (old_dfd < 0) + return -1; + old_owns = True; + } else { + old_bname = old_path; + } + + if (new_slash) { + new_dlen = new_slash - new_path; + if (new_dlen >= sizeof new_dirpath) { + e = ENAMETOOLONG; + if (old_owns) close(old_dfd); + errno = e; + return -1; + } + memcpy(new_dirpath, new_path, new_dlen); + new_dirpath[new_dlen] = '\0'; + new_bname = new_slash + 1; + if (old_owns && old_dlen == new_dlen + && memcmp(old_dirpath, new_dirpath, old_dlen) == 0) { + new_dfd = old_dfd; + } else { + new_dfd = secure_relative_open(NULL, new_dirpath, O_RDONLY | O_DIRECTORY, 0); + if (new_dfd < 0) { + e = errno; + if (old_owns) close(old_dfd); + errno = e; + return -1; + } + new_owns = True; + } + } else { + new_bname = new_path; + } + + ret = linkat(old_dfd, old_bname, new_dfd, new_bname, 0); + e = errno; + if (new_owns) + close(new_dfd); + if (old_owns) + close(old_dfd); + errno = e; + return ret; +#else + return do_link(old_path, new_path); +#endif +} #endif int do_lchown(const char *path, uid_t owner, gid_t group) @@ -165,6 +386,66 @@ int do_lchown(const char *path, uid_t owner, gid_t group) return lchown(path, owner, group); } +/* + Symlink-race-safe variant of do_lchown() for receiver-side use. See the + comment on do_chmod_at() for the threat model and design rationale. + + Resolves the parent directory under secure_relative_open() and invokes + fchownat(..., AT_SYMLINK_NOFOLLOW) against that dirfd, so that an + attacker who substitutes a symlink into one of the parent components + cannot redirect the chown outside the receiver's confinement. The + AT_SYMLINK_NOFOLLOW flag matches lchown()'s "do not follow a final- + component symlink" semantics. + + Falls through to do_lchown() in the dry-run / non-daemon / chrooted / + absolute-path / no-parent cases, identical to do_chmod_at(). +*/ +int do_lchown_at(const char *fname, uid_t owner, gid_t group) +{ +#ifdef AT_FDCWD + extern int am_daemon, am_chrooted; + char dirpath[MAXPATHLEN]; + const char *bname; + const char *slash; + int dfd, ret, e; + size_t dlen; + + if (dry_run) return 0; + RETURN_ERROR_IF_RO_OR_LO; + + if (!am_daemon || am_chrooted) + return do_lchown(fname, owner, group); + + if (!fname || !*fname || *fname == '/') + return do_lchown(fname, owner, group); + + slash = strrchr(fname, '/'); + if (!slash) + return do_lchown(fname, owner, group); + + dlen = slash - fname; + if (dlen >= sizeof dirpath) { + errno = ENAMETOOLONG; + return -1; + } + memcpy(dirpath, fname, dlen); + dirpath[dlen] = '\0'; + bname = slash + 1; + + dfd = secure_relative_open(NULL, dirpath, O_RDONLY | O_DIRECTORY, 0); + if (dfd < 0) + return -1; + + ret = fchownat(dfd, bname, owner, group, AT_SYMLINK_NOFOLLOW); + e = errno; + close(dfd); + errno = e; + return ret; +#else + return do_lchown(fname, owner, group); +#endif +} + int do_mknod(const char *pathname, mode_t mode, dev_t dev) { if (dry_run) return 0; @@ -215,6 +496,76 @@ int do_mknod(const char *pathname, mode_t mode, dev_t dev) #endif } +/* + Symlink-race-safe variant of do_mknod() for receiver-side use. See + the comment on do_chmod_at() for the threat model. Defence: open + the parent of pathname under secure_relative_open() and use + mknodat() against that dirfd. mknodat() covers both regular-file + (S_IFREG with dev=0) and FIFO (S_IFIFO) and device-node creation. + + Falls through to do_mknod() for fake-super (am_root < 0) and for + sockets, both of which use auxiliary path-based syscalls that + don't have an *at() variant in any portable form. +*/ +int do_mknod_at(const char *pathname, mode_t mode, dev_t dev) +{ +#ifdef AT_FDCWD + extern int am_daemon, am_chrooted; + char dirpath[MAXPATHLEN]; + const char *bname; + const char *slash; + int dfd, ret, e; + size_t dlen; + + if (dry_run) return 0; + RETURN_ERROR_IF_RO_OR_LO; + + if (!am_daemon || am_chrooted) + return do_mknod(pathname, mode, dev); + + if (am_root < 0) + return do_mknod(pathname, mode, dev); + +#if !defined MKNOD_CREATES_SOCKETS && defined HAVE_SYS_UN_H + if (S_ISSOCK(mode)) + return do_mknod(pathname, mode, dev); +#endif + + if (!pathname || !*pathname || *pathname == '/') + return do_mknod(pathname, mode, dev); + + slash = strrchr(pathname, '/'); + if (!slash) + return do_mknod(pathname, mode, dev); + + dlen = slash - pathname; + if (dlen >= sizeof dirpath) { + errno = ENAMETOOLONG; + return -1; + } + memcpy(dirpath, pathname, dlen); + dirpath[dlen] = '\0'; + bname = slash + 1; + + dfd = secure_relative_open(NULL, dirpath, O_RDONLY | O_DIRECTORY, 0); + if (dfd < 0) + return -1; + +#if !defined MKNOD_CREATES_FIFOS && defined HAVE_MKFIFO + if (S_ISFIFO(mode)) + ret = mkfifoat(dfd, bname, mode); + else +#endif + ret = mknodat(dfd, bname, mode, dev); + e = errno; + close(dfd); + errno = e; + return ret; +#else + return do_mknod(pathname, mode, dev); +#endif +} + int do_rmdir(const char *pathname) { if (dry_run) return 0; @@ -222,6 +573,57 @@ int do_rmdir(const char *pathname) return rmdir(pathname); } +/* + Symlink-race-safe variant of do_rmdir(). See do_unlink_at() above; + same shape but with AT_REMOVEDIR set to require the target be a + directory. +*/ +int do_rmdir_at(const char *pathname) +{ +#ifdef AT_FDCWD + extern int am_daemon, am_chrooted; + char dirpath[MAXPATHLEN]; + const char *bname; + const char *slash; + int dfd, ret, e; + size_t dlen; + + if (dry_run) return 0; + RETURN_ERROR_IF_RO_OR_LO; + + if (!am_daemon || am_chrooted) + return rmdir(pathname); + + if (!pathname || !*pathname || *pathname == '/') + return rmdir(pathname); + + slash = strrchr(pathname, '/'); + if (!slash) + return rmdir(pathname); + + dlen = slash - pathname; + if (dlen >= sizeof dirpath) { + errno = ENAMETOOLONG; + return -1; + } + memcpy(dirpath, pathname, dlen); + dirpath[dlen] = '\0'; + bname = slash + 1; + + dfd = secure_relative_open(NULL, dirpath, O_RDONLY | O_DIRECTORY, 0); + if (dfd < 0) + return -1; + + ret = unlinkat(dfd, bname, AT_REMOVEDIR); + e = errno; + close(dfd); + errno = e; + return ret; +#else + return do_rmdir(pathname); +#endif +} + int do_open(const char *pathname, int flags, mode_t mode) { if (flags != O_RDONLY) { @@ -370,6 +772,89 @@ int do_rename(const char *old_path, const char *new_path) return rename(old_path, new_path); } +/* + Symlink-race-safe variant of do_rename() for receiver-side use. See + the comment on do_chmod_at() for the threat model and design rationale. + + rename() is the central tmp -> final operation in rsync; if either the + source or the destination has an attacker-substituted symlink in one + of its parent components, the rename can publish or vanish files + outside the module. Defence: open the parent of *each* path under + secure_relative_open() and use renameat() against the resulting + dirfds. When old_path and new_path share the same parent (the common + case -- tmp file living next to its final name), we reuse the same + dirfd for both sides. + + Falls through to do_rename() in dry-run, non-daemon, chrooted, no- + parent and absolute-path cases, identical to the other do_*_at() + wrappers. +*/ +int do_rename_at(const char *old_path, const char *new_path) +{ +#ifdef AT_FDCWD + extern int am_daemon, am_chrooted; + char old_dirpath[MAXPATHLEN], new_dirpath[MAXPATHLEN]; + const char *old_bname, *new_bname; + const char *old_slash, *new_slash; + int old_dfd = -1, new_dfd = -1, ret = -1, e; + size_t old_dlen, new_dlen; + + if (dry_run) return 0; + RETURN_ERROR_IF_RO_OR_LO; + + if (!am_daemon || am_chrooted) + return do_rename(old_path, new_path); + + if (!old_path || !*old_path || *old_path == '/' + || !new_path || !*new_path || *new_path == '/') + return do_rename(old_path, new_path); + + old_slash = strrchr(old_path, '/'); + new_slash = strrchr(new_path, '/'); + if (!old_slash || !new_slash) + return do_rename(old_path, new_path); + + old_dlen = old_slash - old_path; + new_dlen = new_slash - new_path; + if (old_dlen >= sizeof old_dirpath || new_dlen >= sizeof new_dirpath) { + errno = ENAMETOOLONG; + return -1; + } + memcpy(old_dirpath, old_path, old_dlen); + old_dirpath[old_dlen] = '\0'; + memcpy(new_dirpath, new_path, new_dlen); + new_dirpath[new_dlen] = '\0'; + old_bname = old_slash + 1; + new_bname = new_slash + 1; + + old_dfd = secure_relative_open(NULL, old_dirpath, O_RDONLY | O_DIRECTORY, 0); + if (old_dfd < 0) + return -1; + + if (old_dlen == new_dlen && memcmp(old_dirpath, new_dirpath, old_dlen) == 0) { + new_dfd = old_dfd; + } else { + new_dfd = secure_relative_open(NULL, new_dirpath, O_RDONLY | O_DIRECTORY, 0); + if (new_dfd < 0) { + e = errno; + close(old_dfd); + errno = e; + return -1; + } + } + + ret = renameat(old_dfd, old_bname, new_dfd, new_bname); + e = errno; + if (new_dfd != old_dfd) + close(new_dfd); + close(old_dfd); + errno = e; + return ret; +#else + return do_rename(old_path, new_path); +#endif +} + #ifdef HAVE_FTRUNCATE int do_ftruncate(int fd, OFF_T size) { @@ -412,6 +897,66 @@ int do_mkdir(char *path, mode_t mode) return mkdir(path, mode); } +/* + Symlink-race-safe variant of do_mkdir() for receiver-side use. See + the comment on do_chmod_at() for the threat model and design rationale. + + mkdir() resolves parent symlinks at every component, so a parent- + component swap can place an attacker-named directory outside the + module. Defence: open the parent of fname under secure_relative_open() + and call mkdirat() against that dirfd. + + Mutates path in place to trim trailing slashes (matches do_mkdir()). + Falls through to do_mkdir() in dry-run, non-daemon, chrooted, no- + parent and absolute-path cases. +*/ +int do_mkdir_at(char *path, mode_t mode) +{ +#ifdef AT_FDCWD + extern int am_daemon, am_chrooted; + char dirpath[MAXPATHLEN]; + const char *bname; + const char *slash; + int dfd, ret, e; + size_t dlen; + + if (dry_run) return 0; + RETURN_ERROR_IF_RO_OR_LO; + trim_trailing_slashes(path); + + if (!am_daemon || am_chrooted) + return mkdir(path, mode); + + if (!path || !*path || *path == '/') + return mkdir(path, mode); + + slash = strrchr(path, '/'); + if (!slash) + return mkdir(path, mode); + + dlen = slash - path; + if (dlen >= sizeof dirpath) { + errno = ENAMETOOLONG; + return -1; + } + memcpy(dirpath, path, dlen); + dirpath[dlen] = '\0'; + bname = slash + 1; + + dfd = secure_relative_open(NULL, dirpath, O_RDONLY | O_DIRECTORY, 0); + if (dfd < 0) + return -1; + + ret = mkdirat(dfd, bname, mode); + e = errno; + close(dfd); + errno = e; + return ret; +#else + return do_mkdir(path, mode); +#endif +} + /* like mkstemp but forces permissions */ int do_mkstemp(char *template, mode_t perms) { @@ -465,6 +1010,76 @@ int do_lstat(const char *path, STRUCT_STAT *st) #endif } +/* + Symlink-race-safe variants of do_stat() / do_lstat() for receiver- + side use. See the comment on do_chmod_at() for the threat model. + stat() and lstat() resolve parent components, so a parent-symlink + swap can make the receiver's stat see attributes of a victim file + outside the module -- which then drives later behaviour (e.g. + "this isn't a directory, delete it" -> attacker-controlled unlink + on something outside the module). + + Defence: open the parent under secure_relative_open() and use + fstatat() with AT_SYMLINK_NOFOLLOW (lstat) or 0 (stat) against + that dirfd. Same fall-through gating as the other wrappers. +*/ +static int do_xstat_at(const char *path, STRUCT_STAT *st, int at_flags, int (*fallback)(const char *, STRUCT_STAT *)) +{ +#ifdef AT_FDCWD + extern int am_daemon, am_chrooted; + char dirpath[MAXPATHLEN]; + const char *bname; + const char *slash; + int dfd, ret, e; + size_t dlen; + + if (!am_daemon || am_chrooted) + return fallback(path, st); + + if (!path || !*path || *path == '/') + return fallback(path, st); + + slash = strrchr(path, '/'); + if (!slash) + return fallback(path, st); + + dlen = slash - path; + if (dlen >= sizeof dirpath) { + errno = ENAMETOOLONG; + return -1; + } + memcpy(dirpath, path, dlen); + dirpath[dlen] = '\0'; + bname = slash + 1; + + dfd = secure_relative_open(NULL, dirpath, O_RDONLY | O_DIRECTORY, 0); + if (dfd < 0) + return -1; + + ret = fstatat(dfd, bname, st, at_flags); + e = errno; + close(dfd); + errno = e; + return ret; +#else + return fallback(path, st); +#endif +} + +int do_stat_at(const char *path, STRUCT_STAT *st) +{ + return do_xstat_at(path, st, 0, do_stat); +} + +int do_lstat_at(const char *path, STRUCT_STAT *st) +{ +#ifdef SUPPORT_LINKS + return do_xstat_at(path, st, AT_SYMLINK_NOFOLLOW, do_lstat); +#else + return do_xstat_at(path, st, 0, do_stat); +#endif +} + int do_fstat(int fd, STRUCT_STAT *st) { #ifdef USE_STAT64_FUNCS @@ -486,12 +1101,26 @@ OFF_T do_lseek(int fd, OFF_T offset, int whence) #ifdef HAVE_SETATTRLIST int do_setattrlist_times(const char *path, STRUCT_STAT *stp) { + extern int am_daemon, am_chrooted; struct attrlist attrList; struct timespec ts[2]; if (dry_run) return 0; RETURN_ERROR_IF_RO_OR_LO; + /* setattrlist() takes a raw path and follows parent symlinks + * (FSOPT_NOFOLLOW only blocks the final component). On a + * daemon-no-chroot deployment, return ENOSYS so set_times()' + * tier walk falls through to do_utimensat_at(), which routes + * the timestamp update through a secure parent dirfd. The + * macOS-specific attribute set this function would have used + * (ATTR_CMN_MODTIME / ATTR_CMN_ACCTIME) is the same set + * utimensat() handles, so no functionality is lost. */ + if (am_daemon && !am_chrooted) { + errno = ENOSYS; + return -1; + } + /* Yes, this is in the opposite order of utime and similar. */ ts[0].tv_sec = stp->st_mtime; ts[0].tv_nsec = stp->ST_MTIME_NSEC; @@ -508,12 +1137,25 @@ int do_setattrlist_times(const char *path, STRUCT_STAT *stp) #ifdef SUPPORT_CRTIMES int do_setattrlist_crtime(const char *path, time_t crtime) { + extern int am_daemon, am_chrooted; struct attrlist attrList; struct timespec ts; if (dry_run) return 0; RETURN_ERROR_IF_RO_OR_LO; + /* Same path-follows-parent-symlinks concern as + * do_setattrlist_times. There is no portable at-aware variant + * of setattrlist that targets ATTR_CMN_CRTIME, so on a + * daemon-no-chroot deployment we return -1 and accept that + * crtime preservation is silently dropped for that file (the + * caller treats this as "crtime not updated"). The transfer + * itself continues normally. */ + if (am_daemon && !am_chrooted) { + errno = ENOSYS; + return -1; + } + ts.tv_sec = crtime; ts.tv_nsec = 0; @@ -529,10 +1171,19 @@ int do_setattrlist_crtime(const char *path, time_t crtime) time_t get_create_time(const char *path, STRUCT_STAT *stp) { #ifdef HAVE_GETATTRLIST + extern int am_daemon, am_chrooted; static struct create_time attrBuf; struct attrlist attrList; (void)stp; + /* getattrlist() is also path-based and follows parent + * symlinks. In daemon-no-chroot, refuse rather than read the + * crtime of a file the parent-symlink chain might point at + * outside the module. The caller's "no crtime available" + * path returns 0; the file gets a fresh crtime instead of + * preserving the source's. */ + if (am_daemon && !am_chrooted) + return 0; memset(&attrList, 0, sizeof attrList); attrList.bitmapcount = ATTR_BIT_MAP_COUNT; attrList.commonattr = ATTR_CMN_CRTIME; @@ -598,6 +1249,81 @@ int do_utimensat(const char *path, STRUCT_STAT *stp) #endif return utimensat(AT_FDCWD, path, t, AT_SYMLINK_NOFOLLOW); } + +/* + Symlink-race-safe variant of do_utimensat() for receiver-side use. + See the comment on do_chmod_at() for the threat model. utimes() + resolves parent components and follows a final-component symlink; + lutimes() doesn't follow the final component but still resolves + parents. Either way, a parent-symlink swap can redirect the + timestamp update outside the module. Defence: open the parent of + path under secure_relative_open() and call utimensat() with + AT_SYMLINK_NOFOLLOW against that dirfd. + + Falls through to do_utimensat() in the same dry-run / non-daemon / + chrooted / no-parent / absolute-path cases as the other wrappers. + Returns -1 with errno=ENOSYS on systems without utimensat() + (caller is expected to fall back to the legacy tier walk). +*/ +int do_utimensat_at(const char *path, STRUCT_STAT *stp) +{ +#ifdef AT_FDCWD + extern int am_daemon, am_chrooted; + struct timespec t[2]; + char dirpath[MAXPATHLEN]; + const char *bname; + const char *slash; + int dfd, ret, e; + size_t dlen; + + if (dry_run) return 0; + RETURN_ERROR_IF_RO_OR_LO; + + if (!am_daemon || am_chrooted) + return do_utimensat(path, stp); + + if (!path || !*path || *path == '/') + return do_utimensat(path, stp); + + slash = strrchr(path, '/'); + if (!slash) + return do_utimensat(path, stp); + + dlen = slash - path; + if (dlen >= sizeof dirpath) { + errno = ENAMETOOLONG; + return -1; + } + memcpy(dirpath, path, dlen); + dirpath[dlen] = '\0'; + bname = slash + 1; + + t[0].tv_sec = stp->st_atime; +#ifdef ST_ATIME_NSEC + t[0].tv_nsec = stp->ST_ATIME_NSEC; +#else + t[0].tv_nsec = 0; +#endif + t[1].tv_sec = stp->st_mtime; +#ifdef ST_MTIME_NSEC + t[1].tv_nsec = stp->ST_MTIME_NSEC; +#else + t[1].tv_nsec = 0; +#endif + + dfd = secure_relative_open(NULL, dirpath, O_RDONLY | O_DIRECTORY, 0); + if (dfd < 0) + return -1; + + ret = utimensat(dfd, bname, t, AT_SYMLINK_NOFOLLOW); + e = errno; + close(dfd); + errno = e; + return ret; +#else + return do_utimensat(path, stp); +#endif +} #endif #ifdef HAVE_LUTIMES @@ -820,6 +1546,30 @@ int do_open_nofollow(const char *pathname, int flags) The relpath must also not contain any ../ elements in the path. */ +/* Returns 1 if path has any "/"-separated component that is exactly + * "..", 0 otherwise. Used by secure_relative_open's front-door + * validation to reject inputs that the per-component walk fallback + * would otherwise resolve through ".." -- e.g. bare "..", "foo/..", + * "subdir/.." -- which RESOLVE_BENEATH-equivalent kernels reject in + * the kernel but the per-component fallback (NetBSD/OpenBSD/Solaris/ + * Cygwin/pre-5.6 Linux) does not. */ +static int path_has_dotdot_component(const char *path) +{ + const char *p = path; + + while (*p) { + const char *q; + if (*p == '/') { p++; continue; } + q = p; + while (*q && *q != '/') + q++; + if (q - p == 2 && p[0] == '.' && p[1] == '.') + return 1; + p = q; + } + return 0; +} + #ifdef __linux__ static int secure_relative_open_linux(const char *basedir, const char *relpath, int flags, mode_t mode) { @@ -833,10 +1583,25 @@ static int secure_relative_open_linux(const char *basedir, const char *relpath, if (basedir == NULL) { dirfd = AT_FDCWD; - } else { + } else if (basedir[0] == '/') { + /* Absolute basedir: operator-trusted (module_dir and the + * like). Plain openat. */ dirfd = openat(AT_FDCWD, basedir, O_RDONLY | O_DIRECTORY); if (dirfd == -1) return -1; + } else { + /* Relative basedir: may be wire-influenced via + * --link-dest / --copy-dest / --compare-dest. Resolve it + * under the same RESOLVE_BENEATH guarantee as relpath, so + * a parent symlink on basedir cannot redirect the dirfd + * outside the CWD anchor. */ + struct open_how bhow; + memset(&bhow, 0, sizeof bhow); + bhow.flags = O_RDONLY | O_DIRECTORY; + bhow.resolve = RESOLVE_BENEATH | RESOLVE_NO_MAGICLINKS; + dirfd = syscall(SYS_openat2, AT_FDCWD, basedir, &bhow, sizeof bhow); + if (dirfd == -1) + return -1; } retfd = syscall(SYS_openat2, dirfd, relpath, &how, sizeof how); @@ -859,10 +1624,17 @@ static int secure_relative_open_resolve_beneath(const char *basedir, const char if (basedir == NULL) { dirfd = AT_FDCWD; - } else { + } else if (basedir[0] == '/') { + /* Absolute basedir: operator-trusted, plain openat. */ dirfd = openat(AT_FDCWD, basedir, O_RDONLY | O_DIRECTORY); if (dirfd == -1) return -1; + } else { + /* Relative basedir: confine its resolution beneath CWD + * (see secure_relative_open_linux for the rationale). */ + dirfd = openat(AT_FDCWD, basedir, O_RDONLY | O_DIRECTORY | O_RESOLVE_BENEATH); + if (dirfd == -1) + return -1; } retfd = openat(dirfd, relpath, flags | O_RESOLVE_BENEATH, mode); @@ -880,8 +1652,20 @@ int secure_relative_open(const char *basedir, const char *relpath, int flags, mo errno = EINVAL; return -1; } - if (strncmp(relpath, "../", 3) == 0 || strstr(relpath, "/../")) { - // no ../ elements allowed in the relpath + /* Reject any path with a literal ".." component (bare "..", + * "../foo", "foo/..", "foo/../bar", "subdir/.."). The previous + * substring-based check caught only "../" prefix and "/../" + * substring; bare ".." and trailing "/.." escape on the per- + * component walk fallback used by NetBSD/OpenBSD/Solaris/Cygwin + * and pre-5.6 Linux. RESOLVE_BENEATH on Linux/FreeBSD/macOS + * catches some of these in-kernel with EXDEV, but the front + * door must reject them consistently with EINVAL across all + * platforms so callers can rely on the validation. */ + if (path_has_dotdot_component(relpath)) { + errno = EINVAL; + return -1; + } + if (basedir && basedir[0] != '/' && path_has_dotdot_component(basedir)) { errno = EINVAL; return -1; } @@ -911,15 +1695,47 @@ int secure_relative_open(const char *basedir, const char *relpath, int flags, mo #else int dirfd = AT_FDCWD; if (basedir != NULL) { - dirfd = openat(AT_FDCWD, basedir, O_RDONLY | O_DIRECTORY); - if (dirfd == -1) { - return -1; + if (basedir[0] == '/') { + /* Absolute basedir: operator-trusted, plain openat. */ + dirfd = openat(AT_FDCWD, basedir, O_RDONLY | O_DIRECTORY); + if (dirfd == -1) { + return -1; + } + } else { + /* Relative basedir: walk it component-by-component + * with O_NOFOLLOW. This is the per-component + * RESOLVE_BENEATH equivalent for platforms without + * kernel-supported confinement, and matches the + * relpath walk below. Symlinks in basedir are + * rejected outright on this fallback path; the + * Linux openat2 / O_RESOLVE_BENEATH paths above + * still allow within-tree symlinks. */ + char *bcopy = my_strdup(basedir, __FILE__, __LINE__); + if (!bcopy) + return -1; + for (const char *part = strtok(bcopy, "/"); + part != NULL; + part = strtok(NULL, "/")) + { + int next_fd = openat(dirfd, part, O_RDONLY | O_DIRECTORY | O_NOFOLLOW); + if (next_fd == -1) { + int save_errno = errno; + if (dirfd != AT_FDCWD) close(dirfd); + free(bcopy); + errno = save_errno; + return -1; + } + if (dirfd != AT_FDCWD) close(dirfd); + dirfd = next_fd; + } + free(bcopy); } } int retfd = -1; char *path_copy = my_strdup(relpath, __FILE__, __LINE__); if (!path_copy) { + if (dirfd != AT_FDCWD) close(dirfd); return -1; } @@ -945,8 +1761,15 @@ int secure_relative_open(const char *basedir, const char *relpath, int flags, mo dirfd = next_fd; } - // the path must be a directory - errno = EINVAL; + /* All components walked as directories. If the caller asked for + * O_DIRECTORY, return the dirfd we built up; otherwise the path + * resolved to a directory but the caller wanted a regular file. */ + if ((flags & O_DIRECTORY) && dirfd != AT_FDCWD) { + retfd = dirfd; + dirfd = AT_FDCWD; + goto cleanup; + } + errno = EISDIR; cleanup: free(path_copy); diff --git a/t_secure_relpath.c b/t_secure_relpath.c new file mode 100644 index 00000000..a0fdf0d2 --- /dev/null +++ b/t_secure_relpath.c @@ -0,0 +1,151 @@ +/* + * Test harness for secure_relative_open()'s front-door input + * validation. Codex audit Finding 5 noted that the existing check + * + * if (strncmp(relpath, "../", 3) == 0 || strstr(relpath, "/../")) + * + * catches "../foo" and "foo/../bar" but misses bare ".." (an actual + * one-level escape on platforms that fall back to the per-component + * walk), as well as "a/..", "foo/..", and any other form that + * decomposes to a ".." component when split on "/". The kernel- + * enforced RESOLVE_BENEATH (Linux 5.6+) and O_RESOLVE_BENEATH + * (FreeBSD 13+, macOS 15+) reject these in-kernel; the per- + * component fallback used on NetBSD, OpenBSD, Solaris, Cygwin and + * pre-5.6 Linux does not, so the validation must happen at the + * front door. + * + * This helper invokes secure_relative_open() with each suspect + * input and checks both the failure (rc < 0) and the errno + * (EINVAL means "rejected at the front door"). Pre-fix, the kernel + * may reject with a different errno (EXDEV from RESOLVE_BENEATH); + * post-fix, the front-door check catches every variant up front + * with a consistent EINVAL across platforms. + * + * Not linked into rsync itself. + */ + +#include "rsync.h" + +#include + +int dry_run = 0; +int am_root = 0; +int am_sender = 0; +int read_only = 0; +int list_only = 0; +int copy_links = 0; +int copy_unsafe_links = 0; +extern int am_daemon, am_chrooted; + +short info_levels[COUNT_INFO], debug_levels[COUNT_DEBUG]; + +static int errs = 0; + +static void check_relpath(const char *relpath) +{ + int fd; + int saved_errno; + + errno = 0; + fd = secure_relative_open(NULL, relpath, O_RDONLY | O_DIRECTORY, 0); + saved_errno = errno; + + if (fd >= 0) { + fprintf(stderr, + "FAIL [relpath=%-12s]: returned valid fd %d (escape) -- expected -1 EINVAL\n", + relpath, fd); + close(fd); + errs++; + return; + } + + if (saved_errno != EINVAL) { + fprintf(stderr, + "FAIL [relpath=%-12s]: rejected but errno=%d (%s), expected EINVAL\n", + relpath, saved_errno, strerror(saved_errno)); + errs++; + return; + } + + fprintf(stderr, "OK [relpath=%-12s]: rejected with EINVAL\n", relpath); +} + +static void check_basedir(const char *basedir) +{ + int fd; + int saved_errno; + + errno = 0; + fd = secure_relative_open(basedir, "ok", O_RDONLY | O_DIRECTORY, 0); + saved_errno = errno; + + if (fd >= 0) { + fprintf(stderr, + "FAIL [basedir=%-12s]: returned valid fd %d -- expected -1 EINVAL\n", + basedir, fd); + close(fd); + errs++; + return; + } + + if (saved_errno != EINVAL) { + fprintf(stderr, + "FAIL [basedir=%-12s]: rejected but errno=%d (%s), expected EINVAL\n", + basedir, saved_errno, strerror(saved_errno)); + errs++; + return; + } + + fprintf(stderr, "OK [basedir=%-12s]: rejected with EINVAL\n", basedir); +} + +int main(int argc, char **argv) +{ + if (argc != 2) { + fprintf(stderr, "usage: %s \n", argv[0]); + return 2; + } + if (chdir(argv[1]) < 0) { + perror("chdir"); + return 2; + } + + /* secure_relative_open's daemon-only confinement protections only + * fire when am_daemon && !am_chrooted (the threat model is the + * daemon-no-chroot deployment), but the front-door input + * validation runs unconditionally. We set am_daemon anyway so the + * helper exercises the same code shape the receiver does. */ + am_daemon = 1; + am_chrooted = 0; + + mkdir("subdir", 0755); + + /* Each of these relpaths must be rejected with EINVAL at the + * secure_relative_open() front door. ".." is the actual one-level + * escape; the others ("subdir/..", "subdir/../subdir") resolve + * back to the start dir on systems that allow them, but we still + * reject them as defence-in-depth: a path containing a ".." token + * is suspicious and the caller should normalise before passing + * it in. The "../foo" / "foo/../bar" / "/foo" / "/" cases are + * regression checks for the existing checks. */ + check_relpath(".."); + check_relpath("../foo"); + check_relpath("subdir/.."); + check_relpath("subdir/../subdir"); + check_relpath("foo/../bar"); + check_relpath("/foo"); + check_relpath("/"); + + /* Same checks against basedir (which the codex Finding 2 fix + * routes through the same RESOLVE_BENEATH-equivalent). Absolute + * basedirs are operator-trusted and intentionally not validated + * here. */ + check_basedir(".."); + check_basedir("../subdir"); + check_basedir("subdir/.."); + check_basedir("foo/../bar"); + + if (errs) + fprintf(stderr, "\n%d failure(s)\n", errs); + return errs ? 1 : 0; +} diff --git a/testsuite/alt-dest-symlink-race.test b/testsuite/alt-dest-symlink-race.test new file mode 100755 index 00000000..2256f2f2 --- /dev/null +++ b/testsuite/alt-dest-symlink-race.test @@ -0,0 +1,96 @@ +#!/bin/sh + +# Copyright (C) 2026 by Andrew Tridgell + +# This program is distributable under the terms of the GNU GPL (see +# COPYING). + +# Regression test for the basedir-confinement gap in +# secure_relative_open(). The function opens basedir with a plain +# openat(AT_FDCWD, basedir, O_RDONLY | O_DIRECTORY), without +# RESOLVE_BENEATH or a per-component O_NOFOLLOW walk, so a parent +# symlink ON basedir is followed unrestrictedly. RESOLVE_BENEATH is +# then applied only to relpath, anchored at the wrong directory. +# +# The receiver's basis-file lookup at receiver.c passes +# basis_dir[fnamecmp_type] (from --copy-dest / --link-dest / +# --compare-dest -- all sender-controllable in daemon mode) as +# basedir. A daemon-module attacker with write access can plant a +# symlink at module/cd -> /outside, then run --link-dest=cd to +# make the daemon's basis-file lookup resolve into /outside, +# leaking the contents of daemon-readable files via the rsync +# delta-rolling read-disclosure primitive. +# +# We detect the escape by leveraging --link-dest: when basis +# matches source exactly (content + mtime + mode), --link-dest +# hard-links the destination to the basis file. With the bug, the +# destination ends up as a hard link to the outside-the-module +# file (same inode). With the fix, no basis is found and the +# destination is a fresh copy (different inode). +# +# The vulnerable code path is the same on every platform +# (including the per-component fallback on systems without +# RESOLVE_BENEATH), so this test is not platform-gated. + +. "$suitedir/rsync.fns" + +mod="$scratchdir/module" +outside="$scratchdir/outside" +src="$scratchdir/src" +conf="$scratchdir/test-rsyncd.conf" + +rm -rf "$mod" "$outside" "$src" +mkdir -p "$mod" "$outside" "$src" + +# Portable inode-number helper (GNU coreutils stat -c, BSD stat -f). +file_inode() { + stat -c %i "$1" 2>/dev/null || stat -f %i "$1" +} + +# Outside-the-module file an attacker would like the daemon to +# treat as a basis. +echo "OUTSIDE_SECRET_DATA" > "$outside/target.txt" +chmod 0644 "$outside/target.txt" + +# The symlink trap planted in the module by the local attacker. +ln -s "$outside" "$mod/cd" + +# Source file matches outside/target.txt exactly (content + mtime +# + mode) so --link-dest will hard-link the destination to the +# basis file iff the daemon's basedir lookup reaches outside/. +echo "OUTSIDE_SECRET_DATA" > "$src/target.txt" +touch -r "$outside/target.txt" "$src/target.txt" +chmod 0644 "$src/target.txt" + +cat > "$conf" </dev/null 2>&1 || true + +if [ ! -f "$mod/target.txt" ]; then + test_fail "destination file was not created -- daemon transfer failed before the test could observe the basedir behaviour" +fi + +outside_inode=$(file_inode "$outside/target.txt") +dst_inode=$(file_inode "$mod/target.txt") + +if [ "$outside_inode" = "$dst_inode" ]; then + test_fail "basedir-escape: --link-dest hard-linked module/target.txt to outside/target.txt (inode $outside_inode); daemon's basis-file lookup followed the parent symlink on the basedir" +fi + +exit 0 diff --git a/testsuite/secure-relpath-validation.test b/testsuite/secure-relpath-validation.test new file mode 100755 index 00000000..5b77f7cc --- /dev/null +++ b/testsuite/secure-relpath-validation.test @@ -0,0 +1,34 @@ +#!/bin/sh + +# Copyright (C) 2026 by Andrew Tridgell + +# This program is distributable under the terms of the GNU GPL (see +# COPYING). + +# Regression test for codex audit Finding 5: secure_relative_open()'s +# front-door input check rejects "../foo" and "foo/../bar" but +# misses bare "..", "subdir/..", and other variants whose "/"-split +# components contain a literal "..". The kernel-enforced +# RESOLVE_BENEATH (Linux 5.6+) and O_RESOLVE_BENEATH +# (FreeBSD 13+, macOS 15+) reject these in-kernel; the per-component +# walk fallback used on NetBSD, OpenBSD, Solaris, Cygwin and pre-5.6 +# Linux does not -- so the validation must happen at the front door. +# +# This test invokes the t_secure_relpath helper, which calls +# secure_relative_open() with each suspect input and verifies the +# return value is -1 with errno == EINVAL. EINVAL is the marker +# that the front-door rejected the input, not the kernel; pre-fix +# the kernel returns -1 with EXDEV (or, on the per-component +# fallback, may return a valid fd at all -- "escape"). + +. "$suitedir/rsync.fns" + +testdir="$scratchdir/relpath-test" +rm -rf "$testdir" +mkdir -p "$testdir" + +if ! "$TOOLDIR/t_secure_relpath" "$testdir"; then + test_fail "t_secure_relpath rejected one or more inputs incorrectly (see stderr above for the specific case)" +fi + +exit 0 diff --git a/util1.c b/util1.c index 796604f6..f85f33e9 100644 --- a/util1.c +++ b/util1.c @@ -141,7 +141,7 @@ int set_times(const char *fname, STRUCT_STAT *stp) #ifdef HAVE_UTIMENSAT #include "case_N.h" - if (do_utimensat(fname, stp) == 0) + if (do_utimensat_at(fname, stp) == 0) break; if (errno != ENOSYS) return -1; @@ -479,13 +479,13 @@ int copy_file(const char *source, const char *dest, int tmpfilefd, mode_t mode) int robust_unlink(const char *fname) { #ifndef ETXTBSY - return do_unlink(fname); + return do_unlink_at(fname); #else static int counter = 1; int rc, pos, start; char path[MAXPATHLEN]; - rc = do_unlink(fname); + rc = do_unlink_at(fname); if (rc == 0 || errno != ETXTBSY) return rc; @@ -515,7 +515,7 @@ int robust_unlink(const char *fname) } /* maybe we should return rename()'s exit status? Nah. */ - if (do_rename(fname, path) != 0) { + if (do_rename_at(fname, path) != 0) { errno = ETXTBSY; return -1; } @@ -538,7 +538,7 @@ int robust_rename(const char *from, const char *to, const char *partialptr, return 0; while (tries--) { - if (do_rename(from, to) == 0) + if (do_rename_at(from, to) == 0) return 0; switch (errno) { @@ -559,7 +559,7 @@ int robust_rename(const char *from, const char *to, const char *partialptr, } if (copy_file(from, to, -1, mode) != 0) return -2; - do_unlink(from); + do_unlink_at(from); return 1; default: return -1; @@ -1333,20 +1333,20 @@ int handle_partial_dir(const char *fname, int create) dir = partial_fname; if (create) { STRUCT_STAT st; - int statret = do_lstat(dir, &st); + int statret = do_lstat_at(dir, &st); if (statret == 0 && !S_ISDIR(st.st_mode)) { - if (do_unlink(dir) < 0) { + if (do_unlink_at(dir) < 0) { *fn = '/'; return 0; } statret = -1; } - if (statret < 0 && do_mkdir(dir, 0700) < 0) { + if (statret < 0 && do_mkdir_at(dir, 0700) < 0) { *fn = '/'; return 0; } } else - do_rmdir(dir); + do_rmdir_at(dir); *fn = '/'; return 1; diff --git a/xattrs.c b/xattrs.c index e5d0dd43..5f740bb5 100644 --- a/xattrs.c +++ b/xattrs.c @@ -1249,7 +1249,12 @@ int set_stat_xattr(const char *fname, struct file_struct *file, mode_t new_mode) int x_stat(const char *fname, STRUCT_STAT *fst, STRUCT_STAT *xst) { - int ret = do_stat(fname, fst); + /* Use the *_at variants so that on a daemon-no-chroot deployment + * the metadata read goes through a secure parent dirfd instead + * of bare path resolution. The *_at wrappers fall through to + * plain do_stat outside the daemon-no-chroot context, so this + * change is transparent for non-daemon use. */ + int ret = do_stat_at(fname, fst); if ((ret < 0 || get_stat_xattr(fname, -1, fst, xst) < 0) && xst) xst->st_mode = 0; return ret; @@ -1257,7 +1262,7 @@ int x_stat(const char *fname, STRUCT_STAT *fst, STRUCT_STAT *xst) int x_lstat(const char *fname, STRUCT_STAT *fst, STRUCT_STAT *xst) { - int ret = do_lstat(fname, fst); + int ret = do_lstat_at(fname, fst); if ((ret < 0 || get_stat_xattr(fname, -1, fst, xst) < 0) && xst) xst->st_mode = 0; return ret;