From 87f447c1fbf02a6c2b884f70f2e8e9f1ea255e11 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Wed, 27 May 2026 18:22:11 -0700 Subject: [PATCH] fix(pilot-ca): validate beacon hostname to prevent path traversal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit issueBeacon previously accepted any non-empty hostname and built the output paths via filepath.Join(outDir, hostname+".crt"). A hostname like "evil/../host" or "../escape" would write the leaf cert + key outside outDir, silently, while still printing success. The leaf cert itself was also malformed (invalid SAN/DNSName). Add a strict DNS-name allowlist regex (lowercase letters, digits, hyphen, dot only — labels <=63 chars, total <=253) and apply it at the very top of issueBeacon, before any filepath.Join touches outDir. Tests: - TestIssueBeacon_HostnameWithSlash_Rejected (renamed from the CurrentBehavior pin) now asserts rejection + that outDir stays empty. - TestIssueBeacon_HostnameWithParentDir_Rejected covers bare "..", "../escape", "foo/../bar", and a Windows-style "evil\\..\\host". - TestIssueBeacon_ValidHostname_Succeeds pins the happy path on a normal multi-label DNS name. All existing tests still pass under -race. --- main.go | 33 ++++++++++++++++-- zz_branches_test.go | 85 +++++++++++++++++++++++++++++++++++++-------- 2 files changed, 101 insertions(+), 17 deletions(-) diff --git a/main.go b/main.go index 8ec4330..87de3e8 100644 --- a/main.go +++ b/main.go @@ -44,6 +44,7 @@ import ( "math/big" "os" "path/filepath" + "regexp" "time" ) @@ -53,6 +54,34 @@ const ( caCommonName = "Pilot Protocol Root CA" ) +// hostnameRE is a strict DNS-name regex applied to issue-beacon's +// hostname argument. We restrict to lowercase letters, digits, and +// hyphens (with dot separators) — the same shape Caddy and most TLS +// stacks accept as a SAN DNSName. The strict allowlist guarantees no +// path separator (`/`, `\`), no parent-dir token (`..`), and no +// shell-metacharacter ever reaches filepath.Join when we build +// /.{key,crt}. +// +// Length is capped at 253 (the DNS hostname maximum) and each label +// at 63 (the DNS label maximum). +var hostnameRE = regexp.MustCompile(`^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?(\.[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?)*$`) + +// validateHostname enforces hostnameRE and the 253-char total length +// cap. Returns a clear error for the issue-beacon caller — never +// silently accepts a name that could escape outDir via filepath.Join. +func validateHostname(hostname string) error { + if hostname == "" { + return fmt.Errorf("hostname required") + } + if len(hostname) > 253 { + return fmt.Errorf("hostname %q exceeds 253-char DNS limit", hostname) + } + if !hostnameRE.MatchString(hostname) { + return fmt.Errorf("hostname %q is not a valid DNS name (allowed: lowercase letters, digits, hyphen, dot; no path separators, no '..')", hostname) + } + return nil +} + func main() { flag.Usage = func() { fmt.Fprintln(os.Stderr, "usage: pilot-ca [args...]") @@ -156,8 +185,8 @@ func initRoot(outDir string) error { // P-256 ECDSA keypair + leaf cert chained to the root. Leaf cert // validity is 90 days; re-issue before expiry. func issueBeacon(rootDir, hostname, outDir string) error { - if hostname == "" { - return fmt.Errorf("hostname required") + if err := validateHostname(hostname); err != nil { + return err } rootCert, rootKey, err := loadRoot(rootDir) if err != nil { diff --git a/zz_branches_test.go b/zz_branches_test.go index 8600602..e22e284 100644 --- a/zz_branches_test.go +++ b/zz_branches_test.go @@ -120,26 +120,81 @@ func TestVerifyChain_LeafParseError(t *testing.T) { } } -// TestIssueBeacon_HostnameWithSlash documents current behavior: the tool -// builds /.{key,crt} via filepath.Join, so a hostname -// containing a path separator silently writes files in a sibling -// directory rather than failing. Pinning this so a future fix flips -// the assertion intentionally. -// -// NOTE: as of this writing, issueBeacon does NOT validate hostname -// characters beyond emptiness. If/when that lands, update this test -// to expect an error. -func TestIssueBeacon_HostnameWithSlash_CurrentBehavior(t *testing.T) { +// TestIssueBeacon_HostnameWithSlash_Rejected pins the path-traversal +// fix: hostnames containing '/' (and the parent-dir token '..') must +// be rejected before any filepath.Join touches outDir, so a leaf cert +// can never be written outside the operator-chosen directory. +func TestIssueBeacon_HostnameWithSlash_Rejected(t *testing.T) { t.Parallel() rootDir := t.TempDir() if err := initRoot(rootDir); err != nil { t.Fatalf("initRoot: %v", err) } outDir := t.TempDir() - // Hostname with a slash — issueBeacon may or may not reject this. - // We don't assert success or failure; we assert it doesn't panic - // and produces a deterministic outcome. err := issueBeacon(rootDir, "evil/../host", outDir) - // Document current behavior in test output for the maintainer. - t.Logf("issueBeacon('evil/../host') -> err=%v", err) + if err == nil { + t.Fatal("expected error for hostname with slash; got nil") + } + if !strings.Contains(err.Error(), "hostname") { + t.Errorf("error = %q; want it to mention 'hostname'", err.Error()) + } + // And no leaf files should have been created anywhere. + entries, readErr := os.ReadDir(outDir) + if readErr != nil { + t.Fatalf("read outDir: %v", readErr) + } + if len(entries) != 0 { + t.Errorf("outDir not empty after rejected hostname: %v", entries) + } +} + +// TestIssueBeacon_HostnameWithParentDir_Rejected explicitly covers a +// bare '..' traversal attempt — even with no slash present, '..' must +// be rejected because filepath.Join would resolve it relative to outDir. +func TestIssueBeacon_HostnameWithParentDir_Rejected(t *testing.T) { + t.Parallel() + rootDir := t.TempDir() + if err := initRoot(rootDir); err != nil { + t.Fatalf("initRoot: %v", err) + } + outDir := t.TempDir() + for _, host := range []string{"..", "../escape", "foo/../bar", `evil\..\host`} { + err := issueBeacon(rootDir, host, outDir) + if err == nil { + t.Errorf("hostname %q: expected error; got nil", host) + continue + } + if !strings.Contains(err.Error(), "hostname") { + t.Errorf("hostname %q: error = %q; want it to mention 'hostname'", host, err.Error()) + } + } + entries, readErr := os.ReadDir(outDir) + if readErr != nil { + t.Fatalf("read outDir: %v", readErr) + } + if len(entries) != 0 { + t.Errorf("outDir not empty after rejected hostnames: %v", entries) + } +} + +// TestIssueBeacon_ValidHostname_Succeeds proves the happy path still +// works after the strict-allowlist validator landed — a normal DNS name +// produces both files in outDir. +func TestIssueBeacon_ValidHostname_Succeeds(t *testing.T) { + t.Parallel() + rootDir := t.TempDir() + if err := initRoot(rootDir); err != nil { + t.Fatalf("initRoot: %v", err) + } + outDir := t.TempDir() + const host = "beacon-01.example.com" + if err := issueBeacon(rootDir, host, outDir); err != nil { + t.Fatalf("issueBeacon(%q): %v", host, err) + } + for _, name := range []string{host + ".key", host + ".crt"} { + p := filepath.Join(outDir, name) + if _, err := os.Stat(p); err != nil { + t.Errorf("expected %s to exist: %v", p, err) + } + } }