diff --git a/docs/docs/CHANGELOG.md b/docs/docs/CHANGELOG.md index 4f234cf5..d9e1cdc9 100644 --- a/docs/docs/CHANGELOG.md +++ b/docs/docs/CHANGELOG.md @@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +- fix: prevent nil pointer panic in challenge validation when threshold rules match during PassChallenge (#1463) + ## v1.25.0: Necron diff --git a/lib/anubis.go b/lib/anubis.go index feff53a3..7d726937 100644 --- a/lib/anubis.go +++ b/lib/anubis.go @@ -106,6 +106,13 @@ func (s *Server) issueChallenge(ctx context.Context, r *http.Request, lg *slog.L //return nil, errors.New("[unexpected] this codepath should be impossible, asked to issue a challenge for a non-challenge rule") } + if rule.Challenge == nil { + rule.Challenge = &config.ChallengeRules{ + Difficulty: s.policy.DefaultDifficulty, + Algorithm: config.DefaultAlgorithm, + } + } + id, err := uuid.NewV7() if err != nil { return nil, err @@ -491,7 +498,11 @@ func (s *Server) PassChallenge(w http.ResponseWriter, r *http.Request) { chall, err := s.getChallenge(r) if err != nil { lg.Error("getChallenge failed", "err", err) - s.respondWithError(w, r, fmt.Sprintf("%s: %s", localizer.T("internal_server_error"), rule.Challenge.Algorithm), makeCode(err)) + algorithm := "unknown" + if rule.Challenge != nil { + algorithm = rule.Challenge.Algorithm + } + s.respondWithError(w, r, fmt.Sprintf("%s: %s", localizer.T("internal_server_error"), algorithm), makeCode(err)) return } @@ -638,8 +649,16 @@ func (s *Server) check(r *http.Request, lg *slog.Logger) (policy.CheckResult, *p } if matches { + challRules := t.Challenge + if challRules == nil { + // Non-CHALLENGE thresholds (ALLOW/DENY) don't have challenge config. + // Use an empty struct so hydrateChallengeRule can fill from stored + // challenge data during validation, rather than baking in defaults + // that could mismatch the difficulty the client actually solved for. + challRules = &config.ChallengeRules{} + } return cr("threshold/"+t.Name, t.Action, weight), &policy.Bot{ - Challenge: t.Challenge, + Challenge: challRules, Rules: &checker.List{}, }, nil } diff --git a/lib/challenge/error.go b/lib/challenge/error.go index 7a9d3094..8c28a1c6 100644 --- a/lib/challenge/error.go +++ b/lib/challenge/error.go @@ -10,6 +10,7 @@ var ( ErrFailed = errors.New("challenge: user failed challenge") ErrMissingField = errors.New("challenge: missing field") ErrInvalidFormat = errors.New("challenge: field has invalid format") + ErrInvalidInput = errors.New("challenge: input is nil or missing required fields") ) func NewError(verb, publicReason string, privateReason error) *Error { diff --git a/lib/challenge/interface.go b/lib/challenge/interface.go index 4bef2e7f..e6d64fd2 100644 --- a/lib/challenge/interface.go +++ b/lib/challenge/interface.go @@ -1,6 +1,7 @@ package challenge import ( + "fmt" "log/slog" "net/http" "sort" @@ -50,12 +51,44 @@ type IssueInput struct { Store store.Interface } +func (in *IssueInput) Valid() error { + if in == nil { + return fmt.Errorf("%w: IssueInput is nil", ErrInvalidInput) + } + if in.Rule == nil { + return fmt.Errorf("%w: Rule is nil", ErrInvalidInput) + } + if in.Rule.Challenge == nil { + return fmt.Errorf("%w: Rule.Challenge is nil", ErrInvalidInput) + } + if in.Challenge == nil { + return fmt.Errorf("%w: Challenge is nil", ErrInvalidInput) + } + return nil +} + type ValidateInput struct { Rule *policy.Bot Challenge *Challenge Store store.Interface } +func (in *ValidateInput) Valid() error { + if in == nil { + return fmt.Errorf("%w: ValidateInput is nil", ErrInvalidInput) + } + if in.Rule == nil { + return fmt.Errorf("%w: Rule is nil", ErrInvalidInput) + } + if in.Rule.Challenge == nil { + return fmt.Errorf("%w: Rule.Challenge is nil", ErrInvalidInput) + } + if in.Challenge == nil { + return fmt.Errorf("%w: Challenge is nil", ErrInvalidInput) + } + return nil +} + type Impl interface { // Setup registers any additional routes with the Impl for assets or API routes. Setup(mux *http.ServeMux) diff --git a/lib/challenge/metarefresh/metarefresh.go b/lib/challenge/metarefresh/metarefresh.go index cb5b023c..cb8dbf8c 100644 --- a/lib/challenge/metarefresh/metarefresh.go +++ b/lib/challenge/metarefresh/metarefresh.go @@ -24,6 +24,10 @@ type Impl struct{} func (i *Impl) Setup(mux *http.ServeMux) {} func (i *Impl) Issue(w http.ResponseWriter, r *http.Request, lg *slog.Logger, in *challenge.IssueInput) (templ.Component, error) { + if err := in.Valid(); err != nil { + return nil, err + } + u, err := r.URL.Parse(anubis.BasePrefix + "/.within.website/x/cmd/anubis/api/pass-challenge") if err != nil { return nil, fmt.Errorf("can't render page: %w", err) @@ -49,6 +53,10 @@ func (i *Impl) Issue(w http.ResponseWriter, r *http.Request, lg *slog.Logger, in } func (i *Impl) Validate(r *http.Request, lg *slog.Logger, in *challenge.ValidateInput) error { + if err := in.Valid(); err != nil { + return challenge.NewError("validate", "invalid input", err) + } + wantTime := in.Challenge.IssuedAt.Add(time.Duration(in.Rule.Challenge.Difficulty) * 800 * time.Millisecond) if time.Now().Before(wantTime) { diff --git a/lib/challenge/preact/preact.go b/lib/challenge/preact/preact.go index 896642e2..e39cd4d8 100644 --- a/lib/challenge/preact/preact.go +++ b/lib/challenge/preact/preact.go @@ -39,6 +39,10 @@ type impl struct{} func (i *impl) Setup(mux *http.ServeMux) {} func (i *impl) Issue(w http.ResponseWriter, r *http.Request, lg *slog.Logger, in *challenge.IssueInput) (templ.Component, error) { + if err := in.Valid(); err != nil { + return nil, err + } + u, err := r.URL.Parse(anubis.BasePrefix + "/.within.website/x/cmd/anubis/api/pass-challenge") if err != nil { return nil, fmt.Errorf("can't render page: %w", err) @@ -57,6 +61,10 @@ func (i *impl) Issue(w http.ResponseWriter, r *http.Request, lg *slog.Logger, in } func (i *impl) Validate(r *http.Request, lg *slog.Logger, in *challenge.ValidateInput) error { + if err := in.Valid(); err != nil { + return challenge.NewError("validate", "invalid input", err) + } + wantTime := in.Challenge.IssuedAt.Add(time.Duration(in.Rule.Challenge.Difficulty) * 80 * time.Millisecond) if time.Now().Before(wantTime) { diff --git a/lib/challenge/proofofwork/proofofwork.go b/lib/challenge/proofofwork/proofofwork.go index b9be014e..3e4bce3b 100644 --- a/lib/challenge/proofofwork/proofofwork.go +++ b/lib/challenge/proofofwork/proofofwork.go @@ -33,6 +33,10 @@ func (i *Impl) Issue(w http.ResponseWriter, r *http.Request, lg *slog.Logger, in } func (i *Impl) Validate(r *http.Request, lg *slog.Logger, in *chall.ValidateInput) error { + if err := in.Valid(); err != nil { + return chall.NewError("validate", "invalid input", err) + } + rule := in.Rule challenge := in.Challenge.RandomData diff --git a/lib/challenge/proofofwork/proofofwork_test.go b/lib/challenge/proofofwork/proofofwork_test.go index 069636bd..f5d6dfb7 100644 --- a/lib/challenge/proofofwork/proofofwork_test.go +++ b/lib/challenge/proofofwork/proofofwork_test.go @@ -30,6 +30,62 @@ func mkRequest(t *testing.T, values map[string]string) *http.Request { return req } +// TestValidateNilRuleChallenge reproduces the panic from +// https://github.com/TecharoHQ/anubis/issues/1463 +// +// When a threshold rule matches during PassChallenge, check() can return +// a policy.Bot with Challenge == nil. After hydrateChallengeRule fails to +// run (or the error path hits before it), Validate dereferences +// rule.Challenge.Difficulty and panics. +func TestValidateNilRuleChallenge(t *testing.T) { + i := &Impl{Algorithm: "fast"} + lg := slog.With() + + // This is the exact response for SHA256("hunter" + "0") with 0 leading zeros required. + const challengeStr = "hunter" + const response = "2652bdba8fb4d2ab39ef28d8534d7694c557a4ae146c1e9237bd8d950280500e" + + req := mkRequest(t, map[string]string{ + "nonce": "0", + "elapsedTime": "69", + "response": response, + }) + + for _, tc := range []struct { + name string + input *challenge.ValidateInput + }{ + { + name: "nil-rule-challenge", + input: &challenge.ValidateInput{ + Rule: &policy.Bot{}, + Challenge: &challenge.Challenge{RandomData: challengeStr}, + }, + }, + { + name: "nil-rule", + input: &challenge.ValidateInput{ + Challenge: &challenge.Challenge{RandomData: challengeStr}, + }, + }, + { + name: "nil-challenge", + input: &challenge.ValidateInput{Rule: &policy.Bot{Challenge: &config.ChallengeRules{Algorithm: "fast"}}}, + }, + { + name: "nil-input", + input: nil, + }, + } { + t.Run(tc.name, func(t *testing.T) { + err := i.Validate(req, lg, tc.input) + if !errors.Is(err, challenge.ErrInvalidInput) { + t.Fatalf("expected ErrInvalidInput, got: %v", err) + } + }) + } +} + func TestBasic(t *testing.T) { i := &Impl{Algorithm: "fast"} bot := &policy.Bot{ diff --git a/lib/http.go b/lib/http.go index 053470f0..e21d839f 100644 --- a/lib/http.go +++ b/lib/http.go @@ -222,8 +222,12 @@ func (s *Server) RenderIndex(w http.ResponseWriter, r *http.Request, cr policy.C chall, err := s.issueChallenge(r.Context(), r, lg, cr, rule) if err != nil { lg.Error("can't get challenge", "err", err) + algorithm := "unknown" + if rule.Challenge != nil { + algorithm = rule.Challenge.Algorithm + } s.ClearCookie(w, CookieOpts{Name: anubis.TestCookieName, Host: r.Host}) - s.respondWithError(w, r, fmt.Sprintf("%s: %s", localizer.T("internal_server_error"), rule.Challenge.Algorithm), makeCode(err)) + s.respondWithError(w, r, fmt.Sprintf("%s: %s", localizer.T("internal_server_error"), algorithm), makeCode(err)) return } @@ -248,9 +252,13 @@ func (s *Server) RenderIndex(w http.ResponseWriter, r *http.Request, cr policy.C impl, ok := challenge.Get(chall.Method) if !ok { - lg.Error("check failed", "err", "can't get algorithm", "algorithm", rule.Challenge.Algorithm) + algorithm := "unknown" + if rule.Challenge != nil { + algorithm = rule.Challenge.Algorithm + } + lg.Error("check failed", "err", "can't get algorithm", "algorithm", algorithm) s.ClearCookie(w, CookieOpts{Name: anubis.TestCookieName, Host: r.Host}) - s.respondWithError(w, r, fmt.Sprintf("%s: %s", localizer.T("internal_server_error"), rule.Challenge.Algorithm), makeCode(err)) + s.respondWithError(w, r, fmt.Sprintf("%s: %s", localizer.T("internal_server_error"), algorithm), makeCode(err)) return }