mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-14 05:18:55 +00:00
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).
This commit is contained in:
@@ -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())
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user