diff --git a/src/client/client_cli.c b/src/client/client_cli.c index 48815d5..b47a059 100644 --- a/src/client/client_cli.c +++ b/src/client/client_cli.c @@ -117,6 +117,27 @@ static int set_string_option(char** dest, const char* value, const char* option_ return 0; } +/* Append a --chmod clause list to the accumulated spec with a comma. rsync + * 3.2.4+ makes repeated --chmod options cumulative, so they must not replace + * the previous ones. Returns 0 on success, -1 on failure. */ +static int append_chmod_spec(char** dest, const char* value) { + if (!*dest) + return set_string_option(dest, value, "--chmod"); + size_t old_len = strlen(*dest); + size_t add_len = strlen(value); + char* merged = malloc(old_len + add_len + 2); + if (!merged) { + log_message(LOG_LEVEL_ERROR, "memory allocation failed for --chmod"); + return -1; + } + memcpy(merged, *dest, old_len); + merged[old_len] = ','; + memcpy(merged + old_len + 1, value, add_len + 1); + free(*dest); + *dest = merged; + return 0; +} + /* Parse a string as a positive integer into *dest. Returns 0 on success, -1 on error. */ static int set_positive_int_option(int* dest, const char* value, const char* option_name) { if (!parse_positive_int(value, dest)) { @@ -966,6 +987,8 @@ static int apply_table_option(Config* config, const OptionEntry* entry, const ch case OPT_NOOP: return 0; case OPT_STRING: + if (entry->offset == offsetof(Config, chmod_spec)) + return append_chmod_spec((char**)field, value); return set_string_option((char**)field, value, entry->name); case OPT_POS_INT: return set_positive_int_option((int*)field, value, entry->name); @@ -1224,7 +1247,6 @@ static bool cli_handle_table_option(CliParseCtx* ctx) { ctx->exit_code = -1; return true; } - config->preserve_perms = true; } /* Remember that --server-host was explicitly given (the field itself defaults to 127.0.0.1, so a value check cannot distinguish it). Used @@ -1271,7 +1293,7 @@ static bool cli_handle_inline_chmod(CliParseCtx* ctx) { const char* arg = ctx->argv[ctx->i]; if (strncmp(arg, "--chmod=", 8) != 0) return false; - if (set_string_option(&config->chmod_spec, arg + 8, "--chmod") != 0) { + if (append_chmod_spec(&config->chmod_spec, arg + 8) != 0) { ctx->exit_code = -1; return true; } @@ -1281,7 +1303,6 @@ static bool cli_handle_inline_chmod(CliParseCtx* ctx) { ctx->exit_code = -1; return true; } - config->preserve_perms = true; return true; } diff --git a/src/client/usage.c b/src/client/usage.c index a091727..427dd56 100644 --- a/src/client/usage.c +++ b/src/client/usage.c @@ -202,7 +202,8 @@ void print_usage(void) { printf(" --copy-as); --numeric-ids only changes how ids map\n"); printf(" --no-super Forbid those super-user activities even when the\n"); printf(" receiver is running as root\n"); - printf(" --chmod Modify transferred permissions (rsync syntax)\n"); + printf( + " --chmod Modify new/transferred permissions (rsync syntax; implies no -p)\n"); printf(" --numeric-ids Map uid/gid by id instead of by name (a modifier, not\n"); printf(" an ownership request: combine with -o/-g or a map)\n"); printf(" --usermap=MAP Map usernames when applying ownership: comma-separated\n"); diff --git a/src/shared/chmod.c b/src/shared/chmod.c index bfea77b..aa5f33e 100644 --- a/src/shared/chmod.c +++ b/src/shared/chmod.c @@ -1,90 +1,183 @@ #include "chmod.h" +#include "file.h" #include #include -static bool parse_clause(mode_t* mode, const char* begin, const char* end) { - const char* p = begin; - unsigned who = 0; - while (p < end && strchr("ugoa", *p)) { - if (*p == 'a') - who = 7; - else - who |= *p == 'u' ? 1U : (*p == 'g' ? 2U : 4U); - p++; - } - if (who == 0) - who = 7; - if (p == end || (*p != '+' && *p != '-' && *p != '=')) - return false; - char operation = *p++; - mode_t bits = 0; - while (p < end) { - mode_t bit; - switch (*p++) { - case 'r': - bit = 4; - break; - case 'w': - bit = 2; - break; - case 'x': - bit = 1; - break; - default: - return false; - } - bits |= bit; - } - for (unsigned class_index = 0; class_index < 3; class_index++) { - unsigned class_bit = 1U << class_index; - if (!(who & class_bit)) - continue; - mode_t shift = (mode_t)((2U - class_index) * 3U); - mode_t mask = (mode_t)(7U << shift); - mode_t class_bits = (mode_t)(bits << shift); - if (operation == '+') - *mode |= class_bits; - else if (operation == '-') - *mode &= ~class_bits; - else - *mode = (*mode & ~mask) | class_bits; - } - return true; -} +/* rsync's --chmod parser (parse_chmod + tweak_mode). A single clause is + * applied as it is completed, so repeated clauses and repeated --chmod options + * (joined with commas by the CLI) accumulate exactly like rsync. The D/F + * selectors restrict a clause to directories/files; X adds execute only to + * directories or files that were already executable. */ + +#define CHMOD_BITS 07777 +#define CHMOD_FLAG_X_KEEP (1U << 0) +#define CHMOD_FLAG_DIRS_ONLY (1U << 1) +#define CHMOD_FLAG_FILES_ONLY (1U << 2) + +enum chmod_op { CHMOD_OP_ADD = 1, CHMOD_OP_SUB, CHMOD_OP_EQ, CHMOD_OP_SET }; +enum chmod_state { + CHMOD_STATE_ERROR, + CHMOD_STATE_1ST_HALF, + CHMOD_STATE_2ND_HALF, + CHMOD_STATE_OCTAL +}; bool chmod_apply(mode_t mode, const char* spec, mode_t* result) { if (!spec || !*spec || !result) return false; - bool numeric = true; - size_t length = strlen(spec); - if (length > 4) - numeric = false; - for (size_t i = 0; i < length && numeric; i++) - numeric = spec[i] >= '0' && spec[i] <= '7'; - if (numeric) { - if (length == 0 || length > 4) - return false; - mode_t parsed = 0; - for (size_t i = 0; i < length; i++) - parsed = (mode_t)((parsed << 3) | (spec[i] - '0')); - *result = parsed; - return true; - } - + const mode_t nonperm = mode & ~(mode_t)CHMOD_BITS; + const bool initially_executable = (mode & 0111) != 0; mode_t changed = mode; - const char* begin = spec; - while (*begin) { - const char* end = strchr(begin, ','); - if (!end) - end = begin + strlen(begin); - if (!parse_clause(&changed, begin, end)) - return false; - if (*end == '\0') + int state = CHMOD_STATE_1ST_HALF; + unsigned where = 0; + int what = 0, op = 0, topbits = 0, topoct = 0, flags = 0; + const char* p = spec; + while (state != CHMOD_STATE_ERROR) { + if (*p == '\0' || *p == ',') { + int bits; + if (!op) { + state = CHMOD_STATE_ERROR; + break; + } + if (where) + bits = (int)(where * (unsigned)what); + else { + where = 0111; + bits = (int)((where * (unsigned)what) & ~(unsigned)file_process_umask()); + } + int mode_and, mode_or; + switch (op) { + case CHMOD_OP_ADD: + mode_and = CHMOD_BITS; + mode_or = bits + topoct; + break; + case CHMOD_OP_SUB: + mode_and = CHMOD_BITS - bits - topoct; + mode_or = 0; + break; + case CHMOD_OP_EQ: + mode_and = CHMOD_BITS - (int)(where * 7U) - (topoct ? topbits : 0); + mode_or = bits + topoct; + break; + default: + mode_and = 0; + mode_or = bits; + break; + } + bool is_dir = S_ISDIR(nonperm); + if (!((flags & CHMOD_FLAG_DIRS_ONLY) && !is_dir) && + !((flags & CHMOD_FLAG_FILES_ONLY) && is_dir)) { + changed &= (mode_t)mode_and; + if ((flags & CHMOD_FLAG_X_KEEP) && !initially_executable && !is_dir) + changed |= (mode_t)(mode_or & ~0111); + else + changed |= (mode_t)mode_or; + } + if (*p == '\0') + break; + p++; + state = CHMOD_STATE_1ST_HALF; + where = 0; + what = op = topoct = topbits = flags = 0; + continue; + } + switch (state) { + case CHMOD_STATE_1ST_HALF: + switch (*p) { + case 'D': + if (flags & CHMOD_FLAG_FILES_ONLY) { + state = CHMOD_STATE_ERROR; + break; + } + flags |= CHMOD_FLAG_DIRS_ONLY; + break; + case 'F': + if (flags & CHMOD_FLAG_DIRS_ONLY) { + state = CHMOD_STATE_ERROR; + break; + } + flags |= CHMOD_FLAG_FILES_ONLY; + break; + case 'u': + where |= 0100; + topbits |= 04000; + break; + case 'g': + where |= 0010; + topbits |= 02000; + break; + case 'o': + where |= 0001; + break; + case 'a': + where |= 0111; + break; + case '+': + op = CHMOD_OP_ADD; + state = CHMOD_STATE_2ND_HALF; + break; + case '-': + op = CHMOD_OP_SUB; + state = CHMOD_STATE_2ND_HALF; + break; + case '=': + op = CHMOD_OP_EQ; + state = CHMOD_STATE_2ND_HALF; + break; + default: + if (*p >= '0' && *p <= '7' && !where) { + op = CHMOD_OP_SET; + state = CHMOD_STATE_OCTAL; + where = 1; + what = *p - '0'; + } else { + state = CHMOD_STATE_ERROR; + } + break; + } break; - begin = end + 1; - if (!*begin) - return false; + case CHMOD_STATE_2ND_HALF: + switch (*p) { + case 'r': + what |= 4; + break; + case 'w': + what |= 2; + break; + case 'X': + flags |= CHMOD_FLAG_X_KEEP; + /* fall through */ + case 'x': + what |= 1; + break; + case 's': + if (topbits) + topoct |= topbits; + else + topoct = 04000; + break; + case 't': + topoct |= 01000; + break; + default: + state = CHMOD_STATE_ERROR; + break; + } + break; + default: + if (*p >= '0' && *p <= '7') { + what = what * 8 + (*p - '0'); + if (what > CHMOD_BITS) + state = CHMOD_STATE_ERROR; + } else { + state = CHMOD_STATE_ERROR; + } + break; + } + p++; } - *result = changed; + if (state == CHMOD_STATE_ERROR) + return false; + *result = (changed & (mode_t)CHMOD_BITS) | nonperm; return true; } diff --git a/src/shared/chmod.h b/src/shared/chmod.h index c3b259c..9837228 100644 --- a/src/shared/chmod.h +++ b/src/shared/chmod.h @@ -4,7 +4,10 @@ #include #include -/* Apply the supported rsync --chmod syntax to a permission mode. */ +/* Apply rsync's --chmod syntax to a permission mode, including the D/F/X + * selectors and the s/t special bits. `mode` should carry the file type bits + * (S_IFDIR/S_IFREG) so D/F/X can be evaluated; the type bits are preserved in + * `result`. A spec may contain comma-separated clauses, which accumulate. */ bool chmod_apply(mode_t mode, const char* spec, mode_t* result); #endif diff --git a/src/shared/file.c b/src/shared/file.c index bd03e6c..27084d6 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -112,21 +112,16 @@ unsigned file_process_umask(void) { /* Base mode applied when the policy does not take the source mode wholesale * (i.e. --perms is off). A pre-existing destination keeps its own mode; a - * brand-new file is created like rsync: source_mode & 0777 & ~umask, with - * S_IWGRP|S_IWOTH always cleared so a client mode can never grant group/other - * write (the daemon runs with umask(0)). Only when no metadata is available at - * all does the historical fixed 0644 default apply. The -E rule (and no-op for - * a plain -t) is layered on top of this base. */ + * brand-new file is created like rsync: source_mode & 0777 & ~umask (special + * bits are not part of a mode-preserving transfer without -p). Only when no + * metadata is available at all does the historical fixed 0644 default apply. + * The -E rule (and no-op for a plain -t) is layered on top of this base. */ static mode_t file_mode_base(const FileMetadata* metadata, bool existing_known, mode_t existing_mode) { if (existing_known) return existing_mode; if (metadata) - /* A brand-new file follows rsync's source_mode & ~umask base, but a - * client-supplied source mode must never grant group/other write (the - * daemon runs with umask(0), so an unmasked 0666 would otherwise create a - * world-writable file). S_IWGRP|S_IWOTH are always cleared. */ - return metadata->mode & 0777 & ~(mode_t)file_process_umask() & ~(S_IWGRP | S_IWOTH); + return metadata->mode & 0777 & ~(mode_t)file_process_umask(); return S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH; } diff --git a/src/shared/file_receive.c b/src/shared/file_receive.c index 282704c..6da4325 100644 --- a/src/shared/file_receive.c +++ b/src/shared/file_receive.c @@ -453,10 +453,12 @@ static FileSaveResult file_save_special_to_disk(const char* root_directory, cons create_mode = S_IFIFO; } const char* node_kind = (is_char || is_blk) ? "device" : (is_fifo ? "FIFO" : "socket"); - /* The creation permission bits come from the source only under -p/--perms; - * otherwise a safe default (0644, group/other write never granted) keeps an - * unprivileged no--p run from materializing a world-writable node. */ - mode_t perms = config->preserve_perms ? (mode & 0777 & ~(S_IWGRP | S_IWOTH)) : 0644; + /* Under -p/--perms rsync copies the source's permission and special bits; a + * kernel that denies setuid/setgid/sticky reports the failure rather than + * having them masked here. Without -p the node is created like any other new + * entry: source_mode & 0777 & ~umask. */ + mode_t perms = config->preserve_perms ? (mode & (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0777)) + : (mode & 0777 & ~(mode_t)file_process_umask()); int rc = is_fifo ? mkfifoat(parent_fd, leaf, perms) : mknodat(parent_fd, leaf, create_mode | perms, rdev); @@ -2640,11 +2642,9 @@ void dir_metadata_list_apply(const DirTimeList* list, const char* root_directory mode_ready = false; } if (mode_ready) { - /* Route the directory mode through the SAME sanitization as the - * regular-file policy: a client-supplied mode never grants group/other - * write. */ - mode_t safe_mode = - (dir_mode & 0777 & ~(S_IWGRP | S_IWOTH)) | (dir_mode & (S_ISGID | S_ISVTX)); + /* rsync -p copies the source directory mode exactly, including + * group/other write and the setgid/sticky bits. */ + mode_t safe_mode = dir_mode & (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0777); if (dir_fd < 0) { char* escaped_path = output_escape(dir_path, log_get_8_bit_output()); log_message(LOG_LEVEL_WARNING, "Failed to open directory %s to set its mode: %s", diff --git a/src/shared/metadata.c b/src/shared/metadata.c index be701f9..168d574 100644 --- a/src/shared/metadata.c +++ b/src/shared/metadata.c @@ -213,8 +213,11 @@ bool metadata_mode_for_policy(mode_t source_mode, mode_t current_mode, FileAttrP mode_t* out_mode) { const mode_t execute_bits = S_IXUSR | S_IXGRP | S_IXOTH; if (policy.perms) { - /* Group/other write is never granted from a client-supplied mode. */ - *out_mode = source_mode & 0777 & ~(S_IWGRP | S_IWOTH); + /* rsync --perms copies the source's permission and special bits exactly, + * including group/other write and setuid/setgid/sticky. The kernel may + * still clear setgid when the receiver is not in the file's group; the + * caller logs a failed chmod rather than silently masking the bits here. */ + *out_mode = source_mode & (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0777); return true; } if (policy.executability) { @@ -223,9 +226,9 @@ bool metadata_mode_for_policy(mode_t source_mode, mode_t current_mode, FileAttrP * bits from the DESTINATION's own read bits (so a class that can read may * execute); otherwise clear every execute bit. This runs on the * destination-derived base (pre-existing dest mode, or source&~umask for a - * new file), and leaves special bits untouched. --perms wins when both are - * set (handled above). */ - mode_t base = current_mode & 0777; + * new file), and leaves the special bits untouched. --perms wins when both + * are set (handled above). */ + mode_t base = current_mode & (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0777); if (source_mode & 0111) *out_mode = base | ((base & 0444) >> 2); else @@ -310,7 +313,7 @@ bool file_restore_symlink_metadata(const char* path, const FileMetadata* metadat platforms that support it and quietly ignore the unsupported case so the transfer never fails over it. */ if (policy.perms) { - mode_t link_mode = metadata->mode & 0777 & ~(S_IWGRP | S_IWOTH); + mode_t link_mode = metadata->mode & (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0777); if (fchmodat(parent_fd, leaf, link_mode, AT_SYMLINK_NOFOLLOW) != 0 && errno != EOPNOTSUPP && errno != ENOTSUP && errno != ENOSYS) { log_message(LOG_LEVEL_DEBUG, "Could not set symlink mode on %s: %s", path, strerror(errno)); @@ -343,15 +346,6 @@ bool file_restore_metadata_fd(int fd, const FileMetadata* metadata, FileAttrPoli if (fd < 0 || metadata == NULL) return metadata == NULL; bool ok = true; - if (policy.perms || policy.executability) { - struct stat current; - if (fstat(fd, ¤t) != 0) - return false; - mode_t safe_mode = 0; - bool apply_mode = metadata_mode_for_policy(metadata->mode, current.st_mode, policy, &safe_mode); - if (apply_mode && fchmod(fd, safe_mode) != 0) - ok = false; - } /* Client uid/gid values are deliberately not authoritative UNLESS the client explicitly opted in with an identity flag (--numeric-ids / --usermap / --groupmap / --chown / -o/-g). identity_apply_ownership is the controlled, @@ -362,9 +356,20 @@ bool file_restore_metadata_fd(int fd, const FileMetadata* metadata, FileAttrPoli marks this entry as failed instead of reporting a wrong-owner write as success. With no identity flag set it is a no-op, so a default or plain -M transfer keeps FastSync's existing behavior of never applying client - ownership. */ + ownership. Ownership runs BEFORE the mode because a chown clears + setuid/setgid; rsync likewise chowns first and then restores the source + mode (including its special bits). */ if (!identity_apply_ownership(fd, (int32_t)metadata->uid, (int32_t)metadata->gid)) ok = false; + if (policy.perms || policy.executability) { + struct stat current; + if (fstat(fd, ¤t) != 0) + return false; + mode_t safe_mode = 0; + bool apply_mode = metadata_mode_for_policy(metadata->mode, current.st_mode, policy, &safe_mode); + if (apply_mode && fchmod(fd, safe_mode) != 0) + ok = false; + } /* --crtimes captures and transmits the source birth time, but there is no * portable way to set a birth time (utimensat can only set atime/mtime), so * the receiver deliberately does NOT apply it. This is explicit, honest diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index ef96422..1e50d59 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -936,6 +936,133 @@ class TestChmod: received = get_dest_received_dir(DEST_DIR, SOURCE_DIR) assert (os.stat(os.path.join(received, "small.txt")).st_mode & 0o777) == 0o644 + @pytest.mark.ci + def test_chmod_does_not_imply_perms(self, shared_server): + """rsync's --chmod only tweaks the mode used for a NEW destination; it + does not imply -p, so a pre-existing destination keeps its own mode.""" + source = os.path.join(TEST_DATA_DIR, "chmod_nop_src") + dest = os.path.join(TEST_DATA_DIR, "chmod_nop_dst") + clean_dir(source) + clean_dir(dest) + src_file = os.path.join(source, "f.txt") + with open(src_file, "wb") as fh: + fh.write(b"one\n") + os.chmod(src_file, 0o644) + + result, _ = run_client(source, dest, flags=["-p"], port=shared_server.port) + assert result.returncode == 0, f"seed failed: {(result.stderr or '')[:200]}" + dst_file = os.path.join(get_dest_received_dir(dest, source), "f.txt") + os.chmod(dst_file, 0o600) + with open(src_file, "wb") as fh: + fh.write(b"two, changed content\n") + + result, _ = run_client(source, dest, flags=["--chmod=go+w"], + port=shared_server.port) + assert result.returncode == 0, \ + f"--chmod failed: {(result.stderr or result.stdout)[:300]}" + got = stat.S_IMODE(os.stat(dst_file).st_mode) + assert got == 0o600, \ + f"--chmod must not imply -p; existing dest mode changed to {oct(got)}" + + @pytest.mark.ci + def test_chmod_go_w_with_perms(self, shared_server): + source = os.path.join(TEST_DATA_DIR, "chmod_gow_src") + dest = os.path.join(TEST_DATA_DIR, "chmod_gow_dst") + clean_dir(source) + clean_dir(dest) + src_file = os.path.join(source, "f.txt") + with open(src_file, "wb") as fh: + fh.write(b"x\n") + os.chmod(src_file, 0o644) + + result, _ = run_client(source, dest, flags=["-p", "--chmod=go+w"], + port=shared_server.port) + assert result.returncode == 0, \ + f"-p --chmod=go+w failed: {(result.stderr or result.stdout)[:300]}" + got = stat.S_IMODE(os.stat( + os.path.join(get_dest_received_dir(dest, source), "f.txt")).st_mode) + assert got == 0o666, f"--chmod=go+w must grant group/other write, got {oct(got)}" + + @pytest.mark.ci + def test_chmod_repeated_options_accumulate(self, shared_server): + source = os.path.join(TEST_DATA_DIR, "chmod_append_src") + dest = os.path.join(TEST_DATA_DIR, "chmod_append_dst") + clean_dir(source) + clean_dir(dest) + src_file = os.path.join(source, "f.txt") + with open(src_file, "wb") as fh: + fh.write(b"x\n") + os.chmod(src_file, 0o644) + + result, _ = run_client(source, dest, + flags=["-p", "--chmod=a+r", "--chmod=a-w"], + port=shared_server.port) + assert result.returncode == 0, \ + f"append --chmod failed: {(result.stderr or result.stdout)[:300]}" + got = stat.S_IMODE(os.stat( + os.path.join(get_dest_received_dir(dest, source), "f.txt")).st_mode) + assert got == 0o444, f"repeated --chmod must accumulate, got {oct(got)}" + + @pytest.mark.ci + @pytest.mark.skipif(shutil.which("rsync") is None, reason="rsync not installed") + def test_chmod_matches_rsync(self, shared_server): + """Differential --chmod verification against rsync 3.4.1 for D/F/X + selectors, no-/with--p new files, special bits, and append semantics.""" + cases = [ + ("go_w_no_p", ["--chmod=go+w"], {}, {"f.txt": (b"x", 0o644)}, ["f.txt"]), + ("go_w_p", ["-p", "--chmod=go+w"], {}, {"f.txt": (b"x", 0o644)}, ["f.txt"]), + ("world_writable_p", ["-p"], {}, {"f.txt": (b"x", 0o666)}, ["f.txt"]), + ("setgid_sticky_dirs_p", ["-p"], {"sg": 0o2755, "st": 0o1777}, + {"sg/a.txt": (b"x", 0o644), "st/b.txt": (b"x", 0o644)}, + ["sg", "st"]), + ("special_file_p", ["-p"], {}, {"s": (b"x", 0o6755)}, ["s"]), + ("archive_special_file", ["-a"], {}, {"s": (b"x", 0o6755)}, ["s"]), + ("archive_setgid_dir", ["-a"], {"d": 0o2755}, + {"d/a.txt": (b"x", 0o644)}, ["d"]), + ("dfx_p", ["-p", "--chmod=Dg+s,Fo-w,+X"], {"d": 0o700}, + {"d/inner.txt": (b"x", 0o644), "f.txt": (b"x", 0o644)}, ["d", "f.txt"]), + ("x_selector_p", ["-p", "--chmod=a+X"], {"d": 0o600}, + {"d/inner.txt": (b"x", 0o644), "exe": (b"x", 0o755), "noexe": (b"x", 0o644)}, + ["d", "exe", "noexe"]), + ("append_p", ["-p", "--chmod=a+r", "--chmod=a-w"], {}, + {"f.txt": (b"x", 0o644)}, ["f.txt"]), + ] + for name, flags, dirs, files, check in cases: + source = os.path.join(TEST_DATA_DIR, f"chmod_diff_{name}_src") + fdest = os.path.join(TEST_DATA_DIR, f"chmod_diff_{name}_fs") + rdest = os.path.join(TEST_DATA_DIR, f"chmod_diff_{name}_rsync") + clean_dir(source) + clean_dir(fdest) + clean_dir(rdest) + for rel, mode in dirs.items(): + path = os.path.join(source, rel) + os.makedirs(path, exist_ok=True) + os.chmod(path, mode) + for rel, (content, mode) in files.items(): + path = os.path.join(source, rel) + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "wb") as fh: + fh.write(content) + os.chmod(path, mode) + + rsync_result = subprocess.run( + ["rsync", "-r"] + flags + [source + "/", rdest + "/"], + text=True, capture_output=True) + assert rsync_result.returncode == 0, \ + f"rsync {name} failed: {rsync_result.stderr[:300]}" + + result, _ = run_client(source, fdest, flags=flags, port=shared_server.port) + assert result.returncode == 0, \ + f"FastSync {name} failed: {(result.stderr or result.stdout)[:300]}" + + fs_root = get_dest_received_dir(fdest, source) + for rel in check: + rsync_mode = stat.S_IMODE(os.lstat(os.path.join(rdest, rel)).st_mode) + fs_mode = stat.S_IMODE(os.lstat(os.path.join(fs_root, rel)).st_mode) + assert fs_mode == rsync_mode, ( + f"{name}: mode mismatch for {rel}: " + f"FastSync {oct(fs_mode)} != rsync {oct(rsync_mode)}") + class TestPreallocate: """--preallocate allocates the destination file space up front; the final diff --git a/tests/integration/test_preserve_attrs.py b/tests/integration/test_preserve_attrs.py index af78532..66ab361 100644 --- a/tests/integration/test_preserve_attrs.py +++ b/tests/integration/test_preserve_attrs.py @@ -95,14 +95,14 @@ class TestPreservePerms: source = os.path.join(TEST_DATA_DIR, "perms_nop_new_src") dest = os.path.join(TEST_DATA_DIR, "perms_nop_new_dst") # 0664 has group/other bits that the umask strips, so the result is not - # just the source mode. FastSync additionally never grants group/other - # write from a client-supplied mode (S_IWGRP|S_IWOTH are always - # cleared), so the expected mode masks those too. + # just the source mode. Under strict rsync parity the source mode is + # masked only by the umask (group/other write is no longer force-cleared + # on top of it). _seed_file(source, dest, "f.txt", b"new\n", 0o664) result, _ = run_client(source, dest, flags=["-t"], port=shared_server.port) - assert result.returncode == 0, f"-t failed: {(result.stderr or '')[:300]}" - want = 0o664 & ~_process_umask() & ~0o022 + assert result.returncode == 0, f"-t failed: {(result.stderr or result.stdout)[:300]}" + want = 0o664 & ~_process_umask() got = os.stat(_received(dest, source, "f.txt")).st_mode & 0o777 assert got == want, \ f"new no--p destination mode: want {oct(want)}, got {oct(got)}" @@ -254,15 +254,15 @@ class TestDirectoryModes: assert got == 0o750, f"-p must apply the source directory mode, got {oct(got)}" @pytest.mark.ci - def test_p_sanitizes_directory_group_other_write(self, shared_server): - # A 0777 source directory must never produce a group/other-writable - # destination directory: the file-mode sanitization is applied to dirs. - source, dest, _ = self._tree("dirmode_sanitize", 0o777, pin_mtime=False) + def test_p_preserves_directory_group_other_write(self, shared_server): + # Strict rsync parity: -p copies the source directory mode exactly, + # including group/other write (the old sanitization is gone). + source, dest, _ = self._tree("dirmode_go_write", 0o777, pin_mtime=False) result, _ = run_client(source, dest, flags=["-p"], port=shared_server.port) assert result.returncode == 0, f"-p failed: {(result.stderr or result.stdout)[:300]}" mode = os.stat(os.path.join(get_dest_received_dir(dest, source), "sub")).st_mode & 0o777 - assert mode & 0o022 == 0, \ - f"directory must never be group/other writable, got {oct(mode)}" + assert mode == 0o777, \ + f"-p must preserve the source directory mode exactly, got {oct(mode)}" @pytest.mark.ci def test_omit_dir_times_suppresses_times_not_modes(self, shared_server): @@ -443,14 +443,13 @@ class TestPreserveFeatureMatrix: class TestSpecialNodeModes: - """Security: a client can never grant group/other write, including on a - recreated special node (FIFO). The special-node creation path sanitizes - S_IWGRP|S_IWOTH just like the regular-file and directory paths, so a source - FIFO with mode 0777 must land as 0755 (owner/group/other read+exec from the - source otherwise preserved). FIFOs are created unprivileged via mkfifo.""" + """Strict rsync parity: with -p the source FIFO mode is copied exactly, + including group/other write. Without -p the node follows the same + source & ~umask base as any other new entry. FIFOs are created + unprivileged via mkfifo.""" @pytest.mark.ci - def test_specials_p_sanitizes_fifo_group_other_write(self): + def test_specials_p_preserves_fifo_mode(self): source = os.path.join(TEST_DATA_DIR, "specialmode_src") dest = os.path.join(TEST_DATA_DIR, "specialmode_dst") clean_dir(source) @@ -464,8 +463,8 @@ class TestSpecialNodeModes: # Production daemonizes with umask(0) (server.c) so the source mode is # what reaches mkfifo. The session server runs in the foreground and # would inherit the runner's umask, which alone would strip the write - # bits and mask a regression in the sanitization. Start a dedicated - # foreground server under umask(0) to exercise the real path. + # bits and mask a regression. Start a dedicated foreground server under + # umask(0) to exercise the real path. server = ServerManager() saved_umask = os.umask(0) try: @@ -486,7 +485,5 @@ class TestSpecialNodeModes: st = os.lstat(received) assert stat.S_ISFIFO(st.st_mode), f"received entry is not a FIFO: {oct(st.st_mode)}" mode = st.st_mode & 0o777 - assert mode & 0o022 == 0, \ - f"recreated FIFO must never be group/other writable, got {oct(mode)}" - assert mode == 0o755, \ - f"-p must preserve the source FIFO mode minus group/other write (want 0o755), got {oct(mode)}" + assert mode == 0o777, \ + f"-p must preserve the source FIFO mode exactly (want 0o777), got {oct(mode)}" diff --git a/tests/test_client_cli.c b/tests/test_client_cli.c index 564012e..50d1072 100644 --- a/tests/test_client_cli.c +++ b/tests/test_client_cli.c @@ -530,7 +530,9 @@ static void test_parse_args_chmod() { int positional_count = 0; EXPECT_EQ_INT(parse_args(cfg, 4, argv, positional_args, &positional_count), 0); EXPECT_EQ_STR(cfg->chmod_spec, "u=rw,go=r"); - EXPECT_TRUE(cfg->preserve_perms); + /* rsync's --chmod does NOT imply --perms: it only tweaks the mode used for a + * new destination unless -p is also given. */ + EXPECT_FALSE(cfg->preserve_perms); EXPECT_TRUE(cfg->use_metadata); mode_t result; EXPECT_TRUE(chmod_apply(0777, cfg->chmod_spec, &result)); @@ -545,14 +547,39 @@ static void test_parse_args_numeric_chmod() { int positional_count = 0; EXPECT_EQ_INT(parse_args(cfg, 5, argv, positional_args, &positional_count), 0); EXPECT_EQ_STR(cfg->chmod_spec, "7777"); - EXPECT_TRUE(cfg->preserve_perms); + EXPECT_FALSE(cfg->preserve_perms); EXPECT_TRUE(cfg->use_metadata); config_delete(cfg); } +static void test_parse_args_accepts_selector_chmod() { + Config* cfg = config_create(); + char* argv[] = {"fastsync", "--chmod=Dg+s,Fo-w,+X", "/src", "/dst"}; + int positional_args[2]; + int positional_count = 0; + EXPECT_EQ_INT(parse_args(cfg, 4, argv, positional_args, &positional_count), 0); + EXPECT_EQ_STR(cfg->chmod_spec, "Dg+s,Fo-w,+X"); + EXPECT_FALSE(cfg->preserve_perms); + config_delete(cfg); +} + +static void test_parse_args_appends_repeated_chmod() { + Config* cfg = config_create(); + char* argv[] = {"fastsync", "--chmod=a+r", "--chmod=a-w", "/src", "/dst"}; + int positional_args[2]; + int positional_count = 0; + EXPECT_EQ_INT(parse_args(cfg, 5, argv, positional_args, &positional_count), 0); + /* Repeated --chmod options accumulate (rsync >= 3.2.4) instead of replacing. */ + EXPECT_EQ_STR(cfg->chmod_spec, "a+r,a-w"); + mode_t result; + EXPECT_TRUE(chmod_apply(0644, cfg->chmod_spec, &result)); + EXPECT_EQ_INT(result, 0444); + config_delete(cfg); +} + static void test_parse_args_rejects_invalid_chmod() { Config* cfg = config_create(); - char* argv[] = {"fastsync", "--chmod=a+X", "/src", "/dst"}; + char* argv[] = {"fastsync", "--chmod=a+r,", "/src", "/dst"}; int positional_args[2]; int positional_count = 0; EXPECT_EQ_INT(parse_args(cfg, 4, argv, positional_args, &positional_count), -1); @@ -4213,6 +4240,8 @@ void test_client_cli() { test_parse_args_executability(); test_parse_args_chmod(); test_parse_args_numeric_chmod(); + test_parse_args_accepts_selector_chmod(); + test_parse_args_appends_repeated_chmod(); test_parse_args_rejects_invalid_chmod(); test_parse_args_invalid_port(); test_parse_args_non_numeric_port(); diff --git a/tests/test_file.c b/tests/test_file.c index a9a3c27..f5d7255 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -1007,7 +1007,8 @@ static void test_inplace_overwrite_metadata_strips_special_bits() { struct stat st; EXPECT_EQ_INT(stat(path, &st), 0); - /* Metadata-derived mode is applied and never includes setuid/setgid/sticky. */ + /* No -p: the pre-existing destination mode (without its special bits) is + * restored; the source mode is not applied. */ EXPECT_EQ_INT((int)(st.st_mode & (S_ISUID | S_ISGID | S_ISVTX)), 0); EXPECT_EQ_INT((int)(st.st_mode & 0777), 0755); @@ -1089,12 +1090,11 @@ static void test_atomic_no_perms_preserves_destination_mode() { unlink(fresh); } -/* MAJOR 2: a brand-new destination file must never be created group/other - * writable from a client-supplied source mode. The daemon runs with umask(0), - * so without the explicit S_IWGRP|S_IWOTH strip a source 0666 (with no -p) - * would materialize as world-writable. */ -static void test_new_file_mode_never_group_other_writable() { - const char* path = "test_new_file_no_go_write.bin"; +/* Strict rsync parity: a brand-new destination file with no -p follows + * rsync's source_mode & ~umask base, so group/other write in the source mode is + * honored exactly as the umask allows (it is no longer force-cleared). */ +static void test_new_file_mode_honors_source_and_umask() { + const char* path = "test_new_file_mode.bin"; unlink(path); FileMetadata m; memset(&m, 0, sizeof(m)); @@ -1108,19 +1108,15 @@ static void test_new_file_mode_never_group_other_writable() { EXPECT_TRUE(ok); struct stat st; EXPECT_EQ_INT(stat(path, &st), 0); - EXPECT_EQ_INT((int)(st.st_mode & (S_IWGRP | S_IWOTH)), 0); - /* The rest of the source mode is still honored (owner write survives). */ - EXPECT_EQ_INT((int)(st.st_mode & S_IWUSR), S_IWUSR); + EXPECT_EQ_INT((int)(st.st_mode & 0777), (int)(0666 & ~(mode_t)file_process_umask())); unlink(path); } -/* Security: a client-supplied special-node mode must never materialize a - * group/other-writable FIFO. file_save_special_to_disk() sanitizes the - * creation bits the same way the regular-file policy does: under -p the source - * mode loses S_IWGRP|S_IWOTH (0777 -> 0755), and without -p a safe 0644 default - * is used. The daemon runs with umask(0) (server.c), so the explicit strip is - * what keeps the node safe -- the test clears the umask to prove it. */ -static void test_special_fifo_mode_never_group_other_writable_impl() { +/* Strict rsync parity for recreated special nodes: with -p the source mode is + * copied exactly (0777 -> 0777), and without -p the same source & ~umask base + * as any other new entry applies. The process umask is cleared so the source + * bits are what reaches mkfifo. */ +static void test_special_fifo_mode_honors_source_and_umask_impl() { const char* root = "test_special_mode_tmp"; const char* with_p = "test_special_mode_tmp/with_p.fifo"; const char* no_p = "test_special_mode_tmp/no_p.fifo"; @@ -1139,7 +1135,7 @@ static void test_special_fifo_mode_never_group_other_writable_impl() { meta.gid = getegid(); meta.mtime_sec = 1000000000; - /* -p: the source mode is honored minus group/other write. */ + /* -p: the source mode (including group/other write) is copied exactly. */ File* f = file_create("with_p.fifo"); EXPECT_NOT_NULL(f); f->is_special = true; @@ -1151,12 +1147,11 @@ static void test_special_fifo_mode_never_group_other_writable_impl() { struct stat st; EXPECT_EQ_INT(lstat(with_p, &st), 0); EXPECT_TRUE(S_ISFIFO(st.st_mode)); - EXPECT_EQ_INT((int)(st.st_mode & (S_IWGRP | S_IWOTH)), 0); - EXPECT_EQ_INT((int)(st.st_mode & 0777), 0755); + EXPECT_EQ_INT((int)(st.st_mode & 0777), 0777); f->metadata = NULL; file_destroy(f); - /* No -p: the fixed safe default, never the source's 0777. */ + /* No -p: source & ~umask (umask is cleared, so 0777). */ f = file_create("no_p.fifo"); EXPECT_NOT_NULL(f); f->is_special = true; @@ -1165,8 +1160,7 @@ static void test_special_fifo_mode_never_group_other_writable_impl() { EXPECT_EQ_INT(file_save_to_disk_full(root, f, cfg), FILE_SAVE_WRITTEN); EXPECT_EQ_INT(lstat(no_p, &st), 0); EXPECT_TRUE(S_ISFIFO(st.st_mode)); - EXPECT_EQ_INT((int)(st.st_mode & (S_IWGRP | S_IWOTH)), 0); - EXPECT_EQ_INT((int)(st.st_mode & 0777), 0644); + EXPECT_EQ_INT((int)(st.st_mode & 0777), 0777); f->metadata = NULL; file_destroy(f); @@ -1176,15 +1170,15 @@ static void test_special_fifo_mode_never_group_other_writable_impl() { rmdir(root); } -/* The receiver daemon runs umask(0), so an unsanitized source mode would reach - * mkfifo unmasked. Run the body with umask(0) to exercise the explicit strip, - * and restore the process umask from this wrapper so a failing EXPECT inside the - * body (which returns from the body only) cannot leak umask(0) into later - * tests. */ -static void test_special_fifo_mode_never_group_other_writable() { +/* The receiver daemon runs umask(0), so the source mode reaches mkfifo + * unmasked. Run the body with umask(0) and refresh the cached process umask so + * file_process_umask() agrees, then restore both. */ +static void test_special_fifo_mode_honors_source_and_umask() { mode_t saved_umask = umask(0); - test_special_fifo_mode_never_group_other_writable_impl(); + file_umask_capture(); + test_special_fifo_mode_honors_source_and_umask_impl(); umask(saved_umask); + file_umask_capture(); } /* --specials recreates a unix-domain socket via mknod(S_IFSOCK), which Linux @@ -2056,8 +2050,8 @@ void test_file() { test_inplace_overwrite_clears_special_mode_bits(); test_inplace_overwrite_metadata_strips_special_bits(); test_atomic_no_perms_preserves_destination_mode(); - test_new_file_mode_never_group_other_writable(); - test_special_fifo_mode_never_group_other_writable(); + test_new_file_mode_honors_source_and_umask(); + test_special_fifo_mode_honors_source_and_umask(); test_special_socket_recreated(); test_inplace_overwrite_truncates_shorter_payload(); test_inplace_refuses_fifo_destination(); diff --git a/tests/test_metadata.c b/tests/test_metadata.c index 0657900..e67604f 100644 --- a/tests/test_metadata.c +++ b/tests/test_metadata.c @@ -448,9 +448,9 @@ static void test_file_restore_executability_rsync_rule() { /* The shared metadata_mode_for_policy() helper is the single source of truth * used by both the normal metadata path and the --fake-super replay. It must * reproduce the per-attribute split: no mode change when neither -p nor -E is - * set; -p applies the sanitized source mode (group/other write cleared) - * regardless of the destination; -E derives exec bits from the destination and - * --perms wins when both are set. */ + * set; -p applies the source mode exactly (including group/other write and the + * setuid/setgid/sticky bits) regardless of the destination; -E derives exec + * bits from the destination and --perms wins when both are set. */ static void test_metadata_mode_for_policy() { mode_t out = 0xdead; EXPECT_FALSE( @@ -459,7 +459,13 @@ static void test_metadata_mode_for_policy() { EXPECT_TRUE( metadata_mode_for_policy(0777, 0644, (FileAttrPolicy){true, false, false, false}, &out)); - EXPECT_EQ_INT((int)(out & 0777), 0755); /* group/other write always cleared */ + EXPECT_EQ_INT((int)(out & 0777), 0777); /* group/other write is preserved */ + + mode_t specials = (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0672); + EXPECT_TRUE( + metadata_mode_for_policy(specials, 0644, (FileAttrPolicy){true, false, false, false}, &out)); + EXPECT_EQ_INT((int)(out & (S_ISUID | S_ISGID | S_ISVTX | 0777)), + (int)(S_ISUID | S_ISGID | S_ISVTX | 0672)); /* -E: exec bits derive from the DESTINATION's read bits. */ EXPECT_TRUE( @@ -566,6 +572,27 @@ static void test_file_attr_policy_from_config() { config_delete(c); } +/* Strict rsync parity: -p copies the source's setuid/setgid/sticky bits (they + * are attempted, not masked away). On Linux these are settable on a file the + * receiving user owns; a mount that denies them would log a chmod failure. */ +static void test_perms_preserves_special_bits() { + const char* path = "temp_special_bits.txt"; + unlink(path); + FileMetadata m = { + .mode = (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0755), .uid = getuid(), .gid = getgid()}; + + bool ok = file_to_disk_secure_attrs(path, "x", 1, false, false, false, &m, + (FileAttrPolicy){true, false, false, false}, false, false, + false, NULL, false, false, NULL); + EXPECT_TRUE(ok); + struct stat st; + EXPECT_EQ_INT(stat(path, &st), 0); + EXPECT_EQ_INT((int)(st.st_mode & 0777), 0755); + EXPECT_EQ_INT((int)(st.st_mode & (S_ISUID | S_ISGID | S_ISVTX)), + (int)(S_ISUID | S_ISGID | S_ISVTX)); + unlink(path); +} + static void test_chmod_changes() { mode_t result; EXPECT_TRUE(chmod_apply(0777, "u=rw,go=r", &result)); @@ -583,8 +610,45 @@ static void test_chmod_changes() { EXPECT_EQ_INT(result, 0755); EXPECT_FALSE(chmod_apply(0777, "888", &result)); EXPECT_FALSE(chmod_apply(0777, "10000", &result)); - EXPECT_FALSE(chmod_apply(0777, "a+X", &result)); EXPECT_FALSE(chmod_apply(0777, "a+r,", &result)); + + /* go+w is honored (rsync gives 0666 from a 0644 file). */ + EXPECT_TRUE(chmod_apply(0644, "go+w", &result)); + EXPECT_EQ_INT(result, 0666); + + /* X only sets execute on directories or already-executable files. */ + EXPECT_TRUE(chmod_apply(0644, "a+X", &result)); + EXPECT_EQ_INT(result, 0644); + EXPECT_TRUE(chmod_apply(0755, "a+X", &result)); + EXPECT_EQ_INT(result, 0755); + EXPECT_TRUE(chmod_apply((mode_t)(S_IFDIR | 0644), "a+X", &result)); + EXPECT_EQ_INT((int)(result & 0777), 0755); + EXPECT_TRUE(S_ISDIR(result)); + + /* D/F selectors restrict a clause to directories/files. */ + EXPECT_TRUE(chmod_apply((mode_t)(S_IFDIR | 0700), "Dg+s", &result)); + EXPECT_EQ_INT((int)(result & 07777), 02700); + EXPECT_TRUE(chmod_apply((mode_t)(S_IFREG | 0644), "Dg+s", &result)); + EXPECT_EQ_INT((int)(result & 07777), 0644); + EXPECT_TRUE(chmod_apply((mode_t)(S_IFREG | 0644), "Fo-w", &result)); + EXPECT_EQ_INT((int)(result & 07777), 0644); + EXPECT_TRUE(chmod_apply((mode_t)(S_IFREG | 0666), "Fo-w", &result)); + EXPECT_EQ_INT((int)(result & 07777), 0664); + EXPECT_TRUE(chmod_apply((mode_t)(S_IFDIR | 0666), "Fo-w", &result)); + EXPECT_EQ_INT((int)(result & 07777), 0666); + EXPECT_FALSE(chmod_apply(0644, "DFu+w", &result)); + + /* Special bits: s/t map to setuid/setgid/sticky like rsync. */ + EXPECT_TRUE(chmod_apply(0755, "u+s", &result)); + EXPECT_EQ_INT((int)(result & 07777), 04755); + EXPECT_TRUE(chmod_apply(0755, "g+s", &result)); + EXPECT_EQ_INT((int)(result & 07777), 02755); + EXPECT_TRUE(chmod_apply(0755, "a+t", &result)); + EXPECT_EQ_INT((int)(result & 07777), 01755); + + /* Comma-separated clauses accumulate (the CLI joins repeated options). */ + EXPECT_TRUE(chmod_apply(0644, "g+w,u+x", &result)); + EXPECT_EQ_INT((int)(result & 07777), 0764); } /* P7 Wave D: symlink metadata is applied with no-follow primitives, and -J @@ -722,5 +786,6 @@ void test_metadata() { test_file_restore_metadata_fd_attribute_split(); test_file_attr_policy_from_config(); test_file_restore_symlink_metadata(); + test_perms_preserves_special_bits(); test_chmod_changes(); } diff --git a/tests/test_xattr.c b/tests/test_xattr.c index 29666d6..ab3981d 100644 --- a/tests/test_xattr.c +++ b/tests/test_xattr.c @@ -377,13 +377,12 @@ static void test_fake_super_restore() { EXPECT_EQ_INT(fstat(fd, &st), 0); EXPECT_EQ_INT((int)(st.st_mode & 07777), 0751); - /* Mode sanitization: the normal metadata path never grants group/other write - bits, and fake-super replay must not re-add them (a recorded 0666 restores - as 0644, never as world-writable). */ + /* Strict rsync parity: -p restores the recorded mode exactly, including + group/other write (a recorded 0666 restores as 0666). */ fake_super_store_fd(fd, 1001, 1002, 0666, 1700000000, 0); EXPECT_TRUE(fake_super_restore_fd(fd, policy)); EXPECT_EQ_INT(fstat(fd, &st), 0); - EXPECT_EQ_INT((int)(st.st_mode & 0777), 0644); + EXPECT_EQ_INT((int)(st.st_mode & 0777), 0666); /* Restore with a malformed record must skip without failing. */ time_t before = st.st_mtime;