Core Rule Set: the package registries' searches of gitea and Forgejo pass (closes #354)

Recorded from gitea 1.27.3 and Forgejo 17.0.1 with each registry's own
client, and curl for the requests no client sends, with python3,
powershell, ls -la, .htaccess, file names and plain words. q on the
Cargo, Chef, Composer and Conan searches and NuGet's search service
joins 900603. 900605 lets /bin/sh through on Composer and Conan. New:
900609 for npm's text; 900611, in phase 2, lets one q shaped as a Conan
reference with no +, */ or --, such as zlib/*, skip 942100 if 942100
passes it with each /* comment read as a space; 900612 for NuGet's
Search(), Packages() and their counts: searchTerm, $filter, and the
names $filter, $orderby and $top for 942290.

Model: opus-5-5

SPEC.md

@@ -923,8 +923,8 @@ one entry of `apps` in `internal/coreruleset/coreruleset.go`. gitea's fixes are:
         - `text` of the npm registry's search, for the same four rules and the
           rule for shell paths (932160). An npm name can hold a slash, as
           `@alice/tools-cli` does.
-        - `searchTerm` of the NuGet registry's older searches, for the same four
-          rules. These are the searches:
+        - `searchTerm` of the NuGet v2 API's searches, for the same four rules.
+          These are the searches:
             - `Search()` and `Packages()`;
             - `Search()/$count` and `Packages()/$count`, which count their
               results.
@@ -936,10 +936,13 @@ one entry of `apps` in `internal/coreruleset/coreruleset.go`. gitea's fixes are:
         - `q` of a Conan search, for the rule for SQL injection (942100), which
           refused the pattern `zlib/*`. `q` skips it only when the request has
           one `q`, in the query string or the body. That `q` has the shape of a
-          Conan reference and holds no `*/`, which ends an SQL comment. A
-          reference is a name, then optionally `/` and a version, then
-          optionally `@` and a user, then optionally `/` and a channel. Each
-          part is made of letters, digits, `_`, `.`, `+`, `-` and `*`.
+          Conan reference. A reference is a name, then optionally `/` and a
+          version, then optionally `@` and a user, then optionally `/` and a
+          channel. Each part is made of letters, digits, `_`, `.`, `-` and `*`.
+          A part holds no `+`, since 942100 reads `+` as a space. `q` also holds
+          no `*/`, which ends an SQL comment, and no `--`, which starts one. And
+          942100 finds no SQL injection in `q` once each comment that `/*`
+          starts, an unclosed one included, is replaced by a space.
 
     - The path of these requests, which names a file:
         - a file's page;

internal/coreruleset/gitea.conf

@@ -272,9 +272,18 @@ SecRule REQUEST_FILENAME "@rx ^/api/packages/[^/]+/npm/-/v1/search$" \
 # one q, in the query string or the body, with the shape of a Conan reference.
 # A reference is a name, then optionally / and a version, then optionally @
 # and a user, then optionally / and a channel. Each part is made of letters,
-# digits, _, ., +, - and *. q must also hold no */, which ends an SQL comment.
-# Such a q has no quote, no space and no comment between two words, so SQL
-# injection such as 1' OR '1'='1, 1/**/or/**/1 or 1/*@*/or-1 stays refused.
+# digits, _, ., - and *. A part holds no +, since 942100 reads + as a space.
+# q must also hold no */, which ends an SQL comment, and no --, which starts
+# one. Last, 942100 must find no SQL injection in q once each comment that /*
+# starts, an unclosed one included, is replaced by a space. So zlib/* passes,
+# and SQL injection such as these stays refused:
+#   1' OR '1'='1
+#   1/**/or/**/1
+#   1/*@*/or-1
+#   1+or+1--
+#   1-or-1--
+#   x/1.or.1
+#   1.having.1
 # This rule is in phase 2, because a phase 1 rule runs before the body is read
 # and cannot see a q there. It checks tx.app_gitea, since the skip above does
 # not reach phase 2.
@@ -283,11 +292,13 @@ SecRule REQUEST_FILENAME "@rx ^/api/packages/[^/]+/conan/v[12]/conans/search$" \
     SecRule &TX:app_gitea "@eq 1" "chain"
     SecRule &ARGS:q "@eq 1" "chain"
     SecRule ARGS:q "!@contains */" "chain"
