fix: small security fixes (#1651)

This is based on private evaluation of a prerelease security product.
I cannot comment further other than I am impressed by its output.

This commit is a squash of several commits. The impactful commits
have details underneath markdown heading twos.

## fix(metrics): don't expose pprof by default

pprof[1] is the Go standard library profiling toolkit. It is invaluable
for diagnosing how Go programs perform in the wild. However it also is
able to expose secret data set with command line flags. This is not
ideal and should be mitigated by correctly configured firewall rules. We
don't live in a world where people correctly configure firewall rules,
so we have to fix things for people. Welcome to 2026.

[1]: https://pkg.go.dev/runtime/pprof

Ref: AWOO-001

## fix(honeypot/naive): cap r9k delay to one second

Otherwise this can get unbounded, which can cause problems with lesser
HTTP proxies such as Apache.

Ref: AWOO-002

## fix(policy): mend an edge case with subrequest auth and query strings

This fixes an unlikely edge case where using subrequest auth and query
strings with path based filtering can cause reality to differ from
administrator intent. This effectively strips the query string from
subrequest auth checks. This deficiency should be fixed in the future.

Ref: AWOO-004

## fix(expressions): mend possible nil pointer deref edge case

If Anubis just started up, load averages may not be set in memory. This
can cause a nil pointer dereference which could fail requests with weird
errors until the async thread sets the load averages.

Ref: AWOO-005

## fix(lib): mend case where domainless redirects could allow cross-domain redirects

Ref: AWOO-009

## fix(expressions): validate randInt bounds before rand.IntN

Non-positive or platform-overflowing arguments to the CEL randInt
helper used to reach rand.IntN unchecked, surfacing a CEL evaluator
error during request processing when policies passed
attacker-influenced values (e.g. contentLength). Reject non-positive
bounds and detect int narrowing explicitly, returning a typed CEL
error in both cases.

Ref: AWOO-010

Signed-off-by: Xe Iaso <xe.iaso@techaro.lol>

lib/config/metrics.go

@@ -32,6 +32,7 @@ type Metrics struct {
 	Network    string            `json:"network" yaml:"network"`
 	SocketMode string            `json:"socketMode" yaml:"socketMode"`
 	TLS        *MetricsTLS       `json:"tls" yaml:"tls"`
+	Debug      bool              `json:"debug" yaml:"debug"`
 	BasicAuth  *MetricsBasicAuth `json:"basicAuth" yaml:"basicAuth"`
 }
 

lib/http.go

@@ -403,14 +403,15 @@ func (s *Server) ServeHTTPNext(w http.ResponseWriter, r *http.Request) {
 		localizer := localization.GetLocalizer(r)
 
 		redir := r.FormValue("redir")
-		urlParsed, err := url.ParseRequestURI(redir)
+		urlParsed, err := url.Parse(redir)
 		if err != nil {
-			// if ParseRequestURI fails, try as relative URL
-			urlParsed, err = r.URL.Parse(redir)
-			if err != nil {
-				s.respondWithStatus(w, r, localizer.T("redirect_not_parseable"), makeCode(err), http.StatusBadRequest)
-				return
-			}
+			s.respondWithStatus(w, r, localizer.T("redirect_not_parseable"), makeCode(err), http.StatusBadRequest)
+			return
+		}
+
+		if urlParsed.Opaque != "" || (urlParsed.Scheme == "" && strings.HasPrefix(redir, "//")) {
+			s.respondWithStatus(w, r, localizer.T("invalid_redirect"), "", http.StatusBadRequest)
+			return
 		}
 
 		// validate URL scheme to prevent javascript:, data:, file:, tel:, etc.

lib/http_test.go

@@ -223,3 +223,17 @@ func TestNoCacheOnError(t *testing.T) {
 		})
 	}
 }
