Merge pull request #988 from AikidoSec/default-block-invalid-sql-off

Default AIKIDO_BLOCK_INVALID_SQL to off

Author: hansott

docs/invalid-sql-queries.md

@@ -2,10 +2,10 @@
 
 Zen blocks SQL queries that it can't tokenize when they contain user input. This prevents attackers from bypassing SQL injection detection with malformed queries. For example, ClickHouse ignores invalid SQL after `;`, and SQLite runs queries before an unclosed `/*` comment.
 
-This is on by default. In blocking mode, these queries are blocked. In detection-only mode, they are reported but still executed.
-
-If you see false positives (legitimate queries being blocked), disable it with:
+This is off by default. To enable it, set the environment variable:
 
 ```
-AIKIDO_BLOCK_INVALID_SQL=false node app.js
+AIKIDO_BLOCK_INVALID_SQL=true node app.js
 ```
+
+In blocking mode, these queries are blocked. In detection-only mode, they are reported but still executed.

end2end/tests/express-mysql.test.js

@@ -156,7 +156,7 @@ t.test("it does not block in dry mode", (t) => {
     });
 });
 
-t.test("it blocks invalid SQL queries by default", (t) => {
+t.test("it does not block invalid SQL queries by default", (t) => {
   const server = spawn(`node`, [pathToApp, "4003"], {
     env: {
       ...process.env,
@@ -187,7 +187,7 @@ t.test("it blocks invalid SQL queries by default", (t) => {
   timeout(2000)
     .then(() => {
       return fetch(
-        `http://localhost:4003/invalid-query?sql=${encodeURIComponent("I'm a test")}`,
+        `http://localhost:4003/invalid-query?sql=${encodeURIComponent("SELECT * FROM test")}`,
         {
           signal: AbortSignal.timeout(5000),
           method: "POST",
@@ -196,7 +196,7 @@ t.test("it blocks invalid SQL queries by default", (t) => {
     })
     .then((response) => {
       t.equal(response.status, 500);
-      t.match(stdout, /Zen has blocked an SQL injection/);
+      t.notMatch(stdout, /Zen has blocked an SQL injection/);
     })
     .catch((error) => {
       t.fail(error.message);
@@ -207,14 +207,14 @@ t.test("it blocks invalid SQL queries by default", (t) => {
 });
 
 t.test(
-  "it does not block invalid SQL queries when AIKIDO_BLOCK_INVALID_SQL is false",
+  "it blocks invalid SQL queries when AIKIDO_BLOCK_INVALID_SQL is true",
   (t) => {
     const server = spawn(`node`, [pathToApp, "4004"], {
       env: {
         ...process.env,
         AIKIDO_DEBUG: "true",
         AIKIDO_BLOCKING: "true",
-        AIKIDO_BLOCK_INVALID_SQL: "false",
+        AIKIDO_BLOCK_INVALID_SQL: "true",
       },
     });
 
@@ -240,7 +240,7 @@ t.test(
     timeout(2000)
       .then(() => {
         return fetch(
-          `http://localhost:4004/invalid-query?sql=${encodeURIComponent("SELECT * FROM test")}`,
+          `http://localhost:4004/invalid-query?sql=${encodeURIComponent("I'm a test")}`,
           {
             signal: AbortSignal.timeout(5000),
             method: "POST",
@@ -249,7 +249,7 @@ t.test(
       })
       .then((response) => {
         t.equal(response.status, 500);
-        t.notMatch(stdout, /Zen has blocked an SQL injection/);
+        t.match(stdout, /Zen has blocked an SQL injection/);
       })
       .catch((error) => {
         t.fail(error.message);

library/agent/Agent.ts

@@ -36,6 +36,7 @@ import { AttackWaveDetector } from "../vulnerabilities/attack-wave-detection/Att
 import type { FetchListsAPI } from "./api/FetchListsAPI";
 import { PendingEvents } from "./PendingEvents";
 import type { IdorProtectionConfig } from "./IdorProtectionConfig";
+import { warnIfBlockInvalidSqlDisabled } from "../helpers/warnIfBlockInvalidSqlDisabled";
 import { warnIfTsxIsUsed } from "../helpers/warnIfTsxIsUsed";
 
 type WrappedPackage = { version: string | null; supported: boolean };
@@ -521,6 +522,7 @@ export class Agent {
       }
     }
 
+    warnIfBlockInvalidSqlDisabled();
     warnIfTsxIsUsed();
 
     // When our library is required, we are not intercepting `require` calls yet

library/helpers/shouldBlockInvalidSqlQueries.ts

@@ -0,0 +1,9 @@
+import { envToBool } from "./envToBool";
+
+export function shouldBlockInvalidSqlQueries(): boolean {
+  if (process.env.AIKIDO_BLOCK_INVALID_SQL === undefined) {
+    return false;
+  }
+
+  return envToBool(process.env.AIKIDO_BLOCK_INVALID_SQL);
+}

library/helpers/warnIfBlockInvalidSqlDisabled.test.ts

@@ -0,0 +1,38 @@
+import * as t from "tap";
+import { warnIfBlockInvalidSqlDisabled } from "./warnIfBlockInvalidSqlDisabled";
+
+const logs: string[] = [];
+// oxlint-disable-next-line no-console
+console.log = function log(message: string) {
+  logs.push(message);
+};
+
+t.beforeEach(() => {
+  delete process.env.AIKIDO_BLOCK_INVALID_SQL;
+  logs.length = 0;
+});
+
+const expectedMessage =
+  "AIKIDO: We recommend setting AIKIDO_BLOCK_INVALID_SQL=true. See https://github.com/AikidoSec/firewall-node/blob/main/docs/invalid-sql-queries.md";
+
+t.test("it warns when AIKIDO_BLOCK_INVALID_SQL is not set", async (t) => {
+  warnIfBlockInvalidSqlDisabled();
+
+  t.same(logs, [expectedMessage]);
+});
+
+t.test("it does not warn when AIKIDO_BLOCK_INVALID_SQL is true", async (t) => {
+  process.env.AIKIDO_BLOCK_INVALID_SQL = "true";
+
+  warnIfBlockInvalidSqlDisabled();
+
+  t.same(logs, []);
+});
+
+t.test("it warns when AIKIDO_BLOCK_INVALID_SQL is false", async (t) => {
+  process.env.AIKIDO_BLOCK_INVALID_SQL = "false";
+
+  warnIfBlockInvalidSqlDisabled();
+
+  t.same(logs, [expectedMessage]);
+});

library/helpers/warnIfBlockInvalidSqlDisabled.ts

@@ -0,0 +1,12 @@
+import { shouldBlockInvalidSqlQueries } from "./shouldBlockInvalidSqlQueries";
+
+export function warnIfBlockInvalidSqlDisabled() {
+  if (shouldBlockInvalidSqlQueries()) {
+    return;
+  }
+
+  // oxlint-disable-next-line no-console
+  console.log(
+    "AIKIDO: We recommend setting AIKIDO_BLOCK_INVALID_SQL=true. See https://github.com/AikidoSec/firewall-node/blob/main/docs/invalid-sql-queries.md"
+  );
+}

library/sinks/SQLite3.test.ts

@@ -130,6 +130,7 @@ t.test("it detects SQL injections", async () => {
 
     // Query with syntax error and user input should be blocked by Zen
     // because the SQL tokenizer can't parse the query (unclosed quote)
+    process.env.AIKIDO_BLOCK_INVALID_SQL = "true";
     const syntaxError = await t.rejects(async () => {
       await runWithContext(
         { ...dangerousContext, body: { name: "SELECT * FROM test" } },
@@ -144,6 +145,7 @@ t.test("it detects SQL injections", async () => {
         "Zen has blocked an SQL injection: sqlite3.run(...) originating from body.name"
       );
     }
+    delete process.env.AIKIDO_BLOCK_INVALID_SQL;
   } catch (error: any) {
     t.fail(error);
   } finally {

library/vulnerabilities/sql-injection/checkContextForSqlInjection.test.ts

@@ -63,33 +63,22 @@ function createFailingContext(): Context {
   };
 }
 
-t.test("it blocks failed tokenization by default", async () => {
+t.test("it does not block failed tokenization by default", async () => {
   t.same(
     checkContextForSqlInjection({
       sql: failingSQL,
       operation: "mysql.query",
       dialect: new SQLDialectMySQL(),
       context: createFailingContext(),
     }),
-    {
-      operation: "mysql.query",
-      kind: "sql_injection",
-      source: "body",
-      pathsToPayload: [".comment"],
-      metadata: {
-        sql: failingSQL,
-        dialect: "MySQL",
-        failedToTokenize: "true",
-      },
-      payload: failingInput,
-    }
+    undefined
   );
 });
 
 t.test(
-  "it does not block failed tokenization when AIKIDO_BLOCK_INVALID_SQL is false",
+  "it blocks failed tokenization when AIKIDO_BLOCK_INVALID_SQL is true",
   async () => {
-    process.env.AIKIDO_BLOCK_INVALID_SQL = "false";
+    process.env.AIKIDO_BLOCK_INVALID_SQL = "true";
 
     t.same(
       checkContextForSqlInjection({
@@ -98,7 +87,18 @@ t.test(
         dialect: new SQLDialectMySQL(),
         context: createFailingContext(),
       }),
-      undefined
+      {
+        operation: "mysql.query",
+        kind: "sql_injection",
+        source: "body",
+        pathsToPayload: [".comment"],
+        metadata: {
+          sql: failingSQL,
+          dialect: "MySQL",
+          failedToTokenize: "true",
+        },
+        payload: failingInput,
+      }
     );
   }
 );

library/vulnerabilities/sql-injection/checkContextForSqlInjection.ts

@@ -1,23 +1,15 @@
 import { Context } from "../../agent/Context";
 import { InterceptorResult } from "../../agent/hooks/InterceptorResult";
 import { getPathsToPayload } from "../../helpers/attackPath";
-import { envToBool } from "../../helpers/envToBool";
 import { extractStringsFromUserInputCached } from "../../helpers/extractStringsFromUserInputCached";
 import { getSourceForUserString } from "../../helpers/getSourceForUserString";
+import { shouldBlockInvalidSqlQueries } from "../../helpers/shouldBlockInvalidSqlQueries";
 import {
   detectSQLInjection,
   SQLInjectionDetectionResult,
 } from "./detectSQLInjection";
 import { SQLDialect } from "./dialects/SQLDialect";
 
-function shouldBlockInvalidSqlQueries(): boolean {
-  if (process.env.AIKIDO_BLOCK_INVALID_SQL === undefined) {
-    return true;
-  }
-
-  return envToBool(process.env.AIKIDO_BLOCK_INVALID_SQL);
-}
-
 /**
  * This function goes over all the different input types in the context and checks
  * if it's a possible SQL Injection, if so the function returns an InterceptorResult