Core Rule Set reads request bodies up to SWWAF_WAF_BODY_LIMIT (closes #116)

SWWAF_WAF_BODY_LIMIT (default off, at most 1G) has the Core Rule Set read
form data and multipart up to the limit, the rest streaming on, and JSON
and XML no larger than it, with text/json and the application and text
types ending in +json or +xml. The part read is held for the app. A size
or time limit met while reading ends the request. Content-Encoding is
refused again on these kinds. A body Coraza cannot parse, or a multipart
body failing its strict checks, adds 5, as does a multipart body the limit
cuts before the colon of a part's header. Coraza is built with
no_fs_access, so writes no file. Rule 900300 moves to phase 2.

Judgement call: Content-Encoding is refused on a JSON or XML body too
large to read, as SPEC.md allows.

Model: opus-5-5

README.md

@@ -197,12 +197,11 @@ 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: 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.
+  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.
 - 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
@@ -725,25 +724,23 @@ effective settings are logged at start, unless `SWWAF_LOG_LEVEL` is `warn` or
   (rule 900440), and a multipart body that fails Coraza's strict checks (rule
   900450), add 5 to the score, as a rule rated critical does, since no rule
   reads what comes after the fault. So does a multipart body the limit cuts
-  before the colon of a part's header line, or between the carriage return and
-  the line feed that end a part's header line or the empty line after its
-  headers, since Coraza takes the line the limit cuts for a malformed header.
-  The client has until `SWWAF_CLIENT_REQUEST_TIMEOUT` runs out to send the part
-  that is read, and a request whose body passes `SWWAF_REQUEST_MAX_BYTES` within
-  it is refused before anything reaches the app. Of a file in a multipart body,
-  Coraza counts the bytes and writes nothing. Body inspection suits apps whose
-  forms carry no code. In front of gitea it refuses issue and comment text, wiki
-  pages and files saved in the web editor that hold shell commands or code
-  (932125, 932235, 932250 and others), package descriptions that show code, PyPI
-  uploads (922130), and attachments named like `debug.log` or `config.yml`
-  (932180), until the rule ids the request log names are added to
-  `SWWAF_WAF_DISABLED_RULES`.
+  before the colon of a part's header line, which Coraza cannot tell from a
+  header without one. The client has until `SWWAF_CLIENT_REQUEST_TIMEOUT` runs
+  out to send the part that is read, and a request whose body passes
+  `SWWAF_REQUEST_MAX_BYTES` within it is refused before anything reaches the
+  app. Of a file in a multipart body, Coraza counts the bytes and writes
+  nothing. Body inspection suits apps whose forms carry no code. In front of
+  gitea it refuses issue and comment text, wiki pages and files saved in the web
+  editor that hold shell commands or code (932125, 932235, 932250 and others),
+  package descriptions that show code, PyPI uploads (922130), and attachments
+  named like `debug.log` or `config.yml` (932180), until the rule ids the
+  request log names are added to `SWWAF_WAF_DISABLED_RULES`.
 - `SWWAF_TRAP_PATHS` (default empty): paths the app never serves and only
   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 `?`, as one written with a query
-  does, stops the start.
+  that does not start with `/`, or holds a `?`, which no path a `path` rule sees
+  holds, 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
@@ -1523,15 +1520,14 @@ 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`, 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:<Name>`, 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` 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:<Name>`, 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,

SPEC.md

@@ -825,8 +825,7 @@ 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 without the query, both as received and once
-      percent-decoded, so an encoded probe cannot slip past.
+    - `path`: the URL path as received, before any decoding.
     - `query`: the raw query string.
     - `uri`: path and query together, both as received and once percent-decoded,
       so an encoded probe cannot slip past.

internal/bans/bans.go

@@ -123,8 +123,7 @@ 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 as the
-	// client sent it.
+	// one for a request for a path in SWWAF_TRAP_PATHS, that path.
 	RuleID   string `json:"rule_id,omitempty"`
 	Target   string `json:"target,omitempty"`
 	TrapPath string `json:"trap_path,omitempty"`

