diff --git a/src/shared/chunk.c b/src/shared/chunk.c index 1f3be3a..ab0fb4c 100644 --- a/src/shared/chunk.c +++ b/src/shared/chunk.c @@ -1,6 +1,7 @@ #include #include #include +#include #include #include #include @@ -20,6 +21,33 @@ #define MAX_FILE_DATA_SIZE (64ULL * 1024 * 1024) #define MAX_FILES_PER_CHUNK 65536U +/* Reserve `charge` against `session`'s connection budget. This mirrors the + static protocol_reserve_memory() in protocol.c: the receive-side call sites + only have the Data.owner pointer (a ProtocolSession*), and protocol.c is out + of scope for this fix, so the same atomic CAS accounting is reproduced here. + The matching release always goes through data_destroy()'s Data.owner path. */ +static bool chunk_session_reserve(ProtocolSession* session, size_t charge) { + unsigned long long allocated = atomic_load(&session->total_allocated_bytes); + while (true) { + if (allocated > MAX_CONNECTION_MEMORY || + (unsigned long long)charge > MAX_CONNECTION_MEMORY - allocated) + return false; + if (atomic_compare_exchange_weak(&session->total_allocated_bytes, &allocated, + allocated + (unsigned long long)charge)) + return true; + } +} + +bool data_charge_session(Data* data, ProtocolSession* session, size_t charge) { + if (!data || charge == 0 || session == NULL) + return true; + if (!chunk_session_reserve(session, charge)) + return false; + data->owner = session; + data->protocol_charge = charge; + return true; +} + Chunk* chunk_create(File** items, int element_count) { if (element_count < 0 || (element_count > 0 && items == NULL)) return NULL; @@ -370,6 +398,15 @@ Chunk* chunk_deserialize(Data* data, bool use_metadata) { Data* replacement = data_create(file_data, file_data_size); if (replacement == NULL) goto error; + /* Charge the retained per-file copy to the connection budget (when the + inbound chunk carries an owning session) so the queued copies are not + held outside MAX_CONNECTION_MEMORY (B6). A NULL owner (e.g. a local + batch apply) leaves the copy uncharged. */ + if (!data_charge_session(replacement, data->owner, allocation_size)) { + log_message(LOG_LEVEL_ERROR, "Per-connection memory limit exceeded for chunk file data"); + data_destroy(replacement); + goto error; + } data_destroy(file->data); file->data = replacement; data_pointer += file_data_size; @@ -466,12 +503,21 @@ Chunk* receive_chunk_data(int fd, const Config* config) { } Data* data_to_process = chunk_data; if (config->use_compression) { + /* Preserve the inbound session across decompression so the (larger) + decompressed chunk is charged to the same connection budget; the + compressed buffer's own charge is released by data_destroy below. */ + ProtocolSession* owner = chunk_data->owner; data_to_process = data_decompress_limited(chunk_data, MAX_CHUNK_SIZE); data_destroy(chunk_data); if (data_to_process == NULL) { log_message(LOG_LEVEL_ERROR, "Failed to decompress chunk"); return NULL; } + if (!data_charge_session(data_to_process, owner, data_to_process->size)) { + log_message(LOG_LEVEL_ERROR, "Per-connection memory limit exceeded for decompressed chunk"); + data_destroy(data_to_process); + return NULL; + } } // Reject chunks larger than the maximum allowed size to prevent OOM. diff --git a/src/shared/chunk.h b/src/shared/chunk.h index 2c04e85..65202ad 100644 --- a/src/shared/chunk.h +++ b/src/shared/chunk.h @@ -23,4 +23,15 @@ Data* chunk_compress_with_threads(Chunk* chunk, int compression_level, bool use_ int compression_threads); Chunk* receive_chunk_data(int fd, const Config* config); +/* Charge `charge` retained bytes of `data` against `session`'s per-connection + * budget (MAX_CONNECTION_MEMORY), mirroring the protocol layer's accounting, and + * record them on `data` so data_destroy() returns the charge through the + * Data.owner path. Returns false (leaving `data` uncharged) when the ceiling + * would be exceeded. A NULL/zero-size charge or a NULL session is a no-op + * success. The receive-side decompression and chunk-copy paths know the owning + * session only through the Data.owner of the buffer they are processing, so + * this is the entry point that lets them participate in the connection budget + * without a session handle (B6). */ +bool data_charge_session(Data* data, ProtocolSession* session, size_t charge); + #endif diff --git a/src/shared/file.c b/src/shared/file.c index 9d5aa23..47b13ab 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -890,13 +890,35 @@ static bool file_to_disk_secure_impl(const char* path, const void* data, if (inplace) { /* --inplace writes directly into the destination; a scratch --temp-dir does not apply and must never redirect these writes. */ - fd = openat(dirfd, leaf, O_WRONLY | O_CREAT | O_CLOEXEC | O_NOFOLLOW, 0644); + /* Type gate BEFORE opening: an existing destination entry that is not a + regular file (FIFO, socket, char/block device, directory) must never be + opened for writing. Opening a FIFO would block the receive thread + forever and writing into a device would bypass the --write-devices / + super-mode gate (a client-controlled device write). fstatat with + AT_SYMLINK_NOFOLLOW does not follow a symlink and does not block. */ + struct stat pre_stat; + if (fstatat(dirfd, leaf, &pre_stat, AT_SYMLINK_NOFOLLOW) == 0 && !S_ISREG(pre_stat.st_mode)) { + close(dirfd); + free(leaf); + return false; + } + /* O_NONBLOCK: a no-op for a regular file, but a raced-in FIFO cannot block + the open before the post-open S_ISREG re-check rejects it. */ + fd = openat(dirfd, leaf, O_WRONLY | O_CREAT | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK, 0644); if (fd >= 0) { struct stat destination_stat; + /* Re-check the opened descriptor: a concurrent replacement between the + fstatat probe and the open (or a device/FIFO raced in) must never be + written through. */ + if (fstat(fd, &destination_stat) != 0 || !S_ISREG(destination_stat.st_mode)) { + close(fd); + close(dirfd); + free(leaf); + return false; + } bool newer = false; - if (update && metadata && fstat(fd, &destination_stat) == 0 && - S_ISREG(destination_stat.st_mode)) { - newer = stat_is_newer(&destination_stat, metadata); + if (update && metadata && stat_is_newer(&destination_stat, metadata)) { + newer = true; } if (newer) { ok = true; diff --git a/src/shared/file_receive.c b/src/shared/file_receive.c index fc2bd44..8e3c2c7 100644 --- a/src/shared/file_receive.c +++ b/src/shared/file_receive.c @@ -12,6 +12,7 @@ #include "array_list.h" #include "charset.h" #include "chmod.h" +#include "chunk.h" #include "compression.h" #include "config.h" #include "data.h" @@ -27,6 +28,11 @@ #define MAX_SERVER_DELETE_COUNT 100000U #define MAX_FILE_DATA_SIZE MAX_RECEIVE_WHOLE_FILE_SIZE +/* Retained cost of one delete-manifest entry beyond its path bytes: the + ArrayList pointer slot plus an approximate malloc header/rounding for the + heap copy. Charged against MAX_MANIFEST_BYTES so a frame full of tiny paths + cannot retain far more than the byte budget (B5). */ +#define MANIFEST_ENTRY_OVERHEAD (sizeof(char*) + 16) bool file_save_to_disk(const char* root_directory, const File* file, const Config* config) { return file_save_to_disk_full(root_directory, file, config) != FILE_SAVE_ERROR; @@ -127,7 +133,10 @@ static bool hardlink_read_source(const char* path, void** out_buf, unsigned long *source_absent = errno == ENOENT || errno == ENOTDIR; return false; } - int fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); + /* O_NONBLOCK is a no-op for a regular file but makes openat() fail/succeed + immediately for a client-planted FIFO instead of blocking the receive + thread forever; the post-open S_ISREG gate below is the actual type check. */ + int fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK); int saved_errno = errno; free(leaf); close(parent_fd); @@ -944,7 +953,7 @@ static bool receive_file_xattrs(File* file, int fd, const Config* config) { if (!config->use_xattrs) return true; int xok = 0; - FileXattrList* list = xattr_receive(fd, &xok); + FileXattrList* list = xattr_receive(fd, &xok, config->preserve_acls); if (!xok) { xattr_list_free(list); return false; @@ -1009,6 +1018,7 @@ static File* receive_delta_file(int fd, const Config* config, const char* check_ !compression_should_skip_with_suffixes( check_path, config->skip_compress_suffixes, config->skip_compress_set ? config->skip_compress_count : -1)) { + ProtocolSession* owner = delta_data->owner; raw_delta = data_decompress_limited(delta_data, MAX_RECEIVE_WHOLE_FILE_SIZE); data_destroy(delta_data); if (!raw_delta) { @@ -1017,6 +1027,15 @@ static File* receive_delta_file(int fd, const Config* config, const char* check_ *failed = true; return NULL; } + /* Charge the decompressed delta to the connection budget (the paired + wire buffer's charge was just released). */ + if (!data_charge_session(raw_delta, owner, raw_delta->size)) { + data_destroy(raw_delta); + free(old_data); + delta_signature_destroy(sig); + *failed = true; + return NULL; + } } Delta* delta = delta_deserialize(raw_delta); @@ -1131,12 +1150,20 @@ static File* receive_delta_file(int fd, const Config* config, const char* check_ file->path, config->skip_compress_suffixes, config->skip_compress_set ? config->skip_compress_count : -1)) { Data* uncompressed = data_decompress_limited(file_data, MAX_RECEIVE_WHOLE_FILE_SIZE); + ProtocolSession* owner = file_data->owner; data_destroy(file_data); if (uncompressed == NULL) { file_destroy(file); *failed = true; return NULL; } + if (!data_charge_session(uncompressed, owner, uncompressed->size)) { + data_destroy(uncompressed); + file_destroy(file); + send_status(fd, STATUS_ERROR); + *failed = true; + return NULL; + } if (uncompressed->size > MAX_FILE_DATA_SIZE) { data_destroy(uncompressed); file_destroy(file); @@ -1193,7 +1220,9 @@ static bool basis_open_regular(const char* path, unsigned long long expected_siz int parent_fd = file_open_secure_parent(path, &leaf, false); if (parent_fd < 0) return false; - int fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); + /* O_NONBLOCK: a client-planted FIFO must not block the receiver's openat() + forever; the fstat()/S_ISREG gate below rejects it immediately. */ + int fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK); free(leaf); close(parent_fd); if (fd < 0) @@ -1637,11 +1666,17 @@ static File* receive_full_file(int fd, const Config* config, const char* path) { config->skip_compress_set ? config->skip_compress_count : -1)) { Data* uncompressed = data_decompress_limited(file_data, MAX_RECEIVE_WHOLE_FILE_SIZE); + ProtocolSession* owner = file_data->owner; data_destroy(file_data); if (uncompressed == NULL) { file_destroy(file); return NULL; } + if (!data_charge_session(uncompressed, owner, uncompressed->size)) { + data_destroy(uncompressed); + file_destroy(file); + return NULL; + } if (uncompressed->size > MAX_FILE_DATA_SIZE) { data_destroy(uncompressed); file_destroy(file); @@ -1776,7 +1811,9 @@ static IncrementalCheckOutcome incremental_check_open_destination(IncrementalChe char* leaf = NULL; int parent_fd = file_open_secure_parent(full_path, &leaf, false); if (parent_fd >= 0) { - state->old_fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); + /* O_NONBLOCK: an existing FIFO at the destination must not block this + openat(); the S_ISREG gate below rejects the non-regular entry. */ + state->old_fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK); free(leaf); close(parent_fd); state->has_old_file = state->old_fd >= 0 && fstat(state->old_fd, &state->old_st) == 0 && @@ -1815,7 +1852,15 @@ static IncrementalCheckOutcome incremental_check_quick_skip(IncrementalCheckStat bool try_delta = config->use_delta && !config->whole_file && has_old_file && delta_should_attempt(old_size, state->check_size, config->delta_max_file_size); bool checksum_needs_read = size_equal && !config->ignore_times && config->checksum; - bool need_old_data = checksum_needs_read || try_delta; + /* --dry-run must never read the destination file's CONTENTS: a client could + otherwise use `--dry-run --checksum` against a read-only module as a + 1-bit content oracle (hash match / mismatch) and force arbitrary reads. + Decide from metadata alone; when metadata is inconclusive (checksum or + delta would have required the body) report would-transfer. The real + (non-dry-run) behavior below is unchanged. */ + bool need_old_data = !config->dry_run && (checksum_needs_read || try_delta); + if (config->dry_run) + try_delta = false; *out_try_delta = try_delta; if (need_old_data && has_old_file && old_size > 0 && old_size <= MAX_RECEIVE_WHOLE_FILE_SIZE && @@ -1836,7 +1881,12 @@ static IncrementalCheckOutcome incremental_check_quick_skip(IncrementalCheckStat } bool match = false; - if (checksum_needs_read) { + if (config->dry_run) { + /* Metadata-only decision: a size match plus a matching mtime is treated as + up to date; --checksum/--delta cannot be verified without reading, so an + otherwise inconclusive comparison is a would-transfer. */ + match = size_equal && !config->ignore_times && (config->size_only || match_by_metadata); + } else if (checksum_needs_read) { uint8_t old_digest[CHECKSUM_MAX_DIGEST_LEN]; size_t old_len = 0; bool hashed = checksum_digest((ChecksumAlgo)config->checksum_algo, config->checksum_seed, @@ -2050,7 +2100,7 @@ static IncrementalCheckOutcome incremental_check_try_append_resume(IncrementalCh } if (config->use_xattrs) { int xok = 0; - append_xattrs = xattr_receive(fd, &xok); + append_xattrs = xattr_receive(fd, &xok, config->preserve_acls); if (!xok) { xattr_list_free(append_xattrs); return INCREMENTAL_ERROR; @@ -2066,11 +2116,17 @@ static IncrementalCheckOutcome incremental_check_try_append_resume(IncrementalCh config->skip_compress_set ? config->skip_compress_count : -1)) { Data* uncompressed = data_decompress_limited(tail, MAX_RECEIVE_WHOLE_FILE_SIZE); + ProtocolSession* owner = tail->owner; data_destroy(tail); if (uncompressed == NULL) { xattr_list_free(append_xattrs); return INCREMENTAL_ERROR; } + if (!data_charge_session(uncompressed, owner, uncompressed->size)) { + data_destroy(uncompressed); + xattr_list_free(append_xattrs); + return INCREMENTAL_ERROR; + } if (uncompressed->size > MAX_FILE_DATA_SIZE) { data_destroy(uncompressed); xattr_list_free(append_xattrs); @@ -2301,11 +2357,17 @@ File* file_receive(const Config* config, int file_descriptor) { config->skip_compress_set ? config->skip_compress_count : -1)) { Data* file_data_uncompressed = data_decompress_limited(file_data, MAX_RECEIVE_WHOLE_FILE_SIZE); + ProtocolSession* owner = file_data->owner; data_destroy(file_data); if (file_data_uncompressed == NULL) { file_destroy(file); return NULL; } + if (!data_charge_session(file_data_uncompressed, owner, file_data_uncompressed->size)) { + data_destroy(file_data_uncompressed); + file_destroy(file); + return NULL; + } if (file_data_uncompressed->size > MAX_FILE_DATA_SIZE) { data_destroy(file_data_uncompressed); file_destroy(file); @@ -2694,19 +2756,21 @@ File* file_receive_special(int file_descriptor) { manifest). Returns an owned DeleteManifest, or NULL after sending STATUS_ERROR when the frame is malformed (bad count, empty/absolute path, path traversal, or an aggregate size beyond MAX_MANIFEST_BYTES). */ -static bool receive_manifest_section(int fd, ArrayList* list, size_t* manifest_bytes) { +static bool receive_manifest_section(int fd, ArrayList* list, size_t* manifest_bytes, + size_t* manifest_entries) { int count; if (!receive_int(fd, &count)) { send_status(fd, STATUS_ERROR); return false; } - if (count < 0 || count > MAX_MANIFEST_ENTRIES) { + if (count < 0 || count > MAX_MANIFEST_ENTRIES || + (size_t)count > MAX_MANIFEST_ENTRIES - *manifest_entries) { send_status(fd, STATUS_ERROR); return false; } for (int i = 0; i < count; i++) { char* s = receive_wire_str(fd); - size_t entry_size = s ? strlen(s) : 0; + size_t entry_size = s ? strlen(s) + MANIFEST_ENTRY_OVERHEAD : 0; if (!s || s[0] == '\0' || s[0] == '/' || has_path_traversal(s) || entry_size > MAX_MANIFEST_BYTES - *manifest_bytes || (*manifest_bytes += entry_size) > MAX_MANIFEST_BYTES || !array_list_add(list, s)) { @@ -2715,6 +2779,7 @@ static bool receive_manifest_section(int fd, ArrayList* list, size_t* manifest_b return false; } } + *manifest_entries += (size_t)count; return true; } @@ -2733,9 +2798,10 @@ DeleteManifest* receive_manifest_entries(int fd) { return NULL; } size_t manifest_bytes = 0; - if (!receive_manifest_section(fd, manifest->keeps, &manifest_bytes) || - !receive_manifest_section(fd, manifest->protected, &manifest_bytes) || - !receive_manifest_section(fd, manifest->missing, &manifest_bytes)) { + size_t manifest_entries = 0; + if (!receive_manifest_section(fd, manifest->keeps, &manifest_bytes, &manifest_entries) || + !receive_manifest_section(fd, manifest->protected, &manifest_bytes, &manifest_entries) || + !receive_manifest_section(fd, manifest->missing, &manifest_bytes, &manifest_entries)) { delete_manifest_free(manifest); return NULL; } diff --git a/src/shared/xattr.c b/src/shared/xattr.c index d01a38b..269a867 100644 --- a/src/shared/xattr.c +++ b/src/shared/xattr.c @@ -73,13 +73,17 @@ bool xattr_list_append(FileXattrList* list, const char* name, const void* value, /* A Linux xattr name is "namespace.name" with an optional leading "trusted.", * "system.", "security.", "user.", or "trusted." prefix. We only ever touch - * the unprivileged "user.*" namespace and the two POSIX ACL xattrs carried in - * the "system." namespace. Everything else -- especially "security.*" (ACLs, - * capabilities, SELinux labels) and "trusted.*" -- is refused so a client can - * never compel the receiver to apply a privileged attribute it would not - * otherwise be able to set (and which would be a local privilege escalation if - * it could). */ -bool xattr_name_appliable(const char* name) { + * the unprivileged "user.*" namespace and, only when --acls/-A was negotiated, + * the two POSIX ACL xattrs carried in the "system." namespace. Everything else + * -- especially "security.*" (ACLs, capabilities, SELinux labels) and + * "trusted.*" -- is refused so a client can never compel the receiver to apply a + * privileged attribute it would not otherwise be able to set (and which would be + * a local privilege escalation if it could). + * + * The ACL gate is deliberate: --xattrs/-X alone derives use_xattrs but must NOT + * authorize the ACL names, otherwise a -X client could plant an ACL the + * receiver never opted into (B4). */ +bool xattr_name_appliable(const char* name, bool preserve_acls) { if (!name || name[0] == '\0') return false; size_t len = strlen(name); @@ -95,12 +99,21 @@ bool xattr_name_appliable(const char* name) { if (strncmp(name, "user.", 5) == 0) return name[5] != '\0'; if (strcmp(name, "system.posix_acl_access") == 0) - return true; + return preserve_acls; if (strcmp(name, "system.posix_acl_default") == 0) - return true; + return preserve_acls; return false; } +/* The two POSIX ACL xattr names: the only names whose applicablity is + * conditional (they require --acls). Used by the receiver to distinguish "not + * negotiated" (drop the entry, keep user.* working for -X) from a genuinely + * disallowed namespace (hard reject). */ +static bool xattr_name_is_posix_acl(const char* name) { + return name != NULL && (strcmp(name, "system.posix_acl_access") == 0 || + strcmp(name, "system.posix_acl_default") == 0); +} + /* ---- SENDER: capture ---- */ FileXattrList* xattr_capture_path(const char* path) { @@ -130,7 +143,9 @@ FileXattrList* xattr_capture_path(const char* path) { if (name_len == 0) break; /* trailing double NUL not expected; stop */ offset += (ssize_t)name_len + 1; - if (!xattr_name_appliable(name)) + /* Capture is sender-side: the scanner has already gated on -X/-A, so the + per-name whitelist here allows the ACL names (true). */ + if (!xattr_name_appliable(name, true)) continue; ssize_t value_size = getxattr(path, name, NULL, 0); if (value_size < 0) @@ -190,7 +205,7 @@ bool xattr_send(int fd, const FileXattrList* list) { return true; } -FileXattrList* xattr_receive(int fd, int* ok) { +FileXattrList* xattr_receive(int fd, int* ok, bool preserve_acls) { if (ok) *ok = 0; int count; @@ -232,11 +247,19 @@ FileXattrList* xattr_receive(int fd, int* ok) { xattr_list_free(list); return NULL; } - if (!xattr_name_appliable(name)) { - log_message(LOG_LEVEL_ERROR, "rejected xattr block: disallowed namespace for '%s'", name); - free(name); - xattr_list_free(list); - return NULL; + bool skip = false; + if (!xattr_name_appliable(name, preserve_acls)) { + if (!preserve_acls && xattr_name_is_posix_acl(name)) { + /* -X without -A: the sender may still carry ACLs, but the receiver must + never apply an ACL it was not asked to preserve. Consume and drop the + entry (keeping -X compatibility) rather than failing the transfer. */ + skip = true; + } else { + log_message(LOG_LEVEL_ERROR, "rejected xattr block: disallowed namespace for '%s'", name); + free(name); + xattr_list_free(list); + return NULL; + } } int32_t value_len32; if (!receive_n_data(fd, &value_len32, sizeof(value_len32))) { @@ -273,6 +296,12 @@ FileXattrList* xattr_receive(int fd, int* ok) { return NULL; } } + if (skip) { + free(value); + free(name); + budget += (size_t)name_len32 + (size_t)value_len32; + continue; + } if (!xattr_list_append(list, name, value, (size_t)value_len32)) { free(value); free(name); diff --git a/src/shared/xattr.h b/src/shared/xattr.h index 55f22dd..5c16c53 100644 --- a/src/shared/xattr.h +++ b/src/shared/xattr.h @@ -59,9 +59,11 @@ void xattr_list_free(FileXattrList* list); bool xattr_list_append(FileXattrList* list, const char* name, const void* value, size_t value_len); /* True when `name` is a well-formed xattr name AND belongs to a namespace this - * build is authorized to apply (user.* or the two POSIX ACL xattrs). Used for - * both capture and receiver-side validation. */ -bool xattr_name_appliable(const char* name); + * build is authorized to apply. `user.*` is always accepted for -X; the two + * POSIX ACL xattrs are accepted only when `preserve_acls` (--acls/-A) is set, so + * a plain -X run can never carry or apply an ACL the receiver did not ask for. + * Used for both capture and receiver-side validation. */ +bool xattr_name_appliable(const char* name, bool preserve_acls); /* Sender: read the whitelisted xattrs of `path` into a new list. Returns NULL * when the path has no appliable xattrs (or the filesystem has no xattr @@ -70,9 +72,12 @@ FileXattrList* xattr_capture_path(const char* path); /* Wire: bounded serialization. xattr_send returns false on write failure; an * empty/NULL list transmits a zero-count block. xattr_receive returns NULL and - * sets *ok = 0 on any malformed / oversized / non-whitelisted entry. */ + * sets *ok = 0 on any malformed / oversized / non-whitelisted entry. When + * `preserve_acls` is false, any POSIX ACL entries are consumed and DROPPED (so + * a -X transfer still succeeds and never applies an ACL it did not negotiate); + * a genuinely disallowed namespace is still rejected. */ bool xattr_send(int fd, const FileXattrList* list); -FileXattrList* xattr_receive(int fd, int* ok); +FileXattrList* xattr_receive(int fd, int* ok, bool preserve_acls); /* Receiver: apply every entry fd-relative (fsetxattr) to the just-written file * descriptor. A per-attribute failure (e.g. ACL set refused for non-root on a diff --git a/tests/fuzz/fuzz_xattr_block.c b/tests/fuzz/fuzz_xattr_block.c index 26e3775..f1ba210 100644 --- a/tests/fuzz/fuzz_xattr_block.c +++ b/tests/fuzz/fuzz_xattr_block.c @@ -75,7 +75,7 @@ static void write_best_effort(int fd, const void* data, size_t size) { } static void receive_stream(const unsigned char* prefix, size_t prefix_len, const uint8_t* data, - size_t size) { + size_t size, bool preserve_acls) { int sv[2]; if (socketpair(AF_UNIX, SOCK_STREAM, 0, sv) != 0) return; @@ -91,7 +91,7 @@ static void receive_stream(const unsigned char* prefix, size_t prefix_len, const shutdown(sv[0], SHUT_WR); int ok = 0; - FileXattrList* list = xattr_receive(sv[1], &ok); + FileXattrList* list = xattr_receive(sv[1], &ok, preserve_acls); xattr_list_free(list); close(sv[0]); @@ -102,14 +102,18 @@ int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { if (!g_block_ready) build_canonical_block(); - /* Raw bytes as the whole block. */ - receive_stream(NULL, 0, data, size); + /* Raw bytes as the whole block. Exercise both the -X-only (no ACLs) and the + * -A (ACL names accepted) receiver gates. */ + for (int acls = 0; acls < 2; acls++) { + bool preserve_acls = acls != 0; + receive_stream(NULL, 0, data, size, preserve_acls); - /* Valid framing so the fuzzer mutates the entry list, the first value and - * the second entry respectively instead of stopping at the count. */ - receive_stream(g_block, g_off_after_entry0, data, size); - receive_stream(g_block, g_off_value0, data, size); - receive_stream(g_block, g_off_after_count, data, size); + /* Valid framing so the fuzzer mutates the entry list, the first value and + * the second entry respectively instead of stopping at the count. */ + receive_stream(g_block, g_off_after_entry0, data, size, preserve_acls); + receive_stream(g_block, g_off_value0, data, size, preserve_acls); + receive_stream(g_block, g_off_after_count, data, size, preserve_acls); + } return 0; } diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index 96372cb..1c34c00 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -444,9 +444,10 @@ class TestRemoteDryRun: self._seed(source) clean_dir(dest) - # Populate the destination with a real transfer, then make exactly one - # file differ (content+size) and add a brand-new file. - result, _ = run_client(source, dest, port=shared_server.port) + # Populate the destination with a real transfer that preserves mtimes + # (--preserve), then make exactly one file differ (content+size) and add + # a brand-new file. + result, _ = run_client(source, dest, flags=["--preserve"], port=shared_server.port) assert result.returncode == 0, f"seed transfer failed: {result.stderr[:200]}" received = get_dest_received_dir(dest, source) @@ -456,9 +457,11 @@ class TestRemoteDryRun: f.write(b"newly added\n") before = _snapshot_tree(received) - # --checksum makes the up-to-date decision content-based (the seed - # transfer did not preserve mtimes), so keep.txt/deep.txt report skip. - result, _ = run_client(source, dest, flags=["--dry-run", "--checksum"], + # --checksum must NOT read destination contents in a dry-run (B3), so + # the up-to-date decision is metadata-only. The --preserve seed made + # keep.txt and deep.txt size+mtime-identical; the dry-run must also + # transmit metadata (--preserve) for that metadata to be comparable. + result, _ = run_client(source, dest, flags=["--dry-run", "--checksum", "--preserve"], port=shared_server.port) assert result.returncode == 0, f"remote dry-run failed: {result.stderr[:300]}" assert "Dry run:" in result.stdout, result.stdout[:200] @@ -470,6 +473,34 @@ class TestRemoteDryRun: assert "deep.txt" not in result.stdout, result.stdout assert _snapshot_tree(received) == before, "remote dry-run mutated the destination" + @pytest.mark.ci + def test_remote_dry_run_checksum_does_not_read_destination(self, shared_server): + """B3: --dry-run --checksum against a read-only module must not read the + destination file's content (a 1-bit hash oracle). A same-size/same-content + file whose mtime differs is therefore reported as would-transfer because + the metadata-only decision is inconclusive, instead of being hashed and + silently skipped.""" + source = os.path.join(TEST_DATA_DIR, "remote_dry_oracle_src") + dest = os.path.join(TEST_DATA_DIR, "remote_dry_oracle_dst") + self._seed(source) + clean_dir(dest) + result, _ = run_client(source, dest, flags=["--preserve"], port=shared_server.port) + assert result.returncode == 0, result.stderr[:200] + received = get_dest_received_dir(dest, source) + + target = os.path.join(received, "keep.txt") + # Identical size and content, but a deliberately different mtime. + os.utime(target, (1000000000, 1000000000)) + before = _snapshot_tree(received) + + result, _ = run_client(source, dest, flags=["--dry-run", "--checksum", "--preserve"], + port=shared_server.port) + assert result.returncode == 0, result.stderr[:300] + assert "keep.txt" in result.stdout, ( + f"dry-run --checksum must not read the destination to prove equality: {result.stdout}" + ) + assert _snapshot_tree(received) == before, "dry-run mutated the destination" + @pytest.mark.ci def test_remote_dry_run_into_empty_dest_creates_nothing(self, shared_server): source = os.path.join(TEST_DATA_DIR, "remote_dry_empty_src") diff --git a/tests/test_chunk.c b/tests/test_chunk.c index b1d6cf6..6266582 100644 --- a/tests/test_chunk.c +++ b/tests/test_chunk.c @@ -1,8 +1,10 @@ #include "chunk.h" +#include "protocol.h" #include "test_utils.h" #include "utils.h" #include +#include #include #include @@ -279,6 +281,51 @@ static void test_chunk_special_rdev_out_of_range_rejected() { chunk_destroy(chunk); } +/* B6: chunk_deserialize() charges each retained per-file copy to the owning + * session's connection budget (MAX_CONNECTION_MEMORY) so queued chunk payloads + * are not held outside the per-connection ceiling; destroying the chunk returns + * the charge through the Data.owner path. */ +static void test_chunk_deserialize_charges_session_budget() { + const char* path = "temp_chunk_charge.txt"; + const char* content = "charge me to the connection budget"; + unlink(path); + file_write_to_disk(path, content, strlen(content), false, false); + struct stat st; + EXPECT_EQ_INT(stat(path, &st), 0); + + int p[2]; + EXPECT_EQ_INT(socketpair(AF_UNIX, SOCK_STREAM, 0, p), 0); + ProtocolSession session; + protocol_session_init(&session, p[0], p[1]); + protocol_session_set_max_alloc(&session, 4ULL * 1024 * 1024); + + File* f = file_create(path); + EXPECT_NOT_NULL(f); + f->data->size = (unsigned long long)st.st_size; + EXPECT_TRUE(file_load_data(f)); + File* files[1] = {f}; + Chunk* chunk = chunk_create(files, 1); + EXPECT_NOT_NULL(chunk); + Data* serialized = chunk_serialize(chunk, false); + EXPECT_NOT_NULL(serialized); + /* Simulate a received buffer carrying its owning session. */ + serialized->owner = &session; + + Chunk* deserialized = chunk_deserialize(serialized, false); + EXPECT_NOT_NULL(deserialized); + unsigned long long charged = atomic_load(&session.total_allocated_bytes); + EXPECT_EQ_INT((int)charged, (int)strlen(content)); + chunk_destroy(deserialized); + /* The copy's charge is released with the File/Data on destroy. */ + EXPECT_EQ_INT((int)atomic_load(&session.total_allocated_bytes), 0); + + data_destroy(serialized); + chunk_destroy(chunk); + close(p[0]); + close(p[1]); + unlink(path); +} + void test_chunk() { test_file_operations(); test_chunk_operations(); @@ -286,4 +333,5 @@ void test_chunk() { test_chunk_symlink_roundtrip(); test_chunk_special_rdev_roundtrip(); test_chunk_special_rdev_out_of_range_rejected(); + test_chunk_deserialize_charges_session_budget(); } diff --git a/tests/test_file.c b/tests/test_file.c index 73c3431..37fe83e 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -994,6 +995,94 @@ static void test_inplace_overwrite_truncates_shorter_payload() { rmdir(root); } +/* B2: --inplace must refuse an existing non-regular destination entry. A FIFO + would block open(O_WRONLY) forever and a device node would be written + directly, bypassing the --write-devices/super gate. Forked with an alarm so + a regression is a prompt failure instead of a hung suite. */ +static void test_inplace_refuses_fifo_destination() { + const char* root = "test_inplace_fifo_tmp"; + const char* path = "test_inplace_fifo_tmp/fifo"; + unlink(path); + rmdir(root); + EXPECT_EQ_INT(mkdir(root, 0700), 0); + EXPECT_EQ_INT(mkfifo(path, 0600), 0); + + pid_t pid = fork(); + if (pid == 0) { + alarm(10); + File* f = file_create("fifo"); + if (!f) + _exit(1); + const char* content = "payload"; + f->data->data = malloc(strlen(content)); + if (!f->data->data) + _exit(1); + memcpy(f->data->data, content, strlen(content)); + f->data->size = strlen(content); + Config* cfg = config_create(); + if (!cfg) + _exit(1); + cfg->inplace = true; + bool written = file_save_to_disk(root, f, cfg); + file_destroy(f); + config_delete(cfg); + _exit(written ? 1 : 0); /* must be refused */ + } + int status; + waitpid(pid, &status, 0); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); + struct stat st; + EXPECT_EQ_INT(lstat(path, &st), 0); + EXPECT_TRUE(S_ISFIFO(st.st_mode)); /* left untouched */ + unlink(path); + rmdir(root); +} + +/* B2: an existing char device must not be written by --inplace. mknod needs + privilege, so a non-root run skips gracefully. /dev/null's (1:3) rdev makes + the negative case harmless if it ever regresses. */ +static void test_inplace_refuses_device_destination() { + const char* root = "test_inplace_dev_tmp"; + const char* path = "test_inplace_dev_tmp/dev"; + unlink(path); + rmdir(root); + EXPECT_EQ_INT(mkdir(root, 0700), 0); + if (mknod(path, S_IFCHR | 0600, makedev(1, 3)) != 0) { + rmdir(root); + return; /* no privilege to create a device node: skip */ + } + + pid_t pid = fork(); + if (pid == 0) { + alarm(10); + File* f = file_create("dev"); + if (!f) + _exit(1); + const char* content = "payload"; + f->data->data = malloc(strlen(content)); + if (!f->data->data) + _exit(1); + memcpy(f->data->data, content, strlen(content)); + f->data->size = strlen(content); + Config* cfg = config_create(); + if (!cfg) + _exit(1); + cfg->inplace = true; + bool written = file_save_to_disk(root, f, cfg); + file_destroy(f); + config_delete(cfg); + _exit(written ? 1 : 0); /* must be refused */ + } + int status; + waitpid(pid, &status, 0); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); + struct stat st; + EXPECT_EQ_INT(lstat(path, &st), 0); + EXPECT_TRUE(S_ISCHR(st.st_mode)); /* still a device, not replaced */ + unlink(path); + rmdir(root); +} + /* Explicit directory entries (--dirs) create the directory under the receive root through the same save funnel, creating parents as needed, and reject traversal the same way a file path does. */ @@ -1606,4 +1695,6 @@ void test_file() { test_inplace_overwrite_clears_special_mode_bits(); test_inplace_overwrite_metadata_strips_special_bits(); test_inplace_overwrite_truncates_shorter_payload(); + test_inplace_refuses_fifo_destination(); + test_inplace_refuses_device_destination(); } diff --git a/tests/test_server.c b/tests/test_server.c index b2246e8..2dff95e 100644 --- a/tests/test_server.c +++ b/tests/test_server.c @@ -844,6 +844,172 @@ static void test_special_socket_path_log_escaped() { EXPECT_NOT_NULL(strstr(output, "socket not recreated: evil\\#012path")); } +/* B1: a client-planted FIFO at the destination must not block the receiver's + * incremental-check open. With the O_NONBLOCK open plus the post-open S_ISREG + * gate the FIFO is simply "no existing regular file", so the receiver proceeds + * to a full transfer; without O_NONBLOCK the child blocks in openat() and the + * alarm(30) kills it. */ +static void test_incremental_check_fifo_destination_does_not_hang() { + Config* cfg = config_create(); + EXPECT_NOT_NULL(cfg); + char* root = make_check_root("qffo"); + EXPECT_NOT_NULL(root); + cfg->receive_root_directory = str_dup(root); + char path[1024]; + snprintf(path, sizeof(path), "%s/file.txt", root); + EXPECT_EQ_INT(mkfifo(path, 0600), 0); + + int p[2]; + EXPECT_EQ_INT(socketpair(AF_UNIX, SOCK_STREAM, 0, p), 0); + io_set_fds(p[0], p[1]); + io_set_bwlimit(0); + + pid_t pid = fork(); + if (pid == 0) { + alarm(30); + close(p[1]); + io_set_fds(p[0], p[0]); + bool skipped = false; + File* file = receive_incremental_check(p[0], cfg, &skipped); + bool ok = file != NULL && !skipped; + file_destroy(file); + config_delete(cfg); + close(p[0]); + _exit(ok ? 0 : 1); + } else { + close(p[0]); + io_set_fds(p[1], p[1]); + EXPECT_TRUE(send_str(p[1], "file.txt")); + unsigned long long size = 4; + long long mtime = 42; + long long mtime_nsec = 0; + EXPECT_TRUE(send_n_data(p[1], &size, sizeof(size))); + EXPECT_TRUE(send_n_data(p[1], &mtime, sizeof(mtime))); + EXPECT_TRUE(send_n_data(p[1], &mtime_nsec, sizeof(mtime_nsec))); + Status s; + EXPECT_TRUE(receive_status(p[1], &s)); + EXPECT_EQ_INT(s, STATUS_NEXT); + + Data* body = data_create_reserve(4); + EXPECT_NOT_NULL(body); + body->data = malloc(4); + EXPECT_NOT_NULL(body->data); + memcpy(body->data, "data", 4); + body->size = 4; + EXPECT_TRUE(send_data(p[1], body)); + data_destroy(body); + + int status; + waitpid(pid, &status, 0); + close(p[1]); + config_delete(cfg); + unlink(path); + rmdir(root); + free(root); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); + } +} + +/* B1: a FIFO planted in a --link-dest basis directory must not block + * basis_open_regular() either; the basis match is simply declined. */ +static void test_incremental_check_basis_fifo_does_not_hang() { + Config* cfg = config_create(); + EXPECT_NOT_NULL(cfg); + char* root = make_check_root("qbfi"); + EXPECT_NOT_NULL(root); + cfg->receive_root_directory = str_dup(root); + char basis_dir[1024]; + char basis_path[2048]; + snprintf(basis_dir, sizeof(basis_dir), "%s/basis", root); + EXPECT_EQ_INT(mkdir(basis_dir, 0700), 0); + snprintf(basis_path, sizeof(basis_path), "%s/file.txt", basis_dir); + EXPECT_EQ_INT(mkfifo(basis_path, 0600), 0); + EXPECT_EQ_INT(config_basis_append(cfg, BASIS_DEST_LINK, "basis"), 0); + + int p[2]; + EXPECT_EQ_INT(socketpair(AF_UNIX, SOCK_STREAM, 0, p), 0); + io_set_fds(p[0], p[1]); + io_set_bwlimit(0); + + pid_t pid = fork(); + if (pid == 0) { + alarm(30); + close(p[1]); + io_set_fds(p[0], p[0]); + bool skipped = false; + File* file = receive_incremental_check(p[0], cfg, &skipped); + bool ok = file != NULL && !skipped; + file_destroy(file); + config_delete(cfg); + close(p[0]); + _exit(ok ? 0 : 1); + } else { + close(p[0]); + io_set_fds(p[1], p[1]); + EXPECT_TRUE(send_str(p[1], "file.txt")); + unsigned long long size = 4; + long long mtime = 42; + long long mtime_nsec = 0; + EXPECT_TRUE(send_n_data(p[1], &size, sizeof(size))); + EXPECT_TRUE(send_n_data(p[1], &mtime, sizeof(mtime))); + EXPECT_TRUE(send_n_data(p[1], &mtime_nsec, sizeof(mtime_nsec))); + /* config_has_basis() makes the request carry the source digest. */ + uint8_t wire_len = 8; + uint8_t digest[8] = {0}; + EXPECT_TRUE(send_n_data(p[1], &wire_len, sizeof(wire_len))); + EXPECT_TRUE(send_n_data(p[1], digest, sizeof(digest))); + Status s; + EXPECT_TRUE(receive_status(p[1], &s)); + EXPECT_EQ_INT(s, STATUS_NEXT); + + Data* body = data_create_reserve(4); + EXPECT_NOT_NULL(body); + body->data = malloc(4); + EXPECT_NOT_NULL(body->data); + memcpy(body->data, "data", 4); + body->size = 4; + EXPECT_TRUE(send_data(p[1], body)); + data_destroy(body); + + int status; + waitpid(pid, &status, 0); + close(p[1]); + config_delete(cfg); + unlink(basis_path); + rmdir(basis_dir); + rmdir(root); + free(root); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); + } +} + +/* B5: the aggregate entry count across the three manifest sections is capped at + * MAX_MANIFEST_ENTRIES, and a section that would push the total over the cap is + * rejected before its entries are read (so a tiny first section followed by a + * huge claimed second section fails fast). */ +static void test_receive_manifest_total_entry_cap() { + Config* cfg = config_create(); + EXPECT_NOT_NULL(cfg); + cfg->receive_root_directory = str_dup("/tmp/dst"); + int p[2]; + EXPECT_EQ_INT(socketpair(AF_UNIX, SOCK_STREAM, 0, p), 0); + io_set_fds(p[0], p[1]); + + EXPECT_TRUE(send_int(p[1], 1)); + EXPECT_TRUE(send_str(p[1], "keep.txt")); + /* The second section alone is within its per-section cap, but 1 + it exceeds + the cross-section cap; the receiver must reject at the count. */ + EXPECT_TRUE(send_int(p[1], MAX_MANIFEST_ENTRIES)); + EXPECT_NULL(receive_manifest_entries(p[0])); + Status status; + EXPECT_TRUE(receive_status(p[1], &status)); + EXPECT_EQ_INT(status, STATUS_ERROR); + + close(p[0]); + close(p[1]); + config_delete(cfg); +} + void test_server() { test_special_socket_path_log_escaped(); if (!is_running_under_valgrind()) { @@ -856,6 +1022,9 @@ void test_server() { test_incremental_check_size_mismatch_full_transfer(); test_incremental_check_dry_run_reports_transfer_without_writing(); test_incremental_check_delta_oversize_reports_failure(); + test_incremental_check_fifo_destination_does_not_hang(); + test_incremental_check_basis_fifo_does_not_hang(); + test_receive_manifest_total_entry_cap(); test_late_manifest_abort_frees_keepset(); test_late_manifest_eof_frees_keepset(); test_late_second_manifest_frees_both(); diff --git a/tests/test_xattr.c b/tests/test_xattr.c index 4997ae2..f82cb3f 100644 --- a/tests/test_xattr.c +++ b/tests/test_xattr.c @@ -17,7 +17,7 @@ static void run_recv_helper(int fd) { int ok = 0; - FileXattrList* list = xattr_receive(fd, &ok); + FileXattrList* list = xattr_receive(fd, &ok, false); if (!ok) _exit(1); if (!list) { @@ -64,7 +64,7 @@ static void test_xattr_wire_roundtrip() { static void run_recv_must_fail(int fd) { int ok = 0; - FileXattrList* list = xattr_receive(fd, &ok); + FileXattrList* list = xattr_receive(fd, &ok, false); /* A NULL list with ok==0 is the expected rejection. */ if (ok == 0 && list == NULL) _exit(0); @@ -148,15 +148,64 @@ static void test_xattr_count_bound() { /* The captured list on a plain file reflects only whitelisted namespaces * (Linux only; skipped when the filesystem has no xattr support). */ static void test_xattr_capture_and_appliable() { - EXPECT_FALSE(xattr_name_appliable(NULL)); - EXPECT_FALSE(xattr_name_appliable("")); - EXPECT_FALSE(xattr_name_appliable("security.selinux")); - EXPECT_FALSE(xattr_name_appliable("trusted.blob")); - EXPECT_TRUE(xattr_name_appliable("user.foo")); + EXPECT_FALSE(xattr_name_appliable(NULL, false)); + EXPECT_FALSE(xattr_name_appliable("", false)); + EXPECT_FALSE(xattr_name_appliable("security.selinux", false)); + EXPECT_FALSE(xattr_name_appliable("trusted.blob", false)); + EXPECT_TRUE(xattr_name_appliable("user.foo", false)); + EXPECT_TRUE(xattr_name_appliable("user.foo", true)); /* The reserved fake-super key is receiver-only and never forwarded/applied. */ - EXPECT_FALSE(xattr_name_appliable("user.fastsync.stat")); - EXPECT_TRUE(xattr_name_appliable("system.posix_acl_access")); - EXPECT_TRUE(xattr_name_appliable("system.posix_acl_default")); + EXPECT_FALSE(xattr_name_appliable("user.fastsync.stat", false)); + EXPECT_FALSE(xattr_name_appliable("user.fastsync.stat", true)); + /* B4: the ACL names require --acls; -X alone must not authorize them. */ + EXPECT_FALSE(xattr_name_appliable("system.posix_acl_access", false)); + EXPECT_FALSE(xattr_name_appliable("system.posix_acl_default", false)); + EXPECT_TRUE(xattr_name_appliable("system.posix_acl_access", true)); + EXPECT_TRUE(xattr_name_appliable("system.posix_acl_default", true)); +} + +/* B4: a `-X`-only receiver (preserve_acls false) must NOT apply an incoming + * ACL xattr, while a user.* attribute in the same block still survives. The + * ACL entry is dropped, not applied (and the -X transfer is not failed). */ +static void run_recv_drops_acl_keeps_user(int fd) { + int ok = 0; + FileXattrList* list = xattr_receive(fd, &ok, false); + if (!ok || list == NULL) + _exit(1); + bool saw_user = false; + for (int i = 0; i < list->count; i++) { + if (strcmp(list->items[i].name, "system.posix_acl_access") == 0) + _exit(1); /* ACL must have been dropped */ + if (strcmp(list->items[i].name, "user.keep") == 0) + saw_user = true; + } + xattr_list_free(list); + _exit(saw_user ? 0 : 1); +} + +static void test_xattr_receive_drops_acl_without_preserve_acls() { + int p[2]; + EXPECT_EQ_INT(pipe(p), 0); + io_set_fds(p[0], p[1]); + io_set_bwlimit(0); + pid_t pid = fork(); + if (pid == 0) { + close(p[1]); + io_set_fds(p[0], p[0]); + run_recv_drops_acl_keeps_user(p[0]); + } + close(p[0]); + io_set_fds(p[1], p[1]); + FileXattrList* list = xattr_list_new(); + EXPECT_NOT_NULL(list); + EXPECT_TRUE(xattr_list_append(list, "system.posix_acl_access", "\x02\x00\x00\x00", 4)); + EXPECT_TRUE(xattr_list_append(list, "user.keep", "yes", 3)); + xattr_send(p[1], list); + xattr_list_free(list); + int status; + waitpid(pid, &status, 0); + close(p[1]); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); } /* MINOR-2: a --link-dest / -H copy fallback (linkat refused) must still apply @@ -360,6 +409,7 @@ void test_xattr() { test_xattr_reject_oversized_value(); test_xattr_count_bound(); test_xattr_capture_and_appliable(); + test_xattr_receive_drops_acl_without_preserve_acls(); test_link_copy_fallback_preserves_xattrs(); test_fake_super_restore(); test_fake_super_owner_gate();