diff --git a/internal/crypto/signer/file_driver.go b/internal/crypto/signer/file_driver.go index b8170d1..dfe013b 100644 --- a/internal/crypto/signer/file_driver.go +++ b/internal/crypto/signer/file_driver.go @@ -13,6 +13,7 @@ import ( "fmt" "os" "path/filepath" + "strings" ) // FileDriver materializes a Signer from a PEM-encoded private key on @@ -64,11 +65,97 @@ type FileDriver struct { // production. The local package's NewConnector wires this to // return the configured CAKeyPath. GenerateOutPath func(alg Algorithm) (string, error) + + // SafeRoot, if non-empty, restricts every Load + Generate path to + // the absolute filesystem subtree rooted at SafeRoot. Closes CodeQL + // go/path-injection (CWE-22 / CWE-23 / CWE-36): even though the + // driver's path inputs flow from operator-authenticated config + // (admin-only API surface), an admin compromise could otherwise + // write `/etc/passwd` or read `/root/.ssh/id_rsa` via the driver. + // SafeRoot bounds the blast radius. + // + // Validation semantics (validateSafePath): + // + // 1. The supplied path is cleaned (filepath.Clean) to collapse + // ./ and ../ sequences in their literal form. + // 2. If the cleaned path is relative, it's resolved against the + // current working directory via filepath.Abs. + // 3. If SafeRoot is set, the absolute path MUST be SafeRoot or + // a descendant. We use filepath.Rel + strings.HasPrefix on + // the cleaned absolute paths so symlink games (../ disguised + // as a symlink target) inside SafeRoot are bounded by + // SafeRoot's parent permissions, not by the validator. + // + // When SafeRoot is empty, the path is still cleaned + checked for + // the literal ".." element as a baseline defense-in-depth measure; + // callers that don't constrain to a root still get path-traversal + // rejection. + // + // Production wiring SHOULD set SafeRoot. The local-issuer config + // surface accepts CAKeyPath as an absolute path; cmd/server/main.go + // can derive SafeRoot from CERTCTL_CA_KEY_DIR (operator-trusted env + // var, never user-supplied) or from the parent of the configured + // path at issuer-registration time. + SafeRoot string } // Name implements Driver. func (d *FileDriver) Name() string { return "file" } +// validateSafePath enforces the CWE-22 / CWE-23 / CWE-36 path-traversal +// defense documented on FileDriver.SafeRoot. Returns the cleaned +// absolute path on success; an explicit error on rejection. Rejects: +// +// - empty paths +// - paths whose cleaned form contains a literal ".." segment (defense +// against attacker-controlled fragments concatenated upstream — the +// filepath.Clean() before this check collapses any "..", so a +// remaining ".." is structural) +// - when SafeRoot is non-empty: any path whose cleaned absolute form +// is not SafeRoot or a descendant +// +// Apply in every Load + Generate path before any os.ReadFile / +// os.WriteFile call. CodeQL's taint tracker recognizes the validator +// in the same function as the sink and closes the alert. +func (d *FileDriver) validateSafePath(path string) (string, error) { + if path == "" { + return "", errors.New("path is empty") + } + cleaned := filepath.Clean(path) + // Reject any path whose cleaned form still contains a `..` element. + // filepath.Clean collapses `./` and `../` sequences relative to the + // path's structure, so a remaining `..` after Clean means the path + // is rooted (or attempts to escape) above whatever the caller + // intended. + for _, segment := range strings.Split(filepath.ToSlash(cleaned), "/") { + if segment == ".." { + return "", fmt.Errorf("path %q contains parent-directory segment", path) + } + } + abs, err := filepath.Abs(cleaned) + if err != nil { + return "", fmt.Errorf("resolve absolute path %q: %w", path, err) + } + if d.SafeRoot != "" { + safeRoot, err := filepath.Abs(filepath.Clean(d.SafeRoot)) + if err != nil { + return "", fmt.Errorf("resolve SafeRoot %q: %w", d.SafeRoot, err) + } + // Require the cleaned absolute path to be safeRoot itself or a + // strict descendant. The += string.Separator on safeRoot is + // load-bearing — without it a SafeRoot of "/var/lib/foo" would + // erroneously accept "/var/lib/foobar" as a prefix match. + safeRootSlash := safeRoot + if !strings.HasSuffix(safeRootSlash, string(filepath.Separator)) { + safeRootSlash += string(filepath.Separator) + } + if abs != safeRoot && !strings.HasPrefix(abs, safeRootSlash) { + return "", fmt.Errorf("path %q resolves outside SafeRoot %q", path, d.SafeRoot) + } + } + return abs, nil +} + // Load implements Driver. It reads the PEM file at path, decodes the // first PEM block, parses it via the package's parsePrivateKey // (which handles PKCS#1 / SEC 1 / PKCS#8), and wraps the resulting @@ -78,28 +165,33 @@ func (d *FileDriver) Name() string { return "file" } // No key bytes are logged — only the path and (on success) the // inferred Algorithm. func (d *FileDriver) Load(ctx context.Context, path string) (Signer, error) { - if path == "" { - return nil, errors.New("signer.FileDriver.Load: empty path") - } if err := ctx.Err(); err != nil { return nil, fmt.Errorf("signer.FileDriver.Load: %w", err) } - - pemBytes, err := os.ReadFile(path) + // CWE-22 path-traversal defense — reject paths that escape SafeRoot + // (when set) OR contain literal ".." segments. The validator is in + // the same function as the os.ReadFile sink so CodeQL recognizes + // the sanitizer in-scope. + safePath, err := d.validateSafePath(path) if err != nil { - return nil, fmt.Errorf("signer.FileDriver.Load: read %q: %w", path, err) + return nil, fmt.Errorf("signer.FileDriver.Load: %w", err) + } + + pemBytes, err := os.ReadFile(safePath) + if err != nil { + return nil, fmt.Errorf("signer.FileDriver.Load: read %q: %w", safePath, err) } block, _ := pem.Decode(pemBytes) if block == nil { - return nil, fmt.Errorf("signer.FileDriver.Load: %q is not PEM", path) + return nil, fmt.Errorf("signer.FileDriver.Load: %q is not PEM", safePath) } key, err := parsePrivateKey(block) if err != nil { - return nil, fmt.Errorf("signer.FileDriver.Load: parse %q: %w", path, err) + return nil, fmt.Errorf("signer.FileDriver.Load: parse %q: %w", safePath, err) } wrapped, err := Wrap(key) if err != nil { - return nil, fmt.Errorf("signer.FileDriver.Load: wrap %q: %w", path, err) + return nil, fmt.Errorf("signer.FileDriver.Load: wrap %q: %w", safePath, err) } return wrapped, nil } @@ -133,10 +225,19 @@ func (d *FileDriver) Generate(ctx context.Context, alg Algorithm) (Signer, strin return nil, "", fmt.Errorf("signer.FileDriver.Generate: resolve out path: %w", err) } + // CWE-22 path-traversal defense — reject paths that escape SafeRoot + // (when set) OR contain literal ".." segments. The validator is in + // the same function as the os.WriteFile sink below so CodeQL + // recognizes the sanitizer in-scope. + safeOut, err := d.validateSafePath(outPath) + if err != nil { + return nil, "", fmt.Errorf("signer.FileDriver.Generate: %w", err) + } + // Harden the destination directory BEFORE generating the key. If // the directory check fails we bail without touching cryptography. - if err := d.DirHardener(filepath.Dir(outPath)); err != nil { - return nil, "", fmt.Errorf("signer.FileDriver.Generate: harden dir for %q: %w", outPath, err) + if err := d.DirHardener(filepath.Dir(safeOut)); err != nil { + return nil, "", fmt.Errorf("signer.FileDriver.Generate: harden dir for %q: %w", safeOut, err) } // Generate the key for the requested algorithm. @@ -191,15 +292,15 @@ func (d *FileDriver) Generate(ctx context.Context, alg Algorithm) (Signer, strin // Write 0o600 — owner-read-write only. Any read by group/other is // a configuration regression; the dir 0700 above prevents // enumeration of the file's existence. - if err := os.WriteFile(outPath, pemBytes, 0o600); err != nil { - return nil, "", fmt.Errorf("signer.FileDriver.Generate: write %q: %w", outPath, err) + if err := os.WriteFile(safeOut, pemBytes, 0o600); err != nil { + return nil, "", fmt.Errorf("signer.FileDriver.Generate: write %q: %w", safeOut, err) } wrapped, err := Wrap(signerKey) if err != nil { return nil, "", fmt.Errorf("signer.FileDriver.Generate: wrap: %w", err) } - return wrapped, outPath, nil + return wrapped, safeOut, nil } func rsaBitsFor(a Algorithm) int { diff --git a/internal/crypto/signer/signer_test.go b/internal/crypto/signer/signer_test.go index 55a7d66..8799a6f 100644 --- a/internal/crypto/signer/signer_test.go +++ b/internal/crypto/signer/signer_test.go @@ -777,3 +777,129 @@ func TestSigner_AlgorithmMatchesKey(t *testing.T) { } } } + +// TestFileDriver_Load_RejectsParentTraversal pins the CWE-22 defense +// for FileDriver.Load — a relative path that escapes its origin via +// `..` segments (and stays unresolved after Clean) is rejected. Closes +// CodeQL #27 on the read side. +// +// Note: filepath.Clean("/abs/.../etc/passwd") collapses to +// "/etc/passwd" — a perfectly clean absolute path with no surviving +// `..`. The relative-`..`-escape test below catches the case Clean +// CAN'T resolve; the SafeRoot tests below catch the absolute-path +// containment case. +func TestFileDriver_Load_RejectsParentTraversal(t *testing.T) { + d := &signer.FileDriver{} + _, err := d.Load(context.Background(), "../../etc/passwd") + if err == nil { + t.Fatal("Load with relative .. escape should be rejected") + } + if !strings.Contains(err.Error(), "parent-directory") { + t.Fatalf("error should mention parent-directory, got %q", err.Error()) + } +} + +// TestFileDriver_Load_RejectsEmptyPath pins the empty-path rejection +// (was inline before validateSafePath; now lives in the validator). +func TestFileDriver_Load_RejectsEmptyPath(t *testing.T) { + d := &signer.FileDriver{} + _, err := d.Load(context.Background(), "") + if err == nil { + t.Fatal("Load with empty path should error") + } + if !strings.Contains(err.Error(), "empty") { + t.Fatalf("error should mention empty path, got %q", err.Error()) + } +} + +// TestFileDriver_Generate_RejectsParentTraversal pins the CWE-22 defense +// for FileDriver.Generate — a relative path that escapes its origin +// via `..` (and stays unresolved after Clean) is rejected before any +// keygen happens. Closes CodeQL #27 on the write side. +func TestFileDriver_Generate_RejectsParentTraversal(t *testing.T) { + d := &signer.FileDriver{ + DirHardener: func(_ string) error { return nil }, + GenerateOutPath: func(_ signer.Algorithm) (string, error) { + return "../../etc/passwd", nil + }, + } + _, _, err := d.Generate(context.Background(), signer.AlgorithmECDSAP256) + if err == nil { + t.Fatal("Generate with relative .. escape should be rejected") + } + if !strings.Contains(err.Error(), "parent-directory") { + t.Fatalf("error should mention parent-directory, got %q", err.Error()) + } +} + +// TestFileDriver_SafeRoot_AcceptsContainedPath pins the SafeRoot +// containment positive case — a path under SafeRoot succeeds. +func TestFileDriver_SafeRoot_AcceptsContainedPath(t *testing.T) { + dir := t.TempDir() + d := &signer.FileDriver{ + SafeRoot: dir, + DirHardener: func(_ string) error { return nil }, + GenerateOutPath: func(_ signer.Algorithm) (string, error) { + return filepath.Join(dir, "ok.key"), nil + }, + } + _, path, err := d.Generate(context.Background(), signer.AlgorithmECDSAP256) + if err != nil { + t.Fatalf("Generate under SafeRoot should succeed: %v", err) + } + if !strings.HasPrefix(path, dir) { + t.Fatalf("returned path %q should be under SafeRoot %q", path, dir) + } +} + +// TestFileDriver_SafeRoot_RejectsEscape pins the SafeRoot containment +// negative case — a path outside SafeRoot is rejected. Without this +// pin, an admin-compromised CAKeyPath of `/etc/passwd` would write +// system files. +func TestFileDriver_SafeRoot_RejectsEscape(t *testing.T) { + dir := t.TempDir() + d := &signer.FileDriver{ + SafeRoot: dir, + DirHardener: func(_ string) error { return nil }, + GenerateOutPath: func(_ signer.Algorithm) (string, error) { + // Absolute path outside the SafeRoot directory. + return "/tmp/escaped-keys/key.pem", nil + }, + } + _, _, err := d.Generate(context.Background(), signer.AlgorithmECDSAP256) + if err == nil { + t.Fatal("Generate outside SafeRoot should be rejected") + } + if !strings.Contains(err.Error(), "outside SafeRoot") { + t.Fatalf("error should mention SafeRoot, got %q", err.Error()) + } +} + +// TestFileDriver_SafeRoot_RejectsSiblingPrefix pins the load-bearing +// detail: a SafeRoot of "/var/lib/foo" must NOT accept "/var/lib/foobar". +// The naive strings.HasPrefix(abs, safeRoot) check fails this case; +// the validator appends a path separator to prevent the bug. +func TestFileDriver_SafeRoot_RejectsSiblingPrefix(t *testing.T) { + root := t.TempDir() // e.g. /tmp/TestSafeRootSibling12345/001 + // sibling has the same prefix but is NOT a descendant of root. + sibling := root + "-sibling" + if err := os.MkdirAll(sibling, 0o700); err != nil { + t.Fatalf("mkdir sibling: %v", err) + } + t.Cleanup(func() { _ = os.RemoveAll(sibling) }) + + d := &signer.FileDriver{ + SafeRoot: root, + DirHardener: func(_ string) error { return nil }, + GenerateOutPath: func(_ signer.Algorithm) (string, error) { + return filepath.Join(sibling, "key.pem"), nil + }, + } + _, _, err := d.Generate(context.Background(), signer.AlgorithmECDSAP256) + if err == nil { + t.Fatal("Generate into sibling-prefix path should be rejected") + } + if !strings.Contains(err.Error(), "outside SafeRoot") { + t.Fatalf("error should mention SafeRoot, got %q", err.Error()) + } +}