From 9db05f44f76c3d377b5f969f26bce121c22fe944 Mon Sep 17 00:00:00 2001 From: DevCop95 Date: Sat, 17 Oct 2026 12:20:54 -0500 Subject: [PATCH] fix(persist): bound the child processes and stop under-reporting the version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from deploying the console locally and driving the whole route table over raw sockets, none of them exploitable, all of them the kind of thing that is only visible with the service actually running. persist: schtasks and the WMI PowerShell query ran under exec.Command, which waits with no deadline. The Task Scheduler and RPCSS are famous for wedging, and when they do the handler holds its goroutine until they come back — a request that never answers instead of an error. A full List() measures ~5.5s on a stock workstation, so the budget is 20s: a liveness bound, not a latency target. WaitDelay matters as much as the deadline, because killing `cmd /c` or powershell does not kill what they spawned and the inherited pipe stays open until the grandchild exits. persist: the test has teeth. With the context removed it fails at 9.09s ("the context was not enforced"); with it, 0.45s. `timeout /t` cannot be the sleep here — it refuses to run without a console, which is exactly how runHidden spawns its children — so it is a real ping. server: the validHost comment claimed the stdlib HTTP server rejects the malformed Host shapes before a handler sees them. Driving the live listener showed that holds for "127.0.0.1:8080/evil" (400, never reaches us) but not for "127.0.0.1:8a08" or "127.0.0.1:" — both reach the handler and are refused by isPort alone. Still not a bypass (a rebinding page supplies its own hostname, it cannot make the browser send a loopback one), but the doc was describing a belt that is not there, and the guard deserves to be trusted for what it does rather than for what the stdlib does alongside it. cmd: `--version` from a source build reported 2.0.1 while main is past the 2.0.2 release, so the audit JSON and bug reports carried an older version than anything published. Bumped, with a note that it tracks the newest tag. oblpack had no -version at all, which is fine for `go run` and not fine for a binary that ships in the archives, so it gained one — stamped like the console, or it would have been a shipped binary inventing its own version. --- .goreleaser.yml | 4 ++- cmd/obliteratus/main.go | 5 +++- cmd/oblpack/main.go | 11 ++++++++ internal/persist/persist_windows.go | 33 +++++++++++++++++++++--- internal/persist/persist_windows_test.go | 30 +++++++++++++++++++++ internal/server/middleware.go | 18 +++++++++---- 6 files changed, 91 insertions(+), 10 deletions(-) diff --git a/.goreleaser.yml b/.goreleaser.yml index 94ce74a..c9b6982 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -40,7 +40,8 @@ builds: - -s -w - -X main.version={{ .Version }} - # Payload packer. No version stamp — it is stateless tooling. + # Payload packer. Stateless, but stamped anyway so a shipped copy can + # identify itself when an operator reports a problem with it. - id: oblpack main: ./cmd/oblpack binary: oblpack @@ -53,6 +54,7 @@ builds: - arm64 ldflags: - -s -w + - -X main.version={{ .Version }} archives: - id: default diff --git a/cmd/obliteratus/main.go b/cmd/obliteratus/main.go index 527ee5c..ae9b77a 100644 --- a/cmd/obliteratus/main.go +++ b/cmd/obliteratus/main.go @@ -33,7 +33,10 @@ import ( // is a var, not a const, so goreleaser can stamp the tag into it at link time // via -ldflags "-X main.version={{.Version}}" (ldflags cannot write a const). // The literal below is the value used by `go build`/`go run` outside a release. -var version = "2.0.1" +// +// Keep it equal to the newest tag: a from-source build that reports an older +// release than the published one makes bug reports lie. +var version = "2.0.2" func main() { // Hidden internal mode: the collector re-launches this binary as SYSTEM diff --git a/cmd/oblpack/main.go b/cmd/oblpack/main.go index 6db0343..6a37afc 100644 --- a/cmd/oblpack/main.go +++ b/cmd/oblpack/main.go @@ -17,12 +17,23 @@ import ( "github.com/DevCop95/obliteratus/internal/research" ) +// version is stamped by goreleaser (see the oblpack build in .goreleaser.yml) +// and is what `-version` reports. The literal covers a plain `go build`, and +// must track the newest tag for the same reason as cmd/obliteratus. +var version = "2.0.2" + func main() { key := flag.String("key", "", "packing passphrase (hashed to a 32-byte AES-256 key)") in := flag.String("in", "payloads/research_signal.bin", "input payload file") out := flag.String("out", "", "output file (default: .packed)") + showVersion := flag.Bool("version", false, "print version and exit") flag.Parse() + if *showVersion { + fmt.Printf("oblpack %s\n", version) + return + } + if *key == "" { fatalf("-key is required") } diff --git a/internal/persist/persist_windows.go b/internal/persist/persist_windows.go index e15a2de..4137ea2 100644 --- a/internal/persist/persist_windows.go +++ b/internal/persist/persist_windows.go @@ -4,6 +4,7 @@ package persist import ( "bytes" + "context" "encoding/csv" "fmt" "io" @@ -12,6 +13,7 @@ import ( "path/filepath" "strings" "syscall" + "time" "golang.org/x/sys/windows/registry" ) @@ -155,12 +157,37 @@ func removeStartup(name string) error { const taskPrefix = "OBL_" +// runHiddenBudget bounds every child process this module spawns. The Task +// Scheduler and WMI are the classic Windows hang: schtasks can wedge for +// minutes behind a stuck Task Scheduler or RPCSS, and exec.Command waits with +// no deadline at all, which used to pin an HTTP handler (and its goroutine) +// for as long as the wedge lasted. Measured on a stock workstation a full +// List() takes ~5.5s, so this is a liveness bound rather than a latency target. +// +// It is a var only so a test can shrink it; nothing mutates it at runtime. +var runHiddenBudget = 20 * time.Second + +// runHiddenWaitDelay bounds how long Wait may keep draining the child's pipes +// after the context is done. Killing `cmd /c …` or powershell does not kill +// what they spawned, and the inherited pipe stays open until that grandchild +// exits — without this, the timeout would still block on the pipe read. +const runHiddenWaitDelay = 2 * time.Second + // runHidden runs a child process with no visible window and returns its -// combined output. +// combined output within runHiddenBudget. func runHidden(name string, args ...string) ([]byte, error) { - cmd := exec.Command(name, args...) //#nosec G204 -- name/args are internal constants and generated task names, never operator input + ctx, cancel := context.WithTimeout(context.Background(), runHiddenBudget) + defer cancel() + cmd := exec.CommandContext(ctx, name, args...) //#nosec G204 -- name/args are internal constants and generated task names, never operator input cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} - return cmd.CombinedOutput() + cmd.WaitDelay = runHiddenWaitDelay + out, err := cmd.CombinedOutput() + if ctx.Err() == context.DeadlineExceeded { + // Name the cause: "schtasks exit status 1" and "schtasks never came + // back" are different problems for whoever is debugging. + return out, fmt.Errorf("%s: timed out after %s", name, runHiddenBudget) + } + return out, err } func addTask(name, command, schedule, startTime string) error { diff --git a/internal/persist/persist_windows_test.go b/internal/persist/persist_windows_test.go index 7efd167..3c689d8 100644 --- a/internal/persist/persist_windows_test.go +++ b/internal/persist/persist_windows_test.go @@ -5,6 +5,7 @@ package persist import ( "strings" "testing" + "time" ) func TestPsQuote(t *testing.T) { @@ -120,3 +121,32 @@ func TestWMIRemoveCandidates(t *testing.T) { t.Fatalf("wmiRemoveCandidates(prefixed) = %v, want a single entry", got) } } + +// TestRunHiddenTimesOut pins the liveness bound. Before it, a wedged schtasks +// or WMI call held the HTTP handler forever, because exec.Command waits with +// no deadline. +// +// The sleep has to be a real child process: `timeout /t` is unusable here +// because it refuses to run without a console, which is exactly how runHidden +// spawns its children. `ping -n 10` occupies the child for ~9s, so a 300ms +// budget leaves a wide margin between "the context fired" and "the child would +// have finished anyway". +func TestRunHiddenTimesOut(t *testing.T) { + prev := runHiddenBudget + runHiddenBudget = 300 * time.Millisecond + defer func() { runHiddenBudget = prev }() + + start := time.Now() + _, err := runHidden("ping", "-n", "10", "127.0.0.1") + elapsed := time.Since(start) + + if err == nil { + t.Fatal("runHidden returned a nil error for a child that outlived the budget") + } + if !strings.Contains(err.Error(), "timed out") { + t.Fatalf("want an explicit timeout error, got %v", err) + } + if elapsed > 4*time.Second { + t.Fatalf("runHidden returned after %s: the context was not enforced", elapsed) + } +} diff --git a/internal/server/middleware.go b/internal/server/middleware.go index b113447..45ccd07 100644 --- a/internal/server/middleware.go +++ b/internal/server/middleware.go @@ -27,11 +27,19 @@ var loopbackHosts = map[string]bool{ // 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. +// contents, and it is more permissive than it looks: for "127.0.0.1:8080/evil" +// it returns host "127.0.0.1" with port "8080/evil", and for "127.0.0.1:8a08" +// and "127.0.0.1:" it returns no error at all. The stdlib HTTP server only +// rejects the first shape — a Host containing "/" never reaches a handler (400 +// before this code runs) — so for a non-numeric or empty port this explicit +// isPort check is the only control, not a belt over the stdlib's braces. That +// was verified against the live listener, not assumed. +// +// None of the three is a live bypass: a rebinding page cannot make a browser +// send a loopback-looking Host, it supplies its own hostname. But the guard is +// asserted here rather than left to the stdlib, and those exact host strings +// are pinned in the fuzz seed corpus (FuzzValidHost), so the check cannot +// quietly become permissive again. func validHost(hostport string) bool { host := hostport if h, p, err := net.SplitHostPort(hostport); err == nil {