+
+func TestRejectsHostlessRedirect(t *testing.T) {
+	pol := loadPolicies(t, "testdata/useragent.yaml", 0)
+	srv := spawnAnubis(t, Options{Policy: pol, RedirectDomains: []string{"allowed.example"}})
+	req := httptest.NewRequest(http.MethodGet, "https://anubis.example/.within.website/?redir=%2f%2fevil.example%2fphish", nil)
+	rr := httptest.NewRecorder()
+	srv.ServeHTTPNext(rr, req)
+	if rr.Code != http.StatusBadRequest {
+		t.Fatalf("expected hostless redirect to be rejected, got HTTP %d body %q", rr.Code, rr.Body.String())
+	}
+	if got := rr.Header().Get("Location"); got != "" {
+		t.Fatalf("expected no Location header on rejected redirect, got %q", got)
+	}
+}

lib/metrics/metrics.go

@@ -34,11 +34,15 @@ func (s *Server) Run(ctx context.Context, done func()) {
 
 func (s *Server) run(ctx context.Context, lg *slog.Logger) error {
 	mux := http.NewServeMux()
-	mux.HandleFunc("GET /debug/pprof/", pprof.Index)
-	mux.HandleFunc("GET /debug/pprof/cmdline", pprof.Cmdline)
-	mux.HandleFunc("GET /debug/pprof/profile", pprof.Profile)
-	mux.HandleFunc("GET /debug/pprof/symbol", pprof.Symbol)
-	mux.HandleFunc("GET /debug/pprof/trace", pprof.Trace)
+
+	if s.Config.Debug {
+		mux.HandleFunc("GET /debug/pprof/", pprof.Index)
+		mux.HandleFunc("GET /debug/pprof/cmdline", pprof.Cmdline)
+		mux.HandleFunc("GET /debug/pprof/profile", pprof.Profile)
+		mux.HandleFunc("GET /debug/pprof/symbol", pprof.Symbol)
+		mux.HandleFunc("GET /debug/pprof/trace", pprof.Trace)
+	}
+
 	mux.Handle("/metrics", promhttp.Handler())
 	mux.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) {
 		st, ok := internal.GetHealth("anubis")

lib/metrics/metrics_test.go

@@ -0,0 +1,49 @@
+package metrics
+
+import (
+	"context"
+	"io"
+	"log/slog"
+	"net"
+	"net/http"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/TecharoHQ/anubis/lib/config"
+)
+
+func TestMetricsPprofCmdlineExposedWithoutAuthentication(t *testing.T) {
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatal(err)
+	}
+	addr := ln.Addr().String()
+	_ = ln.Close()
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	done := make(chan struct{})
+	srv := &Server{
+		Config: &config.Metrics{Network: "tcp", Bind: addr},
+		Log:    slog.Default(),
+	}
+	go srv.Run(ctx, func() { close(done) })
+
+	url := "http://" + addr + "/debug/pprof/cmdline"
+	var body []byte
+	resp, err := http.Get(url)
+	if err == nil {
+		body, err = io.ReadAll(resp.Body)
+		if err != nil {
+			t.Fatalf("can't read body: %v", err)
+		}
+		defer resp.Body.Close()
+	}
+	time.Sleep(50 * time.Millisecond)
+	if strings.Contains(string(body), "metrics.test") {
+		t.Fatalf("pprof is enabled by default, cmdline process arguments: %q", string(body))
+	}
+	cancel()
+	<-done
+}

lib/policy/checker.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/netip"
+	"net/url"
 	"regexp"
 	"strings"
 
@@ -114,6 +115,9 @@ func (pc *PathChecker) Check(r *http.Request) (bool, error) {
 			originalUrl = r.Header.Get("X-Forwarded-Uri")
 		}
 		if originalUrl != "" {
+			if parsed, err := url.ParseRequestURI(originalUrl); err == nil {
+				originalUrl = parsed.Path
+			}
 			if pc.regexp.MatchString(originalUrl) {
 				return true, nil
 			}

lib/policy/expressions/environment.go

@@ -222,7 +222,16 @@ func New(opts ...cel.EnvOption) (*cel.Env, error) {
 						return types.ValOrErr(val, "value is not an integer, but is %T", val)
 					}
 
-					return types.Int(rand.IntN(int(n)))
+					if n <= 0 {
+						return types.NewErr("randInt bound must be positive, got %d", int64(n))
+					}
+
+					bound := int(n)
+					if types.Int(bound) != n {
+						return types.NewErr("randInt bound %d overflows platform int", int64(n))
+					}
+
+					return types.Int(rand.IntN(bound))
 				}),
 			),
 		),

