mirror of
https://github.com/RsyncProject/rsync.git
synced 2026-09-13 05:38:57 -04:00
2979d8eddc278df2bb261ced46a191ebabc83ea6
7936
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Andrew Tridgell
|
2979d8eddc |
simd: stop the AVX2 rolling checksum reading past its buffer
The loop is software-pipelined: each iteration folds in the 64 bytes it preloaded last time and preloads the next 64. Nothing stopped the final iteration doing that preload, so it always read the 64 bytes after the region it was asked to checksum. Not an edge case. The assembly processes len&~63 and leaves the remainder to the caller, so the remainder is by construction under 64 bytes and the preload passed buf+len on every call, by 64 minus the remainder. It normally landed in slack inside the map_ptr() window and nothing noticed. Where the buffer ended near an unmapped page it was a SIGSEGV in the middle of a transfer -- reported on macOS x86-64 by Roland Kletzing, whose `partial` run died with "connection unexpectedly closed" because the generator had crashed. A guard page reproduces it on Linux too, so it was latent there, not absent. Run the pipelined loop one block short and finish the last block in .last, which does the same arithmetic without the preload. No per-iteration cost, and checksums are bit-identical -- simdtest compares every implementation against the C reference. The earlier fix for that report, "lib: use .balign in md5 x86-64 asm", was to the md5 assembly. It addressed the linker alignment warning that appeared alongside, not this. |
||
|
Andrew Tridgell
|
bb6329bc8c | NEWS: note the --link-dest hard-link fallback | ||
|
Andrew Tridgell
|
d12bdb1579 |
testsuite: cover the --link-dest hard-link refusal, and ask the filesystem
An LD_PRELOAD hook refuses linkat() for a symlink source only, so the arm is reachable on a filesystem that hard-links symlinks perfectly well. Three controls keep it from proving less than it looks: - the regular file in the same transfer must still be hard-linked, or "it fell back" would also be satisfied by --link-dest having been abandoned; - the itemised run must emit exactly one "cL... sym -> some-target" line. A plain -a run cannot see a duplicated itemisation, which is how that defect reached an HFS+ target before this was added; - EPERM and ENOSYS must fall back too, since errno does not separate "cannot" from "may not". itemize picked its expected change-type letter from the build capability, which is the wrong question -- the link happens on whichever filesystem holds the test data. Ask that one too, and drop the XFAIL the old mismatch needed. The hook is Linux-only, so the test joins the macOS and Cygwin skip lists, which are required to be sorted. |
||
|
Andrew Tridgell
|
d09edb85e6 |
generator: fall back to a copy when the destination cannot hard-link
CAN_HARDLINK_SYMLINK and CAN_HARDLINK_SPECIAL are decided by configure running linkat() on whatever filesystem the build tree happened to sit on. The destination is free to disagree, and one host can hold both answers: macOS builds on APFS, which can hard-link a symlink, and backs up to HFS+, which returns ENOTSUP. A build that said yes had no fallback left. try_dests_non() reported the refusal as a transfer error and returned a matched basis, so the caller created the entry anyway -- correctly -- and the run still exited 23. Every neighbouring case copes: a regular file whose link() fails goes to try_a_copy, and a build compiled without either macro resorts to --copy-dest behaviour. This was the same situation, discovered a little later, and the only one treated as fatal. Take the existing fallback on any refusal. Singling out the "cannot" errnos is not possible: link(2) documents EPERM both for a filesystem with no hard-link support and for an ordinary permission refusal, and FUSE reports ENOSYS for the same thing. It is also what the regular-file path next door has always done (try_dests_reg -> hard_link_one -> try_a_copy), and consistency between the two was the point. Where the errno does matter the surrounding transfer says so anyway: ENOSPC, EDQUOT and EROFS fail the creation independently, EMLINK and EXDEV mean the link was never possible. EIO alone goes unremarked; reporting it would put a line into --link-dest's itemised output, so it is left out on purpose. Returning -3 rather than -2 keeps the caller out of the "already up to date, skip it" arm, which under --link-dest would drop the entry entirely. Both callers give -3 the treatment the compile-time fallback already gets -- clearing itemizing and code -- because try_dests_non() has itemised the match itself and would otherwise report the entry twice. The fallback is silent, matching a build that cannot link these at compile time; documented under --link-dest instead. |
||
|
Andrew Tridgell
|
36be3b4e70 | NEWS: note the merge-file confinement half of the filter fix | ||
|
Andrew Tridgell
|
1bea181096 |
testsuite: an rrsync restricted dir must bound merge files
Uses the exclude-only merge form, which leaves no diagnostic to assert on: the escape shows up as a file silently missing from the transfer, so the test reads the oracle the same way an attacker would. Pull mode, so no --delete is involved. Each case requires the transfer to have succeeded as well, since refusing outright would hand the peer a denial of service. A second escape reaches the source through a symlink, which is what makes rsync's tracked cwd and the real one disagree -- the shape that catches a lexical seed. That one drives --confine-root directly: rrsync rejects the argument spellings that would carry it, so routing it through the wrapper would pass either way and prove nothing. Both controls repeat their escape with an in-tree merge target and require it to be read AND obeyed, since "the transfer failed" and "every merge file is refused" would otherwise satisfy the escape assertions on their own. |
||
|
Andrew Tridgell
|
314abcc437 |
rrsync: confine the server's path resolution to the restricted dir
Filter rules arrive over the protocol, long after the wrapper has exec'd rsync, so no argv-level check can see them. A client can name a merge file outside the restricted dir in a dir-merge rule and have the server read it in as filter rules; on a pull that needs neither --delete nor any verbosity. Pass --confine-root so the server bounds the open itself, which is the only end that can. Both directions: a dir-merge is read by whichever side its rule applies to, so unlike --drop-D this is not receiver-only. Skipped for a "/" restricted dir, where there is nothing to confine. |
||
|
Andrew Tridgell
|
3113011218 |
rsync: add --confine-root, bounding operator path resolution
The ownership walk that resolves operator-supplied paths asks who planted a symlink, not where the path came out, so a symlink owned by uid 0 or the euid is followed wherever it points. A daemon already narrows that with the served module root; nothing else has a root to narrow it with. That leaves a wrapper serving a restricted directory over a remote shell with no way to bound the resolution. rrsync can vet the argv it is handed, but filter rules travel over the protocol instead: a dir-merge rule can name a merge file outside the restricted dir and the server reads it in as rules. Redacting the resulting diagnostics does not close it, because an exclude-only merge produces none -- every line becomes a pattern, so nothing fails to parse, and the client reads the file's contents off which of its own names went missing from the file list. --confine-root gives that wrapper the root the daemon has. The existing module check becomes a root check that takes its root from module_dir when we are a daemon and from the option otherwise, so daemon behaviour is unchanged; a daemon ignores the option outright, since it arrives in a peer-supplied argv and could only widen the module. The tracker is seeded from getcwd() rather than curr_dir, which is only the lexical name change_dir() was given: descend into a source argument through a trusted symlink and the two sit at different depths, so a ".." that really escapes looks like it landed inside. When the cwd cannot be read there is nothing to measure against and the open is refused -- an empty tracker does not deny by itself, because a leading ".." pops nothing from it and an empty path reads as an ancestor of the root. An fd pin (/proc/self/fd/N, which rrsync uses so no later symlink can redirect a validated option path) is spelled outside the root by construction, so the walk transits the pin namespace and the pin is judged by what it points at. Only a bare ".../fd/<digits>" is resolved that way, and one that will not readlink to an absolute path is refused; rrsync's ".../fd/N/<leaf>" spelling resolves through the magic link and has its remaining components checked normally. --insecure-links is refused alongside it: that opt-out returns the legacy open before the walk that enforces the root runs, so the pair would have quietly meant no confinement at all. |
||
|
Andrew Tridgell
|
0f6f35e522 |
exclude: give the ":e" self-exclude rule its merge rule's provenance
The exclude-self rule that a ":e" merge synthesizes is built by hand with
new0(), so it inherited no flags. While the merge file was still being
parsed the global parse state masked that, but once parsing finished the
stored rule looked argument-origin, and report_filter_result() printed its
pattern -- a merge file's own text -- verbatim:
[sender] hiding file PAT-x9 because of pattern PAT-[x]9 [per-dir ...]
Plain -vv reaches this on a stock client; no --debug is involved. That is
the fifth site of this shape, and the first to get there by constructing a
rule rather than by printing one, so the redaction helper could not catch it.
Also fix the location a per-directory merge reports. Its fname points into
dirbuf, which is cut back to the directory before the name was saved, so the
error said "<rule from .../src/ line 1>" instead of naming .rsync-filter --
no leak, but it breaks the "redact what, keep where" bargain the rest of this
work depends on. Save the name before the truncation.
The rrsync test's claim to close "the rest of the FILTER trace family" was
too strong and is corrected: options.c maps verbosity onto the debug flags,
so -vvv still raises a restricted server to FILTER2 and its trace metadata
comes back. Rule text stays redacted at every verbosity, which is the
property that matters; -vvv is added to the unaffected-transfer cases.
|
||
|
Andrew Tridgell
|
631f9bd464 |
rrsync: refuse a peer-selected --debug
The --debug=FILTER traces print rule text and merge-file names that came
out of a file's contents -- and a word-split per-dir merge (":w- FILE")
turns every word of a file into a merge-file name, so the trace echoes
what the syntax errors no longer do, with nothing failing to parse.
Redacting every trace would mean carrying provenance on each rule, which
a deferred ":" merge does not currently keep. For a restricted account
the cheaper answer is to deny the peer the switch: server_options() only
ever forwards --info, so no stock client sends --debug to a server and
the only way it arrives is a deliberate -M--debug=. An operator
debugging their own server is unaffected.
Disabled rather than deleted from the table, because that table is
generated by the cull-options script and a regeneration would put the
line back; the test would then catch it.
|
||
|
Andrew Tridgell
|
bb806288b7 |
exclude: don't hand a merge file's contents back to the peer
A filter rule that fails to parse was printed back verbatim. When the rule came from a file rather than an argument, that text is file CONTENT, and the peer picks which file gets merged: a per-directory merge rule travels over the protocol, so no argument of ours ever names it and nothing a wrapper can see mentions it either. Any line that is not valid filter syntax therefore came straight back to the peer -- a read-any-line oracle over an rrsync restricted account or a daemon module, neither of which confines the merge open. The syntax errors turned out to be the smaller half. The MATCH trace names the pattern that acted, and report_filter_result() logs at level 1 for a sender or generator, so plain -vv -- no --debug, nothing a stock client cannot send -- returns a server-side merge file's rules: [generator] protecting file X because of pattern <the file's text> So provenance is carried on the rule itself (FILTRULE_FROM_FILE), not just in the parser: a deferred ":" merge is processed long after the file that named it was read, and its own name is file content too. TEXT_FROM_FILE() consults the parse-time context and the rule, so both the immediate and the deferred paths redact. Rather than test the provenance at each message -- which is how the last few of these were found, one at a time, after the ones before them were fixed -- every string that is or is built from a rule's own text goes through rule_text(). It returns the text for an argument-supplied rule and a description of where it came from otherwise, so a message added later cannot reintroduce the leak by forgetting to check, and there is one place to audit. rule_detail() does the same for the extra detail a message adds ABOUT the text: a character of it, an offset into it, the [not found] bit. Thirteen sites now route through them: the syntax errors; the modifier character (one byte of the file, a slower oracle but still one); the failed-open and merge-depth messages, whose pathname is file content whenever a rule named it -- and errno with them, since it answers "does this path exist"; both over-long messages, the deferred one of which needed no verbosity at all; both merge-name overflows; the long-named directory error; the [not found] openability bit; the match trace; the add_rule, parse_filter_file and daemon-hidden traces; and the per-dir mergelist label, which had the name baked in. rule_detail() covers more than it first looks: the trailing-whitespace CAUTION is computed from the rule's last byte, and "hidden by daemon filter" distinguishes a daemon-filter rejection from an ordinary open, so both would answer questions about text the peer cannot see. The regression proves the chokepoint rather than the sites: making rule_text() return its input unconditionally fails the test. It also pins what must NOT change for the user's own rules -- the whitespace warning still fires, and an over-long argument rule is still reported at full length (the helper buffers at BIGPATHBUFLEN, as rprintf does, so redaction does not quietly truncate what the user typed). Bounded and left alone: the numeric rflags in the FILTER2 trace and the in/exclude wording still describe a file-derived rule without quoting it, and the daemon's own FLOG line records the name it filtered -- that one goes to the operator's log, not the peer. Rules given AS arguments are still echoed in full -- that text is the user's own, and hiding it would only make ordinary typos harder to fix. Where a rule did come from a file, the diagnostic names the file and line instead, which is more useful anyway. Two things the location itself needed: fname can point into parse_merge_name()'s static buffer, which a merge rule inside the same file overwrites while we are still reading it, so a rule after a nested merge was blamed on the nested file -- keep our own copy. And a CRLF pair was counted as two line endings while word-split mode counted tokens rather than lines, so the number pointed at nothing; consume the LF of a CRLF (preserving the byte for the next rule if pushback ever fails), and report word-split sources without a line number. Not covered, deliberately: a rule's provenance is not serialized by send_filter_list(), so it does not survive to the far side. That is right -- only the client sends that list, and the server already knows the patterns the peer gave it. |
||
|
Andrew Tridgell
|
d1756203b9 |
testsuite: stop the test rsyncd stranding a connection child
A daemon test could leave an orphaned rsyncd squatting its port even when every test PASSED, so nothing in the results pointed at it. On Cygwin the orphan then wedged the whole fleet: it kept the ssh session from closing, so fleettest's run_on() blocked until its 2400s timeout and unrelated tests failed with 300s timeouts as collateral. One such wedge cost a fleet run 21 minutes. Cause: rsyncd forks a child per connection, but _stop_rsyncd only killed the parent -- the one pid the Popen handle knows. A child still winding up or down when the test ended survived, inherited the listening socket, and was reparented to init. Cygwin turned that from untidy into unrecoverable: its signals are cooperative, delivered by a helper thread inside the target, so a process sitting in a Windows call ignores even SIGKILL. kill, killpg and pkill all failed against it, which also defeated the orphan reapers and fleettest --cleanup. Snapshot the daemon's children before killing it (once the parent is gone they are reparented and no longer identifiable as ours) and kill them too, re-checking with _pid_is_rsync before each signal so a pid recycled in the meantime is never signalled. Where signals cannot win, fall back to terminating the winpid via taskkill; fleettest --cleanup gets the same fallback, so it can no longer report SURVIVED and leave the port squatted. _reap_group() reports success only once the daemon is confirmed gone rather than when a signal was merely accepted -- on Cygwin a signal is routinely accepted by a process that then ignores it -- and confirms with a bounded poll, because SIGKILL is asynchronous and calling a still-terminating process "alive" would make _probe_bindable() skip its retry and fail a test for a port that was about to free itself. _cleanup_rsyncd() keeps the port's pid record only while it still names a live rsync. Keying that on the port being busy instead looks safer but is worse: a port sits in TIME_WAIT after a passing test, so a record naming an already-dead pid would be retained forever, and nothing clears such a record -- yet no reaper can use it either, since they all reject it at the _pid_is_rsync guard, leaving only the hazard that its pid is recycled onto an unrelated rsync. The daemon stays in the TEST's process group on purpose: runtests.py killpg's that group on a per-test timeout, and that is what keeps a timed-out test from stranding its daemon. An earlier version of this fix gave the daemon its own group so one killpg would catch the children -- which silently broke that sweep, and a full Cygwin pass then stranded two parent daemons when variety hit its timeout. Two residual limitations are documented in the code rather than left to be rediscovered: _kill_pid's check-then-signal is inherently a TOCTOU (narrowed to microseconds, not closed; closing it needs pidfd or retained Windows handles across seven platforms), and _stop_rsyncd cannot collect children when the parent has already exited on its own, because the parent-child link it relies on is gone by then. Measured on a Cygwin VM, 4 proxy/daemon tests x 8 runs at -j4: before 5/8 runs left an orphan (one left two), after 0/10. All tests passed in every run, before and after -- which is the point: the leak was invisible to the suite. A test killed by the runner's timeout still leaves no daemon behind. |
||
|
Andrew Tridgell
|
d95217fbbe |
fleettest: run only the daemon tests in the tcp pass
The tcp pass re-ran the whole suite over the same build the pipe pass had just swept, but --use-tcp is observable through exactly one code path: RSYNC_TEST_USE_TCP is read once (rsyncfns USE_TCP) and acted on once (in start_test_daemon). A test that never reaches there cannot tell the two passes apart, so 186 of the 340 tests were producing the same result twice. runtests.py --daemon-tests-only keeps the tests that can reach the daemon transport, matched against the closure of every rsyncfns helper leading to USE_TCP/start_rsyncd/claim_ports plus the modules that open a daemon connection themselves. The token list is deliberately over-broad and an unreadable test is kept, so the filter can only ever run too much; audited against the tests it drops, none of which reach the transport (their "daemon" hits are the unix username, a macOS ACL principal, mount --bind, and docstrings declaring the test local-only). The dropped count is always printed rather than left implicit. The narrowing is only sound as the second half of a pipe+tcp pair, so it is gated on the pipe pass having run: under --transport tcp that pass is the only one there is, and narrowing it would drop the other 186 tests from the run altogether. --full-tcp forces the full sweep either way. Measured on the full suite: serial work 558s -> 367s. |
||
|
Andrew Tridgell
|
653c4e8db9 |
testsuite: make --race-timeout actually control the race budget
The race tests are the suite's slowest by a wide margin -- a race test is a negative oracle, so it passes by spending its entire budget. Most of them wrote `max(RACE_TIMEOUT, 10.0)`, which ignored --race-timeout below 10s: the documented knob did nothing for 10 of the 16 tests. Replace the floor idiom with race_budget(default), where the per-test default applies only when the operator did not pass --race-timeout, and runtests.py exports race_timeout only when the flag was actually given. Defaults are unchanged (measured identical at 15.3s/10.3s/5.2s). Validate the value rather than take it on trust. A race test loops `while monotonic() < deadline`, so a zero, negative or NaN budget runs the body zero times and the test reports PASS without ever racing, and an infinite one runs until the unrelated per-test timeout; the old max(..., 10.0) floor had made all of that unreachable, so removing the floor had to come with rejecting the input. An unparsable value in the environment counts as unset for the same reason -- falling back to the 5s baseline while still counting as "set" would silently halve a 10s or 15s oracle that nobody asked to shorten. NB the *_test.py glob spans four committed symlinks (chown-fake, devices-fake, exclude-lsh, xattrs-hlink); sed -i would replace each with a copy of its target, so they are rewritten with --follow-symlinks semantics and left as symlinks. |
||
|
Andrew Tridgell
|
501165218c |
fleettest: add --keep-on-fail, and pass --timing to the targets
A fleet run costs a full configure+build on every machine, and the report only names the tests that failed -- so seeing WHY one failed meant paying for a second whole run, against a race test that may not fail the same way twice. --keep-on-fail saves the full build/test output of every target that came back with anything unexpected, and keeps that target's remote run dir (with the scratch trees the failing tests left). Clean targets are swept as before. --timing now also asks each target's runtests.py for its own per-test table, so a slow cell can be attributed to actual tests rather than just named as the hold-up. |
||
|
Andrew Tridgell
|
6bad3be6fc |
runtests: report per-test wall-clock with --timing
The suite reported which tests ran, never how long any of them took, so "the fleet is slow" could not be attributed to anything. Time each test and, with --timing, print the slowest first. The footer gives the two bounds that decide what to do about a slow run: the serial sum (what one worker would take) and the floor set by the longest single test, which no amount of -j can beat. |
||
|
Andrew Tridgell
|
2a20d6ec0a |
testsuite: list daemon-handshake-timeout in the shared skip list
The test needs a real listening socket to stall, so it require_tcp()s and skips on the default pipe transport -- like daemon-chroot-acl and the proxy tests alongside it. runtests.py compares the skip set against RSYNC_EXPECT_SKIPPED on a FULL run, so without an entry every pipe-mode CI job reports an unexpected skip and fails, while the tcp jobs pass. |
||
|
Andrew Tridgell
|
4d7c243ca6 |
daemon: bound the pre-transfer handshake with a timeout
rsyncd.conf(5) says of "timeout": "Using this parameter you can ensure that
rsync won't wait on a dead client forever." That did not hold before a module
was known. set_io_timeout() ran at the very end of rsync_module(), so the
greeting, authentication and the whole argument list were read with no I/O
timeout at all -- a peer could stall at any of them and the child waited
indefinitely. Measured: 20 connections sending "@RSYNCD: 31.0" with no newline
were all still alive well past timeout=5, and only went away when the client
hung up.
The consequence is worse than an idle process. claim_connection() runs BEFORE
auth_server(), so naming a module is enough to take a slot: an attacker with no
credentials could occupy every "max connections" slot of an authenticated
module and hold them for as long as it kept the sockets open, with the
documented control unable to recover them. It costs the attacker nothing --
five stalled children measured 0 CPU ticks over 5s -- so this is descriptor and
slot exhaustion, not load.
Bound the handshake at min(configured, 60s). "timeout" is a Locals parameter,
so lp_timeout(-1) reads the global section -- the same -1 idiom start_daemon()
already uses for lp_reverse_lookup().
Both halves of that minimum matter. "timeout" DEFAULTS TO 0, so honouring only
the configured value would leave the daemon most exposed to this -- one whose
administrator never set a timeout -- exactly as pinnable as before. And capping
matters because an operator who sets "timeout = 86400" for slow links is asking
for patience during a TRANSFER, not for a stranger to hold a pre-auth slot for a
day. The pre-module phase has no legitimate reason to take even a minute.
The bound is retired the moment the module is known, which is what lets the
configured value still govern the transfer. That retirement is load-bearing:
the per-module test only ever LOWERS the timeout (`lp_timeout(module_id) <
io_timeout`), so leaving the handshake bound in place would silently clamp a
module that asked for more -- "timeout = 300" would get 60. It is cleared
before that test runs, and only when io_timeout is still the value we armed,
since the client's own --timeout is parsed in between and must win on its own
terms. Verified: with no global timeout and "timeout = 120" in the module, a
connection idles past 75s rather than being dropped at 60.
Applied only for a real socket daemon (am_daemon > 0): an rsh-run daemon has no
listener to exhaust.
Verified end to end with max connections = 2 and timeout = 5: with two stalled
unauthenticated connections holding both slots, a legitimate client is refused
during the timeout window and served once it elapses. Before this change it was
refused both times.
Reported by Chamal De Silva. Not a regression -- 3.2.7 behaves the same way.
An idle timeout alone is not enough, which the review of the first version of
this change made concrete: safe_read() consults it only when poll() TIMES OUT,
so a peer sending a byte more often than allowed_lull (timeout/2) is never
checked at all. Measured: one byte every 20s held the handshake open for 182s
against a 60s bound, keeping its max-connections slot the whole time -- the
reported attack, merely with the attacker typing.
Non-positive configured values are treated as "use the built-in bound":
"timeout" is parsed with atoi(), so "timeout = -1" would otherwise reach
set_io_timeout() (which reads it as no timeout) and alarm() (which would take it
as a huge unsigned count), disabling the very bound it looks like it configures.
The client's own --timeout is no longer inferred by comparing values, which could
not distinguish it from an identical armed value: io_timeout is zeroed before
parse_arguments(), so anything non-zero afterwards came from the client.
So the bound is absolute and lives in the READ PATH, next to the idle timeout
it complements: safe_read() caps each poll() at whatever is left of it and
gives up when it expires, so it is re-checked on every iteration and a peer
that keeps typing cannot outrun it.
It is deliberately NOT alarm()/SIGALRM. Three earlier attempts used one and
each regressed something: fork() clears pending alarms, so the "post-xfer exec"
parent -- which waits for the ENTIRE transfer -- kept the deadline and _exit()ed
mid-transfer, skipping the hook and releasing the max-connections fcntl lock
while the transfer child ran on; "pre-xfer exec" and the name converter are
operator scripts that may legitimately outlast any handshake bound; and the
cancellation sat inside an exec-environment compile guard, so a build without
setenv/putenv kept it armed through the transfer. A deadline consulted only
where the daemon is already blocked reading a peer has none of those hazards.
It is also kept entirely separate from io_timeout, which is an idle timeout the
module or client may set. Mixing them clamped a module asking for more than the
bound ("timeout = 300" became 60) and leaked the handshake value into the
transfer. Verified: module 300 stays 300, and a client --timeout=7 still wins.
Armed for each peer-driven phase and cleared between them: at the start of the
handshake, tightened by the module's own timeout once the module is known and
its slot claimed, cleared across the hook/fork setup, re-armed before
"@RSYNCD: OK" so it spans BOTH read_args() calls including secluded args, and
cleared before the transfer.
That argument-read coverage is the part that matters most. auth_server()
returns immediately when a module sets no "auth users", so on an ANONYMOUS
module nothing is authenticated: without a bound there, a peer could claim the
slot, take the OK, and trickle an unterminated argument line forever. Measured:
still open after 150s before, closed at 60s after.
|
||
|
Andrew Tridgell
|
cf15b1bb24 |
gitignore: cover the newer test helper binaries
.gitignore lists the older helpers (tls, getgroups, wildtest, trimslash, t_unsafe, getfsdev) but not the ones the security work added, so a `git add -A` in a built tree stages ~4 MB of ELF -- which is exactly how nine of them ended up committed on this branch before being removed again. |
||
|
Andrew Tridgell
|
ba3e9d681b |
testsuite: don't report a bogus escape when the scratch path has a space
The three operator-path-traversal daemon tests failed with "escaped: a '..' traversal reached the excluded subtree" when the build path contained a space. That reads like a confinement failure and is not one. rsyncd.conf's "exclude" is a SPACE-SEPARATED list of patterns, so "exclude = /ws test/.../secret/" is two patterns, neither of which is the directory meant to be protected. Nothing was excluded, so the traversal reached a subtree that was never actually off limits. Confirmed by running the same case with a "filter" rule, which the parser deliberately does not split at an internal space: it passes, so the traversal protection itself holds. Left on "exclude" rather than switched to "filter" -- these tests exist to cover the exclude path -- and skipped with the reason when the scratch path makes that config inexpressible. Worth knowing outside the testsuite: an operator whose module paths contain a space gets no warning that "exclude" silently matched nothing. |
||
|
Andrew Tridgell
|
84832f0ff7 |
rsync-ssl, testsuite: quote paths interpolated into shell text
Third layer of the space-in-build-path work, and the first part that is not
test-only.
rsync-ssl expanded the helper program paths unquoted -- "exec
$RSYNC_SSL_OPENSSL s_client ...", likewise for gnutls and stunnel -- so an
openssl installed under a path containing a space is split and never runs.
That affects anyone with such a path, not just the testsuite. Quoted; the
neighbouring $caopt/$certopt/... stay unquoted because they are option lists
that rely on word splitting. Its own re-exec passes --rsh="$0 --HELPER",
which rsync then tokenises, so $0 is single-quoted for rsync's parser.
On the test side, the same shape in generated shell scripts: redirect targets
("printf ... > {capture}") and daemon hook commands, which rsync runs through a
shell, both interpolated a path with no quoting.
In a directory with a space: 235 pass, 18 fail, from 0 able to run.
Unchanged in a normal path: 257 passed, 0 failed.
|
||
|
Andrew Tridgell
|
0789709aeb |
testsuite: quote the rsync command everywhere a shell re-parses it
Second layer of the space-in-build-path work. Quoting the Makefile got the
runner started; these are the places that then hand the binary's path to
something that splits on whitespace.
- RSYNC_CONNECT_PROG is run by a shell. This was the big one: an unquoted
daemon command turned every daemon-mode test into
"sh: 1: /path/to/ws: Permission denied".
- RSYNC_RSH / --rsh is tokenised by rsync itself (do_cmd() in main.c, which
honours ' and "), so support/lsh.sh needs quoting when srcdir has a space.
- --rsync-path is a command line run by the REMOTE shell, so rsync passes it
through unsplit and lsh.sh's eval re-parses it.
- The generated rsync-shim scripts interpolate RSYNC into "#!/bin/sh\nexec
...", where it is shell syntax rather than an argv entry.
rsync_path_arg() and rsh_cmd() build those strings by splitting the command and
re-joining with shlex, so a plain path with a space comes back quoted while a
wrapper command ("valgrind ... /build/rsync") stays several words.
split_rsync_cmd() also has to cope with RSYNC once a test has appended options
to it -- chown-fake and friends do -- where the string is no longer a filename.
It now takes the longest leading run that names an existing file as the program
and splits only what follows.
In a directory with a space: 231 pass, 22 fail, from 0 able to run before the
first commit. Unchanged in a normal path: 257 passed, 0 failed.
|
||
|
Andrew Tridgell
|
b1d2c32b1d |
build/testsuite: survive a build path that contains a space
`make check` died immediately when the build directory had a space in it:
./runtests.py --rsync-bin=`pwd`/rsync -j 8
rsync_bin /Volumes/Untitled is not a file
Reported by Roland Kletzing building in "/Volumes/Untitled 2"; it reproduces
anywhere, and is not macOS-specific.
Makefile.in interpolated an unquoted `pwd` into --rsync-bin at five sites, so
the shell word-split it. Quote those, and --tooldir at the installcheck site,
which had the same bug and was not in the report. Quote "$(srcdir)/runtests.py"
too: the script's own path word-splits just as readily.
That alone only gets as far as starting the runner. rsync_argv() then did
shlex.split(RSYNC), which turns "/ws test/rsync" into two nonexistent programs.
RSYNC may legitimately be a wrapper command line ("valgrind ... /build/rsync"),
so it cannot simply stop splitting; split_rsync_cmd() checks whether the string
names an existing file first -- a path that exists is one word by definition --
and only falls back to shlex for a real command line. Nine tests that called
shlex.split(RSYNC)/(RSYNC_PEER) directly go through it as well.
Deliberately a function called at use time rather than a pre-split constant:
chown-fake, devices-fake, chown, devices and partial_nowrite append
' --fake-super' or ' --super' to rsyncfns.RSYNC part-way through, and a cached
split hands back the pre-mutation command. Caching it is what broke those two
tests while I was writing this.
The suite is still not space-clean -- in a directory with a space 157 pass and
97 fail, against 0 able to run before. The rest is a separate problem: mostly
transfers whose --rsync-path is re-parsed by a remote shell, which needs
quoting at a different layer. No change in a normal path: 257 passed, 0 failed.
|
||
|
Andrew Tridgell
|
a0952930a1 |
build: require <poll.h>, not just poll()
io.c and socket.c include <poll.h> unconditionally, but configure only required the function. A system that exposes poll() through some other header would pass configure and then fail to compile -- the AC_CHECK_HEADERS result for poll.h was collected and never used. Require the header too, with its own message. Verified both ways: a normal configure still succeeds, and forcing ac_cv_header_poll_h=no now stops with "rsync requires <poll.h>" rather than failing later in the build. |
||
|
Andrew Tridgell
|
8e0bd08a62 |
testsuite: CC is a command, not a filename
highfd-hang probes FD_SETSIZE by compiling a snippet, and passed $CC to subprocess as a single argv[0]. CC='ccache gcc' then looks for a program literally named "ccache gcc" and the test dies with FileNotFoundError instead of probing -- and ccache is wired into PATH on the CI fleet, so this was reachable rather than theoretical. Split it with shlex, and treat an unusable CC as "cannot probe" (skip) rather than an error: the fallback to cc/gcc already handles a missing CC, and a broken one should behave the same way. |
||
|
Andrew Tridgell
|
d5cce08503 |
io: describe the timeout guards in terms of poll(), not select()
The MSG_IO_TIMEOUT cap and set_io_timeout()'s negative/overflow guards were written when these loops used select(), and their comments explain the danger as a tight select()-EINVAL spin on a negative tv_sec. Under poll() the failure mode inverts: the timeout is a millisecond count where a negative value means "wait forever", so a wrapped allowed_lull hangs the process instead of spinning it. The guards are still needed and unchanged -- only their stated reason was wrong, and a rationale that no longer matches the code is what gets a guard removed later. poll_timeout_ms() clamps the value too, so the guards are now belt and braces; noted so neither looks redundant on its own. |
||
|
Stuart Inglis
|
bf3a11cf24 |
io/socket: address review of the poll() conversion
Follow-up to the FD_SETSIZE fix, covering the points raised in review.
Negative/overflowing I/O timeouts. set_io_timeout() could produce a negative
select_timeout (a peer-supplied MSG_IO_TIMEOUT value was applied unchecked),
and every wait now passes select_timeout * 1000 to poll(), where a negative
millisecond count means "wait forever" -- so a hostile or buggy peer could
stall the other side and bypass keepalives entirely. select() used to reject
that with EINVAL, which kept the loop and check_timeout() running. Clamp a
negative argument to 0, compute allowed_lull without overflowing near INT_MAX
(secs / 2 + secs % 2), ignore a non-positive MSG_IO_TIMEOUT value, and funnel
all three waits through poll_timeout_ms(), which keeps the count positive and
bounded.
The daemon accept loop had the same fd_set overflow. start_accept_loop() still
stored listening sockets in an fd_set, so a daemon started with enough
descriptors already open got listener fds >= FD_SETSIZE and hit the same
undefined behaviour at startup -- verified: with the old code a transfer
through such a daemon yields nothing, with this change it succeeds. Converted
it to poll() as well.
Readiness testing. Treating any non-zero revents as ordinary readiness was
wrong: poll() reports POLLERR/POLLHUP/POLLNVAL unrequested, and an invalid fd
shows up as POLLNVAL on a successful poll() rather than -1/EBADF, which left
the EBADF branches dead and let an invalid ff_forward_fd reach
forward_filesfrom_data() (where EBADF reads as EOF). Use role-specific masks
(POLL_RD_BITS / POLL_WR_BITS), handle POLLNVAL explicitly in all three loops,
and request POLLPRI so select()'s old exception set is not silently dropped.
A bidirectional fd is no longer entered twice. A direct daemon connection uses
one fd for both directions; it now occupies a single pollfd row with OR-ed
events instead of two rows carrying different masks, which also avoids the
Cygwin < 3.3.6 duplicate-entry readiness bug.
poll() is now a declared requirement: configure.ac checks for poll.h and
poll(), failing with a clear message rather than leaving it implicit.
The test no longer hardcodes FD_SETSIZE (1024 on glibc but 65536 on 64-bit
Solaris, where it would have opened too few fds and passed vacuously); it asks
the C library for the real value via a small compiled probe and skips if that
is unavailable. Its description now also covers the fortified-libc case, where
the pre-fix result is an abort rather than a hang.
(cherry picked from commit
|
||
|
Stuart Inglis
|
44fdf0bc43 |
io: use poll() instead of select() to avoid an FD_SETSIZE hang (issue #231)
rsync's I/O loops (safe_read, safe_write, and the main perform_io
multiplexer) waited for readiness with select() and fd_set bitmaps. An
fd_set can only represent descriptors below FD_SETSIZE (1024 with glibc).
When rsync is started with many descriptors already open -- e.g. inherited
from a parent process that leaked fds, a high "ulimit -n", or a busy daemon
-- its own socket and pipe fds get allocated at or above 1024. FD_SET() and
FD_ISSET() then index past the end of the fixed-size fd_set, which is
undefined behavior: select() reports the fd as ready, but FD_ISSET() reads
the out-of-bounds bit as 0, so the read or write never happens and rsync
spins at 100% CPU forever with no progress. This is the long-standing
"rsync hangs at 100% CPU on large systems" report, and it matches the
MemorySanitizer use-of-uninitialized-value seen in perform_io.
Convert the three loops to poll(), which identifies descriptors by value in
a small array and has no FD_SETSIZE ceiling, so a high-numbered fd works
fine. rsync only ever waits on a handful of fds (at most three in
perform_io: in_fd, out_fd, and the files-from forward fd), so poll() is as
fast as -- or faster than -- select() here; the select()-vs-poll() cost gap
only appears when watching thousands of descriptors, which rsync never
does. The remaining select(0, ...) call is a pure timed sleep with no fds
and is unaffected.
The conversion is behavior-preserving: the same max_fd bookkeeping decides
when there is nothing to wait on, the per-fd readiness checks map to the
matching pollfd revents, and the timeout is the same (now expressed in
milliseconds).
testsuite/highfd-hang_test.py reproduces the hang deterministically by
opening enough inheritable dummy fds to push rsync's descriptors past
FD_SETSIZE before an ordinary transfer; it hangs (caught by a timeout) on
the select() code and passes instantly with poll().
(cherry picked from commit
|
||
|
Andrew Tridgell
|
1078876a31 |
testsuite: cover sparse holes in matched inplace blocks
(cherry picked from commit
|
||
|
Stuart Inglis
|
f7c67d4a1d |
testsuite: drop the strace-based --sparse write-count test
The test needed strace, so it skipped on every platform without it (macOS, the
BSDs, Solaris, and the AlmaLinux container). runtests.py compares the skip set
against RSYNC_EXPECT_SKIPPED and treats any unexpected skip as a failure, so it
turned the macOS and AlmaLinux 8 jobs red and would have needed an entry in
each platform's expected-skip list -- an entry that would itself go stale the
moment strace became available.
It also earned its keep poorly: it guarded a syscall-count property rather than
correctness, and it was not what caught the --inplace --sparse hole regression
in this series (review and differential fuzzing did). Correctness of the sparse
paths is already covered by the sparse and preallocate tests; the write-count
improvement is recorded, with measurements, in the commit that made it.
(cherry picked from commit
|
||
|
Stuart Inglis
|
bb804f80f6 |
fileio: keep punching interior holes on the matched (--inplace) path
Review caught a release-blocking regression in the previous commit: with
--inplace --sparse, interior zero runs inside *matching* blocks were left
allocated.
The scan I added applied only to the write path. The use_seek branch --
reached via skip_matched() when an in-place update finds identical data --
still trimmed just the leading and trailing zeros and lseek()'d over the whole
middle. Before the change, write_file() fed that data through
SPARSE_WRITE_SIZE slices, so an all-zero slice in the middle of a large
matching block became a deferred hole like any other; afterwards those blocks
stayed fully allocated. Reproduced with the reported case: an 8 MiB file whose
every 32 KiB block is 4 KiB data / 24 KiB zeros / 4 KiB data, copied onto an
identical destination with --inplace --sparse --no-whole-file
--block-size=32768, occupied 2048 KiB before this series, 8192 KiB after the
previous commit, and 2048 KiB again with this one. Content was byte-identical
throughout; only the on-disk sparseness regressed.
Rather than duplicate the scan in the use_seek branch, drop that branch and run
both cases through the one loop, with the sole difference factored into
emit_sparse_span(): a span that is not becoming a hole is written normally, or
merely seeked past when the bytes on disk already match. The hole itself is
flushed by the existing flush_sparse_hole(), which already picks do_punch_hole()
over do_lseek() while inside the preallocated extent -- and for an in-place
transfer the receiver sets preallocated_len to the basis size, so a matched
interior hole is genuinely deallocated rather than skipped over.
Verified by differential fuzzing against the pre-series binary: 37 file shapes
(including the reported one, runs either side of the SPARSE_WRITE_SIZE
threshold, all-zero and hole-free files, and randomised mixes) across both the
plain and --inplace modes, comparing contents and allocated blocks. Contents
match and allocation is never worse than before the series.
(cherry picked from commit
|
||
|
Stuart Inglis
|
6a15079cab |
fileio: coalesce --sparse writes instead of 1-KiB dribbles (issue #773)
write_file()'s sparse path sliced each span into SPARSE_WRITE_SIZE (1024-byte)
pieces and write_sparse() issued one write() syscall per slice. Copying a
large *non-sparse* file with --sparse therefore cost roughly one write() per
kilobyte -- about a million write() calls for a 1 GiB file -- which on real
storage ran far slower than the same copy without --sparse (the bug report
measured 1.36 MB/s vs 391 MB/s, ~280x). The 1024-byte chunk is also smaller
than a filesystem block, so it cannot even create finer holes than a plain
copy could.
Rewrite write_sparse() to scan the whole span itself: it looks for interior
runs of zeros that are at least SPARSE_WRITE_SIZE long -- the same hole
granularity rsync has always used -- and emits each intervening non-zero
region (which may include shorter zero runs not worth a hole) with a single
write(). do_punch_hole() advances the file offset just like the lseek() path,
so flushing a deferred hole between segments keeps the position correct.
The hole granularity is unchanged, so sparseness is identical; only the
syscall pattern changes. Measured on a 100 MiB random (hole-free) file:
write() syscalls drop from 100,730 to 6,125 (~16x), now tracking the data's
natural chunking rather than its size in kilobytes. Verified byte-identical
and equally sparse output for hole-free, large-hole, small-interior-hole,
all-zero, --inplace, and --preallocate cases.
testsuite/sparse-write-count_test.py copies a 16 MiB hole-free file under
strace and asserts the write() count stays far below the old size/1024
behaviour (it skips where strace is unavailable).
(cherry picked from commit
|
||
|
Andrew Tridgell
|
56f6b67453 |
generator: decide the unconfined mknod fallback at compile time
The ENOSYS test was the wrong discriminator. It was meant to detect "this build compiled no fd-relative create at all", but a live mknodat() or mkfifoat() returns ENOSYS too -- an unimplemented FUSE mknod does, and seccomp can synthesise it -- so a runtime failure could route a create through the unconfined path on a platform that has the secure primitive. On Linux the fall-through lands in do_mknod_at(), which re-confines with secure_relative_open(), so no escape was reachable there. The real gap is a mixed-capability build (mkfifoat() but no mknodat()): a runtime ENOSYS from a real mkfifoat() reached the unconfined fallback even though an fd-relative FIFO primitive existed. Whether a primitive exists is a property of the build, so decide it there: no_atfd_mknod_primitive() is false wherever mknodat() covers the node type, where mkfifoat() covers a FIFO, or where --fake-super creates through openat() -- which is always present and was previously able to fall back on its own unrelated failures. Also correct the SECURITY.md residual, which overstated the loss. Plain mknod()/mkfifo() do not follow a planted leaf symlink; they fail EEXIST, verified directly against a symlink to a victim file. What a no-mknodat platform actually loses is the pinned parent, so the residual is a parent-component race rather than a followed basename, and ordinary fake-super placeholder creation stays confined via openat(O_NOFOLLOW). And narrow t_symlink_secure's skip: it skipped the whole helper without mknodat(), including do_symlink_at() assertions that do not depend on it. Only the do_mknod_at() checks are now skipped, and the helper still skips outright when neither applies rather than passing vacuously. |
||
|
Andrew Tridgell
|
fd8bc41967 |
testsuite: itemize's no-hardlink-symlink expectations described no rsync
The hardlink_symlinks==false branch set five values that differ from the true branch. Only one of them is real. Measured on macOS 10.13 -- the one platform that takes it, where linkat(AT_FDCWD, sym, ..., 0) is EOPNOTSUPP so a symlink cannot be hard-linked even though ordinary hard links work -- rsync prints the attribute field as blanks rather than 'c.t.' + dots, says "foo/sym is uptodate" rather than "foo/sym -> ../bar/baz/rsync", emits no trailing --copy-dest line at all, and uses .L where the branch expected cL. Only the change-type letter genuinely differs, and only where the symlink itself is transferred: hL when hard-linked, cL when copied. So the other four knobs are gone rather than corrected -- keeping them as variables that hold the same value on both paths would preserve the suggestion that something varies. It went unnoticed because every platform that had run this test takes the other branch: Linux, FreeBSD and OpenBSD all report hardlink_symlinks true, and Cygwin skips itemize. macOS before 13 is the first target to reach it. Derived by collecting every mismatching block in one run rather than fixing them one at a time, so the values are what rsync emits rather than a guess that makes one block pass and leaves the next wrong. |
||
|
Andrew Tridgell
|
2a44bf4e9f |
fleettest: record mac-x86's two skip-list differences from macOS
That host is macOS 10.13 and has no mknodat(), so it differs from the
shared macOS list in both directions:
symlink-mknod-fakesuper-symlink-race skips there and only there --
do_mknod_at() is the unconfined fallback on such a build, so the
test skips itself rather than asserting a property the build does
not have. mac2 is macOS 26 and still runs it.
sender-remove-source-root-anchor the macOS list expects it to
skip; this host runs it.
Neither can go in testsuite/skiplist/macos.txt: both Macs share that
file and they disagree. The per-target extra/omit fields exist for
exactly this.
|
||
|
Andrew Tridgell
|
abe4a2717e |
generator: fall back where the platform has no fd-relative mknod
do_mknod_atfd() returns ENOSYS on a platform that compiled no
fd-relative create at all -- older macOS has mknod() and mkfifo() but
neither mknodat() nor mkfifoat() -- and gen_entry_mknod() returned that
straight to the caller, so a FIFO or device node could not be created:
rsync: [generator] mknod ".../afifo" failed: Function not implemented (78)
That is not the stance SECURITY.md sets out. Where an operation can be
secured on some platforms but not others, rsync takes the race-safe path
where it exists and falls back to the historical unconfined behaviour
where it does not, "rather than refusing the operation outright". Its
one stated exception is the nested-socket bind(), which gen_entry_mknod()
already routes away from this path.
So fall through to do_mknod_at(), which on such a platform is do_mknod()
by design. Only on ENOSYS: any other errno is a real failure and must
not be retried through the unconfined path.
None of this was reachable before: the platform did not link at all until
the previous commit, which is why a refusal sitting where the documented
rule says fall back went unnoticed.
The race helper skips itself where mknodat() is absent. do_mknod_at() IS
do_mknod() there -- the held-dirfd walk and the O_NOFOLLOW leaf create are
compiled out, not failing -- so its checks were asserting a property the
build deliberately does not have, and reported the accepted residual as a
module escape.
SECURITY.md gains that residual under "Known residuals": it previously
covered only the socket case, and said nothing about the whole special-
file path degrading where mknodat() is missing.
Checked by rewriting config.h the way macOS 10.13 has it (mknod and
mkfifo yes, mknodat and mkfifoat no): --specials now creates the FIFO
where it previously failed with ENOSYS, the race test skips instead of
failing, and the suite is 254/0. Unchanged on Linux at 255/0.
|
||
|
Andrew Tridgell
|
c458873481 |
syscall: guard do_mknod_atfd()'s mknodat() with HAVE_MKNODAT
It used HAVE_MKNOD. Older Darwin has mknod() but not mknodat(), so the
call was compiled and then failed to link:
"_mknodat", referenced from:
_do_mknod_atfd in syscall.o
ld: symbol(s) not found for architecture x86_64
Reported on macOS 10.13.6 x86_64, where config.h carries
/* #undef HAVE_MKNODAT */ next to #define HAVE_MKNOD 1. The sibling
do_mknod_at() already keys off HAVE_MKNODAT and its comment names this
exact platform.
Both occurrences change together. The second guards
return -1; /* mknodat()'s errno (regular/device node) */
against an ENOSYS fallback, so leaving it on HAVE_MKNOD would report
"mknodat()'s errno" on a build where the call was never compiled -- a
stale errno from whatever ran last.
Checked by rewriting config.h the way that platform has it and compiling
syscall.c: the parent leaves one unresolved mknodat reference, this
leaves none. A FIFO still goes to mkfifoat() where that exists, and a
socket still returns EOPNOTSUPP; only the regular/device-node path
becomes ENOSYS, which is what a platform without mknodat() can offer.
|
||
|
Filipe Casal
|
41f411d6e9 |
rrsync: pin receiver option directories the peer can pivot
A checked receiver-side directory option whose leaf does not exist can
be created as attacker-controlled transfer content and then consumed by
the same transfer: --backup-dir=a, with "a" arriving in-band as a
symlink pointing out of the restricted directory. The same shape works
against --copy-dest, where it reads an outside file and delivers it to
the client. Passing the leaf under the parent's /proc/self/fd pin is
not enough: the peer's symlink wins the race to the name, and rsync
creates or reads through it.
The answer is to hand rsync an inode rather than a name -- but what
inode depends on what rsync does with the option, so the policy is per
option rather than per type ("type 2" means "check when receiving", not
"is a directory"):
--backup-dir, --partial-dir rsync creates them on demand. rrsync
creates them instead, walking down from
the restricted dir one component at a
time with O_NOFOLLOW (nested names too:
make_bak_dir() builds a hierarchy), and
pins the result. The partial dir is
made 0700, as rsync makes it.
--temp-dir rsync requires it to exist, so a missing
one stays an error.
--link-dest, --compare-dest, rsync only reads through these, and a
--copy-dest missing one is the ordinary first-run
case that must keep working. rrsync
pins an empty directory it then unlinks:
the transfer behaves as with a missing
one, and there is no name left for the
peer to take over. Not quite identical:
rsync prints "--link-dest arg does not
exist" for a genuinely missing basis and
the placeholder suppresses that.
If the transfer later replaces a created name, the held inode is merely
detached -- the backup fails, it does not escape.
Without /proc/self/fd there is no way to name an inode, so on those
platforms every missing type-2 option path is refused instead -- the six
above, not the type-3 paths covered at the end.
That is the same fail-closed behaviour this change originally had
everywhere; the pinning is what buys back first use where it can.
An earlier version refused every missing type-2 leaf on every platform.
That closed the pivot but broke first-use --backup-dir and --partial-dir
-- including --partial-dir=.rsync-partial, the documented resumable-
upload idiom -- and would have broken first-run --link-dest, which is
how every rotating-snapshot script starts.
The regressions run their security assertions BEFORE their controls, so
an environment where a control fails for an unrelated reason cannot mask
the escape check by aborting first -- which is exactly what happened to
one reviewer. The alt-dest one also has to beat a race: the generator
runs ahead of the receiver, so sorting the pivot symlink first does not
guarantee it is installed before the basis lookup, and a run where the
generator won would pass vacuously. A few thousand files in between
give the receiver the head start, and the test asserts the symlink was
really installed rather than trusting the ordering. Measured 5/5 RED on
the parent, 3/3 GREEN here.
The regressions cover each branch of the policy: the backup-dir pivot,
the --copy-dest read escape (its own test), first-use --backup-dir,
nested first-use --backup-dir, first-use --partial-dir including its
mode, a refused missing --temp-dir, an accepted first-run --link-dest
with no placeholder left behind, and an existing basis still working.
Each requires the specific mechanism rather than just "the outside file
was left alone", which any unrelated failure would satisfy, and each
takes the refusal branch where the pin primitive is unavailable.
Behaviour worth knowing about, since rrsync now creates these rather
than rsync: they are created while the ARGUMENTS are parsed, so they
appear even under --dry-run, where rsync's own make_path() deliberately
does not mkdir; and one is left behind if the transfer then fails. A
pinned --partial-dir is also one directory rather than one per
destination directory, and rsync will not auto-remove it -- which is not
new, rrsync already rewrote an existing relative partial dir to its pin.
A nested --partial-dir now behaves differently from plain rsync, which
documents creating "just the last directory -- not the whole path". With
--partial-dir=a/b and "a" missing, rsync creates nothing and rrsync
creates both. Containment is unaffected -- each component is made
beneath the fd already held, O_NOFOLLOW -- but rrsync accepts a shape
rsync would not honour, and since rsync only removes the last component
of a relative partial dir, an empty parent is left behind.
Not fixed here, and NOT claimed to be: this covers the six type-2
directory options. --files-from, --log-file and receiver positional
paths are type 3; where the leaf and its parent are both missing, or on
a platform without /proc/self/fd, those still reach rsync unpinned, as
SECURITY.md describes. That is the rest of the issue, not this one.
The wrapper is handed a shim rather than RSYNC directly: RSYNC is a
multi-word command whenever the runner forces --protocol=N, and rrsync
execlp()s its RSYNC as a single executable name, so this test died
before reaching the policy under test. A fleet run caught it on the
protocol columns of three targets.
|
||
|
Andrew Tridgell
|
95e7f04148 |
runtests: let a skip spec remove a name a composed list added
The expected-skip lists are now files referenced as @FILE and composed (common + platform + protocol), and they are expanded here, on the target, against the tree that shipped. That is deliberate, but it means nobody upstream of this point can SUBTRACT: the name lives inside a file the composer does not read. fleettest needs exactly that. A target may run the same build against a different filesystem, and then tests its platform list expects to skip genuinely run -- a scratch dir on a second volume makes backup-crossdev -copy and chmod-temp-dir work. With additions only, such a target can never be green. Accept a '-name' entry, applied after every addition so order does not matter. A test name never begins with '-', so the token is unambiguous. Every removal must remove something: a name nothing added is stale, and a repeated removal is that same no-op written twice. Quietly shrinking the expected set is the failure this parser exists to prevent, so both are refused rather than left to sit in a config unnoticed. |
||
|
Andrew Tridgell
|
4b4bf80f8d |
fleettest: never run two targets on one machine at the same time
Targets were all submitted to the pool at once, and the per-run build directory was named for the run alone -- <builddir>-<run_id>, identical on every target. Both assume one target per machine. Two targets naming the same host break that, and the fleet now has such a pair: mac2 and mac2-hfs are one Mac, differing only in where the tests' scratch trees live. They pushed into the same directory and built over each other, and BOTH reported BUILD-FAIL -- a failure that looks exactly like the code under test not compiling. Either target run by itself was fine, which is the worst way for this to present. The build directory now carries the target name too, and a machine's targets run one after another, with a log line saying so. Serialising matters beyond the shared directory: two suites on one host would fight over the fixed ports the daemon tests claim, and over every other piece of host-global state, so separate directories alone would not be enough. Different machines still run concurrently, which is where the parallelism actually was. The target name is reduced to [A-Za-z0-9._-] before it goes into a path that cleanup later feeds to rm -rf, so a name cannot contribute a path separator, a shell metacharacter or a leading dash. --cleanup still globs <builddir>-*, which the longer name matches. |
||
|
Andrew Tridgell
|
dc72d0409a |
testsuite: xattr_set must refuse the same way on every platform
make_variety_tree() sets an xattr on every file, including the ones it
deliberately creates read-only, and tolerates a refusal:
try:
xattr_set('variety', os.path.basename(str(p)), p)
except OSError:
pass
That handler works only on Linux. There xattr_set() calls os.setxattr()
and a refusal is an OSError; every other platform shells out to a CLI
with check=True and raises CalledProcessError, which is not an OSError
and sails straight past. So a refusal the suite tolerates on Linux can
kill variety and variety-symlink-traversal on any CLI-backed platform.
macOS, Cygwin, FreeBSD and Solaris all carry the defect; macOS is where
Roland Kletzing hit it, on test8 through test10, on the perm7 file:
xattr: [Errno 13] Permission denied: '.../d1/d2/.../d7/perm7'
subprocess.CalledProcessError: ... returned non-zero exit status 1
What triggers it there is a non-root run meeting the mode-0400 files the
tree deliberately creates, which their own owner cannot attach an xattr
to. That is why the fleet, which runs as root, never saw it. Root is
not immune to every refusal, just to that one.
Route the four CLI branches through a helper that raises XattrError, an
OSError subclass, so one handler covers every platform. It carries an
errno only when the tool named one, and only macOS's xattr(1) does:
setfattr and setextattr just say "Permission denied", and guessing an
errno back out of localised strerror text would be worse than admitting
we do not know. The match is anchored to that tool's own prefix on the
first line -- the rest of the line is a filename, and a file can perfectly
well be called "[Errno 5]".
devices/devices-fake had already worked around this locally by catching
both types; that catch is now dead, so drop it.
Verified by forcing the CLI branch on Linux and running variety non-root:
it fails with Roland's traceback, on the same perm7, and passes with
this. That establishes the exception path every CLI branch takes, not
macOS's xattr(1) in particular -- his report supplies that half.
|
||
|
Andrew Tridgell
|
47df88d5fb |
fleettest: run a target's tests on another filesystem, and add two Macs
runtests.py already honours $scratchbase, but a target could not use it: the sudo branch runs `sudo -n env PATH="$PATH" ...`, which drops whatever env_prefix exported. Setting it there looked like it worked and silently ran on the default filesystem instead -- the first HFS+ run came back green for that reason. Give it a target field carried inside the env string, on both the root and non-root paths, shell-quoted so a volume name containing a space does not turn into a stray argument. The non-root pass also clears the relocated scratch, which a prior sudo run leaves root-owned outside builddir. expect_skip_omit is the mirror of expect_skip_extra: entries the workflow expects to skip which a target actually RUNS. Relocating the scratch supplies conditions the workflow's host lacks -- a separate volume makes backup-crossdev-copy and chmod-temp-dir reachable -- and without a way to subtract, such a target can never be green. mac2-hfs runs the same host and build as mac2 with the scratch on HFS+. It verifies the mount rather than assuming it: a stale directory, or a name collision attaching at "RsyncHFS 1", would otherwise leave the tests on APFS reporting green, which is how the first version lied. Ownership must be on as well, and is now checked rather than attempted: a user-attached image mounts "noowners", under which every uid/gid and permission check is meaningless, and that alone accounted for 28 of the 31 failures the first honest run produced. mac-x86 is the x86-64 Mac -- the only target that can build the x86-64 md5 assembly, since mac2 is arm64 where configure refuses --enable-md5-asm outright. It needs MacPorts for autotools, python3 and the crypto/hash libs. --enable-roll-simd is not set and cannot be: that probe uses GCC-style function multiversioning, which clang does not support on Mach-O, failing identically under Apple clang 10 and clang 19. mac-x86 currently BUILD-FAILs on the unguarded mknodat() in do_mknod_atfd() (#161), which it reproduced on its first run. |
||
|
Andrew Tridgell
|
ab373ad1d0 |
testsuite: itemize XFAILs where the filesystem cannot hard-link a symlink
itemize picks between two expectation sets using rsync's own
"hardlink_symlinks" build capability. That says nothing about the
filesystem underneath: on macOS the build reports true while HFS+ returns
ENOTSUP for link()ing a symlink, and the run dies with
failed to hard-link .../foo/sym with foo/sym: Operation not supported (45)
Selecting the other expectation set does not help and would assert
something untrue: the itemisation follows the BUILD capability, so rsync
still prints "foo/sym is uptodate" and ".L foo/sym -> ..." even though
the link failed. Neither set describes that combination.
It is an rsync gap rather than a test one. generator.c reports the
runtime linkat() failure as FERROR_XFER and the transfer exits 23, after
which rsync creates the symlink anyway -- while a regular file in the
same position falls back to a local copy, and so does a build compiled
WITHOUT symlink-hardlink support. Falling back on ENOTSUP would make
this pass by itself.
So XFAIL rather than skip: the failure stays visible and flips back to a
pass once rsync falls back. XFAILing the whole test is blunter than the
one --link-dest case deserves, but the symlink expectations are threaded
through every assertion here rather than confined to one.
The probe answers only the question it is asked: a link() refused for any
other reason -- EPERM, ENOSPC, EMLINK, a quota -- propagates instead of
being reported as a capability difference and quietly reshaping the
expectations.
|
||
|
Andrew Tridgell
|
de38616b25 |
testsuite: detect an operator-path escape without sub-second mtimes
operator-path-temp-dir and operator-path-partial-dir decided whether a symlink had been followed by sampling the target directory's st_mtime_ns, sleeping 10ms, and looking for a change. The temp file is renamed away, so an mtime bump was the only trace left. On a filesystem whose timestamps have 1-second granularity -- HFS+, and it is not alone -- a change within the same second is invisible. The delta is zero, the test concludes the symlink was not followed, and reports the operator's OWN euid-owned symlink as refused when it was followed correctly. Both fail that way on HFS+ while passing on APFS, and operator-path-partial-dir is one of the failures Roland Kletzing reported on macOS. Pin the directory's mtime to a fixed past epoch instead, read back what the filesystem actually stored, and ask afterwards whether it still holds -- reading back because a filesystem may clamp or round the value, and comparing against the requested epoch would then read an unfollowed symlink as followed. temp-dir-symlink-injection already works this way. This is not proof against every clock: a directory whose mtime lands exactly on the stored sentinel would still read as unfollowed. That needs the host clock set to 2001 or a deliberate restore, where the old 10ms delta failed on any coarse-granularity filesystem. Verified in both directions by running as root, where the matrix also exercises the cross-uid cells: a followed symlink moves the mtime off the sentinel, a refused one leaves it. |
||
|
fcasal
|
c933f6227d | backup: preserve backup-dir while creating it | ||
|
Andrew Tridgell
|
dcc3c9c51a |
socket: refuse a meaning-changing first character, and stop refusing aliases
The %H allow-list was wrong in both directions.
Several accepted characters change an argument's MEANING rather than its
text when they lead the value, which quoting cannot prevent because the
word stays intact -- that IS the problem:
'-' and '+' introduce options to plenty of programs; with
RSYNC_CONNECT_PROG="prog %H", hosts "-c" and "+x" arrive as
options, and "sh +x" is as real as "sh -x";
'~' is tilde-expanded by the nested shell, turning ~root into /root;
'%' is expanded by a nested fish, where %self becomes its pid.
An empty host has the same shape from the other end: it survives a direct
exec as an empty argument but disappears when a nested shell re-splits the
command, shifting everything after it. rsync://:873/m/ and ::m/ both
produce one. None of these can begin a real hostname, so refuse them in
first position only -- mid-word each is literal, which matters because an
IPv6 zone id carries its '%' mid-word.
The other direction: '+' and '~' were refused outright. They execute
nothing, and RSYNC_CONNECT_PROG exists for custom transports where %H is
often an alias the program resolves itself rather than a name the
resolver sees. Refusing them mid-word breaks that use case for no gain.
A non-ASCII host stays refused. That is a policy choice rather than a
free one: a custom connect program never calls getaddrinfo, so a Unicode
alias would otherwise work, and this does exclude it. A punycode A-label
is unaffected.
What this cannot do is bound what the named program makes of the value.
"host:-rf" arrives intact, and a program that splits on ':' may
reinterpret the tail; that boundary belongs to whoever writes the
command.
The test asserted only that a marker file was absent -- equally true when
rsync failed to parse its arguments, when socketpair_tcp is blocked, or
when touch was missing. Worse, the marker path was absolute, and a URL
authority ends at the first '/', so the injected `touch` never received
an operand and the check could not fail even with the guard gone. It now
runs with cwd set to the scratch directory and injects a bare name, so
the marker is genuinely reachable; requires the specific refusal message;
checks the exit status; and checks that ordinary hosts still arrive at
the connect program with their text intact, which is the part an
absence-only test can never show.
Fault-injected separately: dropping the guard, dropping just the
first-character check, and narrowing the set back each fail the test on
their own, in both the default and --use-tcp transports.
|
||
|
Filipe Casal
|
78e10e7e32 | socket: reject shell-active connect hosts | ||
|
Andrew Tridgell
|
f704aa4aed |
daemon: refuse '!', '~' and braces in a hook expansion too
The refused set was built from the characters that obviously execute
something, and missed three that a SECOND shell acts on:
'!' negates in command position. A hook written as an access check --
`pre-xfer exec = sh -c '%RSYNC_USER_NAME% false'` -- becomes
`! false`, reports success, and serves the transfer. An
authenticated user named "!" turns a denial into an approval, which
is precisely the case the fail-closed comment above exists for.
'~' is tilde-expanded, so ~root becomes /root.
'{' and '}' brace-expand in bash and zsh.
None of them execute anything on their own, which is how a set built from
the obvious metacharacters came to miss them. That is also the standing
weakness of the approach: this is a deny-list, and the two rounds of
review it took to find '!' are the argument for eventually inverting it.
The documentation is corrected with it -- it claimed every shell-active
character was refused, which this disproves -- and now lists the set.
Each listed character is pinned by the test, which needed its module
paths to EXIST first: a missing path fails the transfer on its own, so
checking the exit status alone passed whether or not the character was
refused. Removing any single character from the set now fails the test.
|
||
|
Andrew Tridgell
|
8da62816c8 |
testsuite: make the nested-shell test fail for the right reason
It asserted that a marker was absent and that rsync exited non-zero. Both are equally true when authentication failed, when the daemon never started, or when the globbed command was missing -- so it passed for any of those, with or without the guard. Changing only the password to a wrong value left it green. Require the daemon log to carry the specific refusal, so a transfer stopped for some other reason no longer reads as the value having been refused. Add a positive control. The test now runs the same nested-shell expansion itself first and requires it to work; without that, a pass could equally mean the attack was inert here and rsync was never tested against a live one. Stop globbing onto /usr/bin/touch. The command the expansion selects is now one the test writes into its own directory: the build never guaranteed a system touch, and with the old oracle a missing one produced a pass. Exactly one file there matches "touc?", so the expansion is unambiguous. The docstring described the value becoming "find arguments", which is not what the hook does -- it selects /usr/bin/touc? and glob-expands it to a command. Say what actually happens. Checked in both directions: the wrong-password mutation that used to pass now fails, narrowing the refused set back to the pre-existing one fails, and the unmodified test passes under both the default transport and --use-tcp. |
||
|
Andrew Tridgell
|
5bf9940de4 |
rsyncd.conf: document that the hook metacharacter refusal covers your own values
Refusing a shell-active %VAR% in an exec hook is a real usability cost and it is not confined to hostile input: the check runs on every %RSYNC_*% value, so a module whose path holds a space cannot be interpolated into a hook at all. `path = /srv/My Backups` with a command mentioning %RSYNC_MODULE_PATH% refuses every transfer of that module, with no attacker involved. That is deliberate rather than an oversight. rsync escapes a substitution for the quoting context it sits in, which is right for the one shell that runs the command and wrong for a command that starts a second one -- `sh -c '... %RSYNC_USER_NAME% ...'` hands the inner shell a bare value, where "touc?" is glob-expanded against /usr/bin and chooses the command rather than being data for it. Escaping for an unknown number of passes is not possible, so the value is refused instead. Nor can the check exempt operator-supplied values: `path` may itself be templated from a peer one (`path = /home/%RSYNC_USER_NAME%`), and once both are inside the same string rsync cannot tell them apart. Say so in the manual, and give the way out: the same names are exported to the command, so $RSYNC_MODULE_PATH inside a script is unrestricted. The test pins both halves, since a documented behaviour with no oracle drifts. Both are fault-injected: narrowing the refused set back makes the interpolated half pass when the manual says it must not, and dropping the RSYNC_MODULE_PATH export fails the workaround half. |