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 (with +json, text/json and +xml) no larger than it. 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, but
not a multipart body reaching the limit. 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
@@ -715,19 +714,15 @@ effective settings are logged at start, unless `SWWAF_LOG_LEVEL` is `warn` or
   request body. A size, such as `128K`, at most `1G`, has it read a body of form
   data or multipart up to that size, the rest of a longer one passing on to the
   app as it arrives, without being held, and a JSON or XML body no larger than
-  that size, since those cannot be read in part. A body is JSON when its type is
-  `application/json` or `text/json`, or an `application/` or `text/` type ending
-  in `+json`, and XML when its type is `application/xml` or `text/xml`, or an
-  `application/` or `text/` type ending in `+xml`. Any other body reaches the
-  app uninspected, and so does a larger JSON or XML body: the Core Rule Set
-  would read any other body as form data, where binary content such as a git
-  push trips rules written for text. A body it reads that Coraza cannot parse
-  (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.
+  that size, since those cannot be read in part. A body whose type ends in
+  `+json`, or is `text/json`, is JSON, and one whose type ends in `+xml` is XML.
+  Any other body reaches the app uninspected, and so does a larger JSON or XML
+  body: the Core Rule Set would read any other body as form data, where binary
+  content such as a git push trips rules written for text. A body it reads that
+  Coraza cannot parse (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; a multipart body that
+  reaches the limit does not, since the limit can cut it inside a part's 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,
@@ -742,8 +737,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 `?`, 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 +1518,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

@@ -105,10 +105,9 @@ SecRuleUpdateTargetById 944110 "!REQUEST_HEADERS:Referer"
 // bodyDirectives have the Core Rule Set read the part of a request body
 // Inspect gives it, which is at most one byte longer than the limit, up to
 // the limit, %d bytes, and read JSON and XML as Coraza's recommended
-// configuration has it in its rules 200000, 200001 and 200006, with
-// text/json, and any application or text type ending in +xml or +json,
-// besides; form data and multipart Coraza knows by itself. %% stands for
-// a % Coraza reads.
+// configuration has it in its rules 200000, 200001 and 200006, with any
+// type ending in +xml, and text/json, besides; form data and multipart
+// Coraza knows by itself. %% stands for a % Coraza reads.
 const bodyDirectives = `
 SecRequestBodyAccess On
 SecRequestBodyLimit %d
@@ -133,11 +132,13 @@ SecRule REQBODY_PROCESSOR "@rx ^(?:URLENCODED|MULTIPART|JSON|XML)$" \
 # its strict checks (900450), each add 5 to the score, as a rule the Core
 # 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.
+# them in its rules 200002 and 200003. A multipart body that reaches the
+# limit is let off both (900430), since the limit can cut it inside a
+# part's header, which Coraza takes for such a fault. Coraza parses any
+# form data body.
+SecRule INBOUND_DATA_ERROR "@eq 1" "id:900430,phase:2,pass,nolog,chain"
+    SecRule REQBODY_PROCESSOR "@streq MULTIPART" \
+        "ctl:ruleRemoveById=900440,ctl:ruleRemoveById=900450"
 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

@@ -613,57 +613,29 @@ func TestBodyCorazaCannotParseIsAMatch(t *testing.T) {
 
 	// The multipart bodies Coraza cannot parse fail its strict checks too:
 	// one whose type names its boundary twice, and one with a part header
-	// that has no colon, before the attack. They do so padded past the
-	// limit too, which cuts them in the padding.
-	noColon := "--b\r\nContent-Disposition form-data; name=\"a\"\r\n\r\n1\r\n"
-	pad := field("pad", strings.Repeat("a", bodyLimit))
+	// that has no colon, before the attack.
+	body = field("q", injection) + end
+	wantBody(t, crs, post(multipart+"; boundary=c"), body, matched(900440, 900450),
+		body)
 
-	for _, tc := range []struct{ header, head string }{
-		{multipart + "; boundary=c", ""},
-		{multipart, noColon},
-	} {
-		body = tc.head + field("q", injection) + end
-		wantBody(t, crs, post(tc.header), body, matched(900440, 900450), body)
-
-		body = tc.head + field("q", injection) + pad + end
-		wantBody(t, crs, post(tc.header), body, matched(900440, 900450),
-			body[:bodyLimit+1])
-	}
+	body = "--b\r\nContent-Disposition form-data; name=\"a\"\r\n\r\n1\r\n" +
+		field("q", injection) + end
+	wantBody(t, crs, post(multipart), body, matched(900440, 900450), body)
 }
 
-func TestMultipartBodyCutBeforeAPartHeadersColonIsAMatch(t *testing.T) {
+func TestMultipartBodyCutInsideAPartHeaderIsNotAMatch(t *testing.T) {
 	t.Parallel()
 
 	// The limit falls in the middle of the name of the second part's
-	// header, which Coraza, reading up to the limit, cannot tell from a
-	// header without a colon.
+	// header, which Coraza, reading up to the limit, finds without a colon.
 	cut := "--b\r\nContent-Di"
 	first := field("pad", strings.Repeat("a", bodyLimit-len(field("pad", ""))-len(cut)))
 	body := first + cut + "sposition: form-data; name=\"q\"\r\n\r\n1\r\n" + end
 
-	wantBody(t, readingBodies(t), post(multipart), body, matched(900440, 900450),
+	wantBody(t, readingBodies(t), post(multipart), body, waf.Result{},
 		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) {