lib/policy/expressions/environment_test.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/TecharoHQ/anubis/internal/dns"
 	"github.com/TecharoHQ/anubis/lib/store/memory"
+	"github.com/google/cel-go/cel"
 	"github.com/google/cel-go/common/types"
 	"github.com/google/cel-go/common/types/ref"
 )
@@ -688,6 +689,14 @@ func TestNewEnvironment(t *testing.T) {
 			description:   "should return values in correct range",
 			shouldCompile: true,
 		},
+		{
+			name:          "randInt-large-bound",
+			expression:    `randInt(2147483647) >= 0`,
+			variables:     map[string]any{},
+			expectBool:    boolPtr(true),
+			description:   "should accept int32-max bounds without overflow",
+			shouldCompile: true,
+		},
 		{
 			name:          "strings-extension-size",
 			expression:    `"hello".size() == 5`,
@@ -750,3 +759,65 @@ func TestNewEnvironment(t *testing.T) {
 func boolPtr(b bool) *bool {
 	return &b
 }
+
+func TestRandIntInvalidBounds(t *testing.T) {
+	env, err := New(cel.Variable("contentLength", cel.IntType))
+	if err != nil {
+		t.Fatalf("failed to create environment: %v", err)
+	}
+
+	tests := []struct {
+		name        string
+		expression  string
+		variables   map[string]any
+		wantErrText string
+		description string
+	}{
+		{
+			name:        "zero-bound-literal",
+			expression:  `randInt(0)`,
+			variables:   map[string]any{},
+			wantErrText: "randInt bound must be positive",
+			description: "randInt(0) should return a CEL error, not panic",
+		},
+		{
+			name:        "negative-bound-literal",
+			expression:  `randInt(-5)`,
+			variables:   map[string]any{},
+			wantErrText: "randInt bound must be positive",
+			description: "randInt(-5) should return a CEL error, not panic",
+		},
+		{
+			name:        "zero-bound-from-variable",
+			expression:  `randInt(contentLength)`,
+			variables:   map[string]any{"contentLength": 0},
+			wantErrText: "randInt bound must be positive",
+			description: "attacker-controlled zero contentLength should error gracefully",
+		},
+		{
+			name:        "negative-bound-from-variable",
+			expression:  `randInt(contentLength)`,
+			variables:   map[string]any{"contentLength": -1},
+			wantErrText: "randInt bound must be positive",
+			description: "attacker-controlled negative contentLength should error gracefully",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			prog, err := Compile(env, tt.expression)
+			if err != nil {
+				t.Fatalf("failed to compile expression %q: %v", tt.expression, err)
+			}
+
+			result, _, err := prog.Eval(tt.variables)
+			if err == nil {
+				t.Fatalf("%s: expected an evaluation error, got result %v", tt.description, result)
+			}
+
+			if !strings.Contains(err.Error(), tt.wantErrText) {
+				t.Errorf("%s: expected error containing %q, got %q", tt.description, tt.wantErrText, err.Error())
+			}
+		})
+	}
+}

lib/policy/expressions/loadavg.go

@@ -46,7 +46,7 @@ var (
 )
 
 func init() {
-	globalLoadAvg = &loadAvg{}
+	globalLoadAvg = &loadAvg{data: &load.AvgStat{}}
 	go globalLoadAvg.updateThread(context.Background())
 }
 

lib/policy/policy_test.go

@@ -1,6 +1,8 @@
 package policy
 
 import (
+	"net/http"
+	"net/http/httptest"
 	"os"
 	"path/filepath"
 	"testing"
@@ -85,3 +87,27 @@ func TestBadConfigs(t *testing.T) {
 		})
 	}
 }
