Merge branch 'fix/sec-server' into fix/sec-integration

tests/conftest.py

@@ -16,7 +16,13 @@ def shared_server():
     Under pytest-xdist this session fixture is instantiated once per worker
     process, so each worker gets its own server on an ephemeral port."""
     server = ServerManager()
-    server.start()
+    # --allow-super keeps the historical permissive super mode for a root
+    # receiver: the integration suite's root-only ownership/device/copy-as tests
+    # exercise that opted-in configuration.  The secure default (a root
+    # standalone server without --allow-super forces SUPER_MODE_OFF) is covered
+    # explicitly by TestStandaloneSuperDefault in test_features.py.  Non-root
+    # runs are unaffected by the flag.
+    server.start(extra_args=["--allow-super"])
     yield server
     server.stop()
 

tests/integration/test_features.py

@@ -1592,8 +1592,33 @@ class TestDelete:
         assert not missing, f"Missing: {missing}"
         assert not mismatches, f"Mismatch: {mismatches}"
 
-
-class TestProgress:
+    @pytest.mark.ci
+    def test_force_cannot_replace_directory_without_allow_delete(self):
+        """C2: --force is deletion authority (an incoming file may recursively
+        remove a non-empty destination directory tree).  A server started without
+        --allow-delete must clear it, so the operator's delete policy cannot be
+        bypassed with --force."""
+        source = os.path.join(TEST_DATA_DIR, "force_src")
+        dest = os.path.join(TEST_DATA_DIR, "force_dst")
+        clean_dir(source)
+        clean_dir(dest)
+        with open(os.path.join(source, "blocker"), "wb") as f:
+            f.write(b"incoming file\n")
+        received = get_dest_received_dir(dest, source)
+        blocker = os.path.join(received, "blocker")
+        os.makedirs(blocker)
+        nested = os.path.join(blocker, "nested.txt")
+        with open(nested, "w") as f:
+            f.write("survivor")
+        # Deliberately NO --allow-delete.
+        server = ServerManager()
+        server.start()
+        try:
+            run_client(source, dest, flags=["--force"], port=server.port)
+        finally:
+            server.stop()
+        assert os.path.isdir(blocker), "unauthorized --force removed a destination directory"
+        assert os.path.exists(nested), "unauthorized --force removed a nested file"
     def test_progress_output(self, shared_server):
         clean_dir(DEST_DIR)
         result, dur = run_client(
@@ -4630,6 +4655,61 @@ class TestSuperPrivilege:
             f"--no-super must suppress fake-super's owner replay: uid={st.st_uid} gid={st.st_gid}"
 
 
+class TestStandaloneSuperDefault:
+    """C3: a privileged (root) STANDALONE server without --allow-super forces
+    SUPER_MODE_OFF, so a client cannot make it create device nodes, write raw
+    devices, apply ownership, or use --copy-as.  The shared_server fixture opts in
+    with --allow-super to keep the historical behavior available to the existing
+    root-only tests; these tests start their own un-opted server."""
+
+    @pytest.mark.ci
+    def test_copy_as_refused_without_allow_super(self):
+        """--copy-as is a client-chosen-ownership request and must be refused by
+        a standalone server that did not opt in with --allow-super (on a non-root
+        receiver it is refused for lack of privilege either way)."""
+        source = os.path.join(TEST_DATA_DIR, "super_default_src")
+        dest = os.path.join(TEST_DATA_DIR, "super_default_dst")
+        clean_dir(source)
+        clean_dir(dest)
+        with open(os.path.join(source, "f.txt"), "wb") as f:
+            f.write(b"no copy-as\n")
+        server = ServerManager()
+        server.start()  # deliberately no --allow-super
+        try:
+            result, _ = run_client(source, dest,
+                                   flags=["--preserve", "--copy-as=@65534:@65534"],
+                                   port=server.port)
+        finally:
+            server.stop()
+        assert result.returncode != 0, (
+            "standalone server accepted --copy-as without --allow-super"
+        )
+
+    @pytest.mark.skipif(os.geteuid() != 0, reason="root can create the source device node")
+    def test_devices_skipped_without_allow_super(self):
+        """Root standalone server without --allow-super must skip device-node
+        creation even for a client --devices request (the run still succeeds and
+        the regular file transfers)."""
+        source = os.path.join(TEST_DATA_DIR, "super_default_dev_src")
+        dest = os.path.join(TEST_DATA_DIR, "super_default_dev_dst")
+        clean_dir(source)
+        clean_dir(dest)
+        with open(os.path.join(source, "plain.txt"), "wb") as f:
+            f.write(b"regular\n")
+        os.mknod(os.path.join(source, "null"), stat.S_IFCHR | 0o666, os.makedev(1, 3))
+        server = ServerManager()
+        server.start()  # deliberately no --allow-super
+        try:
+            result, _ = run_client(source, dest, flags=["--devices"], port=server.port)
+        finally:
+            server.stop()
+        assert result.returncode == 0, f"exit {result.returncode}: {(result.stderr or '')[:200]}"
+        received = get_dest_received_dir(dest, source)
+        assert not os.path.lexists(os.path.join(received, "null")), (
+            "root standalone server created a device node without --allow-super"
+        )
+
+
 class TestHardLinks:
     """-H/--hard-links: source files sharing an inode are re-created as hard
     links to one another on the destination (dedup preserved, first copy

tests/test_config.c

@@ -87,6 +87,34 @@ static void test_config_ssh_dest_no_user() {
   config_delete(cfg);
 }
 
+/* C1: the user@host token is passed to ssh in option position, so a host or user
+ * beginning with '-' (e.g. "-oProxyCommand=...") must be rejected before any
+ * argv is built, and an empty host must be rejected too. */
+static void test_config_ssh_dest_rejects_option_injection() {
+  Config* cfg = make_config("1.0", "/src", "-oProxyCommand=id:/dst", true, false, false, false,
+                            false, 1, false, 0);
+  EXPECT_EQ_INT(config_parse_ssh_dest(cfg), -1);
+  EXPECT_EQ_INT(cfg->transport, TRANSPORT_TCP);
+  EXPECT_NULL(cfg->ssh_destination);
+  config_delete(cfg);
+
+  cfg =
+      make_config("1.0", "/src", "-user@host:/dst", true, false, false, false, false, 1, false, 0);
+  EXPECT_EQ_INT(config_parse_ssh_dest(cfg), -1);
+  config_delete(cfg);
+
+  cfg = make_config("1.0", "/src", "user@:/dst", true, false, false, false, false, 1, false, 0);
+  EXPECT_EQ_INT(config_parse_ssh_dest(cfg), -1);
+  config_delete(cfg);
+
+  /* config_parse_transport_dest propagates the rejection (and still returns 1
+   * for daemon syntax first). */
+  cfg = make_config("1.0", "/src", "-oProxyCommand=id:/dst", true, false, false, false, false, 1,
+                    false, 0);
+  EXPECT_EQ_INT(config_parse_transport_dest(cfg), -1);
+  config_delete(cfg);
+}
+
 static void test_config_daemon_dest_parse() {
   Config* cfg = make_config("1.0", "/src", "dahost::files/sub/dir", true, false, false, false,
                             false, 1, false, 0);
@@ -2789,6 +2817,7 @@ void test_config() {
   test_config_ssh_dest();
   test_config_ssh_dest_local_path();
   test_config_ssh_dest_no_user();
+  test_config_ssh_dest_rejects_option_injection();
   test_config_daemon_dest_parse();
   test_config_daemon_dest_no_path();
   test_config_daemon_dest_double_slash_normalized();

tests/test_credentials.c

@@ -473,6 +473,34 @@ static void test_credentials_store_rejects_legacy_hex() {
   free(path);
 }
 
+/* C9: the legacy-hex detector must check the length before indexing 64 bytes, so
+ * a short secret is never read out of bounds.  Such a line is rejected for the
+ * ordinary "expected verifier" reason, never as legacy. */
+static void test_credentials_store_rejects_short_secret() {
+  char contents[CREDENTIAL_MAX_LINE];
+  snprintf(contents, sizeof(contents), "alice:%s\n", "abc");
+  char* path = make_tmp_file(contents);
+  EXPECT_NOT_NULL(path);
+  char err[512];
+  const CredentialStore* store = credentials_load(path, NULL, err, sizeof(err));
+  EXPECT_NULL(store);
+  EXPECT_TRUE(strstr(err, "legacy unsalted") == NULL);
+  rm_temp(path);
+  free(path);
+
+  char short_hex[64];
+  memset(short_hex, 'a', 63);
+  short_hex[63] = '\0';
+  snprintf(contents, sizeof(contents), "alice:%s\n", short_hex);
+  path = make_tmp_file(contents);
+  EXPECT_NOT_NULL(path);
+  store = credentials_load(path, NULL, err, sizeof(err));
+  EXPECT_NULL(store);
+  EXPECT_TRUE(strstr(err, "legacy unsalted") == NULL);
+  rm_temp(path);
+  free(path);
+}
+
 static void test_credentials_store_duplicate_rejected() {
   char line[CREDENTIAL_MAX_LINE];
   EXPECT_TRUE(make_store_line("alice", KAT_PASSWORD, CREDENTIAL_MIN_ITERS, line, sizeof(line)));
@@ -1066,6 +1094,7 @@ void test_credentials(void) {
   test_credentials_store_parse_valid();
   test_credentials_store_parse_rejects_malformed();
   test_credentials_store_rejects_legacy_hex();
+  test_credentials_store_rejects_short_secret();
   test_credentials_store_duplicate_rejected();
   test_credentials_store_rejects_nonuniform_iters();
   test_credentials_store_parse_missing_file();

tests/test_daemon_conf.c

@@ -391,6 +391,22 @@ static void test_daemon_conf_auth_users_validated() {
   EXPECT_EQ_STR(ok_conf->modules[0].auth_users[0], "alice");
   EXPECT_EQ_STR(ok_conf->modules[0].auth_users[1], "bob");
   daemon_conf_free(ok_conf);
+
+  /* C4: an empty or separator-only `auth users` value is a parse error.  It
+   * would otherwise leave the module with a zero-length allow-list, silently
+   * disabling the authentication the operator asked for. */
+  const char* empty_auth[] = {
+      "[m]\npath = /x\nauth users = \n",
+      "[m]\npath = /x\nauth users = , ,\n",
+      "[m]\npath = /x\nauth users = \t\n",
+  };
+  for (size_t i = 0; i < sizeof(empty_auth) / sizeof(empty_auth[0]); i++) {
+    EXPECT_EQ_INT(write_conf(empty_auth[i], &path), 0);
+    const DaemonConf* rejected = daemon_conf_load(path, err, sizeof(err));
+    free(path);
+    EXPECT_NULL(rejected);
+    EXPECT_TRUE(strstr(err, "'auth users' must list at least one user") != NULL);
+  }
 }
 
 /* Wave 3 daemon hardening: configurable global/per-module connection caps,
@@ -475,12 +491,55 @@ static void test_daemon_conf_limits_and_hosts_parse() {
   EXPECT_EQ_INT(conf->modules[0].max_connections, 0);
   daemon_conf_free(conf);
 
-  /* An empty hosts list is not an error (no patterns are added). */
-  EXPECT_EQ_INT(write_conf("hosts allow = \n[m]\npath = /x\n", &path), 0);
+  /* C4: a present hosts key with an empty/separator-only value must not silently
+   * install a zero-length (allow-everyone) list. */
+  const char* empty_hosts[] = {
+      "hosts allow = \n[m]\npath = /x\n",
+      "hosts deny = \n[m]\npath = /x\n",
+      "hosts allow = , ,\n[m]\npath = /x\n",
+      "hosts deny = \t\n[m]\npath = /x\n",
+  };
+  for (size_t i = 0; i < sizeof(empty_hosts) / sizeof(empty_hosts[0]); i++) {
+    EXPECT_EQ_INT(write_conf(empty_hosts[i], &path), 0);
+    const DaemonConf* rejected = daemon_conf_load(path, err, sizeof(err));
+    free(path);
+    EXPECT_NULL(rejected);
+    EXPECT_TRUE(strstr(err, "must list at least one host pattern") != NULL);
+  }
+
+  const char* empty_module_hosts[] = {
+      "[m]\npath = /x\nhosts allow = \n",
+      "[m]\npath = /x\nhosts deny = ,\n",
+  };
+  for (size_t i = 0; i < sizeof(empty_module_hosts) / sizeof(empty_module_hosts[0]); i++) {
+    EXPECT_EQ_INT(write_conf(empty_module_hosts[i], &path), 0);
+    const DaemonConf* rejected = daemon_conf_load(path, err, sizeof(err));
+    free(path);
+    EXPECT_NULL(rejected);
+    EXPECT_TRUE(strstr(err, "must list at least one host pattern") != NULL);
+    /* The diagnostic must name the offending module. */
+    EXPECT_TRUE(strstr(err, "module 'm'") != NULL);
+  }
+
+  /* Per-module host lists APPEND across lines like the global ones.  (A swapped
+   * store_host_list call passed the module name as `replace`, so each line
+   * silently replaced the previous one and only the last survived.) */
+  EXPECT_EQ_INT(write_conf("[m]\npath = /x\n"
+                           "hosts allow = 127.0.0.1\n"
+                           "hosts allow = 10.0.0.0/8\n"
+                           "hosts deny = 192.168.0.1\n"
+                           "hosts deny = 2001:db8::/32\n",
+                           &path),
+                0);
   conf = daemon_conf_load(path, err, sizeof(err));
   free(path);
   EXPECT_NOT_NULL(conf);
-  EXPECT_EQ_INT(conf->global.hosts_allow_count, 0);
+  EXPECT_EQ_INT(conf->modules[0].hosts_allow_count, 2);
+  EXPECT_EQ_STR(conf->modules[0].hosts_allow[0], "127.0.0.1");
+  EXPECT_EQ_STR(conf->modules[0].hosts_allow[1], "10.0.0.0/8");
+  EXPECT_EQ_INT(conf->modules[0].hosts_deny_count, 2);
+  EXPECT_EQ_STR(conf->modules[0].hosts_deny[0], "192.168.0.1");
+  EXPECT_EQ_STR(conf->modules[0].hosts_deny[1], "2001:db8::/32");
   daemon_conf_free(conf);
 }
 

tests/test_server_cli.c

@@ -32,6 +32,35 @@ static void test_server_cli_defaults() {
   EXPECT_FALSE(opts.allow_delete);
   EXPECT_FALSE(opts.allow_unauthenticated);
   EXPECT_FALSE(opts.no_super);
+  EXPECT_FALSE(opts.allow_super);
+  server_cli_options_free(&opts);
+}
+
+/* C3: --allow-super is the locally-launched standalone TCP opt-in for a
+ * privileged receiver; it never combines with --no-super, is refused with
+ * --stdio (whose client-composed remote argv must not defeat the default), and
+ * daemon modules use their own per-module `client owner = yes` opt-in instead. */
+static void test_server_cli_allow_super() {
+  const char* args[] = {"fastsync-server", "--allow-super", "--destination-root", "/srv"};
+  ServerCliOptions opts;
+  EXPECT_EQ_INT(parse_ok(args, 4, &opts), 0);
+  EXPECT_TRUE(opts.allow_super);
+  server_cli_options_free(&opts);
+
+  char err[256];
+  const char* a1[] = {"s", "--allow-super", "--no-super"};
+  EXPECT_EQ_INT(server_cli_parse(3, (char**)a1, &opts, err, sizeof(err)), -1);
+  EXPECT_TRUE(strstr(err, "mutually exclusive") != NULL);
+
+  const char* a2[] = {"s", "--daemon", "--config=/tmp/x.conf", "--allow-super"};
+  EXPECT_EQ_INT(server_cli_parse(4, (char**)a2, &opts, err, sizeof(err)), -1);
+  EXPECT_TRUE(strstr(err, "client owner") != NULL);
+
+  /* The SSH/--stdio receiver argv is composed by the client, so --allow-super
+   * must be rejected there and the C3 secure default stays in force. */
+  const char* a3[] = {"s", "--stdio", "--allow-super", "--destination-root", "/srv"};
+  EXPECT_EQ_INT(server_cli_parse(5, (char**)a3, &opts, err, sizeof(err)), -1);
+  EXPECT_TRUE(strstr(err, "--stdio") != NULL);
   server_cli_options_free(&opts);
 }
 
@@ -224,5 +253,6 @@ void test_server_cli() {
   test_server_cli_password_and_early_input();
   test_server_cli_password_requires_daemon();
   test_server_cli_no_super();
+  test_server_cli_allow_super();
   test_server_cli_help();
 }

tests/test_transport_ssh.c

@@ -66,16 +66,18 @@ static void test_ssh_remote_command_argument_modes() {
   free(command);
 }
 
-/* The build for a single-word argv is [prog, six -o args, user, command]. */
+/* The build for a single-word argv is [prog, six -o args, "--", user, command]. */
 
 static void test_ssh_build_client_argv_default_is_ssh() {
   char** argv = ssh_build_client_argv(NULL, 0, "u@h", "'srv' --stdio");
   EXPECT_NOT_NULL(argv);
   EXPECT_EQ_STR(argv[0], "ssh");
   EXPECT_EQ_STR(argv[1], "-o");
-  EXPECT_EQ_STR(argv[7], "u@h");
-  EXPECT_EQ_STR(argv[8], "'srv' --stdio");
-  EXPECT_NULL(argv[9]);
+  /* The "--" end-of-options marker precedes the destination token. */
+  EXPECT_EQ_STR(argv[7], "--");
+  EXPECT_EQ_STR(argv[8], "u@h");
+  EXPECT_EQ_STR(argv[9], "'srv' --stdio");
+  EXPECT_NULL(argv[10]);
   ssh_free_client_argv(argv);
 }
 
@@ -84,7 +86,7 @@ static void test_ssh_build_client_argv_uses_custom_rsh() {
   char** argv = ssh_build_client_argv("myrsh", 0, "u@h", "rc");
   EXPECT_NOT_NULL(argv);
   EXPECT_EQ_STR(argv[0], "myrsh");
-  EXPECT_NULL(argv[9]);
+  EXPECT_NULL(argv[10]);
   ssh_free_client_argv(argv);
 }
 
@@ -96,21 +98,37 @@ static void test_ssh_build_client_argv_whitespace_command_and_port() {
   EXPECT_EQ_STR(argv[0], "ssh");
   EXPECT_EQ_STR(argv[1], "-p");
   EXPECT_EQ_STR(argv[2], "2222");
-  EXPECT_NULL(argv[11]);
+  EXPECT_NULL(argv[12]);
   ssh_free_client_argv(argv);
 
   argv = ssh_build_client_argv("ssh", 2222, "u@h", "rc");
   EXPECT_NOT_NULL(argv);
   EXPECT_EQ_STR(argv[0], "ssh");
-  /* Flat [prog, -o x6, -p, port, user, command]. */
+  /* Flat [prog, -o x6, -p, port, "--", user, command]. */
   EXPECT_EQ_STR(argv[7], "-p");
   EXPECT_EQ_STR(argv[8], "2222");
-  EXPECT_EQ_STR(argv[9], "u@h");
-  EXPECT_EQ_STR(argv[10], "rc");
-  EXPECT_NULL(argv[11]);
+  EXPECT_EQ_STR(argv[9], "--");
+  EXPECT_EQ_STR(argv[10], "u@h");
+  EXPECT_EQ_STR(argv[11], "rc");
+  EXPECT_NULL(argv[12]);
   ssh_free_client_argv(argv);
 }
 
+/* C1: a destination host/user beginning with '-' would be parsed by ssh as an
+ * option (argument injection: -oProxyCommand=...), and an empty host is never
+ * valid.  These are refused before any child is forked, so no Client is
+ * returned and no command can run. */
+static void test_ssh_connect_rejects_option_host() {
+  /* cppcheck-suppress constVariablePointer */
+  Client* client = client_connect_ssh("-oProxyCommand=touch /tmp/pwned:/remote", 22, NULL, false,
+                                      NULL, false, NULL, 0);
+  EXPECT_NULL(client);
+  client = client_connect_ssh("-evil:/remote", 22, NULL, false, NULL, false, NULL, 0);
+  EXPECT_NULL(client);
+  client = client_connect_ssh("user@:/remote", 22, NULL, false, NULL, false, NULL, 0);
+  EXPECT_NULL(client);
+}
+
 /* --remote-option=OPT appends OPT to the remote command line after " --stdio",
  * each escaped as its own single-quoted shell word.  Metacharacters that could
  * break out of the quoting are neutralized (never injected), matching the
@@ -162,6 +180,7 @@ void test_transport_ssh() {
   test_ssh_connect_invalid_dest_empty();
   test_ssh_connect_malformed();
   test_ssh_connect_unreachable();
+  test_ssh_connect_rejects_option_host();
   test_ssh_remote_command_argument_modes();
   test_ssh_build_client_argv_default_is_ssh();
   test_ssh_build_client_argv_uses_custom_rsh();

tests/test_transport_tls.c

@@ -24,6 +24,17 @@ static void test_server_create_tls_without_certs() {
 #ifdef SSL_OP_NO_RENEGOTIATION
   EXPECT_TRUE((SSL_CTX_get_options(ctx) & SSL_OP_NO_RENEGOTIATION) != 0);
 #endif
+  /* C5: the server's preference order decides the cipher and the TLS 1.2 list is
+   * AEAD-only (no CBC/RC4/3DES legacy suites). */
+  EXPECT_TRUE((SSL_CTX_get_options(ctx) & SSL_OP_CIPHER_SERVER_PREFERENCE) != 0);
+  STACK_OF(SSL_CIPHER)* ciphers = SSL_CTX_get_ciphers(ctx);
+  EXPECT_NOT_NULL(ciphers);
+  for (int i = 0; i < sk_SSL_CIPHER_num(ciphers); i++) {
+    const char* name = SSL_CIPHER_get_name(sk_SSL_CIPHER_value(ciphers, i));
+    EXPECT_TRUE(name != NULL && strstr(name, "CBC") == NULL);
+    EXPECT_TRUE(name != NULL && strstr(name, "RC4") == NULL);
+    EXPECT_TRUE(name != NULL && strstr(name, "3DES") == NULL);
+  }
   server_delete(&s);
   EXPECT_NULL(s);
 }