-    SecRule ARGS:q "@rx ^[\w.+*-]+(?:/[\w.+*-]+)?(?:@[\w.+*-]+(?:/[\w.+*-]+)?)?$" \
+    SecRule ARGS:q "!@contains --" "chain"
+    SecRule ARGS:q "!@detectSQLi" "t:none,t:replaceComments,chain"
+    SecRule ARGS:q "@rx ^[\w.*-]+(?:/[\w.*-]+)?(?:@[\w.*-]+(?:/[\w.*-]+)?)?$" \
         "ctl:ruleRemoveTargetById=942100;ARGS:q"
 
-# The NuGet registry's older searches, Search() and Packages(), send their words
-# in searchTerm, or in $filter as an expression such as
+# The NuGet v2 API's searches, Search() and Packages(), send their words in
+# searchTerm, or in $filter as an expression such as
 # tolower(Id) eq 'python3'. Search()/$count and Packages()/$count count a
 # search's results and read the same fields. searchTerm skips these rules:
 #   930120: .htaccess

internal/coreruleset/gitea_test.go

@@ -384,12 +384,12 @@ var giteaRegistrySearches = []string{
 	"/api/packages/alice/nuget/Packages()/$count?$filter=",
 }
 
-// giteaRegistryNameSearches are the package registries' searches whose field
-// also lets /bin/sh through, since a Composer, Conan or npm name can hold a
-// slash.
+// giteaRegistrySearchesLettingBinSh are the package registries' searches
+// whose field also lets /bin/sh through, since a Composer, Conan or npm name
+// can hold a slash.
 //
 //nolint:gochecknoglobals // a constant cannot be a list
-var giteaRegistryNameSearches = []string{
+var giteaRegistrySearchesLettingBinSh = []string{
 	"/api/packages/alice/composer/search.json?q=",
 	"/api/packages/alice/conan/v1/conans/search?q=",
 	"/api/packages/alice/conan/v2/conans/search?q=",
@@ -402,7 +402,7 @@ func TestGiteaAttacksAreRefusedInItsRegistrySearches(t *testing.T) {
 	checkSearchesRefuseAttacks(
 		t,
 		giteaRegistrySearches,
-		giteaRegistryNameSearches,
+		giteaRegistrySearchesLettingBinSh,
 	)
 }
 
@@ -441,7 +441,8 @@ func TestGiteaRegistrySearchExclusionsAreNarrow(t *testing.T) {
 	// another operator on NuGet's searches, and $filter on NuGet's other
 	// paths. The rule for SQL injection (942100) still refuses tools/* in a
 	// field other than a Conan search's, SQL injection in a second q after a
-	// Conan pattern, and SQL comments that stand in for spaces.
+	// Conan pattern, SQL comments or dots that stand in for spaces, and SQL
+	// injection holding + or --.
 	for _, target := range []string{
 		"/api/packages/alice/nuget/Search()?$where=1",
 		"/api/packages/alice/nuget/Packages()?$ne=1",
@@ -454,6 +455,11 @@ func TestGiteaRegistrySearchExclusionsAreNarrow(t *testing.T) {
 			sqlInjection,
 		"/api/packages/alice/conan/v2/conans/search?q=1%2F%2A%2A%2For%2F%2A%2A%2F1",
 		"/api/packages/alice/conan/v2/conans/search?q=1%2F%2A%40%2A%2For-1",
+		"/api/packages/alice/conan/v2/conans/search?q=1%2Bor%2B1--",
+		"/api/packages/alice/conan/v2/conans/search?q=1-or-1--",
+		"/api/packages/alice/conan/v2/conans/search?q=x%2F1.or.1",
+		"/api/packages/alice/conan/v2/conans/search?q=x%2F1.and.1",
+		"/api/packages/alice/conan/v2/conans/search?q=1.having.1",
 	} {
 		if got := inspect(t, crs, get(target)); got.Score < threshold {
 			t.Errorf("%s let through: %+v", target, got)
@@ -473,6 +479,14 @@ func TestGiteaRegistrySearchExclusionsAreNarrow(t *testing.T) {
 		t.Errorf("POST %s %s let through: %+v", search.target, body, got)
 	}
 
+	// So is SQL injection with dots for spaces, in a q sent only in the body.
+	search.target = "/api/packages/alice/conan/v2/conans/search"
+
+	body = "q=x%2F1.or.1"
+	if got, _ := inspectBody(t, readingBodies(t), search, body); got.Score < threshold {
+		t.Errorf("POST %s %s let through: %+v", search.target, body, got)
+	}
+
 	// The rule that lets a Conan pattern through is in phase 2, where it
 	// checks tx.app_gitea itself: a hostname that does not serve gitea
 	// refuses the pattern.