diff --git a/.github/scripts/test-security-contracts.sh b/.github/scripts/test-security-contracts.sh index 4ace321e..69b49f27 100644 --- a/.github/scripts/test-security-contracts.sh +++ b/.github/scripts/test-security-contracts.sh @@ -67,4 +67,28 @@ grep -F 'sandboxSSHAgentDenied' dory-core-swift/Sources/DoryVMMKit/DoryVMM.swift grep -F -- '--env-json-stdin' dory-core-swift/Sources/dorydctl/main.swift scripts/dory >/dev/null \ || fail "ephemeral sandbox secrets can no longer avoid process argv" +# Automatic local HTTPS listens on the wildcard address so the privileged :443 bind keeps working; +# admitting loopback peers only is what keeps container backends off the LAN. +for tls_proxy in dory-core-swift/Sources/DorydKit/DoryTLSProxyServer.swift Dory/Net/DoryTLSProxy.swift; do + grep -F 'isLoopbackPeer(client.endpoint)' "$tls_proxy" >/dev/null \ + || fail "$tls_proxy accepts TLS peers without a loopback check" +done + +# Remote daemon mode carries Exec with no in-band authentication and is only ever reachable +# through doryd's SSH tunnel. +grep -F 'is_loopback_listen_addr(&local)' dory-core/agent/src/daemon.rs >/dev/null \ + || fail "dory-agent daemon can serve unauthenticated control on a routable address" +grep -F 'is_loopback_peer(&peer)' dory-core/agent/src/daemon.rs >/dev/null \ + || fail "dory-agent daemon admits non-loopback control peers" + +# The local CA signs certificates the user trusts in their login keychain. +grep -F 'validateCertificateName' Dory/Net/LocalCA.swift >/dev/null \ + || fail "app-side local CA no longer validates certificate names before building SAN arguments" +grep -F 'env:\(passphraseVariable)' \ + Dory/Net/LocalCA.swift dory-core-swift/Sources/DorydKit/LocalCA.swift >/dev/null \ + || fail "PKCS#12 export passphrases are visible in process argv" +if grep -R -E --include='*.swift' 'password: "dory"' Dory dory-core-swift/Sources >/dev/null; then + fail "a production TLS identity uses a hardcoded passphrase" +fi + echo "security contracts: PASS" diff --git a/Dory/Models/AppStore.swift b/Dory/Models/AppStore.swift index ae1fa900..ca01632a 100644 --- a/Dory/Models/AppStore.swift +++ b/Dory/Models/AppStore.swift @@ -2574,6 +2574,13 @@ final class AppStore { } } + nonisolated static func ephemeralIdentityPassword() -> String { + var generator = SystemRandomNumberGenerator() + return (0..<24) + .map { _ in String(format: "%02x", UInt8.random(in: .min ... .max, using: &generator)) } + .joined() + } + private func startTLS() { let table = domainTable let suffix = domainSuffix @@ -2584,8 +2591,11 @@ final class AppStore { let extraSANs = ["*.k8s.\(suffix)", "*.default.k8s.\(suffix)", "*.kube-system.k8s.\(suffix)"] Task { [weak self] in let proxy = await Task.detached { () -> DoryTLSProxy? in - guard let p12 = try? LocalCA().issuePKCS12(domain: suffix, password: "dory", extraSANs: extraSANs) else { return nil } - return DoryTLSProxy(p12Path: p12.path, password: "dory", resolve: { table.backend(for: $0) }) + // The identity lives only for this proxy instance, so its passphrase is generated + // per start rather than shipped in the binary. + let password = Self.ephemeralIdentityPassword() + guard let p12 = try? LocalCA().issuePKCS12(domain: suffix, password: password, extraSANs: extraSANs) else { return nil } + return DoryTLSProxy(p12Path: p12.path, password: password, resolve: { table.backend(for: $0) }) }.value guard let self, let proxy else { return } do { diff --git a/Dory/Net/DoryTLSProxy.swift b/Dory/Net/DoryTLSProxy.swift index f903581f..05be38a9 100644 --- a/Dory/Net/DoryTLSProxy.swift +++ b/Dory/Net/DoryTLSProxy.swift @@ -38,7 +38,30 @@ nonisolated final class DoryTLSProxy: @unchecked Sendable { listener = nil } + /// Whether an inbound peer is on loopback. `NWListener` cannot pin a bind address without + /// losing the privileged `:443` bind (macOS grants an unprivileged low-port bind only on the + /// wildcard address), so automatic HTTPS admits loopback peers only — matching the plaintext + /// reverse proxy's explicit loopback bind — and keeps container backends off the LAN. Anything + /// not recognisably loopback fails closed. + static func isLoopbackPeer(_ endpoint: NWEndpoint) -> Bool { + guard case let .hostPort(host, _) = endpoint else { return false } + switch host { + case let .ipv4(address): + return address.isLoopback + case let .ipv6(address): + return address.isLoopback || address.asIPv4?.isLoopback == true + case let .name(name, _): + return name.lowercased() == "localhost" + @unknown default: + return false + } + } + private func accept(_ client: NWConnection) { + guard Self.isLoopbackPeer(client.endpoint) else { + client.cancel() + return + } client.start(queue: queue) readHead(client, buffer: Data()) } diff --git a/Dory/Net/LocalCA.swift b/Dory/Net/LocalCA.swift index 7ad6d82d..9f3d4f72 100644 --- a/Dory/Net/LocalCA.swift +++ b/Dory/Net/LocalCA.swift @@ -1,9 +1,11 @@ +import Darwin import Foundation nonisolated enum ShellError: Error, Sendable { case launchFailed(String) case nonZeroExit(Int32, String) case toolNotFound(String) + case invalidArgument(String) } nonisolated enum Shell { @@ -51,11 +53,17 @@ nonisolated enum Shell { } @discardableResult - static func run(_ launchPath: String, _ arguments: [String], cwd: URL? = nil) throws -> String { + static func run( + _ launchPath: String, + _ arguments: [String], + cwd: URL? = nil, + environment: [String: String]? = nil + ) throws -> String { let process = Process() process.executableURL = URL(fileURLWithPath: launchPath) process.arguments = arguments if let cwd { process.currentDirectoryURL = cwd } + if let environment { process.environment = environment } let output = Pipe() process.standardOutput = output process.standardError = output @@ -100,18 +108,48 @@ nonisolated struct LocalCA: Sendable { guard let openssl = opensslPath else { throw ShellError.toolNotFound("openssl") } if caExists { return } try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) - try Shell.run(openssl, [ - "req", "-x509", "-newkey", "ec", "-pkeyopt", "ec_paramgen_curve:prime256v1", "-nodes", - "-keyout", caKey.path, "-out", caCertificate.path, "-days", "3650", - "-subj", "/CN=Dory Local CA/O=Dory", - "-addext", "basicConstraints=critical,CA:TRUE", - "-addext", "keyUsage=critical,keyCertSign,cRLSign", - ]) + try FileManager.default.setAttributes([.posixPermissions: 0o700], ofItemAtPath: directory.path) + // This key signs certificates the user is asked to trust in their login keychain, so it is + // created 0600 via umask rather than chmod-after: it is never briefly world-readable. + let previousMask = umask(0o177) + do { + try Shell.run(openssl, [ + "req", "-x509", "-newkey", "ec", "-pkeyopt", "ec_paramgen_curve:prime256v1", "-nodes", + "-keyout", caKey.path, "-out", caCertificate.path, "-days", "3650", + "-subj", "/CN=Dory Local CA/O=Dory", + "-addext", "basicConstraints=critical,CA:TRUE", + "-addext", "keyUsage=critical,keyCertSign,cRLSign", + ]) + umask(previousMask) + } catch { + umask(previousMask) + throw error + } + try FileManager.default.setAttributes([.posixPermissions: 0o600], ofItemAtPath: caKey.path) + try FileManager.default.setAttributes([.posixPermissions: 0o644], ofItemAtPath: caCertificate.path) + } + + /// Certificate names reach openssl inside a comma-separated SAN string and a file path, so a + /// name carrying a comma or a path separator could inject extra SAN entries or redirect the + /// written key. Domains come from user settings and container labels; validate them all. + static func validateCertificateName(_ name: String) throws { + var value = name + if value.hasPrefix("*.") { value = String(value.dropFirst(2)) } + let labels = value.split(separator: ".", omittingEmptySubsequences: false) + guard !labels.isEmpty else { throw ShellError.invalidArgument("certificate name: \(name)") } + for label in labels { + guard !label.isEmpty, + label.allSatisfy({ $0.isASCII && ($0.isLetter || $0.isNumber || $0 == "-") }) else { + throw ShellError.invalidArgument("certificate name: \(name)") + } + } } @discardableResult func issue(domain: String, extraSANs: [String] = []) throws -> CertificatePair { guard let openssl = opensslPath else { throw ShellError.toolNotFound("openssl") } + try Self.validateCertificateName(domain) + for name in extraSANs where !name.isEmpty { try Self.validateCertificateName(name) } try ensureCA() let certificate = directory.appendingPathComponent("\(domain).crt") let key = directory.appendingPathComponent("\(domain).key") @@ -131,6 +169,8 @@ nonisolated struct LocalCA: Sendable { "x509", "-req", "-in", csr.path, "-CA", caCertificate.path, "-CAkey", caKey.path, "-CAcreateserial", "-out", certificate.path, "-days", "825", "-copy_extensions", "copyall", ]) + try FileManager.default.setAttributes([.posixPermissions: 0o600], ofItemAtPath: key.path) + try FileManager.default.setAttributes([.posixPermissions: 0o644], ofItemAtPath: certificate.path) return CertificatePair(certificate: certificate, privateKey: key) } @@ -141,11 +181,17 @@ nonisolated struct LocalCA: Sendable { guard let openssl = opensslPath else { throw ShellError.toolNotFound("openssl") } let pair = try issue(domain: domain, extraSANs: extraSANs) let p12 = directory.appendingPathComponent("\(domain).p12") + // Pass the export passphrase through the environment rather than argv, so it is not + // visible in `ps` output while openssl runs. + let passphraseVariable = "DORY_LOCALCA_P12_PASS" + var childEnvironment = ProcessInfo.processInfo.environment + childEnvironment[passphraseVariable] = password try Shell.run(openssl, [ "pkcs12", "-export", "-inkey", pair.privateKey.path, "-in", pair.certificate.path, "-certfile", caCertificate.path, "-out", p12.path, - "-passout", "pass:\(password)", "-legacy", - ]) + "-passout", "env:\(passphraseVariable)", "-legacy", + ], environment: childEnvironment) + try FileManager.default.setAttributes([.posixPermissions: 0o600], ofItemAtPath: p12.path) return p12 } diff --git a/DoryTests/LocalCATests.swift b/DoryTests/LocalCATests.swift index d2ef97a3..8c70264e 100644 --- a/DoryTests/LocalCATests.swift +++ b/DoryTests/LocalCATests.swift @@ -28,6 +28,41 @@ struct LocalCATests { #expect(text.contains("Dory Local CA")) } + @Test func caAndLeafPrivateKeysAreOwnerReadableOnly() throws { + let directory = URL(fileURLWithPath: NSTemporaryDirectory()).appendingPathComponent("dory-ca-perms-\(UUID().uuidString)") + defer { try? FileManager.default.removeItem(at: directory) } + + let ca = LocalCA(directory: directory) + guard ca.opensslPath != nil else { return } + + try ca.ensureCA() + let pair = try ca.issue(domain: "web.dory.local") + let p12 = try ca.issuePKCS12(domain: "dory.local", password: AppStore.ephemeralIdentityPassword()) + + for path in [ca.caKey.path, pair.privateKey.path, p12.path] { + let mode = try #require( + FileManager.default.attributesOfItem(atPath: path)[.posixPermissions] as? NSNumber + ).intValue + #expect(mode & 0o077 == 0) + } + } + + @Test func certificateNamesThatCouldInjectSANEntriesOrPathsAreRejected() throws { + for name in ["dory.local,DNS:evil.example.com", "../../etc/dory", "dory.local/../evil", "", "*.", "a..b"] { + #expect(throws: (any Error).self) { try LocalCA.validateCertificateName(name) } + } + for name in ["dory.local", "*.dory.local", "*.default.k8s.dory.local", "my-project.local"] { + try LocalCA.validateCertificateName(name) + } + } + + @Test func ephemeralIdentityPasswordIsRandomAndNotAConstant() { + let first = AppStore.ephemeralIdentityPassword() + let second = AppStore.ephemeralIdentityPassword() + #expect(first.count == 48) + #expect(first != second) + } + @Test func localTrustParserAcceptsOnlyAValidDoryCA() throws { let directory = URL(fileURLWithPath: NSTemporaryDirectory()) .appendingPathComponent("dory-trust-parser-\(UUID().uuidString)") diff --git a/dory-core-swift/Sources/DorydKit/DoryTLSProxyServer.swift b/dory-core-swift/Sources/DorydKit/DoryTLSProxyServer.swift index 45da5b12..7f9508c5 100644 --- a/dory-core-swift/Sources/DorydKit/DoryTLSProxyServer.swift +++ b/dory-core-swift/Sources/DorydKit/DoryTLSProxyServer.swift @@ -155,7 +155,33 @@ public final class DoryTLSProxyServer: @unchecked Sendable { } } + /// Whether an inbound peer is on loopback. + /// + /// `NWListener` has no bind-address knob equivalent to the plaintext proxy's explicit + /// `127.0.0.1` bind, and pinning it to loopback would break the privileged `:443` bind (macOS + /// lets an unprivileged process bind a low port only on the wildcard address). Automatic HTTPS + /// therefore listens on the wildcard and admits loopback peers only — the policy + /// `LoopbackTCPForwarder` already applies to the standard ports — so container backends are not + /// reachable from the LAN. Anything not recognisably loopback fails closed. + static func isLoopbackPeer(_ endpoint: NWEndpoint) -> Bool { + guard case let .hostPort(host, _) = endpoint else { return false } + switch host { + case let .ipv4(address): + return address.isLoopback + case let .ipv6(address): + return address.isLoopback || address.asIPv4?.isLoopback == true + case let .name(name, _): + return DoryHTTPProxyServer.isLoopbackHost(name) + @unknown default: + return false + } + } + private func accept(_ client: NWConnection) { + guard Self.isLoopbackPeer(client.endpoint) else { + client.cancel() + return + } guard let lease = connectionBudget.tryAcquire() else { client.cancel() return diff --git a/dory-core-swift/Tests/DorydKitTests/DoryTLSProxyServerTests.swift b/dory-core-swift/Tests/DorydKitTests/DoryTLSProxyServerTests.swift index 43de6f11..0035946a 100644 --- a/dory-core-swift/Tests/DorydKitTests/DoryTLSProxyServerTests.swift +++ b/dory-core-swift/Tests/DorydKitTests/DoryTLSProxyServerTests.swift @@ -1,5 +1,6 @@ import Darwin @testable import DorydKit +import Network import XCTest final class DoryTLSProxyServerTests: XCTestCase { @@ -118,6 +119,27 @@ final class DoryTLSProxyServerTests: XCTestCase { XCTAssertEqual(response, "hello custom tls") XCTAssertTrue(backend.lastRequest.contains("Host: admin.myproject.local")) } + + func testAdmitsOnlyLoopbackPeers() throws { + let port = NWEndpoint.Port(rawValue: 51234)! + XCTAssertTrue(DoryTLSProxyServer.isLoopbackPeer(.hostPort(host: .ipv4(.loopback), port: port))) + XCTAssertTrue(DoryTLSProxyServer.isLoopbackPeer(.hostPort(host: .ipv6(.loopback), port: port))) + XCTAssertTrue(DoryTLSProxyServer.isLoopbackPeer( + .hostPort(host: .ipv6(IPv6Address("::ffff:127.0.0.1")!), port: port) + )) + XCTAssertTrue(DoryTLSProxyServer.isLoopbackPeer(.hostPort(host: .name("localhost", nil), port: port))) + + XCTAssertFalse(DoryTLSProxyServer.isLoopbackPeer( + .hostPort(host: .ipv4(IPv4Address("192.168.1.20")!), port: port) + )) + XCTAssertFalse(DoryTLSProxyServer.isLoopbackPeer( + .hostPort(host: .ipv6(IPv6Address("fe80::1")!), port: port) + )) + XCTAssertFalse(DoryTLSProxyServer.isLoopbackPeer( + .hostPort(host: .name("attacker.example.com", nil), port: port) + )) + XCTAssertFalse(DoryTLSProxyServer.isLoopbackPeer(.unix(path: "/tmp/dory.sock"))) + } } private func availableTCPPort() throws -> UInt16 { diff --git a/dory-core/agent/src/daemon.rs b/dory-core/agent/src/daemon.rs index fc0f6522..8dde6940 100644 --- a/dory-core/agent/src/daemon.rs +++ b/dory-core/agent/src/daemon.rs @@ -3,6 +3,7 @@ //! stack SSH-tunnels a channel to this listener and speaks the identical `handshake + mux + dispatch` //! it speaks over VZ vsock. No vsock here, so it is portable and exercised on the host in tests. +use std::net::SocketAddr; use std::sync::Arc; use dory_proto::handshake::{handshake, Hello}; @@ -13,10 +14,40 @@ use tokio::net::TcpListener; use crate::dispatch::agent_build; use crate::handler::handle; +/// The control protocol carries `Exec`, and daemon mode has no in-band authentication: it is +/// reachable only through the SSH tunnel doryd opens to the loopback listener. Enforce that +/// contract at both ends — refuse to serve a routable listener, and drop non-loopback peers — so a +/// misconfigured `--daemon ` cannot turn into unauthenticated remote execution. +pub fn is_loopback_listen_addr(address: &SocketAddr) -> bool { + address.ip().is_loopback() +} + +pub fn is_loopback_peer(peer: &SocketAddr) -> bool { + match peer.ip() { + std::net::IpAddr::V4(address) => address.is_loopback(), + std::net::IpAddr::V6(address) => { + address.is_loopback() || address.to_ipv4_mapped().is_some_and(|v4| v4.is_loopback()) + } + } +} + /// Accept loop over a bound TCP listener; one control mux per connection. pub async fn serve(listener: TcpListener) -> std::io::Result<()> { + let local = listener.local_addr()?; + if !is_loopback_listen_addr(&local) { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "dory-agent daemon refuses to serve unauthenticated control on {local}: \ + bind a loopback address and reach it through an SSH tunnel" + ), + )); + } loop { - let (stream, _peer) = listener.accept().await?; + let (stream, peer) = listener.accept().await?; + if !is_loopback_peer(&peer) { + continue; + } tokio::spawn(async move { serve_conn(stream).await; }); @@ -41,3 +72,65 @@ where Arc::new(|req: Vec| Box::pin(async move { handle(&req).await }) as HandlerFuture); let _mux = Mux::start(stream, handler); } + +#[cfg(test)] +mod tests { + use super::*; + use std::net::{Ipv4Addr, Ipv6Addr}; + + #[test] + fn only_loopback_listeners_may_serve_unauthenticated_control() { + assert!(is_loopback_listen_addr(&SocketAddr::from(( + Ipv4Addr::LOCALHOST, + 2377 + )))); + assert!(is_loopback_listen_addr(&SocketAddr::from(( + Ipv6Addr::LOCALHOST, + 2377 + )))); + assert!(!is_loopback_listen_addr(&SocketAddr::from(( + Ipv4Addr::UNSPECIFIED, + 2377 + )))); + assert!(!is_loopback_listen_addr(&SocketAddr::from(( + Ipv6Addr::UNSPECIFIED, + 2377 + )))); + assert!(!is_loopback_listen_addr(&SocketAddr::from(( + Ipv4Addr::new(203, 0, 113, 7), + 2377 + )))); + } + + #[test] + fn only_loopback_peers_are_admitted() { + assert!(is_loopback_peer(&SocketAddr::from(( + Ipv4Addr::new(127, 0, 0, 2), + 40000 + )))); + assert!(is_loopback_peer(&SocketAddr::from(( + Ipv6Addr::LOCALHOST, + 40000 + )))); + // IPv4-mapped loopback arrives on a dual-stack listener as ::ffff:127.0.0.1. + assert!(is_loopback_peer(&SocketAddr::from(( + Ipv4Addr::LOCALHOST.to_ipv6_mapped(), + 40000 + )))); + assert!(!is_loopback_peer(&SocketAddr::from(( + Ipv4Addr::new(192, 168, 1, 10), + 40000 + )))); + assert!(!is_loopback_peer(&SocketAddr::from(( + Ipv4Addr::new(192, 168, 1, 10).to_ipv6_mapped(), + 40000 + )))); + } + + #[tokio::test] + async fn serve_refuses_a_routable_listener() { + let listener = TcpListener::bind((Ipv4Addr::UNSPECIFIED, 0)).await.unwrap(); + let error = serve(listener).await.expect_err("wildcard bind must fail"); + assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput); + } +} diff --git a/dory-core/agent/src/main.rs b/dory-core/agent/src/main.rs index 79e293b3..aaf6e122 100644 --- a/dory-core/agent/src/main.rs +++ b/dory-core/agent/src/main.rs @@ -39,7 +39,8 @@ async fn main() -> std::io::Result<()> { /// `--daemon ` selects remote-VPS daemon mode; absent, the guest PID-1 path runs. /// Daemon mode exposes `Exec` with no in-band authentication; it relies entirely on the transport -/// (loopback default + SSH tunnel). Never bind it to a routable address. +/// (loopback default + SSH tunnel), which `daemon::serve` enforces by refusing a routable listener +/// and dropping non-loopback peers. fn daemon_addr() -> Option { let args: Vec = std::env::args().collect(); let idx = args.iter().position(|a| a == "--daemon")?;