From 27f0007b542434e0c6f80e92b9a2dce1fe626098 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Wed, 14 Oct 2026 18:20:31 +0200 Subject: [PATCH] Path rules and trap paths match the path decoded once too (closes #85) A path rule matched only the path as the client sent it, so /%2eenv got past the default env-file rule while an app that decodes the path serves /.env. A path rule now matches the path as sent or decoded once, as a uri rule already did, and so does a path in SWWAF_TRAP_PATHS: /%77p-login.php matches /wp-login.php. The query is cut off before the path is decoded, and the path is decoded only once, so /%252eenv is not /.env. A ban's notes keep the path as sent. SPEC.md and README.md say so. Model: opus-5-5 --- README.md | 32 ++++++++++++------------ SPEC.md | 3 ++- internal/bans/bans.go | 3 ++- internal/config/config.go | 3 ++- internal/proxy/rules.go | 12 +++++---- internal/proxy/trappaths_test.go | 23 ++++++++++++++--- internal/rules/rules.go | 42 ++++++++++++++++++++------------ internal/rules/rules_test.go | 6 +++-- 8 files changed, 79 insertions(+), 45 deletions(-) diff --git a/README.md b/README.md index 95e62e2..3d706dc 100644 --- a/README.md +++ b/README.md @@ -197,11 +197,12 @@ in `bin/state` unless `SWWAF_STATE_DIR` is set, and the default rule file of rate limits and before the rule files, which it does not need. A request for one is refused with `SWWAF_BAN_RESPONSE` and bans the client's netblock for a clear sign of attack, as a `ban` rule's match does. A trap path is matched - against the path a `path` rule sees: as the client sent it, before any - decoding and without the query, the whole of it, character for character. - `/wp-login.php` matches `/wp-login.php?redirect_to=x`, but not - `/wp-login.php/`, `/WP-LOGIN.PHP`, `/blog/wp-login.php` or `/%77p-login.php`. - A client in `SWWAF_ALLOW_NETS` is not checked. + against the path a `path` rule sees: without the query, as the client sent it + and once percent-decoded, the whole of it, character for character. + `/wp-login.php` matches `/wp-login.php?redirect_to=x` and `/%77p-login.php`, + but not `/wp-login.php/`, `/WP-LOGIN.PHP`, `/blog/wp-login.php` or + `/%2577p-login.php`. The ban's notes give the path as the client sent it. A + client in `SWWAF_ALLOW_NETS` is not checked. - Inspects each request with the OWASP Core Rule Set 4.25.0, run by Coraza, after the rule files, unless `SWWAF_WAF_MODE` is `off`: its method, its URL with the query, and its headers, and, once `SWWAF_WAF_BODY_LIMIT` is set, its @@ -741,8 +742,8 @@ effective settings are logged at start, unless `SWWAF_LOG_LEVEL` is `warn` or scanners ask for, such as `/wp-login.php,/xmlrpc.php` in front of gitea; a request for one bans its client for a clear sign of attack, as a `ban` rule does, with or without rule files (see "What it does so far" above). A path - that does not start with `/`, or holds a `?`, which no path a `path` rule sees - holds, stops the start. + that does not start with `/`, or holds a `?`, as one written with a query + does, stops the start. - `SWWAF_ERROR_BURST_THRESHOLD` (default `30`): the most requests of a client in a minute that `smallwebwaf` may refuse after a rule file match, a trap path or a Core Rule Set match, or for a missing or wrong token; one more breaks the @@ -1522,14 +1523,15 @@ scanner-agent user_agent ban (?i)\b(sqlmap|nikto|nuclei|wpscan)\b - The id is letters, digits, `-` and `_`, and no two rules share one. The request log, the metrics and a ban's notes name the rule by it. -- The target is what the regex is matched against: `path` or `query`, as the - client sent it, before any decoding; `uri`, the path and the query together, - both as sent and once percent-decoded, so that an encoded probe does not slip - past; `method`; `host`; `user_agent`; `referer`; or `header:`, any one - request header but `Host` and `Transfer-Encoding`, which Go's HTTP server - takes out of every request; the request's host is the target `host`. A header - sent more than once is matched with its values joined by `, `, and one not - sent as empty text. No body is read. +- The target is what the regex is matched against: `path`, the path without the + query, and `uri`, the path and the query together, each both as sent and once + percent-decoded, so that an encoded probe does not slip past; `query`, as the + client sent it, before any decoding; `method`; `host`; `user_agent`; + `referer`; or `header:`, any one request header but `Host` and + `Transfer-Encoding`, which Go's HTTP server takes out of every request; the + request's host is the target `host`. A header sent more than once is matched + with its values joined by `, `, and one not sent as empty text. No body is + read. - The action is `log`, `block` or `ban` (see "What it does so far" above). Keep `ban` for requests no real visitor sends, and anchor a path at the site root with `^/`: a file of the same name deeper in a site can be ordinary content, diff --git a/SPEC.md b/SPEC.md index 3751ee5..c6e8fb3 100644 --- a/SPEC.md +++ b/SPEC.md @@ -825,7 +825,8 @@ a file edited, added or removed there takes effect while it runs. - `id`: a short name of letters, digits, `-` and `_`, unique across all files. It appears in the request log, metrics, alerts and ban notes. - `target`: what the regex is matched against. - - `path`: the URL path as received, before any decoding. + - `path`: the URL path without the query, both as received and once + percent-decoded, so an encoded probe cannot slip past. - `query`: the raw query string. - `uri`: path and query together, both as received and once percent-decoded, so an encoded probe cannot slip past. diff --git a/internal/bans/bans.go b/internal/bans/bans.go index e67ea08..c07b52f 100644 --- a/internal/bans/bans.go +++ b/internal/bans/bans.go @@ -123,7 +123,8 @@ type Notes struct { LimitPercentSetting string `json:"limit_percent_setting,omitempty"` // RuleID and Target are, for a ban for a clear sign of attack, the id // of the rule file rule that matched, and its target; TrapPath is, for - // one for a request for a path in SWWAF_TRAP_PATHS, that path. + // one for a request for a path in SWWAF_TRAP_PATHS, that path as the + // client sent it. RuleID string `json:"rule_id,omitempty"` Target string `json:"target,omitempty"` TrapPath string `json:"trap_path,omitempty"` diff --git a/internal/config/config.go b/internal/config/config.go index bfc90f3..c86de80 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1665,7 +1665,8 @@ func parsePathPrefixes(value string) ([]string, error) { // parseTrapPaths reads a comma-separated list of trap paths. Each is // matched against a request's path as a path rule is, without the query, -// so a path that does not start with / or holds a ? would never match. +// so a path that does not start with / would never match, and one that +// holds a ? is written with a query. func parseTrapPaths(value string) ([]string, error) { paths, err := parseList(value) if err != nil { diff --git a/internal/proxy/rules.go b/internal/proxy/rules.go index 3ac50d8..9b11f7d 100644 --- a/internal/proxy/rules.go +++ b/internal/proxy/rules.go @@ -10,13 +10,15 @@ import ( ) // trapPath reports whether the request asks for a path in -// SWWAF_TRAP_PATHS: its path as a path rule sees it, before any decoding -// and without the query, is one of them. Such a request is a clear sign of -// attack, as a ban rule's match is: it bans the client's netblock, or in -// observe mode raises the alert for the ban it would have made. +// SWWAF_TRAP_PATHS: its path as a path rule sees it, without the query, +// as sent or once percent-decoded, is one of them. Such a request is a +// clear sign of attack, as a ban rule's match is: it bans the client's +// netblock, or in observe mode raises the alert for the ban it would have +// made. The ban's notes give the path as sent. func (rq *request) trapPath(now time.Time) bool { path := rules.Path(rq.in) - if !slices.Contains(rq.h.config.TrapPaths, path) { + if !slices.Contains(rq.h.config.TrapPaths, path) && + !slices.Contains(rq.h.config.TrapPaths, rules.DecodeOnce(path)) { return false } diff --git a/internal/proxy/trappaths_test.go b/internal/proxy/trappaths_test.go index 1bafd0b..f8b1952 100644 --- a/internal/proxy/trappaths_test.go +++ b/internal/proxy/trappaths_test.go @@ -32,10 +32,9 @@ func TestTrapPathBansAsABanRuleDoes(t *testing.T) { }) start := clk.Now() - // Only the path itself, as the client sent it, is a trap path. - for _, path := range []string{ - "/wp-login.php/", "/WP-LOGIN.PHP", "/blog/xmlrpc.php", "/%77p-login.php", - } { + // Only the path itself, the whole of it in the same case, is a trap + // path. + for _, path := range []string{"/wp-login.php/", "/WP-LOGIN.PHP", "/blog/xmlrpc.php"} { s.request(otherClient, path, http.StatusOK, requestlog.ActionForward) } @@ -84,6 +83,22 @@ func TestTrapPathBansAsABanRuleDoes(t *testing.T) { } } +func TestTrapPathMatchesThePathDecodedOnce(t *testing.T) { + t.Parallel() + + s, _, server := startWithClock(t, "", map[string]string{trapPaths: trapPathList}) + + // %2577 decodes once to %77, not to w. + s.request(otherClient, "/%2577p-login.php", http.StatusOK, requestlog.ActionForward) + s.request(client, "/%77p-login.php", http.StatusForbidden, requestlog.ActionBanned) + + // The ban's notes give the path as the client sent it. + got := server.Ledger.Bans(netip.MustParsePrefix(client + "/32")) + if len(got) != 1 || got[0].Notes.TrapPath != "/%77p-login.php" { + t.Fatalf("bans %+v, want one whose notes give the trap path /%%77p-login.php", got) + } +} + func TestTrapPathsNeedNoRuleFiles(t *testing.T) { t.Parallel() diff --git a/internal/rules/rules.go b/internal/rules/rules.go index f0de5b2..ba024ab 100644 --- a/internal/rules/rules.go +++ b/internal/rules/rules.go @@ -50,6 +50,13 @@ const quietTime = 2 * time.Second // header:. const headerTarget = "header:" +// pathTarget and uriTarget are the targets matched both as received and +// once percent-decoded: the path, and the path and query together. +const ( + pathTarget = "path" + uriTarget = "uri" +) + // escapeLength is the length of a percent escape, such as %2e. const escapeLength = 3 @@ -374,7 +381,7 @@ func parse(line string) (Rule, bool, error) { // isTarget reports whether target is one a rule may have. func isTarget(target string) bool { switch target { - case "path", "query", "uri", "method", "host", "user_agent", "referer": + case pathTarget, "query", uriTarget, "method", "host", "user_agent", "referer": return true } @@ -384,35 +391,38 @@ func isTarget(target string) bool { } // matches reports whether the rule's regex matches its target in r. For -// uri it is matched against the path and query as received, and against -// them once percent-decoded, so that an encoded probe cannot slip past. +// path and uri it is matched against the target as received, and against +// it once percent-decoded, so that an encoded probe cannot slip past. func (rule Rule) matches(r *http.Request) bool { - if rule.Target == "uri" { - uri := pathAndQuery(r) + target := value(rule.Target, r) - return rule.regex.MatchString(uri) || rule.regex.MatchString(decodeOnce(uri)) + if rule.Target == pathTarget || rule.Target == uriTarget { + return rule.regex.MatchString(target) || rule.regex.MatchString(DecodeOnce(target)) } - return rule.regex.MatchString(value(rule.Target, r)) + return rule.regex.MatchString(target) } // Path returns r's path as the client sent it, before any decoding or -// re-encoding, up to the first ?: what a path rule is matched against. +// re-encoding, up to the first ?: what a path rule is matched against, as +// it is and once percent-decoded. func Path(r *http.Request) string { path, _, _ := strings.Cut(pathAndQuery(r), "?") return path } -// value returns what a rule with target, other than uri, is matched -// against in r: the path, as Path gives it, and the query as the client -// sent it, before any decoding or re-encoding, after the first ?, and a -// header's values joined by ", ", as HTTP joins those of a header sent -// more than once. +// value returns what a rule with target is matched against in r: the +// path, as Path gives it, the path and query, as pathAndQuery gives +// them, and the query as the client sent it, before any decoding or +// re-encoding, after the first ?, and a header's values joined by ", ", +// as HTTP joins those of a header sent more than once. func value(target string, r *http.Request) string { switch target { - case "path": + case pathTarget: return Path(r) + case uriTarget: + return pathAndQuery(r) case "query": _, query, _ := strings.Cut(pathAndQuery(r), "?") @@ -464,11 +474,11 @@ func header(r *http.Request, name string) string { return strings.Join(r.Header.Values(name), ", ") } -// decodeOnce returns s with each percent escape, such as %2e, replaced by +// DecodeOnce returns s with each percent escape, such as %2e, replaced by // the byte it stands for. A % that is not followed by two hex digits is // left as it is, so that a malformed escape cannot keep the rest of s // from being decoded. -func decodeOnce(s string) string { +func DecodeOnce(s string) string { var decoded strings.Builder for i := 0; i < len(s); i++ { diff --git a/internal/rules/rules_test.go b/internal/rules/rules_test.go index 31a426e..43d8956 100644 --- a/internal/rules/rules_test.go +++ b/internal/rules/rules_test.go @@ -49,7 +49,9 @@ func TestEachTargetMatchesWhatItNames(t *testing.T) { want bool }{ {"path as received", `path log ^/%2eenv$`, "/%2eenv", nil, true}, - {"path not decoded", `path log ^/\.env$`, "/%2eenv", nil, false}, + {"path decoded", `path log ^/\.env$`, "/%2Eenv", nil, true}, + {"path decoded only once", `path log ^/\.env$`, "/%252eenv", nil, false}, + {"path decoded without the query", `path log ^/a\?b$`, "/a%3fb?c", nil, true}, {"path without the query", `path log ^/a$`, "/a?b=c", nil, true}, {"query as received", `query log ^b=%2e$`, "/a?b=%2e", nil, true}, {"uri as received", `uri log ^/a\?b=%2e$`, "/a?b=%2e", nil, true}, @@ -418,7 +420,7 @@ func TestDefaultFileBansProbesAtTheSiteRootAlone(t *testing.T) { // Probes sent by a browser, by the rule that refuses them. for rule, targets := range map[string][]string{ - "env-file": {"/.env", "/.env.production", "/.ENV"}, + "env-file": {"/.env", "/.env.production", "/.ENV", "/%2eenv"}, "vcs-dir": {"/.git/config", "/.git", "/.svn/entries"}, "secrets-dir": {"/.aws/credentials", "/.ssh/id_rsa"}, "secret-file": {"/.htpasswd", "/.DS_Store", "/.git-credentials"},