]> git.ipfire.org Git - thirdparty/rsync.git/commitdiff
receiver: fix absolute --partial-dir delta resume (false verification)
authorAndrew Tridgell <andrew@tridgell.net>
Sat, 23 May 2026 22:48:42 +0000 (08:48 +1000)
committerAndrew Tridgell <andrew@tridgell.net>
Sun, 24 May 2026 02:31:52 +0000 (12:31 +1000)
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.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
receiver.c

index f49931bf1701fcea55e19c39098abf65daf3dbb0..7d429fe8431ff983f2fed3a42e15ef85b30f6140 100644 (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