Agent ready-file O_NOFOLLOW + workspace consolidation + threat-model refresh

Cargo.toml

@@ -29,15 +29,15 @@ sshenc-agent = { path = "crates/sshenc-agent" }
 sshenc-test-support = { path = "crates/sshenc-test-support" }
 
 # libenclaveapp (shared hardware-backed key management)
-enclaveapp-core = { path = "../crates/enclaveapp-core" }
-enclaveapp-apple = { path = "../crates/enclaveapp-apple", features = ["signing"] }
-enclaveapp-windows = { path = "../crates/enclaveapp-windows", features = ["signing"] }
-enclaveapp-keyring = { path = "../crates/enclaveapp-keyring", features = ["signing"] }
-enclaveapp-linux-tpm = { path = "../crates/enclaveapp-linux-tpm", features = ["signing"] }
-enclaveapp-wsl = { path = "../crates/enclaveapp-wsl" }
-enclaveapp-app-storage = { path = "../crates/enclaveapp-app-storage" }
-enclaveapp-build-support = { path = "../crates/enclaveapp-build-support" }
-enclaveapp-tpm-bridge = { path = "../crates/enclaveapp-tpm-bridge" }
+enclaveapp-core = { path = "../libenclaveapp/crates/enclaveapp-core" }
+enclaveapp-apple = { path = "../libenclaveapp/crates/enclaveapp-apple", features = ["signing"] }
+enclaveapp-windows = { path = "../libenclaveapp/crates/enclaveapp-windows", features = ["signing"] }
+enclaveapp-keyring = { path = "../libenclaveapp/crates/enclaveapp-keyring", features = ["signing"] }
+enclaveapp-linux-tpm = { path = "../libenclaveapp/crates/enclaveapp-linux-tpm", features = ["signing"] }
+enclaveapp-wsl = { path = "../libenclaveapp/crates/enclaveapp-wsl" }
+enclaveapp-app-storage = { path = "../libenclaveapp/crates/enclaveapp-app-storage" }
+enclaveapp-build-support = { path = "../libenclaveapp/crates/enclaveapp-build-support" }
+enclaveapp-tpm-bridge = { path = "../libenclaveapp/crates/enclaveapp-tpm-bridge" }
 
 # Serialization
 serde = { version = "1", features = ["derive"] }

THREAT_MODEL.md

@@ -242,12 +242,15 @@ descriptor) can accept SSH clients' signing requests.
   before starting `sshenc-agent`, so clients connect to `sshenc`.
 - `sshenc-agent` uses `ServerOptions::first_pipe_instance(true)` and
   refuses to attach to an existing pipe.
+- The named pipe is created with an explicit DACL
+  (`ConvertStringSecurityDescriptorToSecurityDescriptorW`) that grants
+  full control only to the creator-owner (the current user) and
+  `SYSTEM`, cutting off `Administrators` and `Everyone` who would
+  otherwise have default read/write access (`sshenc-agent/src/server.rs`
+  `SecurityDescriptor`).
 - The CLI surfaces an actionable error when the pipe is in use.
 
 **Residual risk**:
-- The named pipe is currently created with the default security
-  descriptor. A best-practice hardening is an explicit DACL that grants
-  only the owner user and `SYSTEM`; that work is not yet in place.
 - An attacker with admin rights can always create the pipe first; admin
   rights on Windows already implies full control over the TPM.
 
@@ -263,20 +266,31 @@ fields.
   the hardware's enforcement — Touch ID / Windows Hello still fires on
   sign regardless of what the metadata file claims.
 - Metadata files are written 0600 via `atomic_write`.
+- On the Linux keyring / software backend, `.meta` now has an HMAC
+  sidecar `<label>.meta.hmac` generated at key-creation time. The
+  HMAC key is a random per-app 32-byte value stored in the system
+  keyring (`enclaveapp-keyring::meta_hmac_key`). `enclaveapp-app-storage`
+  verifies the sidecar on load and rejects HMAC-mismatched reads with
+  a hard error (`meta_hmac_verify`). An attacker who rewrites `.meta`
+  without also having keyring access is caught.
 
 **Residual risk**:
 - UI and library-level policy checks (`sshenc list`, `sshenc inspect`,
-  `PromptPolicy::KeyDefault`) trust the metadata file. An attacker can
-  **mislead** the user into believing a key is unprotected (or vice
-  versa).
-- On the Linux software backend there is no hardware enforcement. Metadata
-  tamper there is a full downgrade: the key signs without any prompt.
+  `PromptPolicy::KeyDefault`) still trust the metadata file on
+  hardware backends, where the sidecar is not written (hardware-side
+  enforcement makes the check redundant). An attacker who rewrites
+  `.meta` on a hardware backend can still **mislead** the user into
+  believing a key is unprotected — but signing will still prompt.
+- On the keyring / software backend, a same-UID attacker who also has
+  keyring access can still rewrite both `.meta` and the sidecar. This
+  is the same threshold as decrypting the key material itself, so no
+  net loss of protection.
 - The migration from the legacy `biometric: bool` field to `AccessPolicy`
   is handled by compatibility code; a missing `access_policy` field is
   treated per the legacy bool. A same-UID attacker who strips the new
   field from `.meta` can rely on the legacy-compat path behaving
   intuitively but should not gain anything the hardware does not already
-  allow.
+  allow (and on the keyring backend the HMAC check catches the edit).
 
 ## Threat: `SSH_AUTH_SOCK` / `IdentityAgent` Trust
 
