2 Commits

Author	SHA1	Message	Date
shankar0123	586308ee71	security(signer): bound FileDriver paths with SafeRoot + reject .. (CodeQL #27, CWE-22) ... CodeQL alert #27 (go/path-injection, CWE-22 / CWE-23 / CWE-36) flagged the os.WriteFile sink at internal/crypto/signer/file_driver.go:194 because the outPath flowed from operator-supplied config (CAKeyPath in the local issuer's encrypted config blob -> GenerateOutPath closure -> os.WriteFile) without a containment check. Threat model: Production wiring (cmd/server/main.go) constructs &signer.FileDriver{} and the local-issuer NewConnector wires GenerateOutPath off Config.CAKeyPath. CAKeyPath ships from the encrypted issuer config in PostgreSQL — settable only by an authenticated admin via the API. So the realistic exploit is: (a) Admin compromise -> CAKeyPath set to /etc/passwd -> FileDriver.Generate overwrites system files. (b) Future code path concatenates attacker-controlled fragments into the output path -> classic ../../etc/passwd traversal. Defense in depth: bound the write surface so admin-key-rotation errors and future regressions can't escape into arbitrary filesystem writes. Fix: internal/crypto/signer/file_driver.go gains: - SafeRoot string field on FileDriver. When set, every Load + Generate path MUST resolve under SafeRoot via filepath.Abs + strings.HasPrefix on cleaned paths. - validateSafePath helper that: * rejects empty paths * filepath.Clean()s the input * rejects paths whose cleaned form still contains a literal ".." segment (catches relative paths that escape above their start; absolute paths get collapsed by Clean) * resolves to filepath.Abs and (when SafeRoot non-empty) verifies containment via filepath.Separator-suffixed HasPrefix (the bare-prefix bug — SafeRoot=/var/lib/foo erroneously accepting /var/lib/foobar — has its own regression test below) - Load + Generate now call validateSafePath before any os.ReadFile / os.WriteFile. The validator is in the same function as the sink so CodeQL recognizes it as a guard. Tests (internal/crypto/signer/signer_test.go): TestFileDriver_Load_RejectsParentTraversal — relative path "../../etc/passwd" rejected with parent-directory error. TestFileDriver_Load_RejectsEmptyPath — empty path rejected. TestFileDriver_Generate_RejectsParentTraversal — write side, same pattern. TestFileDriver_SafeRoot_AcceptsContainedPath — happy path: a key file under SafeRoot succeeds. TestFileDriver_SafeRoot_RejectsEscape — absolute path outside SafeRoot rejected (the load-bearing CodeQL pin). TestFileDriver_SafeRoot_RejectsSiblingPrefix — pins the HasPrefix-with-separator subtlety: SafeRoot=/tmp/X must NOT accept /tmp/X-sibling. Verified locally: gofmt: clean. go vet ./...: exit 0. go test -short -count=1 ./internal/crypto/signer/...: ok 1.605s go test -short -count=1 ./internal/connector/issuer/local/...: ok 4.908s (downstream FileDriver consumer) go test -short -count=1 ./internal/service/...: ok 4.029s Backwards-compat: when SafeRoot is unset, only the structural .. + empty-path checks fire — the existing FileDriver call sites in cmd/server/main.go and the existing unit tests pass unchanged. Production wiring SHOULD set SafeRoot via cmd/server/main.go in a follow-up commit (env-var-supplied CERTCTL_CA_KEY_DIR or similar). Reference: https://github.com/certctl-io/certctl/security/code-scanning/27 Closes CodeQL alert #27 (go/path-injection).	2026-05-04 05:04:35 +00:00
Shankar	fdd445c09f	crypto/signer: introduce Signer interface; refactor local issuer to use it ... This is a load-bearing internal refactor with no user-visible behavior change. The new internal/crypto/signer package abstracts CA private-key signing behind a Signer interface (embeds stdlib crypto.Signer + adds Algorithm()). The local issuer now consumes this interface; the historical c.caKey crypto.Signer field is renamed c.caSigner signer.Signer. What landed: * internal/crypto/signer/ — new stdlib-only package - Signer interface: crypto.Signer + Algorithm() - Algorithm enum: RSA-2048, RSA-3072, RSA-4096, ECDSA-P256, ECDSA-P384 - Driver interface: Load / Generate / Name - FileDriver: production driver, wraps file-on-disk PEM, hooks for DirHardener + Marshaler so the local package can inject Bundle 9 keystore.ensureKeyDirSecure + keymem.marshalPrivateKeyAndZeroize - MemoryDriver: in-memory test driver; safe for concurrent use - parse.go: ParsePrivateKey moved here from local.go (PKCS#1, SEC 1, PKCS#8) - 91.6% coverage (gate ≥85) * internal/connector/issuer/local/local.go — refactor - Rename c.caKey crypto.Signer → c.caSigner signer.Signer - Rewire 4 signing call sites: leaf cert (line ~613), CRL (~849), OCSP response (~887), CA bootstrap (~482) — all access the interface; the bootstrap also switches to interface-level Public() + Signer - Wrap freshly-generated and freshly-loaded keys; reject Ed25519 and other unsupported algorithms at load time (was silently accepted before, would have failed at first sign) - Delete the duplicated parsePrivateKey helper (single source of truth now lives in the signer package) - Update the L-014 threat-model comment block (lines 1-29) with a forward-reference paragraph: file-on-disk caveats apply only to FileDriver-backed signers; alternative drivers close that leg - Coverage 86.7 → 86.5 (above CI floor of 86); the 0.2pp drop is mechanical from deleting parsePrivateKey, partially recovered by a new test pinning the Wrap error path * internal/crypto/signer/equivalence_test.go — Phase 3 safety net - RSA byte-strict equality for leaf certs / CRLs / OCSP responses (PKCS#1 v1.5 is deterministic) - ECDSA TBS-strict equality (signature differs because of random k) - Both signatures independently validate against the CA - Negative sentinel proves the equivalence checker isn't trivially- passing * docs/architecture.md — new 'CA Signing Abstraction' section under Security Model, with ASCII diagram of FileDriver / MemoryDriver / future PKCS11Driver / future CloudKMSDriver * Test file mechanical edits (only): - bundle9_coverage_test.go: parsePrivateKey → signer.ParsePrivateKey (function moved, not behavior changed) - local_test.go: append one targeted test (TestSubCA_LoadCAFromDisk_RejectsUnsupportedKeyAlgorithm) that pins the new Wrap error path I introduced — recovers coverage cost of the deletion above What did NOT change (verified empty diffs): * api/openapi.yaml * migrations/ * internal/connector/issuer/interface.go * go.mod / go.sum (no new dependencies; stdlib only) This refactor is the prerequisite for three downstream items: - PKCS#11/HSM driver (V3-Pro) - CRL/OCSP responder (V2) - SSH CA lifecycle (V2) Each of those adds a new signing call site. Doing the abstraction now costs once; deferring would cost three times.	2026-04-28 22:03:55 +00:00