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

Dockerfile

@@ -36,11 +36,12 @@ RUN go mod tidy -diff || \
     { echo "go.mod or go.sum is not tidy: run make tidy" >&2; exit 1; }
 
 # Go's build cache is kept on a tmpfs, out of the image: nothing uses it
-# after this step, and writing it into the image takes seconds.
+# after this step, and writing it into the image takes seconds. The tests
+# are built with the no_fs_access tag, as the binary is in the build stage.
 RUN --mount=type=tmpfs,target=/root/.cache/go-build \
-    go test -timeout 90s -race -cover ./... || \
+    go test -tags no_fs_access -timeout 90s -race -cover ./... || \
     { echo "--- Rerunning with -v for details ---"; \
-      go test -timeout 90s -race -v ./...; exit 1; }
+      go test -tags no_fs_access -timeout 90s -race -v ./...; exit 1; }
 
 # Tidy stage: `go mod tidy` in the test phase's Go, so that the files it
 # writes pass the test phase's check. Nothing else depends on it, so only
@@ -84,7 +85,10 @@ COPY . .
 # The VERSION build arg when one is given, otherwise
 # `git describe --tags --always` on the .git in the build context. With
 # .git present, a version that is still empty, dev or unknown fails the
-# build: git is missing or could not read the checkout.
+# build: git is missing or could not read the checkout. The no_fs_access
+# tag keeps Coraza from writing the files of a multipart body to the
+# system's temporary directory, since smallwebwaf writes only to its state
+# directory.
 ARG VERSION
 RUN VERSION="${VERSION:-$(git describe --tags --always)}"; \
     if [ -e .git ]; then \
@@ -93,7 +97,7 @@ RUN VERSION="${VERSION:-$(git describe --tags --always)}"; \
             exit 1 ;; \
         esac; \
     fi; \
-    CGO_ENABLED=0 go build -trimpath \
+    CGO_ENABLED=0 go build -tags no_fs_access -trimpath \
         -ldflags="-s -w -X main.Version=${VERSION}" \
         -o /usr/local/bin/smallwebwaf ./cmd/smallwebwaf
 

README.md

@@ -711,23 +711,28 @@ effective settings are logged at start, unless `SWWAF_LOG_LEVEL` is `warn` or
   `SWWAF_RATE_LIMIT_EXEMPT_PATHS` matches its own: a request whose path holds
   `..`, a backslash or an encoded slash is inspected whatever its prefix.
 - `SWWAF_WAF_BODY_LIMIT` (default `off`): `off` has the Core Rule Set read no