internal/config/config.go

@@ -1665,8 +1665,7 @@ 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 / would never match, and one that
-// holds a ? is written with a query.
+// so a path that does not start with / or holds a ? would never match.
 func parseTrapPaths(value string) ([]string, error) {
 	paths, err := parseList(value)
 	if err != nil {

internal/proxy/request.go

@@ -789,19 +789,9 @@ func (rq *request) responseTimedOut() {
 	}
 }
 
-// responseReceived is called once the app has sent its whole answer. An
-// app may answer before the client has sent its whole body, as with a 404
-// it sends without reading it. The timeouts then run on: until the
-// transport's read of the body returns, Go's server cannot send the
-// answer, and SWWAF_CLIENT_REQUEST_TIMEOUT is what ends that read for a
-// client that stops sending.
+// responseReceived is called once the app has sent its whole answer.
 func (rq *request) responseReceived() {
 	rq.complete = true
-
-	if rq.body != nil && !rq.body.received.Load() {
-		return
-	}
-
 	rq.stopTimers()
 }
 

internal/proxy/rules.go

@@ -10,15 +10,13 @@ import (
 )
 
 // trapPath reports whether the request asks for a path in
-// 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.
+// 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.
 func (rq *request) trapPath(now time.Time) bool {
 	path := rules.Path(rq.in)
-	if !slices.Contains(rq.h.config.TrapPaths, path) &&
-		!slices.Contains(rq.h.config.TrapPaths, rules.DecodeOnce(path)) {
+	if !slices.Contains(rq.h.config.TrapPaths, path) {
 		return false
 	}
 

internal/proxy/timeouts_test.go

@@ -194,28 +194,6 @@ func sendLargeBody(t *testing.T, addr string) net.Conn {
 	return conn
 }
 
-func TestClientStopsSendingAfterTheAppHasAnswered(t *testing.T) {
-	t.Parallel()
-
-	// The app answers at once without reading the body, as an app does
-	// for a path it does not serve.
-	app := startApp(t, http.NotFound)
-	addr, out := startProxy(t, app.URL, map[string]string{
-		clientRequestTimeout: shortTimeoutSetting,
-		metricsToken:         token,
-	})
-
-	start := time.Now()
-	got := readResponse(t, sendPartOfBody(t, addr))
-	wantTimedOut(t, start)
-
-	// The app's answer came before the time ran out, so the client is
-	// sent that answer, not smallwebwaf's 408.
-	wantStatus(t, got, http.StatusNotFound)
-	wantLine(t, out.requestLine(t), http.StatusNotFound, requestlog.ActionTimedOut)
-	wantLimitHits(t, addr, clientRequestTimeout, 1)
-}
-
 func TestAppTooSlowToAnswer(t *testing.T) {
 	t.Parallel()
 

internal/proxy/trappaths_test.go

@@ -32,9 +32,10 @@ func TestTrapPathBansAsABanRuleDoes(t *testing.T) {
 	})
 	start := clk.Now()
 
-	// 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"} {
+	// 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",
+	} {
 		s.request(otherClient, path, http.StatusOK, requestlog.ActionForward)
 	}
 
@@ -83,22 +84,6 @@ 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()
 

internal/rules/rules.go

@@ -50,13 +50,6 @@ const quietTime = 2 * time.Second
 // header:<Name>.
 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
 
@@ -381,7 +374,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 pathTarget, "query", uriTarget, "method", "host", "user_agent", "referer":
+	case "path", "query", "uri", "method", "host", "user_agent", "referer":
 		return true
 	}
 
@@ -391,38 +384,35 @@ func isTarget(target string) bool {
 }
 
 // matches reports whether the rule's regex matches its target in r. For
-// 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.
+// 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.
 func (rule Rule) matches(r *http.Request) bool {
-	target := value(rule.Target, r)
+	if rule.Target == "uri" {
+		uri := pathAndQuery(r)
 
-	if rule.Target == pathTarget || rule.Target == uriTarget {
-		return rule.regex.MatchString(target) || rule.regex.MatchString(DecodeOnce(target))
+		return rule.regex.MatchString(uri) || rule.regex.MatchString(decodeOnce(uri))
 	}
 
-	return rule.regex.MatchString(target)
+	return rule.regex.MatchString(value(rule.Target, r))
 }
 
 // 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, as
-// it is and once percent-decoded.
+// re-encoding, up to the first ?: what a path rule is matched against.
 func Path(r *http.Request) string {
 	path, _, _ := strings.Cut(pathAndQuery(r), "?")
 
 	return path
 }
 
-// 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.
+// 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.
 func value(target string, r *http.Request) string {
 	switch target {
-	case pathTarget:
+	case "path":
 		return Path(r)
-	case uriTarget:
-		return pathAndQuery(r)
 	case "query":
 		_, query, _ := strings.Cut(pathAndQuery(r), "?")
 
@@ -474,11 +464,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++ {

internal/rules/rules_test.go

@@ -49,9 +49,7 @@ func TestEachTargetMatchesWhatItNames(t *testing.T) {
 		want   bool
 	}{
 		{"path as received", `path log ^/%2eenv$`, "/%2eenv", nil, true},
-		{"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 not decoded", `path log ^/\.env$`, "/%2eenv", nil, false},
 		{"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},
@@ -420,7 +418,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", "/%2eenv"},
+		"env-file":    {"/.env", "/.env.production", "/.ENV"},
 		"vcs-dir":     {"/.git/config", "/.git", "/.svn/entries"},
 		"secrets-dir": {"/.aws/credentials", "/.ssh/id_rsa"},
 		"secret-file": {"/.htpasswd", "/.DS_Store", "/.git-credentials"},

internal/waf/waf.go

@@ -134,10 +134,8 @@ SecRule REQBODY_PROCESSOR "@rx ^(?:URLENCODED|MULTIPART|JSON|XML)$" \
 # Rule Set rates critical does: no rule reads what comes after the fault,
 # which the app may still read. Coraza's recommended configuration refuses
 # them in its rules 200002 and 200003. A multipart body the limit cuts
-# before the colon of a part's header line, or between the carriage return
-# and the line feed that end a part's header line or the empty line after
-# its headers, adds 5 too, since Coraza takes the line the limit cuts for a
-# malformed header. Coraza parses any form data body.
+# before the colon of a part's header line adds 5 too, since Coraza cannot
+# tell it from a header without one. Coraza parses any form data body.
 SecRule REQBODY_ERROR "!@eq 0" "id:900440,phase:2,pass,severity:'CRITICAL',\
     setvar:'tx.inbound_anomaly_score_pl1=+5'"
 SecRule MULTIPART_STRICT_ERROR "!@eq 0" "id:900450,phase:2,pass,\

internal/waf/waf_test.go

@@ -645,25 +645,6 @@ func TestMultipartBodyCutBeforeAPartHeadersColonIsAMatch(t *testing.T) {
 		body[:bodyLimit+1])
 }
 
-func TestMultipartBodyCutBeforeALineFeedInAPartsHeadersIsAMatch(t *testing.T) {
-	t.Parallel()
-
-	crs := readingBodies(t)
-	headerLine := "--b\r\nContent-Disposition: form-data; name=\"q\"\r"
-
-	// The limit falls between the carriage return and the line feed that
-	// end the second part's header line, and then between those that end
-	// the empty line after it. Coraza, reading up to the limit, takes the
-	// line ending in a lone carriage return for a malformed header.
-	for _, cut := range []int{len(headerLine), len(headerLine + "\n\r")} {
-		first := field("pad", strings.Repeat("a", bodyLimit-len(field("pad", ""))-cut))
-		body := first + field("q", "1") + end
-
-		wantBody(t, crs, post(multipart), body, matched(900440, 900450),
-			body[:bodyLimit+1])
-	}
-}
-
 // TestCorazaWritesNoFile is not parallel, since it sets TMPDIR, the
 // system's temporary directory, for the whole test process.
 func TestCorazaWritesNoFile(t *testing.T) {