mirror of
https://github.com/RsyncProject/rsync.git
synced 2026-11-05 19:43:25 -05:00
7277040af7349c8ac9a94de6f2d06f3c00cc606c
7703
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Zen Dodd
|
7277040af7 | Document sec-patches4 | ||
|
Zen Dodd
|
f47dce87e9 | Backport 3.5.1 fixes | ||
|
Andrew Tridgell
|
c549abd02f |
testsuite: fix the two macOS cells the 3.5.0 fleet run flagged
Running the 3.5.0 suite against this branch across the fleet left four not-OK cells, all macOS, identical on both backport branches. Both causes are here, not in the code under test. t_symlink_secure.c lives at the tree ROOT, so fleettest's overlay -- which replaces runtests.py and testsuite/ -- does not update it. This branch was still running a version predating the HAVE_MKNODAT guard, so on a platform without mknodat() (macOS 10.13) it asserted a confinement the build deliberately compiles out and reported "basename symlink was followed -> module escape". Alarming and wrong: there is no escape, the helper was just older than the test driving it. Taking the current helper makes it skip there, which is what the fleet config already expects for that target. partial-protected-regular-retry-policy exercises the receiver's EACCES recovery path. That path is 3.5.0-only -- the chmod-on-denied-open work is not backported, which is also why partial_nowrite is already excluded here -- so the test cannot reach what it checks and reports "inconclusive" rather than skipping. Excluded for the same reason as its sibling. Verified on the x86-64 macOS box: the policy test is excluded and symlink-mknod-fakesuper-symlink-race now skips, overall result 0. A 3.5.0 build on the same hardware passes both, so neither is a platform limitation. |
||
|
Andrew Tridgell
|
49a7dfa95d |
ci: overlay the stable testsuite instead of running it out of a second tree
The stable-testsuite job ran the newer suite via --srcdir, pointing at the v34-stable-testsuite checkout while the binary came from here. The tests resolve their fixtures relative to the tree they live in -- support/lsh.sh, support/rrsync, old_versions/ -- so that pairing handed this branch's rsync someone else's helpers and failed 94 tests for reasons unrelated to 3.4.1. Overlay the suite onto this checkout instead, which is the composition fleettest.py --testsuite-repo already builds: this branch's code, the newer suite, one tree. old_versions/ is copied rather than git-archived because its static binaries are export-ignore'd. Also point the push/PR triggers at -sec-patches3; they still named -sec-patches2, so the job had stopped firing on push entirely. partial_nowrite joins the exclude list: it covers the receiver chmod-on-denied-open behaviour added in 3.5.0, which this base does not have. With this, the full 3.5.0 suite runs clean here and the expected-skip oracle matches exactly. |
||
|
Andrew Tridgell
|
07caddfb7d |
testsuite: declare the newer-suite tests this branch cannot run
This branch is tested with the 3.5.0 suite (fleettest --repo <here> --testsuite-repo <3.5.0>), which contains tests for fixes and features 3.4.1 does not carry, and unit-test helpers this Makefile cannot build. Those came back as failures and unexpected skips on every fleet cell, drowning out anything real. testsuite/skiplist/backport.txt lists them. fleettest reads it from the built tree and passes the names as RSYNC_EXCLUDE, so they are not run and not reported. It is an exclude rather than an expected-skip list because several of them fail rather than skip. Only structural cases belong here -- a fix or feature this branch lacks, or a helper it cannot build. Anything else that fails is a real signal. |
||
|
Andrew Tridgell
|
f33d42a31e |
options: fix safe_arg() uninitialized-byte leak in filename-mode quoting
The escape counter reserved a slot for every backslash, but the writer
suppresses the escaping backslash before a wildcard and -- via the
strchr(WILD_CHARS, '\0') footgun -- on a trailing backslash, so counter and
writer disagreed and left an uninitialized heap byte in the returned string that
is handed to the legacy remote shell (protect_args=0). Make the counter mirror
the writer and guard the strchr with f[1] (which also correctly doubles a
trailing backslash).
Reported-by: Leonid Bugaev
(cherry picked from commit
|
||
|
Zen Dodd
|
439d6e1545 |
configure: avoid runtime IPv6 availability probe
(cherry picked from commit
|
||
|
Andrew Tridgell
|
202db7d329 |
socket: don't let open_socket_out() hang in connect()
The same kernel-side missed wakeup on the other side of the connection: a
blocking connect() can sleep forever on a connection that is already
established, with the 4-tuple ESTABLISHED at both ends and the listener's
greeting queued unread. Without --contimeout nothing breaks it.
Wait for the connect with poll() in slices rather than blocking in the
kernel, re-checking the socket on each pass, and take the result from
SO_ERROR. A finished slice is not a failure -- looping is what re-examines
the socket and recovers a missed wakeup.
--contimeout is unchanged: the alarm still fires and the caller still
reports RERR_CONTIMEOUT. The per-address errno is now stashed before
close()/alarm() can overwrite it.
Measured the same way, against a real loopback daemon: 20 hangs in 48,000
connects before, 0 in 48,000 after, with equal wall clock. This is the half
of the OpenBSD flakiness that the socketpair_tcp() fix does not cover: the
--use-tcp pass talks to a real rsyncd over a port, so it hangs here rather
than in accept().
(cherry picked from commit
|
||
|
Andrew Tridgell
|
598295e018 |
socket: don't let socketpair_tcp() hang in accept()
On OpenBSD a blocking accept() can sleep forever on a connection the kernel
has already completed: the 4-tuple is ESTABLISHED at both ends, the
connection is queued on the listener, and the accept()ing process is still
asleep in netacc. Nothing bounds that wait, so rsync hangs for good.
Poll the listener instead, with a non-blocking accept(), so a missed wakeup
costs another pass rather than the process. The accepted fd is put back
into blocking mode explicitly because BSD accept() gives it the listener's
non-blocking flag. A time(NULL) deadline bounds the whole wait the way
io.c bounds its own, rather than counting passes -- a signal on every pass
must not extend it and a poll() that returns at once must not consume it.
A listener that reports ready without yielding a connection (the peer can
reset first) pauses rather than spinning.
Measured on an OpenBSD 7.8 VM, driving the real binary through
RSYNC_CONNECT_PROG with 8 concurrent workers, alternating stock/patched
rounds: 111 hangs in 120,000 invocations before, 0 in 120,000 after, with
no change in throughput.
Every daemon test reaches socketpair_tcp() through RSYNC_CONNECT_PROG in
the default transport, so the hang landed on whichever daemon test happened
to be connecting. See dev-notes/openbsd-socketpair-accept-wedge.txt.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
0284340a54 |
testsuite: run the SIMD checksums against an unreadable page
simdtest allocated 64 spare bytes so it could test an unaligned buffer, which
is exactly the slack that hid a 64-byte over-read in the AVX2 assembly for as
long as it existed. Add a pass that places the buffer flush against a
PROT_NONE page, so a read past the end faults in the test rather than in
somebody's transfer.
Every length from 128 to 4096, so each remainder mod 64 and both alignments
are covered, and all four implementations are checked -- the assembly was the
one at fault here, but the intrinsic paths preload too.
It fails closed. A guard this test cannot set up means it is not testing what
the caller thinks, so a failed sysconf/mmap/mprotect is a failure rather than a
pass that looks identical to a real one. And because the dispatcher falls back
on a CPU without AVX2 -- where the guard loop proves nothing about the code
under test -- it says which of the two happened rather than letting a fallback
run read as coverage.
Without the fix this segfaults; the suite's simd-checksum test reports the
non-zero exit.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
b8a12ba32d |
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.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
570ae16b70 |
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.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
31cf17fb2e |
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.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
7166aff0f3 |
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.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
6cf2efd853 |
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.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
a504944bb1 |
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.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
c42e314384 |
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.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
861bfa0812 |
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.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
2e925412f9 |
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.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
83da1cefed |
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.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
a3d8ecc312 |
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.
(cherry picked from commit
|
||
|
Stuart Inglis
|
4cfdb22863 |
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
|
75bafbf11d |
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 |
||
|
Stuart Inglis
|
3b238246a9 |
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
|
8e53c48515 |
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
|
ef1a4c1300 |
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.
(cherry picked from commit
|
||
|
Filipe Casal
|
93d4d72f93 |
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.
(cherry picked from commit
|
||
|
fcasal
|
fa70cc902e |
backup: preserve backup-dir while creating it
(cherry picked from commit
|
||
|
Andrew Tridgell
|
464410967c |
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.
(cherry picked from commit
|
||
|
Filipe Casal
|
53c525041d |
socket: reject shell-active connect hosts
(cherry picked from commit
|
||
|
Andrew Tridgell
|
346c31037f |
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.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
838513e96d |
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.
(cherry picked from commit
|
||
|
Filipe Casal
|
0fd69bfdc8 |
daemon: reject shell-active hook expansions
(cherry picked from commit
|
||
|
Filipe Casal
|
4076aa6825 |
rrsync: protect no-overwrite auxiliary paths
--ignore-existing protects the live transfer destination, but three
peer-selectable options reach other existing objects inside the
restricted directory: --log-file appends to one, --partial-dir consumes
and then renames or unlinks one, and --delay-updates does the same
through its implicit .~tmp~ directory. Refuse all three under
-no-overwrite.
Refusal rather than confinement, because confining these paths does not
help: keeping the --log-file append inside the tree still appends to an
existing file, and for the partial directories the peer controls both
the directory and the transferred basename, so a collision is always
reachable. A pre-exec emptiness check would be raceable. The cost is
that a push naming --partial-dir on the remote receiver, or using
--delay-updates, is now refused for a -no-overwrite account.
Each regression drives the option through a wrapper WITHOUT
-no-overwrite as well, and requires that to be accepted. Without that
control the tests cannot tell "refused under -no-overwrite" from
"refused always", and would still pass if the options were disabled for
every rrsync deployment -- verified by making the refusal unconditional,
which the controls then catch.
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 every one of these
tests died before reaching the policy under test. A fleet run caught it
on the protocol columns of three targets.
Backup mode belongs in the same set. Publishing a backup onto a name
that already exists deletes what is there (backup.c make_backup()), and
deleting a file backs it up first (delete.c), so a --delete of an
unrelated file can land on a protected name -- overwriting a file that
--ignore-existing was holding, with rc=0 and no diagnostic. Disable -b
and --backup-dir. --suffix is left enabled: with both of those refused
nothing can turn backups on, so it is inert.
The short option must be disabled before short_no_arg_re is built, since
a stock client sends b inside the remote short-option bundle and the
regex is snapshotted there; the whole -no-overwrite block therefore
moves up beside the other policy gates rather than sitting after the
chdir. Its regression drives the collision through a wrapper without
-no-overwrite, passing --ignore-existing by hand, which both proves the
refusal is conditional and shows the primitive defeating the very
protection -no-overwrite forces.
rrsync.1.md now states what -no-overwrite costs: no explicit
--partial-dir, no --delay-updates, no server-side --log-file, no backups.
(cherry picked from commit
|
||
|
Filipe Casal
|
2e37d629c9 |
rrsync: keep remote files-from out of write-only mode
Write-only mode rejects ordinary downloads, but a remote --files-from
makes the receiving child open a server-side file and send it back over
the upload protocol. Reject server-local --files-from paths under -wo,
keeping the exact "-" sentinel that a client-local files-from upload
sends.
The control now uploads a second, unlisted source file and requires it
NOT to arrive. With a single file present an ordinary recursive upload
looks identical, so the control passed whether or not the list selected
anything.
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.
Known gap: this closes the argv route only. A per-directory merge
filter delivered over the protocol still reads a file outside the
restricted directory and returns its content in an "Unknown filter rule"
error -- reproduced with a stock client against -wo. rsync's existing
filter-file confinement does not apply because it is gated on am_daemon
and an rrsync server is not a daemon. Tracked separately.
(cherry picked from commit
|
||
|
Filipe Casal
|
f1e1bcb027 |
options: refuse aliases for exact option rules
parse_one_refuse_match() marked only the first long_options row whose
long name matched the configured spelling, then broke out for a
non-wildcard rule. --compress-threads and --zt are separate popt rows
that both write &do_compression_threads, so "refuse options =
compress-threads" disabled the canonical row and left the alias
accepted: the refused capability was still reachable under its other
name. The same shape covers zc/compress-choice and zl/compress-level.
An exact rule names a capability, not one spelling of it, so mark every
row that does the same thing. Comparing the raw table fields is not
enough for that: popt's `val` means different things per argInfo. For
POPT_ARG_VAL it IS the value stored in `arg`, while elsewhere a nonzero
`val` is an action code for the parser's switch, and POPT_ARG_NONE with
a destination stores 1 whatever `val` says. --del is
POPT_ARG_NONE/&delete_during/0 and --delete-during is
POPT_ARG_VAL/&delete_during/1: the same destination and the same
resulting value, but unequal as table entries, so "refuse options =
delete-during" was still evaded by --del and the mirror held too.
Compare what a row does instead -- the destination and the constant it
assigns, falling back to table-entry equality for rows that store a
runtime value or only dispatch an action. Enumerating all 258 rows,
this couples exactly one pair the field comparison missed, del and
delete-during, and changes nothing else. Opposite switches such as
--foo and --no-foo stay distinct because they assign different values.
Two regressions. The compress-threads one drives the raw daemon
protocol -- not to preserve the spelling, which -M--zt=N would do just as
well, but because it goes on to observe the worker pool the bypass
delivers. Its oracle is the refusal itself -- the alias connection torn
down and
"configured to refuse --zt" logged -- and deliberately not the resulting
worker count: an accepted --zt is a defeated refuse rule however few
threads it produces, and the daemon worker cap being added alongside
this holds that count to 9, so a count-based assertion passes while the
alias is still accepted. Run that test against the cap without this
parser fix and it does exactly that; the two changes were covering for
each other.
The delete one needs neither zstd nor a socket: --remote-option puts the
option in the daemon's argv verbatim, which is the reach an ordinary
user already has, so it drives a stock client both ways round against
modules refusing each spelling, with an unrefused module as the control.
The compress-threads test needs --use-tcp, so it skips in every other
column and is declared in the workflows that enforce a skip set, which a
fleet run otherwise reports as an unexpected skip on fourteen cells.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
33e54e68ca |
rrsync: deny device/special creation where creation happens
A restricted dir must not let a client have the spawned rsync create devices or special files in the served tree, but -a bundles -D into the client's short options, so refusing -D outright breaks every ordinary `rsync -a`. |
||
|
Andrew Tridgell
|
fc89bf6ddc |
rsync: add --drop-D, refusing device/special creation only
-D and --no-D do two jobs at once: they decide whether devices and
special files are created, and they decide whether those entries carry
their rdev fields on the wire. send_file_entry() and recv_file_entry()
frame those fields with the same condition, but each end evaluates its
own preserve_devices/preserve_specials, so the two only agree because
both normally parse the same command line.
That makes --no-D unusable for a wrapper that controls one end of a
connection and wants to deny creation. Give it to the receiver alone
and the client's -D sender writes rdev the receiver never reads: the
file list desynchronises from that entry on. A FIFO or socket breaks
below protocol 31 -- a hang at 29, "File-list index 0 not in 0 - -1" at
30 -- and a device node breaks at EVERY protocol, current ones included,
because its arm of the condition has no protocol clause at all.
--drop-D separates the two jobs: it refuses the creation and leaves the
encoding alone. The entry is skipped through the existing non-regular
fall-through, so the visible result matches --no-D, and because it
touches no wire state it can be applied to one end by itself.
It has no effect on a sending rsync, which creates nothing.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
c3ba28c19e |
rrsync: don't content-open a sender leaf rsync will never open
Two shapes a pristine 3.4.4 rrsync transfers, and |
||
|
Andrew Tridgell
|
d097106112 |
rrsync: only claim the inode pin where the kernel actually provides it
The HAVE_PROC_SELF_FD probe checked that readlink of a DIRECTORY's entry
returned the right path, which is not evidence of an inode pin, and two
platforms fail that assumption in opposite directions:
* NetBSD makes the entry a symlink for directories only -- readlink of
a regular file's entry fails with EINVAL -- so the probe passed and
then every pull of a file died in the post-pin check with
"post-pin readlink failed (race?): f1 Invalid argument". This is
not new: the same failure reproduces on the branch base.
* Cygwin's readlink returns the right path, but opening the magic link
RE-RESOLVES it. Renaming a directory out from under a held fd lets
the magic link reach the replacement, so the pin protected nothing
while appearing to. rrsync-sender-leaf-flip caught this as a real
outside-content leak, not as flakiness.
Only Linux provides the inode-bound magic link this depends on, so
require that explicitly and keep the runtime probes as a guard for
Linux-like environments where /proc is absent or restricted. Elsewhere
rrsync falls through to the unpinned path, as it already did on the BSDs
and macOS. proc_self_fd_pins() mirrors the rule so the race tests skip
rather than report the intended gap, and Cygwin's workflow expects both
of them to skip -- rrsync-symlink only ran there because the old probe
wrongly reported support.
(cherry picked from commit
|
||
|
Andrew TridgellandLeonid Bugaev
|
0894e0422e |
rrsync: pin a sender argument where rsync will actually resolve it
|
||
|
Andrew Tridgell
|
0875194e7b |
rrsync: pass the --files-from stdin sentinel through unchecked
A pull with a local --files-from does not send the list file to the server: it sends the literal "--files-from=-" and streams the names down the protocol connection. |
||
|
Andrew Tridgell
|
d569bb8a2d |
auth: parse "auth users" with conf_strtok so a leading comma means commas only
auth_server() tokenised on commas AND whitespace, ignoring the documented
comma-only form, so an entry containing a space was torn in two: the rule
the administrator wrote never matched, and a rule they never wrote
appeared from its tail. For "@Group Name:deny" that means the deny is
skipped and a later :rw entry can match instead -- an authorization
bypass for a member of the denied group.
conf_strtok() already implements the documented behaviour and the
daemon's gid field already uses it (clientserver.c); this consumer was
missed when that one was fixed.
Reported by Andres Berbescu. Refs #137.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
76b54cdba4 |
exclude: exempt the daemon's own filter parameters from the confinement
Confining every parse_filter_file() open to the module root also caught
"filter", "include from" and "exclude from" from rsyncd.conf. Those name
operator-configured paths and pointing them outside the module -- at
/etc/rsync/excludes, say -- is the ordinary way to write them; rsyncd.conf(5)
puts no constraint on where the file lives. The result was not a refused
rule but a refused connection:
failed to open exclude file /etc/rsync/excludes:
Too many levels of symbolic links (40)
rsync error: error in file IO (code 11) at exclude.c(1582)
with no symlink involved anywhere -- just a regular file outside the module.
Mark the window in which the daemon loads its own parameters and skip the
confinement there. Everything else, in particular the peer-driven dir-merge
the leak test exercises, is still confined. Also fix the trailing whitespace
in the original hunk.
(cherry picked from commit
|
||
|
Omar Elsayed
|
12d133c625 |
exclude: path resolving to operator path supplied --filter file
(cherry picked from commit
|
||
|
Filipe Casal
|
c6327e5b0e |
syscall: preserve ordinary mode when setgid is denied
macOS's fchmodat(..., AT_SYMLINK_NOFOLLOW) returns EPERM and applies NOTHING when the requested setgid bit is ungrantable, so "rsync -a --chmod=D2750,F0640" exits 23 and leaves the destination at 0755/0700 where 3.4.4 leaves 0750. fchmod() on an already-open descriptor does the right thing: it succeeds, drops the setgid bit it cannot grant, and applies the ordinary bits. That fd path existed but was fenced behind "#if defined __linux__". Guard it on O_NOFOLLOW so every platform that can open a leaf without following a symlink uses it, and leave the fchmodat/fchmodat2 fallbacks under __linux__. Measured on macOS with an ungrantable group: fchmodat(2750, NOFOLLOW) EPERM, 0700 -> 0700 (dir and FIFO alike) fchmod(fd, 2750) ok, 0700 -> 0750 fchmodat(0750, NOFOLLOW) ok, 0700 -> 0750 A FIFO observed by the lstat takes the pathname call instead, then one retry without S_ISGID. Opening a FIFO -- even O_NONBLOCK -- makes this process a reader for as long as the descriptor lives, which wakes a writer blocked in open(O_WRONLY) and can cost it a SIGPIPE or the bytes it writes before we close; the third line above is why that is avoidable. This does not make the function FIFO-open-free: the type comes from the lstat, so a leaf swapped to a FIFO after it is still opened, and set_file_attrs() opens FIFOs elsewhere for ACL/xattr work. Closing that needs the open constrained to the observed type, which is tracked separately. The retry is a pathname call and does not pin the inode, so a leaf swapped for another object of the same name is chmod'd instead; AT_SYMLINK_NOFOLLOW still keeps it off a symlink's target, and the held parent fd still confines the ancestors, so the out-of-tree boundary is unaffected. Only S_ISGID is retried -- clearing S_ISUID too could discard a bit that was grantable when only setgid caused the failure. Linux keeps the fd-first order it has always had. The raced-to-a-symlink refusal after openat() also accepts EMLINK and EFTYPE. ELOOP is not universal for O_NOFOLLOW on a symlink -- FreeBSD documents EMLINK and NetBSD EFTYPE -- so those two silently skipped the refusal and fell through to the (still symlink-safe) pathname call. Verified on real macOS: the regression test FAILS on |
||
|
Filipe Casal
|
0d7cd20362 |
syscall: preserve fake-super backups as placeholders
(cherry picked from commit
|
||
|
Andrew Tridgell
|
593abd13b4 |
sender: null-check the anchor before comparing it to the module root
The copy-links confinement gate null-checks module_dir but then passes
anchor to strcmp() without checking it, which the scan-build gate flags:
sender.c:291:7: warning: Null pointer passed to 1st parameter
expecting 'nonnull' [core.NonNullParamChecker]
Not reachable today -- the one caller passes module_dir -- but NULL is a
legitimate value for this parameter: secure_relative_open() reads it as
"relative to the cwd", which the else branch relies on. Only this branch
would dereference it.
(cherry picked from commit
|
||
|
Andrew Tridgell
|
375f4fd152 |
syscall: defer a literal ".." to the walk before the leaf fast paths
secure_walk_at() has two fast paths for the final component that call
openat(ds_cur(&ds), part, ...) directly instead of going through
ds_descend(). A final component of ".." therefore never met the anchor
floor, and what happened depended entirely on the caller's flags:
".." with O_DIRECTORY -> ELOOP (refused)
".." with O_DIRECTORY|O_NOFOLLOW -> fd for the directory ABOVE
the anchor
".." without O_DIRECTORY -> parent opened, then closed,
EISDIR returned
That was harmless while every literal ".." was rejected at the front
door, but secure_relative_open_at_beneath() now admits them and
documents the held-fd stack as refusing every climb above the anchor.
The sender's own call passes O_RDONLY|O_DIRECTORY and so was never
affected -- but the guarantee the new API advertises has to hold for
whatever flags the next caller picks.
Route a literal "." or ".." through ds_descend() before the leaf fast
paths. All three flag combinations now refuse a bare ".." with ELOOP,
and t_secure_relpath covers the matrix.
(cherry picked from commit
|
||
|
Filipe Casal
|
ad17cbd298 |
sender: allow confined parent-relative copy-links targets
(cherry picked from commit
|