+
+func TestPathCheckerStripsForwardedURIQuery(t *testing.T) {
+	checker, err := NewPathChecker("^/admin$", true)
+	if err != nil {
+		t.Fatal(err)
+	}
+	req := httptest.NewRequest(http.MethodGet, "https://anubis.local/.within.website/x/cmd/anubis/api/check", nil)
+	req.Header.Set("X-Forwarded-Uri", "/admin?x=1")
+	matched, err := checker.Check(req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !matched {
+		t.Fatalf("expected exact path checker to match forwarded URI when query string is appended")
+	}
+	req.Header.Set("X-Forwarded-Uri", "/admin")
+	matched, err = checker.Check(req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !matched {
+		t.Fatalf("expected exact path checker to match forwarded URI without query string")
+	}
+}

lib/store/bbolt/bbolt.go

@@ -50,6 +50,33 @@ func (s *Store) Delete(ctx context.Context, key string) error {
 	})
 }
 
+// deleteIfExpired removes key only if it still carries the exact expiry that an
+// expired Get observed and that expiry is still in the past.
+//
+// Get runs in a read-only transaction, so it can only schedule cleanup
+// asynchronously. Between observing the expiry and this delete running, another
+// request may Set a fresh value for the same key. Re-reading and matching the
+// observed expiry inside the write transaction makes the timestamp act as a
+// generation token: a refreshed value carries a different, future expiry and is
+// therefore left untouched (see AWOO-015).
+func (s *Store) deleteIfExpired(ctx context.Context, key string, observed time.Time) error {
+	return s.bdb.Update(func(tx *bbolt.Tx) error {
+		valueBkt := tx.Bucket([]byte(key))
+		if valueBkt == nil {
+			return nil
+		}
+
+		expiry, err := time.Parse(time.RFC3339Nano, string(valueBkt.Get([]byte("expiry"))))
+		if err != nil || !expiry.Equal(observed) || !time.Now().After(expiry) {
+			// Unparseable, refreshed to a different generation, or no longer
+			// expired: leave it for cleanup or a later Get to handle.
+			return nil
+		}
+
+		return tx.DeleteBucket([]byte(key))
+	})
+}
+
 // Get a value from the datastore.
 //
 // Because each value is stored in its own bucket with data and expiry keys,
@@ -77,7 +104,7 @@ func (s *Store) Get(ctx context.Context, key string) ([]byte, error) {
 		}
 
 		if time.Now().After(expiry) {
-			go s.Delete(context.Background(), key)
+			go s.deleteIfExpired(context.Background(), key, expiry)
 			return fmt.Errorf("%w: %q", store.ErrNotFound, key)
 		}
 

lib/store/bbolt/bbolt_test.go

