Explorar el Código

fix: bound IPs search `q` to anchored IP-shaped prefix (SEC_REVIEW F30)

The admin IP search accepted any free-text `q` and degraded the SQL to
`s.ip_text LIKE '%q%'` whenever `q` did not look like an IP, forcing a
non-indexed full-table scan over `ip_scores` (50k rows at the SPEC's
target). A logged-in Viewer could repeat that with no rate-limit ceiling
on the admin group at the time the finding was filed (since closed by
F29).

`IpsController::parseSearchFilters` now validates `q` against
`/^[0-9a-fA-F:.]+$/` with a 64-char cap (IPv6 max is 39) and returns
400 `validation_failed` otherwise. `IpScoreRepository::searchIps`
drops the substring fallback entirely — it always builds an anchored
`s.ip_text LIKE 'q%'` and silently ignores any `q` that slips through
the controller without matching the same regex (defence-in-depth).

The same input shape closes F46 (`%`/`_` LIKE wildcard injection in
the IPs search) as a side effect, since neither character can survive
the regex.

Regression tests in `api/tests/Integration/Admin/IpsControllerTest.php`:
- `testSearchRejectsNonIpShapedQuery` — `q=foo`, `q=%X%`, `q=_____`,
  `q=g00d`, `q=203.0.113.10%`, and a SQL-injection-shaped value all
  return 400 with `details.q`.
- `testSearchRejectsOverlongQuery` — 65-char `q` returns 400.
- `testSearchQueryIsPrefixAnchoredNotSubstring` — `q=113` no longer
  matches `203.0.113.10`, but `q=203.0.113` still does.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa hace 3 días
padre
commit
2cc1924a4e

+ 12 - 0
api/src/Application/Admin/IpsController.php

@@ -190,6 +190,18 @@ final class IpsController
         $pageSize = max(1, min(200, $pageSize));
 
         $q = isset($query['q']) && is_string($query['q']) && trim($query['q']) !== '' ? trim((string) $query['q']) : null;
