diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a533e5f..db2e3b4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -42,7 +42,7 @@ jobs: # or Edge profile, a spawning schtasks.exe / powershell.exe, or a # domain to talk to. This floor exists to stop coverage going *down* # — raise it whenever it improves, never lower it to green a build. - min=42 + min=43 if awk "BEGIN {exit !(${total} < ${min})}"; then echo "::error::coverage ${total}% is below the ${min}% floor" exit 1 @@ -88,11 +88,17 @@ jobs: - name: unit tests (platform-neutral packages) run: go test ./internal/token ./internal/audit ./internal/ui ./internal/server - # Fuzzing on every push/PR. The packages exercised here eat untrusted input — - # registry hives, KDC replies, payload envelopes — inside a process that holds - # an elevated token, so a panic is a real availability bug, not a test - # artefact. Runs are short and bounded: this is a smoke gate, not a soak. Any - # crasher is written to testdata/fuzz/ and fails the job. + # Fuzzing on every push/PR. Two groups of targets: + # * the module parsers — registry hives, KDC replies, payload envelopes — + # which read bytes this tool did not author; + # * the console's own guard: the Host allow-list (DNS rebinding), the + # WebSocket Origin parser and the route policy (never reaches a handler + # without the run token; its inputs cannot match the token by + # construction, so it only ever exercises refusals). + # A panic in the first group is an availability bug in a process holding an + # elevated token; a hole in the second exposes every module at once. Runs are + # short and bounded: this is a smoke gate, not a soak. Any crasher is written + # to testdata/fuzz/ and fails the job. # # One target per invocation: `go test -fuzz` matches a single target. fuzz-smoke: @@ -113,6 +119,16 @@ jobs: run: go test -run='^$' -fuzz=FuzzMaskRoundTrip -fuzztime=30s ./internal/obfuscate - name: fuzz persist name sanitizer run: go test -run='^$' -fuzz=FuzzSanitizeName -fuzztime=30s ./internal/persist + # The console targets are cheaper per execution and carry hostile seeds, + # so they get a shorter budget than the parsers. + - name: fuzz host allow-list + run: go test -run='^$' -fuzz=FuzzValidHost -fuzztime=20s ./internal/server + - name: fuzz ws origin guard + run: go test -run='^$' -fuzz=FuzzAllowedOrigin -fuzztime=20s ./internal/server + - name: fuzz route policy + run: go test -run='^$' -fuzz=FuzzRoutesRejectBadToken -fuzztime=20s ./internal/server + - name: fuzz token comparison + run: go test -run='^$' -fuzz=FuzzEqual -fuzztime=20s ./internal/token # Static analysis (gosec) of exactly the code that ships. GOOS is pinned to # windows even though the runner is Linux, because the entire diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3b25853..9ca08c2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -32,7 +32,9 @@ build the project), run the underlying commands straight out of the `Makefile`. Never lower it to make a build green. `make cover` prints the total. - **Fuzzing** — the parsers that eat attacker-influenced input have fuzz targets (`FuzzRegf`, `FuzzParseRep`, `FuzzUnpack`, `FuzzMaskRoundTrip`, - `FuzzSanitizeName`). Seeds run as ordinary tests; to actually fuzz one: + `FuzzSanitizeName`), as does the console's own guard (`FuzzValidHost`, + `FuzzAllowedOrigin`, `FuzzRoutesRejectBadToken`, `FuzzEqual`). Seeds run as + ordinary tests; to actually fuzz one: ```bash go test -run='^$' -fuzz=FuzzRegf -fuzztime=60s ./internal/audit diff --git a/Makefile b/Makefile index 6ab890f..9f1c473 100644 --- a/Makefile +++ b/Makefile @@ -46,6 +46,10 @@ fuzz-check: go test -run='^$$' -fuzz=FuzzUnpack -fuzztime=20s ./internal/research go test -run='^$$' -fuzz=FuzzMaskRoundTrip -fuzztime=20s ./internal/obfuscate go test -run='^$$' -fuzz=FuzzSanitizeName -fuzztime=20s ./internal/persist + go test -run='^$$' -fuzz=FuzzValidHost -fuzztime=15s ./internal/server + go test -run='^$$' -fuzz=FuzzAllowedOrigin -fuzztime=15s ./internal/server + go test -run='^$$' -fuzz=FuzzRoutesRejectBadToken -fuzztime=15s ./internal/server + go test -run='^$$' -fuzz=FuzzEqual -fuzztime=15s ./internal/token # Whole-tree statement coverage; CI fails below its ratcheting floor. cover: diff --git a/internal/server/fuzz_test.go b/internal/server/fuzz_test.go new file mode 100644 index 0000000..31e2b1a --- /dev/null +++ b/internal/server/fuzz_test.go @@ -0,0 +1,332 @@ +package server + +import ( + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" +) + +// This file fuzzes the console's exposed surface: the Host allow-list, the +// WebSocket Origin parser and the route policy (which checks precede which +// handler). The parsers inside the modules are fuzzed separately in +// internal/audit, internal/krb, internal/research and so on; what matters here +// is the *guard*, because a hole in it exposes every module at once. +// +// The four targets below never reach a state-changing handler: they only ever +// present a token that cannot match (a byte is prepended to it), so the tests +// assert the gate without executing anything. + +// routeKind classifies a registered pattern by the checks that must precede its +// handler. A route added to (*Server).routes() without s.guard(...) shows up +// here as a kindAPI entry that answers something other than 403. +type routeKind int + +const ( + kindPage routeKind = iota // host check only; the token travels inside the page + kindWS // host + origin (for browsers) + token from ?t= + kindAPI // host + token + kindGatedAPI // host + token + an --enable-* flag +) + +type testRoute struct { + method, path string + kind routeKind + flag string // the flag that unlocks a kindGatedAPI route +} + +// registeredRoutes mirrors the patterns registered in (*Server).routes(). +// TestRouteTableMatchesServer fails if a listed pattern does not exist, and +// TestGuardedRoutesRejectBadToken fails if a guarded pattern stops being +// guarded — so a new route cannot be added without this table noticing. +var registeredRoutes = []testRoute{ + // pages + {"GET", "/", kindPage, ""}, + {"GET", "/healthz", kindPage, ""}, + {"GET", "/ws", kindWS, ""}, + + // read-only system API + {"GET", "/v1/sys/st", kindAPI, ""}, + {"GET", "/v1/sys/hex", kindAPI, ""}, + {"GET", "/v1/sys/b2", kindAPI, ""}, + + // state-changing system API + {"POST", "/v1/sys/cmd", kindAPI, ""}, + {"POST", "/v1/sys/opt", kindAPI, ""}, + + // collection module + {"GET", "/v1/sys/a7", kindGatedAPI, "--enable-collect"}, + {"GET", "/v1/sys/cookies", kindGatedAPI, "--enable-collect"}, + {"GET", "/v1/sys/nexus", kindGatedAPI, "--enable-collect"}, + {"POST", "/v1/op/a7", kindGatedAPI, "--enable-collect"}, + {"POST", "/v1/op/cookies", kindGatedAPI, "--enable-collect"}, + {"POST", "/v1/sys/sam", kindGatedAPI, "--enable-collect"}, + {"POST", "/v1/op/roast-asrep", kindGatedAPI, "--enable-collect"}, + {"POST", "/v1/op/roast-tgs", kindGatedAPI, "--enable-collect"}, + {"POST", "/v1/op/spn-enum", kindGatedAPI, "--enable-collect"}, + {"POST", "/v1/op/lsass", kindGatedAPI, "--enable-collect"}, + {"POST", "/v1/op/lsass-wipe", kindGatedAPI, "--enable-collect"}, + + // persistence module + {"GET", "/v1/sys/persist", kindGatedAPI, "--enable-persist"}, + {"POST", "/v1/op/persist-add", kindGatedAPI, "--enable-persist"}, + {"POST", "/v1/op/persist-del", kindGatedAPI, "--enable-persist"}, + + // evasion module + {"POST", "/v1/op/etw", kindGatedAPI, "--enable-evasion"}, + {"POST", "/v1/op/amsi", kindGatedAPI, "--enable-evasion"}, + + // lateral movement module + {"POST", "/v1/op/lateral-winrm", kindGatedAPI, "--enable-lateral"}, + {"POST", "/v1/op/lateral-scm", kindGatedAPI, "--enable-lateral"}, + + // injection module + {"POST", "/v1/op/d4", kindGatedAPI, "--enable-inject"}, + + // defensive audit (always available) + {"POST", "/v1/audit/report", kindAPI, ""}, + {"POST", "/v1/audit/csv", kindAPI, ""}, +} + +// guardedRoutes returns only the routes that must present the run token. +func guardedRoutes() []testRoute { + var out []testRoute + for _, r := range registeredRoutes { + if r.kind == kindAPI || r.kind == kindGatedAPI { + out = append(out, r) + } + } + return out +} + +// TestRouteTableMatchesServer proves every listed pattern is really registered: +// a typo in the table (or a route that was removed) answers 404, and a wrong +// method answers 405. +func TestRouteTableMatchesServer(t *testing.T) { + s := newTestServer(t, Config{}) + h := s.Handler() + for _, r := range registeredRoutes { + req := httptest.NewRequest(r.method, r.path, strings.NewReader("{}")) + req.Host = "127.0.0.1:8080" + req.Header.Set("X-Signal-Token", "not-the-token") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code == http.StatusNotFound || rec.Code == http.StatusMethodNotAllowed { + t.Errorf("%s %s: status %d — pattern is not registered as listed", r.method, r.path, rec.Code) + } + } +} + +// TestGuardedRoutesRejectBadToken is the route-policy invariant: on every +// guarded route, a request without the run token must be refused before the +// handler runs. A route registered without s.guard(...) — the realistic way to +// expose a module by accident — fails here. +func TestGuardedRoutesRejectBadToken(t *testing.T) { + s := newTestServer(t, Config{}) + h := s.Handler() + for _, r := range guardedRoutes() { + for _, tc := range []struct { + name string + token string + }{ + {"absent token", ""}, + {"wrong token", "not-the-token"}, + {"token prefix", testToken[:len(testToken)-1]}, + {"token as substring", testToken[:8]}, + } { + req := httptest.NewRequest(r.method, r.path, strings.NewReader("{}")) + req.Host = "127.0.0.1:8080" + if tc.token != "" { + req.Header.Set("X-Signal-Token", tc.token) + } + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusForbidden { + t.Errorf("%s %s with %s: status = %d, want 403", r.method, r.path, tc.name, rec.Code) + } + // A refused request must not leak the secret it refused to accept. + if strings.Contains(rec.Body.String(), testToken) { + t.Errorf("%s %s with %s: response body carries the run token", r.method, r.path, tc.name) + } + } + } +} + +// TestGatedRoutesFailsClosedWithValidToken proves the module gate sits behind +// the token gate but in front of every effect: with a correct token and the +// module disabled, a gated route is still 403 — and it never reports the token +// is the problem, which would mislead an operator into re-checking it. +func TestGatedRoutesFailsClosedWithValidToken(t *testing.T) { + s := newTestServer(t, Config{}) // no --enable-* flags + h := s.Handler() + for _, r := range registeredRoutes { + if r.kind != kindGatedAPI { + continue + } + req := httptest.NewRequest(r.method, r.path, strings.NewReader("{}")) + req.Host = "127.0.0.1:8080" + req.Header.Set("X-Signal-Token", testToken) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusForbidden { + t.Errorf("%s %s with a valid token and %s off: status = %d, want 403", + r.method, r.path, r.flag, rec.Code) + } + if !strings.Contains(rec.Body.String(), r.flag) { + t.Errorf("%s %s: refusal does not name %s: %s", r.method, r.path, r.flag, rec.Body.String()) + } + } +} + +// loopbackOracle is an independent, deliberately naive statement of the same +// policy validHost implements: the only strings that may be accepted are the +// canonical loopback names, optionally followed by a numeric port. It returns +// "" for anything else. Where the two disagree, validHost is accepting +// something that is not a loopback host:port at all. +func loopbackOracle(hostport string) string { + s := strings.ToLower(hostport) + if i := strings.LastIndexByte(s, ':'); i >= 0 { + if !isPort(s[i+1:]) { + return "" + } + s = s[:i] + } + s = strings.TrimSuffix(s, ".") + switch s { + case "127.0.0.1", "localhost": + return s + case "[::1]": + return "::1" + } + return "" +} + +// FuzzValidHost is the DNS-rebinding control under fuzz: whatever it accepts +// must be a loopback authority, and nothing else. This is the check that keeps +// an attacker-controlled domain that resolves to 127.0.0.1 from reading the +// dashboard's token. +func FuzzValidHost(f *testing.F) { + for _, seed := range []string{ + "127.0.0.1:8080", "localhost:8080", "[::1]:8080", "127.0.0.1", "LOCALHOST", + "localhost.", "127.0.0.1.:8080", "evil.com:8080", "127.0.0.1.evil.com:8080", + // Shapes whose *authority* a naive parser reads as loopback even though + // the string is not one — the interesting mutations. + "127.0.0.1:8080/evil", "127.0.0.1:@evil", "localhost:80?x=1", "127.0.0.1:8a08", + "127.0.0.1:", "[::1]", "::1", "127.0.0.1\t", "127.0.0.1%2e", "user@127.0.0.1", + } { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, hostport string) { + if !validHost(hostport) { + return + } + if got := loopbackOracle(hostport); got == "" { + t.Fatalf("validHost(%q) = true, but it is not a loopback authority", hostport) + } + // Independent of the oracle: a host:port authority cannot contain these. + if strings.ContainsAny(hostport, `/\\@#?%`+" \t\r\n") { + t.Fatalf("validHost(%q) = true, but the value contains a non-authority character", hostport) + } + }) +} + +// FuzzAllowedOrigin is the WebSocket handshake guard under fuzz. An accepted +// Origin must be exactly scheme://loopback-host[:port] — no userinfo, no path, +// no query. The userinfo case is the one that matters: url.Parse splits +// "http://evil.com@127.0.0.1" into User=evil.com and Host=127.0.0.1, so a guard +// that only inspects Host reads a remote origin as local. +func FuzzAllowedOrigin(f *testing.F) { + s := newTestServer(f, Config{Addr: "127.0.0.1:8080"}) + for _, seed := range []string{ + "http://127.0.0.1:8080", "http://localhost:8080", "https://[::1]:8080", + "", "null", "http://evil.com", "https://evil.com:443", + "http://evil.com@127.0.0.1", "http://127.0.0.1@evil.com", "http://user:pw@localhost", + "file://127.0.0.1", "http://127.0.0.1/#x", "http://127.0.0.1/?a=b", + "http://127.0.0.1.evil.com", "//127.0.0.1", "http:127.0.0.1", + `http://evil.com\@127.0.0.1`, "http://127.0.0.1\t", + } { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, origin string) { + if !s.allowedOrigin(origin) { + return + } + if origin == "" { + return // documented: non-browser clients send no Origin + } + if strings.Contains(origin, "@") { + t.Fatalf("allowedOrigin(%q) = true, but an Origin carries no userinfo", origin) + } + u, err := url.Parse(origin) + if err != nil { + t.Fatalf("allowedOrigin(%q) = true, but the value does not parse as a URL", origin) + } + if u.Scheme != "http" && u.Scheme != "https" { + t.Fatalf("allowedOrigin(%q) = true with scheme %q", origin, u.Scheme) + } + if u.User != nil { + t.Fatalf("allowedOrigin(%q) = true with userinfo %q", origin, u.User) + } + if u.Path != "" && u.Path != "/" { + t.Fatalf("allowedOrigin(%q) = true with path %q", origin, u.Path) + } + if u.RawQuery != "" || u.Fragment != "" { + t.Fatalf("allowedOrigin(%q) = true with query/fragment", origin) + } + if got := loopbackOracle(u.Host); got == "" { + t.Fatalf("allowedOrigin(%q) = true, but its authority %q is not loopback", origin, u.Host) + } + }) +} + +// FuzzRoutesRejectBadToken drives the whole mux with an arbitrary Host, Origin, +// body and a token that cannot match, and asserts the policy for whichever +// route is selected: a non-loopback Host is always 403, a page route with a +// loopback Host is served, and everything else is 403 without ever disclosing +// the run token. The route is chosen from registeredRoutes so the fuzzer +// explores the guard, never invalid paths (which would 404 and prove nothing). +func FuzzRoutesRejectBadToken(f *testing.F) { + s := newTestServer(f, Config{}) + h := s.Handler() + for i := range registeredRoutes { + f.Add(uint8(i), "127.0.0.1:8080", "http://127.0.0.1:8080", "", "{}") + } + f.Fuzz(func(t *testing.T, idx uint8, host, origin, tok, body string) { + r := registeredRoutes[int(idx)%len(registeredRoutes)] + req := httptest.NewRequest(r.method, r.path, strings.NewReader(body)) + req.Host = host + if origin != "" { + req.Header.Set("Origin", origin) + } + // Prepending a byte makes a match impossible regardless of what the + // fuzzer produced, so this target can never reach a state-changing + // handler: the only paths it exercises are the refusals. + req.Header.Set("X-Signal-Token", "x"+tok) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + if !validHost(host) { + if rec.Code != http.StatusForbidden { + t.Fatalf("%s %s: Host %q is not loopback, status = %d, want 403", + r.method, r.path, host, rec.Code) + } + return + } + if r.kind == kindPage { + if rec.Code != http.StatusOK { + t.Fatalf("%s %s: page with loopback Host %q, status = %d, want 200", + r.method, r.path, host, rec.Code) + } + return + } + if rec.Code != http.StatusForbidden { + t.Fatalf("%s %s reached without the run token: status = %d (token %q)", + r.method, r.path, rec.Code, tok) + } + if strings.Contains(rec.Body.String(), testToken) { + t.Fatalf("%s %s: refusal leaked the run token", r.method, r.path) + } + }) +} diff --git a/internal/server/middleware.go b/internal/server/middleware.go index 91a2bcf..b113447 100644 --- a/internal/server/middleware.go +++ b/internal/server/middleware.go @@ -19,15 +19,51 @@ var loopbackHosts = map[string]bool{ } // validHost reports whether the request targeted a loopback Host header. +// +// Strictness matters more than permissiveness here: this is the control that +// stops a DNS-rebinding page (`Host: attacker.example` resolving to 127.0.0.1) +// from reading the token out of the dashboard. Anything that is not a bare +// loopback host, a loopback host with a numeric port, or a bracketed IPv6 +// loopback with a numeric port is refused. +// +// Note that net.SplitHostPort validates the *shape* of an address, not its +// contents: it happily returns host "127.0.0.1" and port "8080/evil" for +// "127.0.0.1:8080/evil". Trusting it would accept host strings no parser ever +// meant to hand us, so the port is checked explicitly. The stdlib HTTP server +// rejects such a Host header before it reaches a handler, which is why this is +// hardening rather than a live bypass — but a guard should mean what it says. func validHost(hostport string) bool { host := hostport - if h, _, err := net.SplitHostPort(hostport); err == nil { + if h, p, err := net.SplitHostPort(hostport); err == nil { + if !isPort(p) { + return false + } host = h + } else if strings.ContainsAny(hostport, ":[]") { + // A port separator or bracket we could not parse: refuse instead of + // guessing which parser the caller intended. + return false } host = strings.TrimSuffix(strings.ToLower(host), ".") + if host == "" { + return false + } return loopbackHosts[host] } +// isPort reports whether p is a non-empty run of ASCII digits. +func isPort(p string) bool { + if p == "" { + return false + } + for i := 0; i < len(p); i++ { + if p[i] < '0' || p[i] > '9' { + return false + } + } + return true +} + // guard is the API middleware: loopback Host + constant-time token check. // Cross-origin pages cannot satisfy it — the custom X-Signal-Token header // forces a CORS preflight, which we never approve, so a hostile page's fetch @@ -87,6 +123,14 @@ func setSecurityHeaders(w http.ResponseWriter, connectSrc string) { // Browsers always attach Origin to cross-origin WS requests, so a page on // evil.com cannot complete the handshake even if it somehow knew the token // (it still needs the token too — both checks must pass). +// +// The check is deliberately stricter than "url.Parse succeeded and the host +// looks loopback". Go's url.Parse puts the *userinfo* in u.User and the host in +// u.Host, so "http://evil.com@127.0.0.1" parses with u.Host == "127.0.0.1" and +// would read as loopback. A browser never sends userinfo in Origin, which is +// why this was not exploitable, but accepting a non-loopback origin because of +// a parsing quirk is the kind of thing that stops being theoretical the moment +// the check is reused. So: an Origin must be exactly scheme://host[:port]. func (s *Server) allowedOrigin(origin string) bool { if origin == "" { // Non-browser clients (curl, a test) send no Origin; they are held to @@ -100,5 +144,11 @@ func (s *Server) allowedOrigin(origin string) bool { if u.Scheme != "http" && u.Scheme != "https" { return false } + if u.User != nil || u.Opaque != "" || u.RawQuery != "" || u.Fragment != "" { + return false + } + if u.Path != "" && u.Path != "/" { + return false + } return validHost(u.Host) } diff --git a/internal/server/middleware_test.go b/internal/server/middleware_test.go index af2a4bc..4c8a32a 100644 --- a/internal/server/middleware_test.go +++ b/internal/server/middleware_test.go @@ -13,8 +13,10 @@ import ( const testToken = "test-token-0123456789abcdef" -func newTestServer(t *testing.T, cfg Config) *Server { - t.Helper() +// newTestServer builds a server around the shared test token. It takes +// testing.TB so fuzz targets (*testing.F) can use it too. +func newTestServer(tb testing.TB, cfg Config) *Server { + tb.Helper() cfg.Token = testToken if cfg.Version == "" { cfg.Version = "test" diff --git a/internal/server/server.go b/internal/server/server.go index 6856d19..b2a8e25 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -123,6 +123,11 @@ func portOf(addr, fallback string) string { // routes registers every endpoint. Method patterns ("POST /path") make the // ServeMux answer 405 automatically for wrong verbs — no handler ever has to // remember to check. +// +// Every API route must be wrapped in s.guard(...). That rule is not enforced +// here but it is enforced by TestGuardedRoutesRejectBadToken, which drives the +// table in fuzz_test.go; adding a pattern to this function means adding it +// there, and a route registered without a guard fails the test. func (s *Server) routes() { // --- pages (host-checked; token travels inside the page) --------------- s.mux.HandleFunc("GET /{$}", s.guardPage(s.handleIndex)) diff --git a/internal/token/token_fuzz_test.go b/internal/token/token_fuzz_test.go new file mode 100644 index 0000000..d1b11d0 --- /dev/null +++ b/internal/token/token_fuzz_test.go @@ -0,0 +1,24 @@ +package token + +import "testing" + +// FuzzEqual pins the comparison to exact equality. The interesting mutations +// are the near-misses: a prefix, a suffix, a case-variant or a truncation of +// the real token. A guard that accepted any of those would be a complete +// authentication bypass, because every console endpoint trusts this one call. +// Constant-time behaviour cannot be fuzzed; correctness of the verdict can. +func FuzzEqual(f *testing.F) { + f.Add("a", "a") + f.Add("", "") + f.Add("abc", "ab") + f.Add("abc", "abcd") + f.Add("ABC", "abc") + f.Add("abc", "abc\x00") + f.Add("\x00", "") + f.Add("deadbeef", "ecafbeef") + f.Fuzz(func(t *testing.T, a, b string) { + if got, want := Equal(a, b), a == b; got != want { + t.Fatalf("Equal(%q, %q) = %v, want %v", a, b, got, want) + } + }) +}