-  request body. A size, such as `128K`, 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. 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. 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. Coraza writes what it reads of each file in a
-  multipart body to the system's temporary directory, and removes it once the
-  request is inspected. 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`.
+  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 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 read 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,
+  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

internal/config/config.go

@@ -398,6 +398,7 @@ var (
 	errNeedsDBPath          = errors.New("it names the file to look clients up in")
 	errDBPathUnused         = errors.New("only file reads it")
 	errNotOver4K            = errors.New("is not a size of more than 4K, such as 32K")
+	errOver1G               = errors.New("is more than 1G, the most Coraza reads")
 	errNotDurationAboveZero = errors.New(
 		"is not a duration above zero, such as 1h or 7d")
 	errNotNumberAboveZero = errors.New(
@@ -561,7 +562,7 @@ func FromEnvironment(lookupEnv func(string) (string, bool)) (*Config, error) {
 		WAFDisabledRules: env.ruleIDs("SWWAF_WAF_DISABLED_RULES",
 			"920340,920420,920440,920640,930130,930140"),
 		WAFExemptPaths:       env.pathPrefixes("SWWAF_WAF_EXEMPT_PATHS", ""),
-		WAFBodyLimit:         env.size("SWWAF_WAF_BODY_LIMIT", off),
+		WAFBodyLimit:         env.wafBodyLimit("SWWAF_WAF_BODY_LIMIT", off),
 		TrapPaths:            env.trapPaths("SWWAF_TRAP_PATHS"),
 		ErrorBurstThreshold:  env.count("SWWAF_ERROR_BURST_THRESHOLD", "30"),
 		LogRemoteURL:         env.logRemoteURL("SWWAF_LOG_REMOTE_URL"),
@@ -767,6 +768,15 @@ func (e *environment) size(name, defaultValue string) int64 {
 	return size
 }
 
+// wafBodyLimit reads the setting that is the most of a request body the
+// Core Rule Set reads.
+func (e *environment) wafBodyLimit(name, defaultValue string) int64 {
+	limit, err := parseWAFBodyLimit(e.value(name, defaultValue))
+	e.check(name, err)
+
+	return limit
+}
+
 // headerSize reads the setting that is the largest request line and
 // headers.
 func (e *environment) headerSize(name, defaultValue string) int64 {
@@ -1435,6 +1445,22 @@ func parseHeaderSize(value string) (int64, error) {
 	return size, nil
 }
 
+// parseWAFBodyLimit reads the most of a request body the Core Rule Set
+// reads: a size as parseSize reads it, or off, but at most 1G, since
+// Coraza, which runs the Core Rule Set, refuses to load with more.
+func parseWAFBodyLimit(value string) (int64, error) {
+	limit, err := parseSize(value)
+	if err != nil {
+		return 0, err
+	}
+
+	if limit > gibibyte {
+		return 0, fmt.Errorf("%q %w", value, errOver1G)
+	}
+
+	return limit, nil
+}
+
 // splitUnit splits a size into its number and the bytes its suffix
 // stands for.
 func splitUnit(value string) (string, int64) {

internal/config/config_test.go

@@ -615,6 +615,15 @@ func TestCoreRuleSetSettings(t *testing.T) {
 	}
 }
 
+func TestWAFBodyLimitOf1G(t *testing.T) {
+	t.Parallel()
+
+	cfg := fromEnvironment(t, environment{wafBodyLimit: "1G"})
+	if cfg.WAFBodyLimit != 1<<30 {
+		t.Errorf("1G read as %d", cfg.WAFBodyLimit)
+	}
+}
+
 func TestInvalidCoreRuleSetSettingStopsTheStart(t *testing.T) {
 	t.Parallel()
 
@@ -643,12 +652,15 @@ func TestInvalidCoreRuleSetSettingStopsTheStart(t *testing.T) {
 			`"-942100" is not the id of a Core Rule Set rule, ` +
 				`a whole number such as 942100`,
 		},
-		// The paranoia level, the allowed methods, the headers refused, and
-		// a request with more query parameters than Coraza keeps.
+		// The paranoia level, the allowed methods, the headers refused, a
+		// request with more query parameters than Coraza keeps, and a body
+		// Coraza cannot parse or that fails its strict checks.
 		{wafDisabledRules, "942100,900000", `"900000"` + setupRule},
 		{wafDisabledRules, "942100,900200", `"900200"` + setupRule},
 		{wafDisabledRules, "942100,900250", `"900250"` + setupRule},
 		{wafDisabledRules, "942100,900300", `"900300"` + setupRule},
+		{wafDisabledRules, "942100,900440", `"900440"` + setupRule},
+		{wafDisabledRules, "942100,900450", `"900450"` + setupRule},
 		{
 			wafExemptPaths, "api/",
 			`"api/" is not a path prefix starting with /, such as /assets/`,
@@ -657,6 +669,11 @@ func TestInvalidCoreRuleSetSettingStopsTheStart(t *testing.T) {
 			wafBodyLimit, "128KB",
 			`"128KB" is not a size such as 512K, 100M or 5G, or off`,
 		},
+		{wafBodyLimit, "2G", `"2G" is more than 1G, the most Coraza reads`},
+		{
+			wafBodyLimit, "1073741825",
+			`"1073741825" is more than 1G, the most Coraza reads`,
+		},
 	} {
 		t.Run(tc.name+"="+tc.value, func(t *testing.T) {
 			t.Parallel()

internal/proxy/proxy.go

@@ -245,8 +245,8 @@ func newCoreRuleSet(cfg *config.Config) *waf.CoreRuleSet {
 	})
 	if err != nil {
 		// The Core Rule Set is built in, and the settings cannot break it:
-		// the paranoia level is from 1 to 4, and the id of no rule switches
-		// nothing off.
+		// the paranoia level is from 1 to 4, the body limit at most 1G, and
+		// the id of no rule switches nothing off.
 		panic(err)
 	}
 

internal/waf/waf.go

@@ -3,6 +3,11 @@
 // body while SWWAF_WAF_BODY_LIMIT is set, with the six changes smallwebwaf
 // makes to it, as "Attack detection" under "Configuration surface" in
 // SPEC.md describes them. It reads no response.
+//
+// smallwebwaf writes only to its state directory, so Coraza is built with
+// its no_fs_access tag, as the Dockerfile and script/build build it: of a
+// file in a multipart body, Coraza then counts the bytes instead of
+// writing them to the system's temporary directory.
 package waf
 
 import (
@@ -100,16 +105,19 @@ 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; 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
 SecRequestBodyLimitAction ProcessPartial
 
-SecRule REQUEST_HEADERS:Content-Type "@rx ^(?:application(?:/soap[+]|/)|text/)xml" \
+SecRule REQUEST_HEADERS:Content-Type \
+    "@rx ^(?:application|text)/(?:[a-z0-9.-]+[+])?xml" \
     "id:900410,phase:1,pass,nolog,t:none,t:lowercase,ctl:requestBodyProcessor=XML"
-SecRule REQUEST_HEADERS:Content-Type "@rx ^application/(?:[a-z0-9.-]+[+])?json" \
+SecRule REQUEST_HEADERS:Content-Type \
+    "@rx ^(?:application|text)/(?:[a-z0-9.-]+[+])?json" \
     "id:900420,phase:1,pass,nolog,t:none,t:lowercase,ctl:requestBodyProcessor=JSON"
 
 # The rest of the second change: Content-Encoding is refused again on a
@@ -119,6 +127,22 @@ SecRule REQBODY_PROCESSOR "@rx ^(?:URLENCODED|MULTIPART|JSON|XML)$" \
     "id:900260,phase:1,pass,nolog,\
     setvar:'tx.restricted_headers_basic=%%{tx.restricted_headers_basic} \
     /content-encoding/'"
+
+# A body Coraza fails to parse (900440), and a multipart body that fails
+# 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 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,\
+    severity:'CRITICAL',setvar:'tx.inbound_anomaly_score_pl1=+5'"
 `
 
 // cookiesNotRead are the cookies the Core Rule Set reads a request