+        if ($q !== null) {
+            // SEC_REVIEW F30 / F46: bound `q` to characters that can appear in
+            // a literal IP (digits, hex, `:`, `.`) and cap its length. The
+            // repository uses `q` as an anchored LIKE prefix; rejecting other
+            // shapes here forecloses non-anchored full-table scans and the
+            // `%`/`_` wildcard-injection vector in one go. 64 is well above
+            // the 39-char IPv6 maximum.
+            if (strlen($q) > 64 || preg_match('/^[0-9a-fA-F:.]+$/', $q) !== 1) {
+                $errors['q'] = 'must be an IP or IP prefix (digits, hex, `:`, `.`; max 64 chars)';
+                $q = null;
+            }
+        }
 
         $categoryId = null;
         if (isset($query['category']) && is_string($query['category']) && $query['category'] !== '') {

+ 11 - 12
api/src/Infrastructure/Reputation/IpScoreRepository.php

@@ -116,8 +116,11 @@ class IpScoreRepository extends RepositoryBase
      * Paginated IP search.
      *
      * Filters:
-     *  - q: ip_text substring (uses LIKE 'q%' if it looks like an IP prefix,
-     *    LIKE '%q%' otherwise — slower but small dataset)
+     *  - q: ip_text **prefix** (LIKE 'q%'). The controller validates that
+     *    `q` only contains IP-shaped characters, so a non-anchored
+     *    `%q%` is never produced (SEC_REVIEW F30). Defence-in-depth: this
+     *    method also rejects any `q` with non-IP characters before
+     *    building the LIKE.
      *  - categoryId: only count IPs with a row in this category (score > 0)
      *  - minScore / maxScore: filter MAX(score) per IP via HAVING
      *  - countryCode / asn: filter via LEFT JOIN on ip_enrichment
@@ -149,16 +152,12 @@ class IpScoreRepository extends RepositoryBase
         $params = [];
 
         $q = $filters['q'] ?? null;
-        if (is_string($q) && $q !== '') {
-            // Prefix path: bare IPv4 octets like "203." or "203.0.113." use prefix.
-            // For everything else fall back to substring; small enough to scan.
-            if (preg_match('/^[\da-fA-F:.]+$/', $q) === 1) {
-                $where[] = 's.ip_text LIKE :q';
-                $params['q'] = $q . '%';
-            } else {
-                $where[] = 's.ip_text LIKE :q';
-                $params['q'] = '%' . $q . '%';
-            }
+        if (is_string($q) && $q !== '' && preg_match('/^[\da-fA-F:.]+$/', $q) === 1) {
+            // Anchored prefix only — never `%q%`. The controller already
+            // validates this shape, but the repo enforces it again so a
+            // future caller cannot accidentally drive a full-table scan.
+            $where[] = 's.ip_text LIKE :q';
+            $params['q'] = $q . '%';
         }
 
         $catId = $filters['category_id'] ?? null;

+ 64 - 0
api/tests/Integration/Admin/IpsControllerTest.php

@@ -106,6 +106,70 @@ final class IpsControllerTest extends AppTestCase
         self::assertSame(400, $response->getStatusCode());
     }
 
+    /**
+     * SEC_REVIEW F30: the substring fallback (`%q%`) used to drive a
+     * full-table scan on `ip_scores`. The controller now rejects any `q`
+     * that isn't IP-shaped so the repo only ever runs an anchored
+     * `LIKE 'q%'` prefix scan.
+     */
+    public function testSearchRejectsNonIpShapedQuery(): void
+    {
+        $token = $this->createToken(TokenKind::Admin, role: Role::Viewer);
+        // Each of these would previously have hit the `%q%` substring path.
+        $bad = ['foo', '203.0.113.10 OR 1=1', '%X%', 'g00d', '203.0.113.10%', '_____'];
+        foreach ($bad as $q) {
+            $response = $this->request('GET', '/api/v1/admin/ips?q=' . rawurlencode($q), [
+                'Authorization' => 'Bearer ' . $token,
+            ]);
+            self::assertSame(400, $response->getStatusCode(), "expected 400 for q={$q}");
+            $body = $this->decode($response);
+            self::assertSame('validation_failed', $body['error']);
+            self::assertArrayHasKey('q', $body['details']);
+        }
+    }
+
+    /**
+     * SEC_REVIEW F30: cap on `q` length so even an IP-shaped value cannot
+     * push a multi-megabyte LIKE parameter through the repository.
+     */
+    public function testSearchRejectsOverlongQuery(): void
+    {
+        $token = $this->createToken(TokenKind::Admin, role: Role::Viewer);
+        $tooLong = str_repeat('1', 65); // 64 is the cap.
+        $response = $this->request('GET', '/api/v1/admin/ips?q=' . $tooLong, [
+            'Authorization' => 'Bearer ' . $token,
+        ]);
+        self::assertSame(400, $response->getStatusCode());
+        $body = $this->decode($response);
+        self::assertArrayHasKey('q', $body['details']);
+    }
+
+    /**
+     * SEC_REVIEW F30: an IP-shaped `q` is still treated as an anchored
+     * prefix — the substring path is gone, so a value that previously
+     * matched mid-string no longer matches at all.
+     */
+    public function testSearchQueryIsPrefixAnchoredNotSubstring(): void
+    {
+        $token = $this->createToken(TokenKind::Admin, role: Role::Viewer);
+        $this->seedScored('203.0.113.10', 'brute_force', 1.0);
+        $this->seedScored('198.51.100.5', 'brute_force', 1.0);
+
+        // "113" is a substring of 203.0.113.10 but not a prefix; with the
+        // substring path removed the result is empty.
+        $body = $this->decode($this->request('GET', '/api/v1/admin/ips?q=113', [
+            'Authorization' => 'Bearer ' . $token,
+        ]));
+        self::assertSame(0, $body['total']);
+
+        // The legitimate prefix still matches.
+        $body = $this->decode($this->request('GET', '/api/v1/admin/ips?q=203.0.113', [
+            'Authorization' => 'Bearer ' . $token,
+        ]));
+        self::assertSame(1, $body['total']);
+        self::assertSame('203.0.113.10', $body['items'][0]['ip']);
+    }
+
     public function testDetailReturnsScoresAndStatus(): void
     {
         $token = $this->createToken(TokenKind::Admin, role: Role::Viewer);