mirror/rsync
1
0
Fork 0
mirror of https://github.com/RsyncProject/rsync.git synced 2026-06-08 06:05:57 -04:00
Code Issues Packages Projects Releases Wiki Activity

receiver: fix absolute --partial-dir delta resume (false verification)

Browse Source 
A delta (--no-whole-file) resume whose basis is an absolute --partial-dir
looped forever on exit code 23 ("failed verification -- update put into
partial-dir"), stranding the correct data in the partial-dir and never
populating the destination.

Cause: an absolute --partial-dir makes the basis path absolute, but the
receiver opened it with secure_relative_open(NULL, fnamecmp, ...), which by
design rejects an absolute relpath (EINVAL). The basis fd was then -1, so
receive_data() mapped no basis and (because the matched-block sum_update() is
guarded by "if (mapbuf)") computed the whole-file verification checksum over
the literal data only -> a spurious mismatch every run. (The data itself was
correct, since the in-place update leaves the matched basis bytes in place.)
Under a non-chroot daemon the in-place write went through the same call and
failed outright.

Fix: add secure_basis_open(), which treats an operator-trusted absolute basis
path as (trusted directory + confined leaf) -- the same way secure_relative_open
already trusts an absolute basedir while keeping O_NOFOLLOW on the leaf -- and
use it for both the basis read and the inplace-partial write. The strict
"reject absolute relpath" contract of secure_relative_open is left intact.

Defense-in-depth: receive_data() now treats a block-match token with no mapped
basis as a protocol inconsistency (it can only arise from a basis that the
generator opened but the receiver could not), failing cleanly instead of
silently dropping those bytes from the verify checksum or the output.

Thanks to @sylvain-ilm for the report (#724, #725).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Andrew Tridgell
2026-05-24 08:48:42 +10:00
parent 3786926703
commit 9e2e9f3362
1 changed files with 54 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

57
receiver.c
View File

@@ -83,6 +83,44 @@ static int updating_basis_or_equiv;
#define MAX_UNIQUE_NUMBER 999999
#define MAX_UNIQUE_LOOP 100
/* Open a basis/output path that may legitimately be an operator-trusted
* ABSOLUTE path -- e.g. an absolute --partial-dir ("a directory reserved for
* partial-dir work") or --backup-dir. secure_relative_open() deliberately
* rejects an absolute relpath, so feeding it the whole absolute partialptr
* (with a NULL basedir) returns EINVAL: the basis fd is then -1, no basis is
* mapped, and receive_data() omits every matched block from the whole-file
* verification checksum -> a spurious "failed verification" that strands the
* (correct) data in the partial-dir forever.
*
* The operator's directory is trusted; only the leaf basename is peer-supplied.
* So when basedir is NULL and relpath is absolute, split it into its directory
* (trusted) and leaf and confine just the leaf -- exactly how secure_relative_
* open already trusts an absolute basedir while O_NOFOLLOW-confining the leaf.
* Anything else is a straight pass-through that preserves the strict contract. */
static int secure_basis_open(const char *basedir, const char *relpath, int flags, mode_t mode)
{
if (!basedir && relpath && *relpath == '/') {
const char *slash = strrchr(relpath, '/');
const char *leaf = slash + 1;
char dirbuf[MAXPATHLEN];
const char *dir;
if (slash == relpath)
dir = "/";
else {
size_t dlen = slash - relpath;
if (dlen >= sizeof dirbuf) {
errno = ENAMETOOLONG;
return -1;
}
memcpy(dirbuf, relpath, dlen);
dirbuf[dlen] = '\0';
dir = dirbuf;
}
return secure_relative_open(dir, leaf, flags, mode);
}
return secure_relative_open(basedir, relpath, flags, mode);
}
/* get_tmpname() - create a tmp filename for a given filename
*
* If a tmpdir is defined, use that as the directory to put it in. Otherwise,
@@ -364,6 +402,18 @@ static int receive_data(int f_in, char *fname_r, int fd_r, OFF_T size_r,
stats.matched_data += len;
/* A block match can only be honored if we actually mapped the
* basis. If we didn't (basis open failed), the sender should
* never have been told a basis existed -- treat it as a protocol
* inconsistency rather than silently omitting these bytes from
* the verification checksum (which yields a spurious failure) or
* leaving a hole in the output. */
if (!mapbuf) {
rprintf(FERROR, "got a block match with no basis file for %s [%s]\n",
full_fname(fname), who_am_i());
exit_cleanup(RERR_PROTOCOL);
}
if (DEBUG_GTE(DELTASUM, 3)) {
rprintf(FINFO,
"chunk[%d] of size %ld at %s offset=%s%s\n",
@@ -793,8 +843,9 @@ int recv_files(int f_in, int f_out, char *local_name)
fnamecmp = fname;
}
/* open the file */
fd1 = secure_relative_open(basedir, fnamecmp, O_RDONLY, 0);
/* open the file (secure_basis_open tolerates an operator-trusted
* absolute fnamecmp, e.g. an absolute --partial-dir basis) */
fd1 = secure_basis_open(basedir, fnamecmp, O_RDONLY, 0);
if (fd1 == -1 && protocol_version < 29) {
if (fnamecmp != fname) {
@@ -884,7 +935,7 @@ int recv_files(int f_in, int f_out, char *local_name)
* attacker could switch a directory to a symlink between
* path validation and file open. */
if (use_secure_symlinks)
fd2 = secure_relative_open(NULL, fnametmp, O_WRONLY|O_CREAT, 0600);
fd2 = secure_basis_open(NULL, fnametmp, O_WRONLY|O_CREAT, 0600);
else
fd2 = do_open(fnametmp, O_WRONLY|O_CREAT, 0600);
#ifdef linux