@@ -195,9 +219,7 @@ func (c *CoreRuleSet) Inspect(
 	r *http.Request, client netip.Addr, body io.Reader,
 ) (Result, []byte, error) {
 	tx := c.waf.NewTransaction()
-	// Closing removes the files Coraza writes, in the system's temporary
-	// directory, for the file parts of a multipart body it reads. One it
-	// cannot remove is left there, and changes nothing in what was found.
+	// Closing would remove the files Coraza wrote, and it writes none.
 	defer func() { _ = tx.Close() }()
 
 	tx.ProcessConnection(client.String(), 0, "", 0)

internal/waf/waf_test.go

@@ -5,6 +5,7 @@ import (
 	"net/http/httptest"
 	"net/netip"
 	"net/url"
+	"path/filepath"
 	"reflect"
 	"strconv"
 	"strings"
@@ -516,8 +517,13 @@ func TestContentEncodingIsRefusedOnTheKindsOfBodyTheCoreRuleSetReads(t *testing.
 
 	crs := readingBodies(t)
 
-	for _, header := range []string{formData, multipart, jsonBody, xmlBody} {
-		wantBody(t, crs, post(header, gzip), "a", matched(920450), "a")
+	for _, tc := range []struct{ header, body string }{
+		{formData, "a=1"},
+		{multipart, field("a", "1") + end},
+		{jsonBody, `{"a":1}`},
+		{xmlBody, "<a>1</a>"},
+	} {
+		wantBody(t, crs, post(tc.header, gzip), tc.body, matched(920450), tc.body)
 	}
 
 	// Whatever its size: a JSON body larger than the limit is not read, but
@@ -578,3 +584,77 @@ func TestGiteaBodiesTheCoreRuleSetRefuses(t *testing.T) {
 		"filename=\"debug.log\"\r\nContent-Type: text/plain\r\n\r\nstarted\r\n" + end
 	wantBody(t, crs, post(multipart), attachment, matched(932180), attachment)
 }
+
+func TestTypesEndingInXMLOrJSONAndTextJSONAreRead(t *testing.T) {
+	t.Parallel()
+
+	crs := readingBodies(t)
+
+	for _, tc := range []struct{ contentType, body string }{
+		{"application/atom+xml", "<q>" + injection + "</q>"},
+		{"application/vnd.example+xml", "<q>" + injection + "</q>"},
+		{"application/vnd.example+json", `{"q":"` + injection + `"}`},
+		{"text/json", `{"q":"` + injection + `"}`},
+	} {
+		wantBody(t, crs, post("Content-Type: "+tc.contentType), tc.body,
+			matched(942100), tc.body)
+	}
+}
+
+func TestBodyCorazaCannotParseIsAMatch(t *testing.T) {
+	t.Parallel()
+
+	crs := readingBodies(t)
+
+	// An end tag after the root element, past which Coraza reads none of
+	// the body, while an app may still read the attack before it.
+	body := "<q>" + injection + "</q></r>"
+	wantBody(t, crs, post(xmlBody), body, matched(900440), body)
+
+	// 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.
+	body = field("q", injection) + end
+	wantBody(t, crs, post(multipart+"; boundary=c"), body, matched(900440, 900450),
+		body)
+
+	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 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, 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, waf.Result{},
+		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) {
+	// The system's temporary directory is one that does not exist, so that
+	// Coraza could write nothing there: built without no_fs_access, it
+	// refuses to load, and could not write a file of a multipart body.
+	t.Setenv("TMPDIR", filepath.Join(t.TempDir(), "missing"))
+
+	body := "--b\r\nContent-Disposition: form-data; name=\"file\"; " +
+		"filename=\"notes.txt\"\r\nContent-Type: text/plain\r\n\r\n" +
+		strings.Repeat("a", 1000) + "\r\n" + field("q", injection) + end
+	wantBody(t, readingBodies(t), post(multipart), body, matched(942100), body)
+}
+
+func TestBodyLimitOf1GLoads(t *testing.T) {
+	t.Parallel()
+
+	_, err := waf.New(waf.Params{ParanoiaLevel: 1, BodyLimit: 1 << 30})
+	if err != nil {
+		t.Errorf("load the Core Rule Set reading bodies up to 1G: %v", err)
+	}
+}

script/build

@@ -1,7 +1,7 @@
 #!/bin/sh
 # script/build: build bin/smallwebwaf on the host, with Go installed, for
 # working on the code by hand. The version it reports comes from git, as
-# in script/docker.
+# in script/docker, and the no_fs_access tag is the Dockerfile's.
 set -eu
 
 SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)"
@@ -11,7 +11,7 @@ main() {
     cd "$ROOT"
     version="$(git describe --tags --always --dirty 2>/dev/null || true)"
     [ -n "$version" ] || version="unknown"
-    go build -trimpath -ldflags "-X main.Version=$version" \
+    go build -tags no_fs_access -trimpath -ldflags "-X main.Version=$version" \
         -o bin/smallwebwaf ./cmd/smallwebwaf
 }