diff --git a/NEWS.md b/NEWS.md index d5c85526..6f83754a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -128,6 +128,19 @@ a symlink that a privileged rsync then follows: either (rsync does not follow a symlink there, and the options that would change that are refused in a restricted dir). +- A filter rule that failed to parse was echoed back verbatim, including when + the rule came from a merge file's contents. A per-directory merge rule names + a file the peer chooses and travels over the protocol rather than in an + argument, so this let a peer read back any line of any file the server process + could open that is not valid filter syntax -- through an `rrsync` restricted + account as well as a daemon module, since neither confines a merge open that + the wrapper never sees. A syntax error in a rule read from a file now reports + the file and line rather than the text; a rule given as an argument is still + shown. The `--debug=FILTER` traces print the same file-derived text, so + `rrsync` now refuses a peer-selected `--debug` (a stock client never sends + one). An operator who turns debugging on for their own server still sees the + rule text. + Daemon protocol / identity: - CVE-2026-53786 (LOW-MEDIUM): A client-supplied `--filter` merge file bypassed diff --git a/exclude.c b/exclude.c index 7d78e3a2..6d775b82 100644 --- a/exclude.c +++ b/exclude.c @@ -46,6 +46,96 @@ extern int operator_path_resolve; /* Set while the daemon loads its own filter parameters; see parse_filter_file(). */ int daemon_config_filter_file = 0; +/* Where the rule text now being parsed came from, when that is a file's + * CONTENTS rather than an argument. A rule that fails to parse used to be + * echoed back verbatim, and the peer chooses which file gets merged (a + * per-directory merge rule travels over the protocol, so no argument of ours + * ever names it), which made the filter parser a read-any-line oracle: any + * line that is not valid filter syntax came straight back in the error. + * Report where the bad rule is, not what it says. */ +static int rule_src_in_file = 0; /* parsing a file's contents right now */ +static const char *rule_src_file = NULL; /* ...and its name is safe to show */ +static int rule_src_line = 0; +/* Where a file whose own name we must NOT print was named, which is a location + * we CAN print: it keeps the diagnostic useful without echoing the pathname a + * merge rule supplied. */ +static const char *rule_src_named_at = NULL; + +/* True while the text we are handling came out of a file's contents: either we + * are parsing that file right now, or this is a deferred per-dir merge whose + * NAME came from one and which carries the provenance on the rule. */ +#define TEXT_FROM_FILE(template) \ + (rule_src_in_file \ + || ((template) && (template)->rflags & FILTRULE_FROM_FILE)) + +/* "FILE line N", or just "FILE" when the count is not a line count. */ +static const char *rule_src_where(void) +{ + static char buf[MAXPATHLEN + 32]; + + if (!rule_src_file) { + if (!rule_src_named_at) + return "a file read earlier"; /* origin not retained */ + snprintf(buf, sizeof buf, "a file named at %s", rule_src_named_at); + return buf; + } + if (rule_src_line < 0) + return rule_src_file; + snprintf(buf, sizeof buf, "%s line %d", rule_src_file, rule_src_line); + return buf; +} + +/* THE chokepoint. Every diagnostic string that is, or is built from, a filter + * rule's own text -- a pattern, a merge-file name, a path composed from one -- + * must be passed through rule_text() on its way to rprintf(). When the rule + * came from an argument the text is returned unchanged, because it is the + * user's own and hiding it only makes typos harder to fix. When it came from + * a FILE's contents it is replaced by a description of where it came from, + * because the peer chooses which file gets merged and any line of it that + * reaches a message is a line the peer can read back. + * + * Doing it here rather than at each site is the point: a message added later + * cannot reintroduce the leak by forgetting to check, and there is one place + * to audit. `template' is the rule the text belongs to, or NULL when the only + * thing that matters is whether we are parsing a file right now. + * + * The returned buffer is rotated, so two calls in one rprintf() are safe. */ +static const char *rule_text_len(const filter_rule *template, + const char *text, int len) +{ + static char buf[2][BIGPATHBUFLEN]; + static int which = 0; + char *b = buf[which]; + + which ^= 1; + if (!TEXT_FROM_FILE(template)) { + if (len < 0) + return text; + snprintf(b, sizeof buf[0], "%.*s", len, text); + return b; + } + snprintf(b, sizeof buf[0], "", rule_src_where()); + return b; +} + +static const char *rule_text(const filter_rule *template, const char *text) +{ + return rule_text_len(template, text, -1); +} + +/* For the extra detail some messages add ABOUT the text -- a character of it, + * an offset into it. Dropped along with the text it describes. */ +static const char *rule_detail(const filter_rule *template, const char *detail) +{ + return TEXT_FROM_FILE(template) ? "" : detail; +} + +static void filter_rule_err(const char *msg, const char *rulestr) +{ + rprintf(FERROR, "%s: %s\n", msg, rule_text(NULL, rulestr)); + exit_cleanup(RERR_SYNTAX); +} + extern char curr_dir[MAXPATHLEN]; extern unsigned int curr_dir_len; extern unsigned int module_dirlen; @@ -178,10 +268,10 @@ static void add_rule(filter_rule_list *listp, const char *pat, unsigned int pat_ else mention_rule_suffix = DEBUG_GTE(FILTER, 2) ? "" : NULL; if (mention_rule_suffix) { - rprintf(FINFO, "[%s] add_rule(%s%.*s%s)%s%s\n", - who_am_i(), get_rule_prefix(rule, pat, 0, NULL), - (int)pat_len, pat, (rule->rflags & FILTRULE_DIRECTORY) ? "/" : "", - listp->debug_type, mention_rule_suffix); + rprintf(FINFO, "[%s] add_rule(%s%s)%s%s\n", + who_am_i(), rule_detail(rule, get_rule_prefix(rule, pat, 0, NULL)), + rule_text_len(rule, pat, (int)pat_len), + listp->debug_type, rule_detail(rule, mention_rule_suffix)); } /* These flags also indicate that we're reading a list that @@ -286,7 +376,7 @@ static void add_rule(filter_rule_list *listp, const char *pat, unsigned int pat_ } lp = new_array0(filter_rule_list, 1); - if (asprintf(&lp->debug_type, " [per-dir %s]", cp) < 0) + if (asprintf(&lp->debug_type, " [per-dir %s]", rule_text(rule, cp)) < 0) out_of_memory("add_rule"); rule->u.mergelist = lp; @@ -603,7 +693,8 @@ static void pop_filter_list(filter_rule_list *listp) * value and will be updated with the length of the resulting name. We * always return a name that is null terminated, even if the merge_file * name was not. */ -static char *parse_merge_name(const char *merge_file, unsigned int *len_ptr, +static char *parse_merge_name(const filter_rule *template, + const char *merge_file, unsigned int *len_ptr, unsigned int prefix_skip) { static char buf[MAXPATHLEN]; @@ -634,7 +725,7 @@ static char *parse_merge_name(const char *merge_file, unsigned int *len_ptr, } if (!sanitize_path(fn, merge_file, r, dirbuf_depth, SP_DEFAULT)) { rprintf(FERROR, "merge-file name overflows: %s\n", - merge_file); + rule_text(template, merge_file)); return NULL; } fn_len = strlen(fn); @@ -647,7 +738,8 @@ static char *parse_merge_name(const char *merge_file, unsigned int *len_ptr, if (fn != buf) { int d_len = dirbuf_len - prefix_skip; if (d_len + fn_len >= MAXPATHLEN) { - rprintf(FERROR, "merge-file name overflows: %s\n", fn); + rprintf(FERROR, "merge-file name overflows: %s\n", + rule_text(template, fn)); return NULL; } memcpy(buf, dirbuf + prefix_skip, d_len); @@ -697,7 +789,7 @@ static BOOL setup_merge_file(int mergelist_num, filter_rule *ex, char *x, *y, *pat = ex->pattern; unsigned int len; - if (!(x = parse_merge_name(pat, NULL, 0)) || *x != '/') + if (!(x = parse_merge_name(ex, pat, NULL, 0)) || *x != '/') return 0; if (DEBUG_GTE(FILTER, 2)) { @@ -823,7 +915,7 @@ void *push_local_filters(const char *dir, unsigned int dirlen) io_error |= IOERR_GENERAL; rprintf(FERROR, "cannot add local filter rules in long-named directory: %s\n", - full_fname(dirbuf)); + rule_text(ex, full_fname(dirbuf))); } dirbuf[dirbuf_len] = '\0'; } @@ -1006,8 +1098,8 @@ static void report_filter_result(enum logcode code, char const *name, : "file"; rprintf(code, "[%s] %sing %s %s because of pattern %s%s%s\n", w, actions[*w=='g'][!(ent->rflags & FILTRULE_INCLUDE)], - t, name, ent->pattern, - ent->rflags & FILTRULE_DIRECTORY ? "/" : "", type); + t, name, rule_text(ent, ent->pattern), + rule_detail(ent, ent->rflags & FILTRULE_DIRECTORY ? "/" : ""), type); } } @@ -1169,6 +1261,8 @@ static filter_rule *parse_rule_tok(const char **rulestr_ptr, /* Inherit from the template. Don't inherit FILTRULES_SIDES; we check * that later. */ rule->rflags = template->rflags & FILTRULES_FROM_CONTAINER; + if (rule_src_in_file) + rule->rflags |= FILTRULE_FROM_FILE; /* before parse_merge_name() */ /* Figure out what kind of a filter rule "s" is pointing at. Note * that if FILTRULE_NO_PREFIXES is set, the rule is either an include @@ -1266,8 +1360,7 @@ static filter_rule *parse_rule_tok(const char **rulestr_ptr, rule->rflags |= FILTRULE_CLEAR_LIST; break; default: - rprintf(FERROR, "Unknown filter rule: `%s'\n", *rulestr_ptr); - exit_cleanup(RERR_SYNTAX); + filter_rule_err("Unknown filter rule", *rulestr_ptr); } while (ch != '!' && *++s && *s != ' ' && *s != '_') { if (template->rflags & FILTRULE_WORD_SPLIT && isspace(*s)) { @@ -1276,11 +1369,15 @@ static filter_rule *parse_rule_tok(const char **rulestr_ptr, } switch (*s) { default: - invalid: - rprintf(FERROR, - "invalid modifier '%c' at position %d in filter rule: %s\n", - *s, (int)(s - (const uchar *)*rulestr_ptr), *rulestr_ptr); + invalid: { + char where[32]; + snprintf(where, sizeof where, " '%c' at position %d", + *s, (int)(s - (const uchar *)*rulestr_ptr)); + rprintf(FERROR, "invalid modifier%s in filter rule: %s\n", + rule_detail(NULL, where), + rule_text(NULL, *rulestr_ptr)); exit_cleanup(RERR_SYNTAX); + } case '-': if (!BITS_SETnUNSET(rule->rflags, FILTRULE_MERGE_FILE, FILTRULE_NO_PREFIXES)) goto invalid; @@ -1352,10 +1449,8 @@ static filter_rule *parse_rule_tok(const char **rulestr_ptr, /* The filter and template both specify side(s). This * is dodgy (and won't work correctly if the template is * a one-sided per-dir merge rule), so reject it. */ - rprintf(FERROR, - "specified-side merge file contains specified-side filter: %s\n", - *rulestr_ptr); - exit_cleanup(RERR_SYNTAX); + filter_rule_err("specified-side merge file contains specified-side filter", + *rulestr_ptr); } rule->rflags |= template->rflags & FILTRULES_SIDES; } @@ -1372,15 +1467,12 @@ static filter_rule *parse_rule_tok(const char **rulestr_ptr, if (rule->rflags & FILTRULE_CLEAR_LIST) { if (!(template->rflags & FILTRULE_NO_PREFIXES) && !(xflags & XFLG_OLD_PREFIXES) && len) { - rprintf(FERROR, - "'!' rule has trailing characters: %s\n", *rulestr_ptr); - exit_cleanup(RERR_SYNTAX); + filter_rule_err("'!' rule has trailing characters", *rulestr_ptr); } if (len > 1) rule->rflags &= ~FILTRULE_CLEAR_LIST; } else if (!len && !(rule->rflags & FILTRULE_CVS_IGNORE)) { - rprintf(FERROR, "unexpected end of filter rule: %s\n", *rulestr_ptr); - exit_cleanup(RERR_SYNTAX); + filter_rule_err("unexpected end of filter rule", *rulestr_ptr); } /* --delete-excluded turns an un-modified include/exclude into a sender-side rule. */ @@ -1439,8 +1531,8 @@ void parse_filter_str(filter_rule_list *listp, const char *rulestr, break; if (pat_len >= MAXPATHLEN) { - rprintf(FERROR, "discarding over-long filter: %.*s\n", - (int)pat_len, pat); + rprintf(FERROR, "discarding over-long filter: %s\n", + rule_text_len(NULL, pat, (int)pat_len)); free_continue: free_filter(rule); continue; @@ -1477,7 +1569,7 @@ void parse_filter_str(filter_rule_list *listp, const char *rulestr, if (parent_dirscan) { const char *p; unsigned int len = pat_len; - if ((p = parse_merge_name(pat, &len, module_dirlen))) + if ((p = parse_merge_name(rule, pat, &len, module_dirlen))) add_rule(listp, p, len, rule, 0); else free_filter(rule); @@ -1486,7 +1578,7 @@ void parse_filter_str(filter_rule_list *listp, const char *rulestr, } else { const char *p; unsigned int len = pat_len; - if ((p = parse_merge_name(pat, &len, 0))) + if ((p = parse_merge_name(rule, pat, &len, 0))) parse_filter_file(listp, p, rule, XFLG_FATAL_ERRORS); free_filter(rule); continue; @@ -1507,6 +1599,14 @@ void parse_filter_file(filter_rule_list *listp, const char *fname, const filter_ char line[BIGPATHBUFLEN]; char *eob = line + sizeof line - 1; BOOL word_split = (template->rflags & FILTRULE_WORD_SPLIT) != 0; + const char *save_src_file, *save_src_named_at; + int save_src_line, save_src_in_file; + int named_by_file; + int pending = EOF; + char named_at[MAXPATHLEN + 32]; + /* Our own copy: fname may point into parse_merge_name()'s static buffer, + * which a merge rule inside THIS file overwrites while we still need it. */ + char src_name[MAXPATHLEN]; if (!fname || !*fname) return; @@ -1514,7 +1614,7 @@ void parse_filter_file(filter_rule_list *listp, const char *fname, const filter_ 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); + who_am_i(), MAX_MERGE_DEPTH, rule_text(template, fname)); /* Match the failed-open path below: abort under a fatal * (operator-supplied) merge, otherwise drop the rule. */ if (xflags & XFLG_FATAL_ERRORS) @@ -1547,9 +1647,12 @@ void parse_filter_file(filter_rule_list *listp, const char *fname, const filter_ * 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); + /* Same rule as everywhere else: the name is + * file content when a rule we read named it, + * and so is "the daemon filter hides it". */ + rprintf(FINFO, "[%s] parse_filter_file(%s)%s\n", + who_am_i(), rule_text(template, fname), + rule_detail(template, " hidden by daemon filter")); } merge_depth--; return; @@ -1583,17 +1686,31 @@ void parse_filter_file(filter_rule_list *listp, const char *fname, const filter_ fp = stdin; if (DEBUG_GTE(FILTER, 2)) { + /* The name is file CONTENT when a rule we read named it, and a + * word-split per-dir merge turns every word of a file into one + * of these -- so the trace would echo what the syntax errors no + * longer do. Say where it came from instead. */ rprintf(FINFO, "[%s] parse_filter_file(%s,%x,%x)%s\n", - who_am_i(), fname, template->rflags, xflags, - fp ? "" : " [not found]"); + who_am_i(), rule_text(template, fname), template->rflags, xflags, + rule_detail(template, fp ? "" : " [not found]")); } if (!fp) { if (xflags & XFLG_FATAL_ERRORS) { - rsyserr(FERROR, errno, - "failed to open %sclude file %s", - template->rflags & FILTRULE_INCLUDE ? "in" : "ex", - fname); + /* rule_src_file is still the PARENT's context here: when it + * is set, this name came out of a file we read, so neither + * the name nor errno (an existence oracle) may be shown. */ + if (TEXT_FROM_FILE(template)) { + /* errno too: it answers "does this path exist". */ + rprintf(FERROR, "failed to open %sclude file %s\n", + template->rflags & FILTRULE_INCLUDE ? "in" : "ex", + rule_text(template, fname)); + } else { + rsyserr(FERROR, errno, + "failed to open %sclude file %s", + template->rflags & FILTRULE_INCLUDE ? "in" : "ex", + fname); + } exit_cleanup(RERR_FILEIO); } merge_depth--; @@ -1601,11 +1718,43 @@ void parse_filter_file(filter_rule_list *listp, const char *fname, const filter_ } dirbuf[dirbuf_len] = '\0'; + /* Rule text from here on is this file's contents, not an argument, so + * a syntax error must not echo it. Saved and restored because a merge + * rule inside this file can bring us back in for another file. */ + save_src_in_file = rule_src_in_file; + save_src_file = rule_src_file; + save_src_line = rule_src_line; + /* If a rule we read named THIS file, our own path is file content too: + * track the location for provenance but do not put it in a message. */ + named_by_file = TEXT_FROM_FILE(template); + save_src_named_at = rule_src_named_at; + if (named_by_file) { + /* Snapshot where we were told to merge this, before that state + * is replaced below (rule_src_where returns a static buffer). + * A DEFERRED merge has no live location to point at -- the file + * that named it was read and finished long ago -- so leave the + * generic description rather than nesting two vague ones. */ + if (rule_src_in_file) { + strlcpy(named_at, rule_src_where(), sizeof named_at); + rule_src_named_at = named_at; + } else + rule_src_named_at = NULL; + } + strlcpy(src_name, fname, sizeof src_name); + rule_src_in_file = 1; + rule_src_file = named_by_file ? NULL : src_name; + rule_src_line = word_split ? -1 : 0; /* -1: tokens, not lines */ + while (1) { char *s = line; int ch, overflow = 0; + if (rule_src_line >= 0) + rule_src_line++; while (1) { - if ((ch = getc(fp)) == EOF) { + if (pending != EOF) { /* a CR lookahead we could not push back */ + ch = pending; + pending = EOF; + } else if ((ch = getc(fp)) == EOF) { if (ferror(fp) && errno == EINTR) { clearerr(fp); continue; @@ -1614,24 +1763,49 @@ void parse_filter_file(filter_rule_list *listp, const char *fname, const filter_ } if (word_split && isspace(ch)) break; - if (eol_nulls? !ch : (ch == '\n' || ch == '\r')) + if (eol_nulls? !ch : (ch == '\n' || ch == '\r')) { + if (ch == '\r') { /* CRLF is one line, not two */ + int nxt; + while ((nxt = getc(fp)) == EOF + && ferror(fp) && errno == EINTR) + clearerr(fp); + if (nxt == EOF) { + if (!ferror(fp)) + ch = EOF; /* real end of file */ + } else if (nxt != '\n' && ungetc(nxt, fp) == EOF) { + /* Pushback failed: hand it to the + * NEXT rule, where it belongs -- + * appending it here would both + * corrupt this rule and skip the + * s < eob bound below. */ + pending = nxt; + } + } break; + } if (s < eob) *s++ = ch; else overflow = 1; } if (overflow) { - rprintf(FERROR, "discarding over-long filter: %s...\n", line); + rprintf(FERROR, "discarding over-long filter: %s\n", + rule_text_len(NULL, line, 0)); s = line; } *s = '\0'; /* Skip an empty token and (when line parsing) comments. */ - if (*line && (word_split || (*line != ';' && *line != '#'))) + if (*line && (word_split || (*line != ';' && *line != '#'))) { + rule_src_file = named_by_file ? NULL : src_name; parse_filter_str(listp, line, template, xflags); + } if (ch == EOF) break; } + rule_src_in_file = save_src_in_file; + rule_src_file = save_src_file; + rule_src_line = save_src_line; + rule_src_named_at = save_src_named_at; fclose(fp); merge_depth--; } diff --git a/rsync.h b/rsync.h index ce39ee2b..b15aa1af 100644 --- a/rsync.h +++ b/rsync.h @@ -1052,6 +1052,7 @@ struct map_struct { #define FILTRULE_CLEAR_LIST (1<<18)/* this item is the "!" token */ #define FILTRULE_PERISHABLE (1<<19)/* perishable if parent dir goes away */ #define FILTRULE_XATTR (1<<20)/* rule only applies to xattr names */ +#define FILTRULE_FROM_FILE (1<<21)/* pattern text came from a file's contents */ #define FILTRULES_SIDES (FILTRULE_SENDER_SIDE | FILTRULE_RECEIVER_SIDE) diff --git a/testsuite/filter-merge-content-echo_test.py b/testsuite/filter-merge-content-echo_test.py new file mode 100644 index 00000000..d78a592b --- /dev/null +++ b/testsuite/filter-merge-content-echo_test.py @@ -0,0 +1,251 @@ +#!/usr/bin/env python3 +"""A merge file's contents must not come back in a filter syntax error. + +A per-directory merge rule names a file the peer chooses and that travels over +the protocol, so no argument of ours ever mentions it. Echoing an unparsable +line back made the filter parser a read-any-line oracle: every line that is not +valid filter syntax was returned verbatim to the client. The diagnostic must +say where the bad rule is, not what it says. + +Rules that came from an ARGUMENT are still echoed -- that text is the user's +own and hiding it would only make ordinary typos harder to fix. +""" + +import os +import subprocess + +from rsyncfns import SCRATCHDIR, makepath, rmtree, rsync_argv + +SECRET = 'TOP-SECRET-PASSWORD-abc123' + +base = SCRATCHDIR / 'filter-merge-content-echo' +rmtree(base) +src = base / 'src' +dest = base / 'dest' +makepath(src, dest) +(src / 'f').write_bytes(b'hi\n') + +# Not valid filter syntax, so the parser reports it -- this stands in for any +# non-filter file on the server: a key, a password store, a config. +secret = base / 'secret' +secret.write_text(SECRET + '\n') + +# The reachable shape: a per-dir merge file whose own contents name the target. +(src / '.rsync-filter').write_text(': ../secret\n') +got = subprocess.run(rsync_argv('-r', '-F', f'{src}/', f'{dest}/'), + capture_output=True, text=True, timeout=30) +out = got.stdout + got.stderr +assert got.returncode != 0, ( + f'the unparsable merge rule was accepted, so this test no longer ' + f'exercises the error path: rc={got.returncode}, out={out!r}') +assert SECRET not in out, ( + 'the contents of a merged file were echoed back in the filter error, so a ' + f'peer that can choose the merge path can read any line: out={out!r}') +# The merged file's own PATH came from a rule too (the .rsync-filter names it), +# so it cannot be printed either -- but the diagnostic still has to say enough +# to be actionable. +assert 'a file read earlier' in out, ( + 'the error says nothing at all about where the bad rule was, leaving no ' + f'way to act on it: out={out!r}') + +# Same for a merge file reached by an explicit argument: the text is still file +# content, and --filter='. FILE' is how an operator-supplied list is read. +(base / 'rules').write_text(SECRET + '\n') +got = subprocess.run( + rsync_argv('-r', f'--filter=. {base}/rules', f'{src}/', f'{dest}/'), + capture_output=True, text=True, timeout=30) +out = got.stdout + got.stderr +assert got.returncode != 0 and SECRET not in out, ( + f'a --filter=. merge file echoed its contents: rc={got.returncode}, ' + f'out={out!r}') +# This one WAS named by an argument, so its own path is the user's own text and +# is still shown -- the location must not be thrown away wholesale. +assert 'rules line 1' in out, ( + 'a merge file named by an argument should still be located precisely: ' + f'out={out!r}') + +# A file merged FROM a file cannot show its own path, but the place that named +# it can be shown, and is what the user needs to fix. +(base / 'outer-known').write_text(f'. {base}/inner-bad\n') +(base / 'inner-bad').write_text(SECRET + '\n') +got = subprocess.run( + rsync_argv('-r', f'--filter=. {base}/outer-known', f'{src}/', f'{dest}/'), + capture_output=True, text=True, timeout=30) +out = got.stdout + got.stderr +assert SECRET not in out and 'inner-bad' not in out, ( + f'the indirectly named merge file leaked its own path: out={out!r}') +assert 'a file named at' in out and 'outer-known line 1' in out, ( + 'the error does not point at the rule that named the bad file, which is ' + f'the only thing the user can act on: out={out!r}') + +# ...but a rule given AS an argument must still be shown, or an ordinary typo +# becomes undiagnosable. +got = subprocess.run( + rsync_argv('-r', '--filter=Zbogus-rule-text', f'{src}/', f'{dest}/'), + capture_output=True, text=True, timeout=30) +out = got.stdout + got.stderr +assert got.returncode != 0 and 'Zbogus-rule-text' in out, ( + 'a bad rule given on the command line is no longer echoed, which is a ' + f'usability regression rather than a leak: rc={got.returncode}, out={out!r}') + +# An ordinary, VALID merge file must still work -- the diagnostic change must +# not have broken merge parsing itself. +(src / '.rsync-filter').write_text('- *.tmp\n') +(src / 'keep.txt').write_bytes(b'keep\n') +(src / 'drop.tmp').write_bytes(b'drop\n') +ok = subprocess.run(rsync_argv('-r', '-F', f'{src}/', f'{dest}/'), + capture_output=True, text=True, timeout=30) +assert ok.returncode == 0, f'a valid merge file failed: out={ok.stderr!r}' +assert (dest / 'keep.txt').exists() and not (dest / 'drop.tmp').exists(), ( + 'the valid merge rule was not applied, so the parser is broken') + +# A merge rule INSIDE a merge file names its own file, so the failed-open +# message echoed text that came out of the file being read. A line of the +# target that happens to parse as a merge rule leaks through it. +(base / 'outer').write_text(f'. {base}/nested\n') +(base / 'nested').write_text(f'. {SECRET}\n') +got = subprocess.run( + rsync_argv('-r', f'--filter=. {base}/outer', f'{src}/', f'{dest}/'), + capture_output=True, text=True, timeout=30) +out = got.stdout + got.stderr +assert got.returncode != 0 and SECRET not in out, ( + 'a nested merge file named by file content was echoed in the failed-open ' + f'message: rc={got.returncode}, out={out!r}') + +# The reported location must be the file the bad rule is actually IN. fname +# can point into parse_merge_name()'s static buffer, which a merge rule inside +# the same file overwrites, so the error blamed the nested file instead. +(base / 'child').write_text('- harmless\n') +(base / 'parent').write_text(f'. {base}/child\nZbad-in-parent\n') +got = subprocess.run( + rsync_argv('-r', f'--filter=. {base}/parent', f'{src}/', f'{dest}/'), + capture_output=True, text=True, timeout=30) +out = got.stdout + got.stderr +assert 'parent line 2' in out, ( + 'the error blames the wrong file or line for a rule that follows a nested ' + f'merge, so the location it prints cannot be trusted: out={out!r}') + +# A CRLF pair is one line ending, not two. +(base / 'crlf').write_bytes(b'- harmless\r\nZbad-on-line-2\r\n') +got = subprocess.run( + rsync_argv('-r', f'--filter=. {base}/crlf', f'{src}/', f'{dest}/'), + capture_output=True, text=True, timeout=30) +out = got.stdout + got.stderr +assert 'crlf line 2' in out, f'CRLF miscounted the line: out={out!r}' + +# The FILTER2 traces print rule text as well, so a word-split merge -- whose +# every word becomes its own no-prefix rule -- had its contents echoed by +# add_rule() with nothing failing to parse at all. +(src / '.rsync-filter').write_text(':w- ../secret\n') +got = subprocess.run( + rsync_argv('-r', '-F', '--debug=FILTER2', f'{src}/', f'{dest}/'), + capture_output=True, text=True, timeout=30) +out = got.stdout + got.stderr +assert SECRET not in out, ( + 'the filter debug trace echoed the contents of a merged file, so the ' + f'oracle survives with --debug=FILTER2: out={out!r}') + +# The MATCH traces name the pattern that acted, and a pattern read from a merge +# file is that file's text. This one is not really debug-gated: plain -vv +# raises the filter level far enough for a sender or generator to print it, so +# a stock client reaches it with no --debug at all. +# +# The pattern is written as a glob whose LITERAL text cannot appear anywhere +# else -- the file it matches is named without the brackets -- so finding it in +# the output can only mean the rule text was echoed. +PATTERN_TEXT = 'sec[r]et-marker-9x' +MATCHED_NAME = 'secret-marker-9x' +rmtree(src) +makepath(src) +(src / 'f').write_bytes(b'keep\n') +(src / MATCHED_NAME).write_bytes(b'hidden\n') +(src / '.rsync-filter').write_text('- ' + PATTERN_TEXT + '\n') +got = subprocess.run(rsync_argv('-r', '-F', '-vv', f'{src}/', f'{dest}/'), + capture_output=True, text=True, timeout=30) +out = got.stdout + got.stderr +assert got.returncode == 0, f'the -vv transfer failed: out={out!r}' +assert 'because of' in out, ( + f'no match was reported, so this case exercises nothing: out={out!r}') +assert PATTERN_TEXT not in out, ( + 'the match trace echoed a pattern that came from a merge file, which plain ' + f'-vv is enough to reach: out={out!r}') +assert not (dest / MATCHED_NAME).exists(), ( + 'the rule did not actually hide the file, so the trace above was not the ' + 'one this checks') + +# A deferred per-dir merge (":" rule) is parsed long after the file that named +# it was read, so the parse-time context is gone by then and the redactions +# have to come off the RULE instead. Two leaks lived here: an over-long +# deferred name needed no verbosity at all, and -vvv printed the name plus +# whether it opened. +rmtree(src) +makepath(src / 'sub') +(src / 'sub' / 'f').write_bytes(b'x\n') +# The name has to survive parse_filter_str's own MAXPATHLEN check and only +# overflow once the scanned directory is prepended, which is a narrow window +# whose position depends on the scratch path and the platform's MAXPATHLEN -- +# so search for it rather than hard-coding a length. +maxpath = os.pathconf(str(base), 'PC_PATH_MAX') +overflowed = False +for slack in range(4, 80, 4): + pad = maxpath - len(str(src)) - slack + if pad < 32: + continue + (src / '.rsync-filter').write_text(': sub/DEFERRED-SECRET-' + 'Q' * pad + '\n') + got = subprocess.run(rsync_argv('-r', '-F', f'{src}/', f'{dest}/'), + capture_output=True, text=True, timeout=30) + out = got.stdout + got.stderr + assert 'DEFERRED-SECRET' not in out, ( + 'an over-long deferred merge name was echoed, with no verbosity ' + f'needed: out={out!r}') + if 'merge-file name overflows' in out: + overflowed = True + break +assert overflowed, ( + 'no length reached the deferred merge-name overflow, so that path was ' + 'never exercised -- widen the search') + +DEFERRED_NAME = 'named-from-f[i]le-x9' # the brackets exist only in the file +(src / '.rsync-filter').write_text(': ' + DEFERRED_NAME + '\n') +got = subprocess.run(rsync_argv('-r', '-F', '-vvv', f'{src}/', f'{dest}/'), + capture_output=True, text=True, timeout=30) +out = got.stdout + got.stderr +assert DEFERRED_NAME not in out, ( + 'the deferred per-dir merge name -- which came from a file -- was printed ' + f'by the -vvv trace, along with whether it opened: out={out!r}') + +# Details ABOUT a file-derived rule leak too, so they go through the same +# helper: the trailing-whitespace warning is computed from the rule's last +# byte, and would otherwise tell a peer whether a hidden rule ended in a space. +rmtree(src) +makepath(src) +(src / 'f').write_bytes(b'x\n') +(src / '.rsync-filter').write_text('- trailing-ws-probe \n') +got = subprocess.run( + rsync_argv('-r', '-F', '--debug=FILTER1', f'{src}/', f'{dest}/'), + capture_output=True, text=True, timeout=30) +out = got.stdout + got.stderr +assert 'trailing-ws-probe' not in out and 'CAUTION' not in out, ( + 'a property of a file-derived rule (its trailing whitespace) was reported, ' + f'which tells the peer about text it cannot otherwise see: out={out!r}') + +# ...but for the user's OWN rule the warning is exactly what they need. +got = subprocess.run( + rsync_argv('-r', '--debug=FILTER1', '--filter=- my-own-rule ', + f'{src}/', f'{dest}/'), + capture_output=True, text=True, timeout=30) +out = got.stdout + got.stderr +assert 'CAUTION' in out and 'my-own-rule' in out, ( + 'the trailing-whitespace warning was lost for an argument-supplied rule, ' + f'which is a usability regression: out={out!r}') + +# An over-long ARGUMENT rule must still be reported at full length -- the +# redaction helper must not quietly truncate the user's own text. +long_arg = 'ARGLONG-' + 'Q' * (maxpath + 200) +got = subprocess.run(rsync_argv('-r', f'--filter=- {long_arg}', f'{src}/', f'{dest}/'), + capture_output=True, text=True, timeout=30) +out = got.stdout + got.stderr +assert 'discarding over-long filter' in out, f'expected the over-long path: out={out!r}' +assert out.count('Q') > maxpath, ( + 'the over-long argument rule was truncated more aggressively than before, ' + f'losing the user\'s own text: {out.count("Q")} Qs of {maxpath + 200}')