@@ -377,14 +391,28 @@ that path to redirect the "ready" write.
 **Mitigations**:
 - The filename includes PID and nanosecond timestamp so collisions are
   unlikely.
-- `signal_ready` writes with 0600 after close.
+- `signal_ready` opens the file with
+  `OpenOptions::new().write(true).create_new(true).custom_flags(libc::O_NOFOLLOW).mode(0o600)`
+  (`sshenc-agent/src/server.rs`). `create_new` atomically fails with
+  `EEXIST` if anything already exists at the path (file, symlink,
+  directory); `O_NOFOLLOW` additionally refuses to dereference a
+  pre-planted symlink and fails with `ELOOP`. Either way the write
+  never lands on an attacker-chosen target, and the error is surfaced
+  to the parent process via the daemonize handshake instead of being
+  silently followed.
+- `mode(0o600)` at open time (load-bearing on umask-permissive
+  systems) + an explicit `set_permissions(0o600)` for belt-and-
+  suspenders.
+- A `signal_ready_refuses_preplanted_symlink` unit test locks in the
+  symlink-refusal semantics.
 
 **Residual risk**:
-- `std::fs::write` follows symlinks. A race between timestamp selection
-  and write is theoretically exploitable on multi-user systems with a
-  shared `/tmp`. `$TMPDIR` is per-user on macOS and modern Linux so the
-  practical risk is limited. A future hardening could use `O_NOFOLLOW` or
-  a user-private temp dir.
+- None for the symlink-redirect vector. An attacker who can predict
+  the path and win a TOCTOU between our `remove_file` (performed by
+  the parent before spawn) and our `create_new` (in the child) still
+  loses — the attacker's file is created, then our `create_new` fails
+  with `EEXIST`, and the parent daemonize handshake surfaces the
+  failure.
 
 ## Threat: MSI Uninstall Resilience
 

crates/sshenc-agent/src/server.rs

@@ -383,12 +383,44 @@ fn signal_ready(path: Option<&Path>) -> Result<()> {
     if let Some(parent) = path.parent() {
         std::fs::create_dir_all(parent)?;
     }
-    std::fs::write(path, b"ready\n")?;
+    // Use create-new + O_NOFOLLOW so a pre-planted symlink at `path`
+    // (shared `/tmp`, multi-user system, etc.) cannot redirect the
+    // "ready" write to an attacker-chosen target. If the path already
+    // exists the open fails with EEXIST; if it's a symlink the open
+    // fails with ELOOP. Either way we surface the error rather than
+    // silently following the link. See sshenc THREAT_MODEL.md
+    // "Ready-File Symlink in $TMPDIR".
+    //
+    // The parent process (`daemonize` in `main.rs`) is responsible
+    // for unlinking the ready-file path before and after spawning
+    // the agent, so we always start from a clean slate here and can
+    // safely demand create_new semantics.
     #[cfg(unix)]
     {
-        use std::os::unix::fs::PermissionsExt;
+        use std::io::Write;
+        use std::os::unix::fs::{OpenOptionsExt, PermissionsExt};
+        let mut file = std::fs::OpenOptions::new()
+            .write(true)
+            .create_new(true)
+            .custom_flags(libc::O_NOFOLLOW)
+            .mode(0o600)
+            .open(path)?;
+        file.write_all(b"ready\n")?;
+        file.sync_all()?;
+        // `mode(0o600)` at open time is load-bearing on umask-0000 systems;
+        // the explicit set_permissions is belt-and-suspenders for the
+        // umask-permissive case.
         std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))?;
     }
+    #[cfg(not(unix))]
+    {
+        use std::io::Write;
+        let mut file = std::fs::OpenOptions::new()
+            .write(true)
+            .create_new(true)
+            .open(path)?;
+        file.write_all(b"ready\n")?;
+    }
     Ok(())
 }
 
@@ -1385,6 +1417,43 @@ mod tests {
         std::fs::remove_file(&path).unwrap();
     }
 
+    #[cfg(unix)]
+    #[test]
+    fn signal_ready_refuses_preplanted_symlink() {
+        use std::os::unix::fs::symlink;
+
+        let path = signal_ready_test_path("symlink-refused");
+        let decoy = signal_ready_test_path("symlink-refused-target");
+        let _unused = std::fs::remove_file(&path);
+        let _unused = std::fs::remove_file(&decoy);
+
+        // Pre-plant a symlink at `path` pointing at a decoy file an
+        // attacker wants us to write to. The decoy need not exist —
+        // O_NOFOLLOW refuses the open before we ever dereference.
+        symlink(&decoy, &path).unwrap();
+
+        let err = signal_ready(Some(&path)).unwrap_err();
+        let msg = err.to_string();
+        assert!(
+            msg.contains("Too many levels of symbolic links")
+                || msg.contains("symbolic link")
+                || msg.contains("ELOOP")
+                || msg.contains("File exists")
+                || msg.contains("EEXIST")
+                || msg.to_lowercase().contains("symlink"),
+            "unexpected error message for symlink refusal: {msg}"
+        );
+
+        // The decoy file must NOT have been created.
+        assert!(
+            !decoy.exists(),
+            "signal_ready followed the symlink — decoy was written"
+        );
+
+        let _unused = std::fs::remove_file(&path);
+        let _unused = std::fs::remove_file(&decoy);
+    }
+
     #[test]
     fn signal_ready_creates_parent_dirs() {
         let base = signal_ready_test_path("nested-parent");