From 3eeedf8d08178ff2f354c09be64eb0f56fa8454f Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Sun, 21 Jun 2026 15:13:27 +1000 Subject: [PATCH] daemon: robustness against malicious-peer protocol input Refuse malformed/hostile wire input that could crash or corrupt the receiver: io.c (out-of-range file index, count*blength OFF_T overflow, read_args NUL room, deferred in_multiplexed), flist.c (sub-flist after the final flist is freed, FLAG_HLINKED on dirs / gated on preserve_hard_links, parent_ndx bound, cleared- slot ndx in the transfer phase), hlink.c (undeclared cross-flist gnum -> error not assert), match.c (clamp peer flength, re-check len before want_i), log.c (drop peer-reachable asserts and F_SUM deref), exclude.c (merge-file recursion cap, trailing-backslash heap fix), lib/pool_alloc.c (ASan redzone for pool underflow). --- exclude.c | 95 +++++++++++++++++-- flist.c | 240 +++++++++++++++++++++++++++++++++++++++++++++-- hlink.c | 16 +++- io.c | 69 +++++++++++--- lib/pool_alloc.c | 45 ++++++++- log.c | 20 +++- match.c | 9 ++ rsync.h | 2 + 8 files changed, 462 insertions(+), 34 deletions(-) diff --git a/exclude.c b/exclude.c index 24de64f8..36917db8 100644 --- a/exclude.c +++ b/exclude.c @@ -71,6 +71,9 @@ static int dirbuf_depth; /* This is True when we're scanning parent dirs for per-dir merge-files. */ static BOOL parent_dirscan = False; +#define MAX_MERGE_DEPTH 32 +static int merge_depth = 0; + /* This array contains a list of all the currently active per-dir merge * files. This makes it easier to save the appropriate values when we * "push" down into each subdirectory. */ @@ -427,7 +430,7 @@ void add_implied_include(const char *arg, int skip_daemon_module) if (cp[1] == ']') { if (!saw_wild) cp++; /* A \] in a non-wild filter causes a problem, so drop the \ . */ - } else if (!strchr("*[?", cp[1])) { + } else if (!cp[1] || !strchr("*[?", cp[1])) { backslash_cnt++; if (saw_wild) *p++ = '\\'; @@ -1033,6 +1036,41 @@ int check_server_filter(filter_rule_list *listp, enum logcode code, const char * return ret; } +/* Returns 1 if `name` matches an implied-parent rule (a directory component + * seeded by add_implied_include() with FILTRULE_DIRECTORY) but not a leaf + * rule -- i.e. the client asked for something under the dir, never the dir + * itself as content. + * + * The receiver uses this to refuse a malicious sender that sets XMIT_TOP_DIR + * without XMIT_NO_CONTENT_DIR on such a dir: the honest encoding is both flags + * (flist.c send path), so otherwise the receiver would set FLAG_CONTENT_DIR + * and delete_in_dir() could sweep pre-existing siblings under --delete. */ +int is_implied_parent_dir(const char *name) +{ + filter_rule *ent; + int parent_match = 0; + + if (!implied_filter_list.head) + return 0; + + for (ent = implied_filter_list.head; ent; ent = ent->next) { + if (ent->rflags & (FILTRULE_PERDIR_MERGE | FILTRULE_CVS_IGNORE)) + continue; + if (!rule_matches(name, ent, NAME_IS_DIR)) + continue; + if (!(ent->rflags & FILTRULE_INCLUDE)) + continue; + if (ent->rflags & FILTRULE_DIRECTORY) { + parent_match = 1; + continue; + } + /* A non-DIRECTORY include rule = a leaf the client asked for, so + * the dir is legitimately in the list, not parent-only. */ + return 0; + } + return parent_match; +} + /* Return -1 if file "name" is defined to be excluded by the specified * exclude list, 1 if it is included, and 0 if it was not matched. */ int check_filter(filter_rule_list *listp, enum logcode code, @@ -1454,16 +1492,57 @@ void parse_filter_file(filter_rule_list *listp, const char *fname, const filter_ if (!fname || !*fname) return; + if (merge_depth >= MAX_MERGE_DEPTH) { + rprintf(FERROR, + "[%s] merge-file include depth limit (%d) exceeded at %s\n", + who_am_i(), MAX_MERGE_DEPTH, fname); + /* Match the failed-open path below: abort under a fatal + * (operator-supplied) merge, otherwise drop the rule. */ + if (xflags & XFLG_FATAL_ERRORS) + exit_cleanup(RERR_FILEIO); + return; + } + merge_depth++; + if (*fname != '-' || fname[1] || am_server) { + /* This path is operator- and (via per-directory merge files like + * .cvsignore) sender-controlled: a planted symlink could leak a + * root-readable file through the filter parser, or redirect an + * --exclude-from open via a planted parent. Refuse symlinks not + * owned by uid 0 or our euid. */ + const char *open_path; + int fd; if (daemon_filter_list.head) { + char *dir; strlcpy(line, fname, sizeof line); - clean_fname(line, CFN_COLLAPSE_DOT_DOT_DIRS); - if (check_filter(&daemon_filter_list, FLOG, line, 0) < 0) - fp = NULL; - else - fp = fopen(line, "rb"); + /* parse_merge_name() prepends module_dir for absolute paths, + * so strip module_dirlen back off before the check or the + * anchored module-relative daemon rule won't match (as + * options.c does for --exclude-from/--include-from). The + * original absolute path is still used for the open below. */ + dir = line + (*line == '/' ? module_dirlen : 0); + clean_fname(dir, CFN_COLLAPSE_DOT_DOT_DIRS); + if (check_filter(&daemon_filter_list, FLOG, dir, 0) < 0) { + /* Hidden by the daemon filter: treat the merge file as + * non-existent rather than tripping XFLG_FATAL_ERRORS + * below, so it neither errors out nor leaks a + * fatal-vs-silent oracle. */ + if (DEBUG_GTE(FILTER, 2)) { + rprintf(FINFO, + "[%s] parse_filter_file(%s) hidden by daemon filter\n", + who_am_i(), fname); + } + merge_depth--; + return; + } + open_path = line; } else - fp = fopen(fname, "rb"); + open_path = fname; + fd = open_no_attacker_symlinks(open_path, O_RDONLY, 0); + if (fd < 0) + fp = NULL; + else if (!(fp = fdopen(fd, "rb"))) + close(fd); } else fp = stdin; @@ -1481,6 +1560,7 @@ void parse_filter_file(filter_rule_list *listp, const char *fname, const filter_ fname); exit_cleanup(RERR_FILEIO); } + merge_depth--; return; } dirbuf[dirbuf_len] = '\0'; @@ -1517,6 +1597,7 @@ void parse_filter_file(filter_rule_list *listp, const char *fname, const filter_ break; } fclose(fp); + merge_depth--; } /* If the "for_xfer" flag is set, the prefix is made compatible with the diff --git a/flist.c b/flist.c index 346540fb..b1ebfdba 100644 --- a/flist.c +++ b/flist.c @@ -29,6 +29,11 @@ extern int am_root; extern int am_server; extern int am_daemon; +extern int am_chrooted; +extern char *module_dir; +extern unsigned int module_dirlen; +extern int module_dirfd; +extern unsigned int curr_dir_len; extern int am_sender; extern int am_generator; extern int inc_recurse; @@ -64,6 +69,7 @@ extern int non_perishable_cnt; extern int prune_empty_dirs; extern int copy_links; extern int copy_unsafe_links; +extern int insecure_links; extern int protocol_version; extern int sanitize_paths; extern int munge_symlinks; @@ -214,13 +220,47 @@ void show_flist_stats(void) * * The stat structure pointed to by stp will contain information about the * link or the referent as appropriate, if they exist. */ +/* Set by send_directory() to the fd of the directory it is currently scanning + * (and that dir's path prefix), so the per-entry stat can go through the + * already-open dir fd instead of re-resolving the full path for every entry. + * Pure performance and sender-side only -- the scanned dir is already open, so + * fstatat(scan_dirfd, basename) is identical to lstat(scandir/basename); no + * confinement is implied or needed. */ +static int scan_dirfd = -1; +static const char *scan_dir_prefix; +static int scan_dir_prefix_len; + +static int scan_link_stat(const char *path, STRUCT_STAT *stp, int follow_dirlinks) +{ + /* Use the held scan fd only for a single component directly inside the + * scanned dir, and only when am_root >= 0 (link_stat_at folds in no + * fake-super %stat xattr; link_stat does so via get_stat_xattr, a no-op + * once am_root >= 0). */ + if (scan_dirfd >= 0 && am_root >= 0 + && strncmp(path, scan_dir_prefix, scan_dir_prefix_len) == 0 + && path[scan_dir_prefix_len] == '/' + && strchr(path + scan_dir_prefix_len + 1, '/') == NULL) + return link_stat_at(scan_dirfd, path + scan_dir_prefix_len + 1, stp, follow_dirlinks); + return link_stat(path, stp, follow_dirlinks); +} + +static int scan_readlink(const char *path, char *linkbuf, size_t bufsiz) +{ + if (scan_dirfd >= 0 && am_root >= 0 + && strncmp(path, scan_dir_prefix, scan_dir_prefix_len) == 0 + && path[scan_dir_prefix_len] == '/' + && strchr(path + scan_dir_prefix_len + 1, '/') == NULL) + return do_readlink_atfd(scan_dirfd, path + scan_dir_prefix_len + 1, linkbuf, bufsiz); + return do_readlink(path, linkbuf, bufsiz); +} + static int readlink_stat(const char *path, STRUCT_STAT *stp, char *linkbuf) { #ifdef SUPPORT_LINKS - if (link_stat(path, stp, copy_dirlinks) < 0) + if (scan_link_stat(path, stp, copy_dirlinks) < 0) return -1; if (S_ISLNK(stp->st_mode)) { - int llen = do_readlink(path, linkbuf, MAXPATHLEN - 1); + int llen = scan_readlink(path, linkbuf, MAXPATHLEN - 1); if (llen < 0) return -1; linkbuf[llen] = '\0'; @@ -261,6 +301,30 @@ int link_stat(const char *path, STRUCT_STAT *stp, int follow_dirlinks) #endif } +/* Held-dirfd variant of link_stat(): stat single-component `name` relative to + * directory fd `dfd`, instead of re-resolving a full path. Equivalent to + * link_stat() only when NOT in --fake-super mode -- x_stat/x_lstat fold the + * fake-super %stat xattr into the result via get_stat_xattr(), which is a + * path-based no-op once am_root >= 0. Callers therefore use this only when + * am_root >= 0 (and a valid dfd), falling back to link_stat() otherwise. */ +int link_stat_at(int dfd, const char *name, STRUCT_STAT *stp, int follow_dirlinks) +{ +#ifdef SUPPORT_LINKS + if (copy_links) + return do_stat_atfd(dfd, name, stp); + if (do_lstat_atfd(dfd, name, stp) < 0) + return -1; + if (follow_dirlinks && S_ISLNK(stp->st_mode)) { + STRUCT_STAT st; + if (do_stat_atfd(dfd, name, &st) == 0 && S_ISDIR(st.st_mode)) + *stp = st; + } + return 0; +#else + return do_stat_atfd(dfd, name, stp); +#endif +} + static inline int path_is_daemon_excluded(char *path, int ignore_filename) { if (daemon_filter_list.head) { @@ -776,7 +840,7 @@ static struct file_struct *recv_file_entry(int f, struct file_list *flist, int x if ((basename = strrchr(thisname, '/')) != NULL) { int len = basename++ - thisname; - if (len != lastdir_len || memcmp(thisname, lastdir, len) != 0) { + if (len != lastdir_len || !lastdir || memcmp(thisname, lastdir, len) != 0) { lastdir = new_array(char, len + 1); memcpy(lastdir, thisname, len); lastdir[len] = '\0'; @@ -1055,7 +1119,8 @@ static struct file_struct *recv_file_entry(int f, struct file_list *flist, int x memcpy(bp, basename, basename_len); #ifdef SUPPORT_HARD_LINKS - if (xflags & XMIT_HLINKED + if (preserve_hard_links && xflags & XMIT_HLINKED + && !S_ISDIR(mode) #ifndef CAN_HARDLINK_SYMLINK && !S_ISLNK(mode) #endif @@ -1110,6 +1175,26 @@ static struct file_struct *recv_file_entry(int f, struct file_list *flist, int x if (basename_len == 1+1 && *basename == '.') /* +1 for '\0' */ F_DEPTH(file)--; if (protocol_version >= 30) { + /* Stop a malicious sender expanding --delete scope by flagging + * an implied parent as a content dir: if we only allowed this + * entry as a parent of the requested leaf, force the flags back + * to the honest implied-parent encoding (XMIT_TOP_DIR | + * XMIT_NO_CONTENT_DIR) so it lands in FLAG_IMPLIED_DIR, not + * FLAG_CONTENT_DIR, and delete_in_dir() can't sweep siblings. + * Not gated on trust_sender_filter: implied_filter_list is + * receiver-owned state, so a per-dir filter must not be able to + * downgrade this defense. */ + if (implied_filter_list.head + && is_implied_parent_dir(thisname) + && (!(xflags & XMIT_NO_CONTENT_DIR) || !(xflags & XMIT_TOP_DIR))) { + if (DEBUG_GTE(FILTER, 1)) { + rprintf(FINFO, + "[%s] receiver downgraded implied-parent dir %s " + "to non-content (sender xflags=0x%x)\n", + who_am_i(), thisname, xflags); + } + xflags |= XMIT_NO_CONTENT_DIR | XMIT_TOP_DIR; + } if (!(xflags & XMIT_NO_CONTENT_DIR)) { if (xflags & XMIT_TOP_DIR) file->flags |= FLAG_TOP_DIR; @@ -1400,7 +1485,7 @@ struct file_struct *make_file(const char *fname, struct file_list *flist, if ((basename = strrchr(thisname, '/')) != NULL) { int len = basename++ - thisname; - if (len != lastdir_len || memcmp(thisname, lastdir, len) != 0) { + if (len != lastdir_len || !lastdir || memcmp(thisname, lastdir, len) != 0) { lastdir = new_array(char, len + 1); memcpy(lastdir, thisname, len); lastdir[len] = '\0'; @@ -1848,6 +1933,77 @@ static void interpret_stat_error(const char *fname, int is_dir) } } +#if defined HAVE_FDOPENDIR && defined HAVE_DIRFD +/* Open a source directory for scanning confined beneath the transfer root. + * secure_relative_open() does a per-component O_NOFOLLOW walk that refuses a + * parent component raced into a symlink pointing out of the tree; fdopendir() + * then turns the held fd into the DIR* the scan reads. This mirrors the + * sender's confined content open (sender.c): the directory enumeration must be + * confined the same way, or a parent-symlink race (or, for a daemon following + * mode, an in-module symlink to outside) lets the scan enumerate an out-of-tree + * directory and leak its names/metadata/symlink targets. O_DIRECTORY without + * O_NOFOLLOW makes secure_relative_open() follow in-tree directory symlinks + * beneath the anchor and refuse escapes, so this serves both the default + * no-follow scan and a daemon's symlink-following scan (see the caller). + * Returns NULL with errno set on failure, like opendir(). */ +static DIR *secure_opendir(const char *fbuf) +{ + int dfd, fl; + DIR *d; + + if (am_daemon && (!am_chrooted || module_dirlen) + && module_dir && module_dir[0] == '/' && *fbuf != '/' && module_dirfd >= 0 + && curr_dir_len >= module_dirlen + && strncmp(curr_dir, module_dir, module_dirlen) == 0 + && (curr_dir[module_dirlen] == '\0' || curr_dir[module_dirlen] == '/')) { + /* Daemon: anchor the confined scan at the module root pinned by identity + * at module setup (module_dirfd, opened while the daemon was positioned + * there and still privileged), and walk the module-relative path of the + * scan target beneath it. This re-follows the same in-module path -- so a + * legitimate in-module ".." climb (sub/climb -> ../sibling) or an in-module + * directory symlink is followed, and an escape refused -- without + * re-walking the absolute module path as the dropped uid (the privilege- + * drop EACCES), and without assuming the lexical curr_dir depth matches the + * real cwd (a followed in-module symlink can desync them; anchoring at the + * pinned module root and walking down the logical path is correct either + * way). */ + const char *p = curr_dir + module_dirlen; + char modrel[MAXPATHLEN]; + while (*p == '/') + p++; + if ((size_t)snprintf(modrel, sizeof modrel, "%s%s%s", + p, *p ? "/" : "", fbuf) >= sizeof modrel) { + errno = ENAMETOOLONG; + return NULL; + } + dfd = secure_relative_open_at(module_dirfd, *modrel ? modrel : ".", + O_RDONLY | O_DIRECTORY, 0); + } else if (*fbuf == '/') { + /* An absolute scan path (an absolute --relative / --files-from name, or a + * "/" transfer root): anchor at "/" -- operator-named, trusted. */ + const char *relp = fbuf; + while (*relp == '/') + relp++; + dfd = secure_relative_open("/", relp, O_RDONLY | O_DIRECTORY, 0); + } else { + /* Non-daemon (or chrooted) sender: confine beneath the cwd the sender + * chdir'd into (the transfer root). */ + dfd = secure_relative_open(NULL, fbuf, O_RDONLY | O_DIRECTORY, 0); + } + + if (dfd < 0) + return NULL; + if ((fl = fcntl(dfd, F_GETFD)) >= 0) + fcntl(dfd, F_SETFD, fl | FD_CLOEXEC); + if (!(d = fdopendir(dfd))) { + int save = errno; + close(dfd); + errno = save; + } + return d; +} +#endif + /* This function is normally called by the sender, but the receiving side also * calls it from get_dirlist() with f set to -1 so that we just construct the * file list in memory without sending it over the wire. Also, get_dirlist() @@ -1866,7 +2022,31 @@ static void send_directory(int f, struct file_list *flist, char *fbuf, int len, assert(flist != NULL); - if (!(d = opendir(fbuf))) { +#if defined HAVE_FDOPENDIR && defined HAVE_DIRFD + /* Confine the enumeration beneath the transfer root. secure_opendir() + * follows in-tree directory symlinks (RESOLVE_BENEATH) and refuses one that + * escapes, so it serves both modes: + * - a daemon/hardened sender (secure_relpath_active()) is confined to the + * module in EVERY mode -- including -L/--copy-dirlinks/--copy-unsafe- + * links, matching the content open (sender_open_copylinks_confined) -- + * so a following mode cannot be lured to enumerate outside the module; + * - a non-daemon sender is confined in the default no-follow mode; its + * symlink-following modes intentionally dereference out of the + * operator's own tree, so they keep the legacy opendir(). + * f >= 0 is the sender's outgoing scan; get_dirlist() passes f < 0 and keeps + * the legacy opendir(). A module opted out of confinement ("insecure links = + * yes", admin-only) -- or a non-daemon --insecure-links -- uses the legacy + * opendir() too, restoring the pre-hardening enumeration (re-opening the + * escape; documented). */ + if (f >= 0 && !symlink_optout_allowed() && (secure_relpath_active() + || !(copy_links || copy_unsafe_links || copy_dirlinks || insecure_links))) + d = secure_opendir(fbuf); + else + d = opendir(fbuf); +#else + d = opendir(fbuf); +#endif + if (!d) { if (errno == ENOENT) { if (am_sender) /* Can abuse this for vanished error w/ENOENT: */ interpret_stat_error(fbuf, True); @@ -1889,6 +2069,14 @@ static void send_directory(int f, struct file_list *flist, char *fbuf, int len, } else remainder = 0; +#ifdef HAVE_DIRFD + /* Let the per-entry stat (readlink_stat -> scan_link_stat) go through the + * already-open directory fd instead of re-resolving fbuf for each name. */ + scan_dirfd = dirfd(d); + scan_dir_prefix = fbuf; + scan_dir_prefix_len = len; +#endif + for (errno = 0, di = readdir(d); di; errno = 0, di = readdir(d)) { unsigned name_len; char *dname = d_name(di); @@ -1917,6 +2105,9 @@ static void send_directory(int f, struct file_list *flist, char *fbuf, int len, send_file_name(f, flist, fbuf, NULL, flags, filter_level); } + scan_dirfd = -1; /* fbuf is about to be reused / d closed */ + scan_dir_prefix = NULL; /* and don't leave the global pointing into fbuf */ + scan_dir_prefix_len = 0; fbuf[len] = '\0'; if (errno) { @@ -2050,7 +2241,8 @@ static void send1extra(int f, struct file_struct *file, struct file_list *flist) int len, dlen, flags = FLAG_DIVERT_DIRS | FLAG_CONTENT_DIR; size_t j; - f_name(file, fbuf); + if (!f_name(file, fbuf)) + return; dlen = strlen(fbuf); if (!change_pathname(file, NULL, 0)) @@ -2620,11 +2812,33 @@ struct file_list *recv_file_list(int f, int dir_ndx) #endif if (inc_recurse && dir_ndx >= 0) { + if (!first_flist) { + /* All flists have already been freed via the NDX_DONE + * chain, so dir_flist is stale: its files[] entries + * point into a destroyed pool. A sub-flist marker now + * is a protocol violation (and would otherwise UAF the + * stale dir entry below, then deref an uninitialised + * slot in the freshly reset dir_flist further down). */ + rprintf(FERROR_XFER, + "rsync: refusing sub-flist after final flist was freed\n"); + exit_cleanup(RERR_PROTOCOL); + } if (dir_ndx >= dir_flist->used) { rprintf(FERROR_XFER, "rsync: refusing invalid dir_ndx %u >= %u\n", dir_ndx, dir_flist->used); exit_cleanup(RERR_PROTOCOL); } struct file_struct *file = dir_flist->files[dir_ndx]; + if (!F_IS_ACTIVE(file)) { + /* flist_sort_and_clean() can clear_file() a directory + * entry that was a duplicate or otherwise pruned, but + * the cleared file_struct stays in dir_flist. A peer + * that then sends a sub-flist for that slot would make + * f_name() return NULL into the dirname strcmp() below. */ + rprintf(FERROR_XFER, + "rsync: refusing flist for cleared dir_ndx %d\n", + dir_ndx); + exit_cleanup(RERR_PROTOCOL); + } if (file->flags & FLAG_GOT_DIR_FLIST) { rprintf(FERROR_XFER, "rsync: refusing malicious duplicate flist for dir %d\n", dir_ndx); exit_cleanup(RERR_PROTOCOL); @@ -2686,7 +2900,7 @@ struct file_list *recv_file_list(int f, int dir_ndx) cur_dir++; if (cur_dir != good_dirname) { const char *d = dir_ndx >= 0 ? f_name(dir_flist->files[dir_ndx], NULL) : empty_dir; - if (strcmp(cur_dir, d) != 0) { + if (!d || strcmp(cur_dir, d) != 0) { rprintf(FERROR, "ABORTING due to invalid path from sender: %s/%s\n", cur_dir, file->basename); @@ -2776,7 +2990,15 @@ struct file_list *recv_file_list(int f, int dir_ndx) if (!ignore_errors) io_error |= err; } else if (inc_recurse && flist->ndx_start == 1) { - if (!file_total || strcmp(flist->sorted[flist->low]->basename, ".") != 0) + /* The first inc_recurse flist has no parent in dir_flist; a + * malicious peer can send a "." entry whose mode is not a + * directory, so it never lands in dir_flist (used stays 0) yet + * the basename test below still passes. That left parent_ndx at + * its default 0 and the consumers dereferenced dir_flist->files[0] + * = uninitialised heap. Require dir_flist to actually hold an + * entry before trusting index 0. */ + if (!file_total || !dir_flist->used + || strcmp(flist->sorted[flist->low]->basename, ".") != 0) flist->parent_ndx = -1; } diff --git a/hlink.c b/hlink.c index eb36730f..b3d1974d 100644 --- a/hlink.c +++ b/hlink.c @@ -125,8 +125,22 @@ static void match_gnums(int32 *ndx_list, int ndx_count) if (inc_recurse) { node = hashtable_find(prior_hlinks, gnum, data_when_new); if (node->data == data_when_new) { + if (gnum < hlink_flist->ndx_start) { + /* A non-first hard-link entry whose + * gnum points before this flist's + * ndx_start should already have been + * recorded in prior_hlinks by an + * earlier flist. A peer that sends + * such a back-reference on the first + * flist (or to a gnum that was never + * declared XMIT_HLINK_FIRST) is + * misbehaving. */ + rprintf(FERROR, + "hard-link gnum %d precedes flist start %d\n", + (int)gnum, (int)hlink_flist->ndx_start); + exit_cleanup(RERR_PROTOCOL); + } node->data = new_array0(char, 5); - assert(gnum >= hlink_flist->ndx_start); file->flags |= FLAG_HLINK_FIRST; prev = -1; } else if (CVAL(node->data, 0) == 0) { diff --git a/io.c b/io.c index 2dfeeaa3..88a0d8fe 100644 --- a/io.c +++ b/io.c @@ -1076,11 +1076,17 @@ void send_msg_success(const char *fname, int num) if (DEBUG_GTE(IO, 1)) rprintf(FINFO, "[%s] send_msg_success(%d)\n", who_am_i(), num); - if (stat(fname, &st) < 0) - memset(&st, 0, sizeof (STRUCT_STAT)); + /* The dev/ino is consumed only by the sender's --remove-source-files + * same-file safety check (successful_send), so skip the per-file + * stat entirely otherwise -- it's sent but never read. */ + if (remove_source_files && stat(fname, &st) == 0) { + SIVAL64(num_dev_ino_buf, 4, st.st_dev); + SIVAL64(num_dev_ino_buf, 4+8, st.st_ino); + } else { + SIVAL64(num_dev_ino_buf, 4, 0); + SIVAL64(num_dev_ino_buf, 4+8, 0); + } SIVAL(num_dev_ino_buf, 0, num); - SIVAL64(num_dev_ino_buf, 4, st.st_dev); - SIVAL64(num_dev_ino_buf, 4+8, st.st_ino); send_msg(MSG_SUCCESS, num_dev_ino_buf, sizeof num_dev_ino_buf, -1); } else send_msg_int(MSG_SUCCESS, num); @@ -1305,6 +1311,8 @@ static void unbackslash_arg(char *s) *t = '\0'; } +#define MAX_DAEMON_ARGS (MAX_ARGS * 16) + void read_args(int f_in, char *mod_name, char *buf, size_t bufsiz, int rl_nulls, int unescape, char ***argv_p, int *argc_p, char **request_p) { @@ -1328,6 +1336,11 @@ void read_args(int f_in, char *mod_name, char *buf, size_t bufsiz, int rl_nulls, if (read_line(f_in, buf, bufsiz, rl_flags) == 0) break; + if (mod_name && argc >= MAX_DAEMON_ARGS - 1) { + rprintf(FERROR, "too many daemon arguments\n"); + exit_cleanup(RERR_PROTOCOL); + } + if (argc == maxargs-1) { maxargs += MAX_ARGS; argv = realloc_array(argv, char *, maxargs); @@ -1358,6 +1371,13 @@ void read_args(int f_in, char *mod_name, char *buf, size_t bufsiz, int rl_nulls, dot_pos = argc; } } + /* glob_expand()/glob_match() reserve glob.argc+1 slots -- room for the + * entry being added but not for this trailing NULL. A post-dot line + * whose " mod/" splits land argc on exactly maxargs (or any later + * ENSURE_MEMSPACE doubling boundary) would otherwise make the next + * store an 8-byte NULL write one slot past the argv allocation. */ + if (argc >= maxargs) + argv = realloc_array(argv, char *, maxargs = argc + 1); argv[argc] = NULL; glob_expand(NULL, NULL, NULL, NULL); @@ -1543,10 +1563,10 @@ static void read_a_msg(void) if (msg_bytes != 4) goto invalid_msg; val = raw_read_int(); - iobuf.in_multiplexed = 1; io_error |= val; if (am_receiver) send_msg_int(MSG_IO_ERROR, val); + iobuf.in_multiplexed = 1; break; case MSG_IO_TIMEOUT: if (msg_bytes != 4 || am_server || am_generator) @@ -1563,17 +1583,17 @@ static void read_a_msg(void) /* Support protocol-30 keep-alive method. */ if (msg_bytes != 0) goto invalid_msg; - iobuf.in_multiplexed = 1; if (am_sender) maybe_send_keepalive(time(NULL), MSK_ALLOW_FLUSH); + iobuf.in_multiplexed = 1; break; case MSG_DELETED: if (msg_bytes >= sizeof data) goto overflow; if (am_generator) { raw_read_buf(data, msg_bytes); - iobuf.in_multiplexed = 1; send_msg(MSG_DELETED, data, msg_bytes, 1); + iobuf.in_multiplexed = 1; break; } #ifdef ICONV_OPTION @@ -1611,7 +1631,6 @@ static void read_a_msg(void) } else #endif raw_read_buf(data, msg_bytes); - iobuf.in_multiplexed = 1; /* A directory name was sent with the trailing null */ if (msg_bytes > 0 && !data[msg_bytes-1]) log_delete(data, S_IFDIR); @@ -1619,6 +1638,7 @@ static void read_a_msg(void) data[msg_bytes] = '\0'; log_delete(data, S_IFREG); } + iobuf.in_multiplexed = 1; break; case MSG_SUCCESS: if (msg_bytes != (local_server ? 4+8+8 : 4)) { @@ -1640,11 +1660,11 @@ static void read_a_msg(void) if (msg_bytes != 4) goto invalid_msg; val = raw_read_int(); - iobuf.in_multiplexed = 1; if (am_generator) got_flist_entry_status(FES_NO_SEND, val); else send_msg_int(MSG_NO_SEND, val); + iobuf.in_multiplexed = 1; break; case MSG_ERROR_SOCKET: case MSG_ERROR_UTF8: @@ -2052,6 +2072,21 @@ void read_sum_head(int f, struct sum_struct *sum) (long)sum->blength, who_am_i()); exit_cleanup(RERR_PROTOCOL); } + if (sum->count && sum->blength == 0) { + rprintf(FERROR, "Invalid zero block length [%s]\n", + who_am_i()); + exit_cleanup(RERR_PROTOCOL); + } +#if SIZEOF_CAPITAL_OFF_T < 8 + /* The append-mode callers compute (OFF_T)count * blength; on a 32-bit + * OFF_T that product can wrap even though both factors are individually + * in range, corrupting the lseek/loop bounds. Reject it early. */ + if (sum->blength > 0 && sum->count > MAX_INT32 / sum->blength) { + rprintf(FERROR, "checksum count*blength overflows OFF_T [%s]\n", + who_am_i()); + exit_cleanup(RERR_PROTOCOL); + } +#endif sum->s2length = protocol_version < 27 ? csum_length : (int)read_int(f); if (sum->s2length < 0 || sum->s2length > xfer_sum_len) { rprintf(FERROR, "Invalid checksum length %d [%s]\n", @@ -2366,6 +2401,7 @@ int32 read_ndx(int f) { static int32 prev_positive = -1, prev_negative = 1; int32 *prev_ptr, num; + uint32 unum; char b[4]; if (protocol_version < 30) @@ -2385,11 +2421,20 @@ int32 read_ndx(int f) b[3] = CVAL(b, 0) & ~0x80; b[0] = b[1]; read_buf(f, b+1, 2); - num = IVAL(b, 0); + unum = IVAL(b, 0); } else - num = (UVAL(b,0)<<8) + UVAL(b,1) + *prev_ptr; + unum = (UVAL(b,0)<<8) + UVAL(b,1) + (uint32)*prev_ptr; } else - num = UVAL(b, 0) + *prev_ptr; + unum = UVAL(b, 0) + (uint32)*prev_ptr; + /* A peer-supplied index that overflows a signed int32 (used unchecked as a + * file-list index) is a protocol violation -- reject it here rather than + * relying on every downstream consumer to bounds-check. */ + if (unum > (uint32)MAX_INT32) { + rprintf(FERROR, "Invalid file index: %lu [%s]\n", + (unsigned long)unum, who_am_i()); + exit_cleanup(RERR_PROTOCOL); + } + num = (int32)unum; *prev_ptr = num; if (prev_ptr == &prev_negative) num = -num; diff --git a/lib/pool_alloc.c b/lib/pool_alloc.c index b49e3a7b..b663ab68 100644 --- a/lib/pool_alloc.c +++ b/lib/pool_alloc.c @@ -44,6 +44,32 @@ struct align_test { #define PTR_ADD(b,o) ( (void*) ((char*)(b) + (o)) ) #define PTR_SUB(b,o) ( (void*) ((char*)(b) - (o)) ) +/* Under AddressSanitizer, fence each pool_alloc() chunk with a poisoned + * redzone just below it (allocations grow downward from the top of an extent). + * A bump allocator hands out chunks from one big malloc, so ASan cannot see a + * write that underflows one chunk into its neighbour -- e.g. a miscomputed + * F_SUM() reaching before a file_struct's extras. The redzone turns that into + * a hard ASan report. We unpoison a whole extent whenever its space is reused + * (reset/reclaim), so legitimate later allocations never trip over old + * redzones; ASan unpoisons freed extents itself via free(). */ +#if defined(__SANITIZE_ADDRESS__) +# define POOL_ASAN 1 +#elif defined(__has_feature) +# if __has_feature(address_sanitizer) +# define POOL_ASAN 1 +# endif +#endif + +#ifdef POOL_ASAN +# include +# define POOL_REDZONE 16 /* >= the largest pool-relative underflow we guard */ +# define POOL_POISON(p,n) ASAN_POISON_MEMORY_REGION((p), (n)) +# define POOL_UNPOISON(p,n) ASAN_UNPOISON_MEMORY_REGION((p), (n)) +#else +# define POOL_POISON(p,n) ((void)0) +# define POOL_UNPOISON(p,n) ((void)0) +#endif + alloc_pool_t pool_create(size_t size, size_t quantum, void (*bomb)(const char*, const char*, int), int flags) { @@ -165,7 +191,18 @@ pool_alloc(alloc_pool_t p, size_t len, const char *bomb_msg) pool->extents->free -= len; - return PTR_ADD(pool->extents->start, pool->extents->free); + { + void *ret = PTR_ADD(pool->extents->start, pool->extents->free); +#ifdef POOL_ASAN + size_t rz = pool->extents->free < POOL_REDZONE + ? pool->extents->free : POOL_REDZONE; + if (rz) { + pool->extents->free -= rz; + POOL_POISON(PTR_ADD(pool->extents->start, pool->extents->free), rz); + } +#endif + return ret; + } bomb_out: if (pool->bomb) @@ -215,6 +252,10 @@ pool_free(alloc_pool_t p, size_t len, void *addr) if (!cur) return; + /* This extent's space may be reused (and POOL_CLEAR may memset it) + * below, so drop any redzones in it first. */ + POOL_UNPOISON(cur->start, pool->size); + if (!prev) { /* The "live" extent is kept ready for more allocations. */ if (cur->free + cur->bound + len >= pool->size) { @@ -272,6 +313,8 @@ pool_free_old(alloc_pool_t p, void *addr) if (!cur) return; + POOL_UNPOISON(cur->start, pool->size); + if (addr == PTR_ADD(cur->start, cur->free)) { if (prev) { prev->next = NULL; diff --git a/log.c b/log.c index 3e9a2edc..5d8672ea 100644 --- a/log.c +++ b/log.c @@ -154,7 +154,15 @@ static void syslog_init() static void logfile_open(void) { mode_t old_umask = umask(022 | orig_umask); - logfile_fp = fopen(logfile_name, "a"); + /* --log-file/`log file =` are operator-supplied paths that may transit + * attacker-writable dirs; a planted symlink could redirect root's log + * into e.g. /root/.ssh/authorized_keys. Refuse symlinks not owned by + * uid 0 or our euid. */ + int fd = open_no_attacker_symlinks(logfile_name, + O_WRONLY | O_APPEND | O_CREAT, 0644); + logfile_fp = fd >= 0 ? fdopen(fd, "a") : NULL; + if (!logfile_fp && fd >= 0) + close(fd); umask(old_umask); if (!logfile_fp) { int fopen_errno = errno; @@ -273,8 +281,12 @@ void rwrite(enum logcode code, const char *buf, int len, int is_utf8) if (am_daemon > 0 && code != FCLIENT) code = FLOG; } else if (send_msgs_to_gen) { - assert(!is_utf8); - /* Pass the message to our sibling in native charset. */ + /* Pass the message to our sibling in native charset. is_utf8 + * may be set here if a malicious peer sends MSG_INFO/MSG_ERROR + * to a daemon receiver (read_a_msg passes !am_generator); the + * old assert(!is_utf8) made that a remotely-reachable abort. + * Forwarding the bytes raw is safe -- the generator's rwrite() + * gets is_utf8=0 and filtered_fwrite escapes non-printables. */ send_msg((enum msgcode)code, buf, len, 0); return; } @@ -691,7 +703,7 @@ static void log_formatted(enum logcode code, const char *format, const char *op, case 'C': n = NULL; if (S_ISREG(file->mode)) { - if (always_checksum) + if (always_checksum && !(iflags & ITEM_DELETED)) n = sum_as_hex(file_sum_nni->num, F_SUM(file), 1); else if (iflags & ITEM_TRANSFER) n = sum_as_hex(xfer_sum_nni->num, sender_file_sum, 0); diff --git a/match.c b/match.c index 82a80a08..4fa3b859 100644 --- a/match.c +++ b/match.c @@ -325,6 +325,7 @@ static void hash_search(int f,struct sum_struct *s, && (!updating_basis_file || s->sums[want_i].offset >= offset || s->sums[want_i].flags & SUMFLG_SAME_OFFSET) && sum == s->sums[want_i].sum1 + && l == s->sums[want_i].len && memcmp(sum2, sum2_at(s, want_i), s->s2length) == 0) { /* we've found an adjacent match - the RLL coder * will be happy */ @@ -402,6 +403,14 @@ void match_sums(int f, struct sum_struct *s, struct map_struct *buf, OFF_T len) sum_init(xfer_sum_nni, checksum_seed); if (append_mode > 0) { + if (s->flength > len) { + /* A hostile or confused peer can claim a verified-prefix + * length that exceeds what we have on disk -- including + * for an empty local file, where buf is NULL and the + * map_ptr() calls below would dereference it. Clamp to + * what we can actually read. */ + s->flength = len; + } if (append_mode == 2) { OFF_T j = 0; for (j = CHUNK_SIZE; j < s->flength; j += CHUNK_SIZE) { diff --git a/rsync.h b/rsync.h index 89eceea4..a6bbf64d 100644 --- a/rsync.h +++ b/rsync.h @@ -708,6 +708,8 @@ typedef unsigned int size_t; # define SIZEOF_INT64 SIZEOF_OFF_T #endif +#define MAX_INT32 ((int32)0x7fffffff) + #define HT_KEY32 0 #define HT_KEY64 1