diff --git a/src/shared/credentials.c b/src/shared/credentials.c index c27152e..79d600b 100644 --- a/src/shared/credentials.c +++ b/src/shared/credentials.c @@ -8,6 +8,7 @@ #include #include #include +#include /* One store entry: a username and its password's SHA-256 hex digest. The * plaintext password never appears here (and never on the daemon host). */ @@ -35,6 +36,23 @@ static bool is_comment_char(char c) { return c == '#' || c == ';'; } +/* A --password-file / --early-input carries plaintext or credential material + * and must not be accessible to group or other, mirroring the TLS private-key + * check in transport_tls.c. Reject any group/other permission bit (including + * execute) with a clear error. A stat failure is left for the caller's fopen + * to report, so a missing file keeps its existing "cannot open" message. */ +static bool secret_file_is_private(const char* path, char* err, size_t err_size) { + struct stat st; + if (stat(path, &st) != 0) + return true; + if (!S_ISREG(st.st_mode) || (st.st_mode & (S_IRWXG | S_IRWXO)) != 0) { + set_error(err, err_size, + "refusing to read secret file '%s': permissions must be owner-only (0600)", path); + return false; + } + return true; +} + /* Trim leading/trailing ASCII space and tab in place; returns the new start. */ static char* trim_space(char* s) { while (*s == ' ' || *s == '\t') @@ -124,6 +142,11 @@ static CredentialStore* load_store_file(const char* path, char* err, size_t err_ if (!path) return store; + if (!secret_file_is_private(path, err, err_size)) { + credentials_free(store); + return NULL; + } + FILE* fp = fopen(path, "r"); if (!fp) { set_error(err, err_size, "cannot open credential file '%s': %s", path, strerror(errno)); @@ -305,6 +328,8 @@ int credentials_read_secret_file(const char* path, char** user_out, char** passw set_error(err, err_size, "no --password-file path"); return -1; } + if (!secret_file_is_private(path, err, err_size)) + return -1; FILE* fp = fopen(path, "r"); if (!fp) { set_error(err, err_size, "cannot open password file '%s': %s", path, strerror(errno)); diff --git a/src/shared/file.c b/src/shared/file.c index 58e2753..70efd9d 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -16,6 +16,7 @@ #include "data.h" #include "delta.h" #include "file.h" +#include "file_store.h" #include "identity.h" #include "log.h" #include "metadata.h" @@ -37,44 +38,6 @@ static bool write_all(int fd, const void* data, unsigned long long size) { return true; } -/* A run of NUL bytes at least this long is emitted as a hole (lseek) rather - * than written, so the resulting file is genuinely sparse on the filesystem. */ -#define SPARSE_HOLE_MIN 4096U - -/* Sparse-aware writer (--sparse/-S). Walks `data`; any all-zero run of at - * least SPARSE_HOLE_MIN bytes is skipped with lseek(SEEK_CUR) so the block is - * never allocated (a real hole on the destination); every other byte is written - * normally. The file is pre-sized with ftruncate by the callers before this - * runs, so holes are guaranteed and the offset bookkeeping stays correct - * (each lseek advances the fd offset exactly as a write of that many bytes - * would). After the final run, ftruncate(size) guarantees the logical size is - * exactly `size` even when the tail was a hole. The full file image is in - * memory, so no wire change is needed. Returns false on I/O error. */ -static bool write_all_sparse(int fd, const unsigned char* data, unsigned long long size) { - unsigned long long i = 0; - while (i < size) { - if (data[i] == 0) { - unsigned long long run_start = i; - while (i < size && data[i] == 0) - i++; - unsigned long long run_len = i - run_start; - if (run_len >= SPARSE_HOLE_MIN) { - if (lseek(fd, (off_t)run_len, SEEK_CUR) < 0) - return false; - } else if (!write_all(fd, data + run_start, run_len)) { - return false; - } - } else { - unsigned long long run_start = i; - while (i < size && data[i] != 0) - i++; - if (!write_all(fd, data + run_start, i - run_start)) - return false; - } - } - return ftruncate(fd, (off_t)size) == 0; -} - /* Preallocate `size` bytes on `fd` before any data is written (--preallocate). * posix_fallocate reserves real disk blocks, so an out-of-space condition * (ENOSPC/EDQUOT) surfaces up front instead of partway through a transfer; @@ -515,6 +478,50 @@ out: return ok; } +/* Open the directory named by canonical absolute `resolved`, which the caller + * has already verified lies beneath `root` (the canonical authorized root). + * Each component is opened relative to the authorized-root fd with O_NOFOLLOW, + * so a directory swapped for a symlink after the realpath() check cannot + * redirect the open outside the root -- the walk simply fails. This replaces + * re-opening the absolute resolved path (TOCTOU). Returns an O_DIRECTORY fd, + * or -1 (the root itself and any error are refused). */ +static int open_dir_beneath_root(const char* resolved, const char* root) { + size_t root_len = strlen(root); + const char* rel = resolved + root_len; + while (*rel == '/') + rel++; + if (*rel == '\0') + return -1; + int fd = dup(authorized_root_fd); + if (fd < 0) + return -1; + char* copy = str_dup(rel); + if (!copy) { + close(fd); + return -1; + } + char* save = NULL; + for (char* component = strtok_r(copy, "/", &save); component; + component = strtok_r(NULL, "/", &save)) { + if (strcmp(component, ".") == 0) + continue; + /* A canonical realpath() output never contains "." or ".."; refuse ".." + defensively rather than let it climb toward the root. */ + int next = strcmp(component, "..") == 0 + ? -1 + : openat(fd, component, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + if (next < 0) { + close(fd); + free(copy); + return -1; + } + close(fd); + fd = next; + } + free(copy); + return fd; +} + int file_open_secure_parent(const char* path, char** leaf_out, bool create_dirs) { char* copy = str_dup(path); if (!copy) @@ -619,16 +626,13 @@ int file_open_secure_parent(const char* path, char** leaf_out, bool create_dirs) (resolved[strlen(root)] == '/' || resolved[strlen(root)] == '\0')) { struct stat rst; if (stat(resolved, &rst) == 0 && S_ISDIR(rst.st_mode)) { - /* Re-open the resolved directory WITHOUT following a symlink and - re-verify it is still a directory inode, so a symlink swapped - in between realpath() and open() (TOCTOU) cannot redirect this - fd outside the root. */ - next = open(resolved, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); - struct stat ofst; - if (next >= 0 && (fstat(next, &ofst) != 0 || !S_ISDIR(ofst.st_mode))) { - close(next); - next = -1; - } + /* Open the resolved directory through a relative no-follow walk + from the authorized-root fd instead of re-opening the + absolute `resolved` path: swapping an intermediate directory + for a symlink between realpath() and open() (TOCTOU) then + merely fails the walk rather than redirecting the fd outside + the root. */ + next = open_dir_beneath_root(resolved, root); } } } @@ -938,7 +942,7 @@ static bool file_to_disk_secure_impl(const char* path, const void* data, ok = ftruncate(fd, (off_t)data_size) == 0; if (ok || !sparse || data_size == 0) ok = sparse && data_size > 0 - ? write_all_sparse(fd, (const unsigned char*)data, data_size) + ? file_store_write_sparse(fd, (const unsigned char*)data, data_size) : write_all(fd, data, data_size); if (ok) ok = ftruncate(fd, (off_t)data_size) == 0; @@ -1046,8 +1050,9 @@ static bool file_to_disk_secure_impl(const char* path, const void* data, failure may leave partial data that --partial retention can rename. */ if (ok || (!sparse || data_size == 0)) { write_attempted = true; - ok = sparse && data_size > 0 ? write_all_sparse(fd, (const unsigned char*)data, data_size) - : write_all(fd, data, data_size); + ok = sparse && data_size > 0 + ? file_store_write_sparse(fd, (const unsigned char*)data, data_size) + : write_all(fd, data, data_size); } if (ok && metadata) ok = file_restore_metadata_fd(fd, metadata, preserve_executability); diff --git a/src/shared/file_store.c b/src/shared/file_store.c index 842e15e..b14da25 100644 --- a/src/shared/file_store.c +++ b/src/shared/file_store.c @@ -152,7 +152,7 @@ static bool write_all(int fd, const void* data, unsigned long long size) { * would). After the final run, ftruncate(size) guarantees the logical size is * exactly `size` even when the tail was a hole. The full file image is in * memory, so no wire change is needed. Returns false on I/O error. */ -static bool write_all_sparse(int fd, const unsigned char* data, unsigned long long size) { +bool file_store_write_sparse(int fd, const unsigned char* data, unsigned long long size) { unsigned long long i = 0; while (i < size) { if (data[i] == 0) { @@ -191,7 +191,7 @@ bool file_store_write_secure(const char* path, const void* data, unsigned long l if (fd >= 0) { if (sparse && data_size > 0) { if (ftruncate(fd, (off_t)data_size) == 0) - ok = write_all_sparse(fd, data, data_size); + ok = file_store_write_sparse(fd, data, data_size); } else { ok = write_all(fd, data, data_size); } @@ -219,8 +219,9 @@ bool file_store_write_secure(const char* path, const void* data, unsigned long l if (sparse && data_size > 0) ok = ftruncate(fd, (off_t)data_size) == 0; if (ok || (!sparse || data_size == 0)) - ok = (sparse && data_size > 0) ? write_all_sparse(fd, (const unsigned char*)data, data_size) - : write_all(fd, data, data_size); + ok = (sparse && data_size > 0) + ? file_store_write_sparse(fd, (const unsigned char*)data, data_size) + : write_all(fd, data, data_size); if (ok && metadata) ok = file_restore_metadata_fd(fd, metadata, preserve_executability); if (close(fd) != 0) diff --git a/src/shared/file_store.h b/src/shared/file_store.h index 743cc96..4bfb39a 100644 --- a/src/shared/file_store.h +++ b/src/shared/file_store.h @@ -10,5 +10,12 @@ bool file_store_rename_secure(const char* old_path, const char* new_path); bool file_store_write_secure(const char* path, const void* data, unsigned long long data_size, bool inplace, bool sparse, const FileMetadata* metadata, bool preserve_executability); +/* Sparse-aware write (--sparse/-S): every all-zero run of at least + * SPARSE_HOLE_MIN bytes is skipped with lseek(SEEK_CUR) so it becomes a real + * hole; every other byte is written. The caller pre-sizes the file with + * ftruncate; this function also ftruncate()s to `size` at the end so a trailing + * hole keeps the exact logical length. Shared by the file_store and file write + * paths. Returns false on write/lseek/ftruncate error. */ +bool file_store_write_sparse(int fd, const unsigned char* data, unsigned long long size); #endif diff --git a/src/shared/transport_ssh.c b/src/shared/transport_ssh.c index bba73a7..b5010ac 100644 --- a/src/shared/transport_ssh.c +++ b/src/shared/transport_ssh.c @@ -84,32 +84,29 @@ char* ssh_build_remote_command(const char* server_path, bool old_args, char* con const char* suffix = " --stdio"; /* Each --remote-option=OPT is appended after " --stdio" as one shell word, - escaped with the SAME single-quote boundary used for the server path. This - stays safe even in --old-args mode (which leaves the server path unquoted): - remote options are always single-quoted individually, so a value containing - shell metacharacters (; & | ` $ ()) can never break out of the quoting to - inject an unrelated remote command. Values are already validated at CLI - parse time (non-empty, no control characters); this layer only adds the - escaping boundary. */ + escaped with the SAME single-quote boundary used for the server path, so a + value containing shell metacharacters (; & | ` $ ()) can never break out of + the quoting to inject an unrelated remote command. Values are already + validated at CLI parse time (non-empty, no control characters); this layer + only adds the escaping boundary. */ size_t path_len = strlen(path); size_t suffix_len = strlen(suffix); - /* The base command (server path, quoted unless --old-args, then " --stdio"). */ - size_t command_len; - if (old_args) { - if (path_len > SIZE_MAX - suffix_len - 1) - return NULL; - command_len = path_len + suffix_len + 1; - } else { - size_t quote_count = 0; - for (const char* p = path; *p; p++) - if (*p == '\'') - quote_count++; - if (path_len > SIZE_MAX - suffix_len - 4 || - quote_count > (SIZE_MAX - path_len - suffix_len - 4) / 4) - return NULL; - command_len = path_len + quote_count * 4 + suffix_len + 4; - } + /* The base command: the server path is ALWAYS quoted as one single-quoted + shell word (remote options below reuse the same escaping), then + " --stdio". Quoting the path is the only injection-safe construction: an + unquoted path would carry shell metacharacters straight into the remote + shell command. --old-args is kept for CLI/ABI compatibility but no longer + disables that protection. */ + (void)old_args; + size_t quote_count = 0; + for (const char* p = path; *p; p++) + if (*p == '\'') + quote_count++; + if (path_len > SIZE_MAX - suffix_len - 4 || + quote_count > (SIZE_MAX - path_len - suffix_len - 4) / 4) + return NULL; + size_t command_len = path_len + quote_count * 4 + suffix_len + 4; /* Add each remote option, escaped as one single-quoted word: " ''", i.e. 1 leading space + 1 open quote + body (len + 3 per @@ -141,25 +138,18 @@ char* ssh_build_remote_command(const char* server_path, bool old_args, char* con if (!command) return NULL; char* out = command; - if (old_args) { - memcpy(out, path, path_len); - out += path_len; - memcpy(out, suffix, suffix_len + 1); - out += suffix_len; - } else { - *out++ = '\''; - for (const char* p = path; *p; p++) { - if (*p == '\'') { - memcpy(out, "'\\''", 4); - out += 4; - } else { - *out++ = *p; - } + *out++ = '\''; + for (const char* p = path; *p; p++) { + if (*p == '\'') { + memcpy(out, "'\\''", 4); + out += 4; + } else { + *out++ = *p; } - *out++ = '\''; - memcpy(out, suffix, suffix_len + 1); - out += suffix_len; } + *out++ = '\''; + memcpy(out, suffix, suffix_len + 1); + out += suffix_len; for (int i = 0; i < remote_option_count; i++) { const char* opt = remote_options[i]; *out++ = ' '; diff --git a/src/shared/transport_tls.c b/src/shared/transport_tls.c index c5f0df4..85bb5a1 100644 --- a/src/shared/transport_tls.c +++ b/src/shared/transport_tls.c @@ -48,6 +48,15 @@ static SSL_CTX* create_ssl_ctx(bool is_server, const char* cert, const char* key return NULL; } + /* Harden the context: never negotiate TLS compression (the CRIME attack + * vector) and never honour a post-handshake renegotiation request. + * SSL_OP_NO_RENEGOTIATION is only available from OpenSSL 1.1.1, so it is + * guarded to keep older headers building. */ + SSL_CTX_set_options(ctx, SSL_OP_NO_COMPRESSION); +#ifdef SSL_OP_NO_RENEGOTIATION + SSL_CTX_set_options(ctx, SSL_OP_NO_RENEGOTIATION); +#endif + if (SSL_CTX_set_min_proto_version(ctx, TLS1_2_VERSION) != 1) { SSL_CTX_free(ctx); return NULL; diff --git a/tests/integration/test_daemon.py b/tests/integration/test_daemon.py index d0781e0..523b640 100644 --- a/tests/integration/test_daemon.py +++ b/tests/integration/test_daemon.py @@ -66,6 +66,7 @@ def _pw_hash(password): def _write_client_password_file(path, user, password): with open(path, "w") as f: f.write("%s:%s\n" % (user, password)) + os.chmod(path, 0o600) return path @@ -164,6 +165,7 @@ def daemon_env(): f.write("# daemon credential store (Wave B)\n") f.write("alice:%s\n" % _pw_hash(ALICE_PASS)) f.write("bob:%s\n" % _pw_hash(BOB_PASS)) + os.chmod(CRED_FILE, 0o600) # The config's port is a free port chosen per worker; the `daemon` fixture # boots on it (the config-port path) and the --dparam override test boots a @@ -582,6 +584,7 @@ class TestDaemonAuthentication: cred_path = os.path.join(TEST_DATA_DIR, "client_empty.pw") with open(cred_path, "w") as f: f.write("# nothing here\n") + os.chmod(cred_path, 0o600) try: cmd = CLIENT_CMD + ["--source-dir", SOURCE_DIR, "--dest-dir", "127.0.0.1::files", diff --git a/tests/test_credentials.c b/tests/test_credentials.c index 10c6888..3ddcf06 100644 --- a/tests/test_credentials.c +++ b/tests/test_credentials.c @@ -33,6 +33,9 @@ static char* make_tmp_file(const char* contents) { return NULL; } fclose(fp); + /* Credential/password files are owner-only; the reader rejects group/other + * permission bits, so create temp files 0600 like the real ones. */ + chmod(path, 0600); return strdup(path); } @@ -353,6 +356,47 @@ static void test_credentials_gate_allows() { free(path); } +static void test_credentials_rejects_group_or_other_accessible() { + char err[512]; + char* path = + make_tmp_file("alice:9b90e524e94995ee4aeae2ee3c428a53405d1e8db147f44facc46797d0caf4c3\n"); + EXPECT_NOT_NULL(path); + + /* 0600 is accepted by the server store loader. */ + EXPECT_EQ_INT(chmod(path, 0600), 0); + CredentialStore* store = credentials_load(path, NULL, err, sizeof(err)); + EXPECT_NOT_NULL(store); + credentials_free(store); + + /* Group-readable and world-readable are both refused, with a clear error. */ + EXPECT_EQ_INT(chmod(path, 0640), 0); + EXPECT_NULL(credentials_load(path, NULL, err, sizeof(err))); + EXPECT_TRUE(strstr(err, "owner-only") != NULL); + EXPECT_EQ_INT(chmod(path, 0604), 0); + EXPECT_NULL(credentials_load(path, NULL, err, sizeof(err))); + + /* The client --password-file reader enforces the same rule. */ + EXPECT_EQ_INT(chmod(path, 0644), 0); + char* user = NULL; + char* password = NULL; + EXPECT_EQ_INT(credentials_read_secret_file(path, &user, &password, err, sizeof(err)), -1); + EXPECT_NULL(user); + EXPECT_NULL(password); + EXPECT_TRUE(strstr(err, "owner-only") != NULL); + + /* An --early-input file is checked too. */ + char* pw = + make_tmp_file("bob:2bb80d537b1da3e38bd30361aa855686bde0eacd7162fef6a25fe97bf527a25b\n"); + EXPECT_NOT_NULL(pw); + EXPECT_EQ_INT(chmod(path, 0644), 0); + EXPECT_NULL(credentials_load(pw, path, err, sizeof(err))); + + rm_temp(pw); + rm_temp(path); + free(pw); + free(path); +} + static void test_credentials_burn() { char secret[32]; memcpy(secret, "supersecretvalue", 17); @@ -374,6 +418,7 @@ void test_credentials(void) { test_credentials_early_input_merge(); test_credentials_read_secret_file(); test_credentials_read_secret_file_bad(); + test_credentials_rejects_group_or_other_accessible(); test_credentials_gate_allows(); test_credentials_burn(); } diff --git a/tests/test_file.c b/tests/test_file.c index 19deef1..e7088c3 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -1370,6 +1370,123 @@ static void test_dir_time_list() { rmdir(root); } +/* -K/--keep-dirlinks secure open: with an authorized root, a destination path + * component that is a symlink to an IN-ROOT directory is used as that directory + * (its referent is opened through a relative O_NOFOLLOW walk from the root fd, + * not by re-opening an absolute realpath() result), while a symlink resolving + * OUTSIDE the root is rejected. With -K off, even the in-root link is not + * followed. */ +static void test_keep_dirlinks_secure_open_impl() { + const char* root = "test_keep_dirlinks_root"; + const char* real = "test_keep_dirlinks_root/realdir"; + const char* link = "test_keep_dirlinks_root/linkdir"; + const char* abslink = "test_keep_dirlinks_root/abslink"; + const char* escape = "test_keep_dirlinks_root/escape"; + const char* outside = "test_keep_dirlinks_outside"; + unlink(link); + unlink(abslink); + unlink(escape); + rmdir(real); + rmdir(root); + rmdir(outside); + EXPECT_EQ_INT(mkdir(root, 0755), 0); + EXPECT_EQ_INT(mkdir(real, 0755), 0); + EXPECT_EQ_INT(mkdir(outside, 0755), 0); + + char root_abs[PATH_MAX]; + char real_abs[PATH_MAX]; + char outside_abs[PATH_MAX]; + EXPECT_NOT_NULL(realpath(root, root_abs)); + EXPECT_NOT_NULL(realpath(real, real_abs)); + EXPECT_NOT_NULL(realpath(outside, outside_abs)); + EXPECT_EQ_INT(symlink("realdir", link), 0); /* relative, in-root */ + EXPECT_EQ_INT(symlink(real_abs, abslink), 0); /* absolute, in-root */ + /* cppcheck-suppress knownConditionTrueFalse */ + EXPECT_EQ_INT(symlink(outside_abs, escape), 0); /* absolute, outside root */ + + int root_fd = open(root_abs, O_RDONLY | O_DIRECTORY | O_CLOEXEC); + EXPECT_TRUE(root_fd >= 0); + // cppcheck-suppress knownConditionTrueFalse + if (root_fd < 0) { + unlink(link); + unlink(abslink); + unlink(escape); + rmdir(real); + rmdir(root); + rmdir(outside); + return; + } + EXPECT_TRUE(file_set_authorized_root(root_fd, root_abs)); + file_set_keep_dirlinks(true); + + struct stat real_st; + EXPECT_EQ_INT(fstatat(root_fd, "realdir", &real_st, 0), 0); + + /* Relative in-root symlink-to-directory: followed to the referent dir. */ + char path[PATH_MAX + 64]; + snprintf(path, sizeof(path), "%s/linkdir/file.txt", root_abs); + char* leaf = NULL; + int parent_fd = file_open_secure_parent(path, &leaf, false); + EXPECT_TRUE(parent_fd >= 0); + EXPECT_NOT_NULL(leaf); + if (leaf) + EXPECT_EQ_STR(leaf, "file.txt"); + if (parent_fd >= 0) { + struct stat st; + EXPECT_EQ_INT(fstat(parent_fd, &st), 0); + EXPECT_TRUE(st.st_dev == real_st.st_dev && st.st_ino == real_st.st_ino); + close(parent_fd); + } + free(leaf); + + /* Absolute-but-in-root symlink-to-directory is followed the same way. */ + snprintf(path, sizeof(path), "%s/abslink/file.txt", root_abs); + leaf = NULL; + parent_fd = file_open_secure_parent(path, &leaf, false); + EXPECT_TRUE(parent_fd >= 0); + if (parent_fd >= 0) { + struct stat st; + EXPECT_EQ_INT(fstat(parent_fd, &st), 0); + EXPECT_TRUE(st.st_dev == real_st.st_dev && st.st_ino == real_st.st_ino); + close(parent_fd); + } + free(leaf); + + /* A symlink resolving outside the authorized root is rejected. */ + snprintf(path, sizeof(path), "%s/escape/file.txt", root_abs); + leaf = NULL; + EXPECT_EQ_INT(file_open_secure_parent(path, &leaf, false), -1); + free(leaf); + + /* With -K off the in-root symlink is not followed either. */ + file_set_keep_dirlinks(false); + snprintf(path, sizeof(path), "%s/linkdir/file.txt", root_abs); + leaf = NULL; + EXPECT_EQ_INT(file_open_secure_parent(path, &leaf, false), -1); + free(leaf); + + file_set_keep_dirlinks(false); + file_set_authorized_root(-1, NULL); + close(root_fd); + unlink(link); + unlink(abslink); + unlink(escape); + rmdir(real); + rmdir(root); + rmdir(outside); +} + +/* Wrapper guarantees the process-wide keep-dirlinks/authorized-root policy is + * cleared even when an EXPECT inside the body returns early (a failing EXPECT + * returns from its own function, so the body's trailing resets may be skipped). */ +static void test_keep_dirlinks_secure_open() { + file_set_authorized_root(-1, NULL); + file_set_keep_dirlinks(false); + test_keep_dirlinks_secure_open_impl(); + file_set_authorized_root(-1, NULL); + file_set_keep_dirlinks(false); +} + void test_file() { test_file_create(); test_file_special_rdev_valid(); @@ -1411,6 +1528,7 @@ void test_file() { } test_file_metadata_create(); test_dir_time_list(); + test_keep_dirlinks_secure_open(); test_inplace_overwrite_clears_special_mode_bits(); test_inplace_overwrite_metadata_strips_special_bits(); test_inplace_overwrite_truncates_shorter_payload(); diff --git a/tests/test_transport_ssh.c b/tests/test_transport_ssh.c index 7a273ef..94a896a 100644 --- a/tests/test_transport_ssh.c +++ b/tests/test_transport_ssh.c @@ -55,8 +55,14 @@ static void test_ssh_remote_command_argument_modes() { EXPECT_EQ_STR(command, "'fast'\\''sync' --stdio"); free(command); + /* --old-args no longer disables injection-safe quoting: the path is still one + single-quoted word, even when it carries shell metacharacters. */ command = ssh_build_remote_command("fast sync; touch /tmp/pwned", true, NULL, 0); - EXPECT_EQ_STR(command, "fast sync; touch /tmp/pwned --stdio"); + EXPECT_EQ_STR(command, "'fast sync; touch /tmp/pwned' --stdio"); + free(command); + + command = ssh_build_remote_command("fast'sync; rm -rf /", true, NULL, 0); + EXPECT_EQ_STR(command, "'fast'\\''sync; rm -rf /' --stdio"); free(command); } @@ -131,9 +137,9 @@ static void test_ssh_remote_command_with_remote_options() { free(command); free(val); - /* --old-args leaves the server path unquoted but still quotes remote options. */ + /* --old-args still quotes both the server path and the remote options. */ command = ssh_build_remote_command("srv", true, multi, 2); - EXPECT_EQ_STR(command, "srv --stdio '-v' '--allow-delete'"); + EXPECT_EQ_STR(command, "'srv' --stdio '-v' '--allow-delete'"); free(command); } diff --git a/tests/test_transport_tls.c b/tests/test_transport_tls.c index 5e21580..28bbba5 100644 --- a/tests/test_transport_tls.c +++ b/tests/test_transport_tls.c @@ -3,6 +3,7 @@ #include "test_utils.h" #include "transport_tcp.h" #include "transport_tls.h" +#include #include #include @@ -17,6 +18,12 @@ static void test_server_create_tls_without_certs() { bool ok = server_create_tls(s, NULL, NULL, NULL); EXPECT_TRUE(ok); EXPECT_NOT_NULL(s->ssl_ctx); + /* The context must disable TLS compression (CRIME) and renegotiation. */ + SSL_CTX* ctx = (SSL_CTX*)s->ssl_ctx; + EXPECT_TRUE((SSL_CTX_get_options(ctx) & SSL_OP_NO_COMPRESSION) != 0); +#ifdef SSL_OP_NO_RENEGOTIATION + EXPECT_TRUE((SSL_CTX_get_options(ctx) & SSL_OP_NO_RENEGOTIATION) != 0); +#endif server_delete(&s); EXPECT_NULL(s); }