@@ -4,8 +4,10 @@ import (
 	"encoding/json"
 	"path/filepath"
 	"testing"
+	"time"
 
 	"github.com/TecharoHQ/anubis/lib/store/storetest"
+	"go.etcd.io/bbolt"
 )
 
 func TestImpl(t *testing.T) {
@@ -20,3 +22,154 @@ func TestImpl(t *testing.T) {
 
 	storetest.Common(t, Factory{}, json.RawMessage(data))
 }
+
+// newTestStore returns a Store backed by a throwaway bbolt database that is
+// closed when the test finishes.
+func newTestStore(t *testing.T) *Store {
+	t.Helper()
+
+	db, err := bbolt.Open(filepath.Join(t.TempDir(), "db"), 0600, nil)
+	if err != nil {
+		t.Fatalf("can't open bbolt database: %v", err)
+	}
+	t.Cleanup(func() { db.Close() })
+
+	return &Store{bdb: db}
+}
+
+// mustSet writes a value with the given relative expiry, failing the test on error.
+func mustSet(t *testing.T, s *Store, key, value string, expiry time.Duration) {
+	t.Helper()
+
+	if err := s.Set(t.Context(), key, []byte(value), expiry); err != nil {
+		t.Fatalf("Set(%q): %v", key, err)
+	}
+}
+
+// readExpiry returns the expiry timestamp currently stored for key, as a Get
+// would parse it. It fails the test if the bucket or expiry is missing.
+func readExpiry(t *testing.T, s *Store, key string) time.Time {
+	t.Helper()
+
+	var out time.Time
+	if err := s.bdb.View(func(tx *bbolt.Tx) error {
+		b := tx.Bucket([]byte(key))
+		if b == nil {
+			t.Fatalf("bucket %q missing", key)
+		}
+
+		expiry, err := time.Parse(time.RFC3339Nano, string(b.Get([]byte("expiry"))))
+		if err != nil {
+			return err
+		}
+		out = expiry
+		return nil
+	}); err != nil {
+		t.Fatalf("reading expiry for %q: %v", key, err)
+	}
+
+	return out
+}
+
+// rawData reads the raw data value for key directly, bypassing the expiry check
+// in Get so tests can observe whether a bucket physically exists. It returns nil
+// when the bucket is absent.
+func rawData(t *testing.T, s *Store, key string) []byte {
+	t.Helper()
+
+	var out []byte
+	if err := s.bdb.View(func(tx *bbolt.Tx) error {
+		b := tx.Bucket([]byte(key))
+		if b == nil {
+			return nil
+		}
+		data := b.Get([]byte("data"))
+		out = make([]byte, len(data))
+		copy(out, data)
+		return nil
+	}); err != nil {
+		t.Fatalf("reading data for %q: %v", key, err)
+	}
+
+	return out
+}
+
+// TestDeleteIfExpired guards against AWOO-015: a stale async delete scheduled by
+// an expired Get must not erase a value that was refreshed (or otherwise differs
+// from) the generation it observed.
+func TestDeleteIfExpired(t *testing.T) {
+	const key = "challenge"
+
+	for _, tt := range []struct {
+		setup       func(t *testing.T, s *Store) time.Time
+		name        string
+		wantValue   string
+		wantPresent bool
+	}{
+		{
+			name: "deletes the observed expired generation",
+			setup: func(t *testing.T, s *Store) time.Time {
+				mustSet(t, s, key, "old", -time.Minute)
+				return readExpiry(t, s, key)
+			},
+			wantPresent: false,
+		},
+		{
+			name: "preserves a refreshed generation",
+			setup: func(t *testing.T, s *Store) time.Time {
+				mustSet(t, s, key, "old", -time.Minute)
+				observed := readExpiry(t, s, key)
+				mustSet(t, s, key, "fresh", time.Hour)
+				return observed
+			},
+			wantPresent: true,
+			wantValue:   "fresh",
+		},
+		{
+			name: "skips on generation mismatch",
+			setup: func(t *testing.T, s *Store) time.Time {
+				mustSet(t, s, key, "old", -time.Minute)
+				// An expiry we never wrote: even though the stored value is
+				// expired, it is a different generation and must be left alone.
+				return time.Now().Add(-2 * time.Hour)
+			},
+			wantPresent: true,
+			wantValue:   "old",
+		},
+		{
+			name: "skips a non-expired observation",
+			setup: func(t *testing.T, s *Store) time.Time {
+				mustSet(t, s, key, "live", time.Hour)
+				return readExpiry(t, s, key)
+			},
+			wantPresent: true,
+			wantValue:   "live",
+		},
+		{
+			name: "no-op when bucket is absent",
+			setup: func(t *testing.T, s *Store) time.Time {
+				return time.Now().Add(-time.Hour)
+			},
+			wantPresent: false,
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			s := newTestStore(t)
+			observed := tt.setup(t, s)
+
+			if err := s.deleteIfExpired(t.Context(), key, observed); err != nil {
+				t.Fatalf("deleteIfExpired(%q): %v", key, err)
+			}
+
+			got := rawData(t, s, key)
+			switch {
+			case tt.wantPresent && got == nil:
+				t.Fatalf("key %q: want present with value %q, got deleted", key, tt.wantValue)
+			case tt.wantPresent && string(got) != tt.wantValue:
+				t.Errorf("key %q: want value %q, got %q", key, tt.wantValue, string(got))
+			case !tt.wantPresent && got != nil:
+				t.Errorf("key %q: want deleted, got value %q", key, string(got))
+			}
+		})
+	}
+}