Merge pull request #308 from AikidoSec/false-positive

Ignore user input that is just alphanumeric

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

@@ -1,7 +1,6 @@
 import * as t from "tap";
 import {
   SQL_DANGEROUS_IN_STRING,
-  COMMON_SQL_KEYWORDS,
   SQL_ESCAPE_SEQUENCES,
   SQL_KEYWORDS,
   SQL_OPERATORS,
@@ -20,18 +19,6 @@ t.test("SQL_KEYWORDS are uppercase", async () => {
   });
 });
 
-t.test("COMMON_SQL_KEYWORDS are not empty", async () => {
-  COMMON_SQL_KEYWORDS.forEach((keyword) => {
-    t.ok(keyword.length > 0);
-  });
-});
-
-t.test("COMMON_SQL_KEYWORDS are uppercase", async () => {
-  COMMON_SQL_KEYWORDS.forEach((keyword) => {
-    t.same(keyword, keyword.toUpperCase());
-  });
-});
-
 t.test("SQL_OPERATORS are not empty", async () => {
   SQL_OPERATORS.forEach((operator) => {
     t.ok(operator.length > 0);

library/vulnerabilities/sql-injection/config.ts

@@ -75,48 +75,6 @@ export const SQL_KEYWORDS = [
   "IS",
 ];
 
-// This is a list of common SQL keywords that are not dangerous by themselves
-// They will appear in almost any SQL query
-// e.g. SELECT * FROM table WHERE column = 'value' LIMIT 1
-// If a query parameter is ?LIMIT=1 it would be blocked
-// If the body contains "LIMIT" or "SELECT" it would be blocked
-export const COMMON_SQL_KEYWORDS = [
-  "SELECT",
-  "INSERT",
-  "FROM",
-  "WHERE",
-  "DELETE",
-  "GROUP",
-  "BY",
-  "ORDER",
-  "LIMIT",
-  "OFFSET",
-  "HAVING",
-  "COUNT",
-  "SUM",
-  "AVG",
-  "MIN",
-  "MAX",
-  "DISTINCT",
-  "AS",
-  "AND",
-  "OR",
-  "NOT",
-  "IN",
-  "LIKE",
-  "BETWEEN",
-  "IS",
-  "NULL",
-  "ALL",
-  "ANY",
-  "EXISTS",
-  "UNIQUE",
-  "UPDATE",
-  "INTO",
-  "KEY",
-  "VALUES",
-];
-
 export const SQL_OPERATORS = [
   "=",
   "!",

library/vulnerabilities/sql-injection/detectSQLInjection.sqlite.test.ts

@@ -3,9 +3,16 @@ import { detectSQLInjection } from "./detectSQLInjection";
 import { SQLDialectSQLite } from "./dialects/SQLDialectSQLite";
 
 t.test("It flags the VACUUM command as SQL injection", async () => {
-  isSqlInjection("VACUUM;", "VACUUM");
+  isSqlInjection("VACUUM;", "VACUUM;");
 });
 
+t.test(
+  "It does not flag the VACUUM command without semicolon as SQL injection",
+  async () => {
+    isNotSQLInjection("VACUUM;", "VACUUM");
+  }
+);
+
 t.test("It flags the ATTACH command as SQL injection", async () => {
   isSqlInjection("ATTACH DATABASE 'test.db' AS test;", "'test.db' AS test");
 });

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

@@ -2,7 +2,7 @@ import { basename, join } from "path";
 import * as t from "tap";
 import { readFileSync } from "fs";
 import { escapeStringRegexp } from "../../helpers/escapeStringRegexp";
-import { SQL_DANGEROUS_IN_STRING } from "./config";
+import { SQL_DANGEROUS_IN_STRING, SQL_KEYWORDS } from "./config";
 import { detectSQLInjection } from "./detectSQLInjection";
 import { SQLDialectMySQL } from "./dialects/SQLDialectMySQL";
 import { SQLDialectPostgres } from "./dialects/SQLDialectPostgres";
@@ -58,10 +58,10 @@ const IS_NOT_INJECTION = [
   ["SELECT * FROM table", "*"],
   [`"COPY/*"`, "COPY/*"], // String encapsulated but dangerous chars
   [`'union'  is not "UNION--"`, "UNION--"], // String encapsulated but dangerous chars
+  [`'union'  is not UNION`, "UNION"], // String not always encapsulated
 ];
 
 const IS_INJECTION = [
-  [`'union'  is not UNION`, "UNION"], // String not always encapsulated
   [`UNTER;`, "UNTER;"], // String not encapsulated and dangerous char (;)
 ];
 
@@ -343,6 +343,115 @@ t.test("It does not match VIEW keyword", async () => {
   isNotSqlInjection(query, "view_id");
   isNotSqlInjection(query, "view_settings");
   isNotSqlInjection(query, "view_settings.user_id");
+
+  const query2 = `
+    SELECT id,
+           business_id,
+           object_type,
+           name,
+           \`condition\`,
+           settings,
+           \`read_only\`,
+           created_at,
+           updated_at
+    FROM views
+    WHERE business_id = ?
+  `;
+
+  isNotSqlInjection(query2, "view");
+});
+
+t.test("It does not flag SQL keyword if part of another word", async () => {
+  SQL_KEYWORDS.forEach((keyword) => {
+    isNotSqlInjection(
+      `
+      SELECT id,
+             business_id,
+             name,
+             created_at,
+             updated_at
+        FROM ${keyword}
+        WHERE business_id = ?
+    `,
+      keyword
+    );
+
+    isNotSqlInjection(
+      `
+      SELECT id,
+             business_id,
+             name,
+             created_at,
+             updated_at
+        FROM ${keyword.toLowerCase()}
+        WHERE business_id = ?
+    `,
+      keyword
+    );
+  });
+});
+
+t.test("It flags SQL keyword if it contains space", async () => {
+  SQL_KEYWORDS.forEach((keyword) => {
+    isSqlInjection(
+      `
+      SELECT id,
+             business_id,
+             name,
+             created_at,
+             updated_at
+        FROM ${keyword}
+        WHERE business_id = ?
+    `,
+      " " + keyword
+    );
+
+    isSqlInjection(
+      `
+      SELECT id,
+             business_id,
+             name,
+             created_at,
+             updated_at
+        FROM ${keyword}
+        WHERE business_id = ?
+    `,
+      " " + keyword.toLowerCase()
+    );
+  });
+});
+
+t.test("It flags SQL keyword if it contains dangerous character", async () => {
+  SQL_KEYWORDS.forEach((keyword) => {
+    SQL_DANGEROUS_IN_STRING.forEach((string) => {
+      const payload = `${string}${keyword}`;
+      isSqlInjection(
+        `
+      SELECT id,
+             business_id,
+             name,
+             created_at,
+             updated_at
+        FROM ${payload}
+        WHERE business_id = ?
+    `,
+        payload
+      );
+
+      isSqlInjection(
+        `
+      SELECT id,
+             business_id,
+             name,
+             created_at,
+             updated_at
+        FROM ${payload}
+        WHERE business_id = ?
+    `,
+        payload.toLowerCase()
+      );
+    });
+  });
 });
 
 const files = [

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

@@ -1,8 +1,10 @@
 import { readFileSync } from "fs";
 import { join } from "path";
 import * as t from "tap";
+import { SQL_DANGEROUS_IN_STRING, SQL_KEYWORDS } from "./config";
 import { SQLDialectMySQL } from "./dialects/SQLDialectMySQL";
 import { SQLDialectPostgres } from "./dialects/SQLDialectPostgres";
+import { SQLDialectSQLite } from "./dialects/SQLDialectSQLite";
 import { userInputContainsSQLSyntax } from "./userInputContainsSQLSyntax";
 
 t.test("it flags dialect specific keywords", async () => {
@@ -13,6 +15,85 @@ t.test("it does not flag common SQL keywords", async () => {
   t.same(userInputContainsSQLSyntax("SELECT", new SQLDialectMySQL()), false);
 });
 
+const alphanumeric = ["1", "123", "1313", "0", "abc", "ABC"];
+
+t.test("it ignores alphanumeric input", async () => {
+  alphanumeric.forEach((input) => {
+    t.same(userInputContainsSQLSyntax(input, new SQLDialectMySQL()), false);
+    t.same(userInputContainsSQLSyntax(input, new SQLDialectPostgres()), false);
+    t.same(userInputContainsSQLSyntax(input, new SQLDialectSQLite()), false);
+  });
+});
+
+t.test("it flags alphanumeric input if contains dangerous string", async () => {
+  alphanumeric.forEach((input) => {
+    SQL_DANGEROUS_IN_STRING.forEach((string) => {
+      const payload = `${input}${string}`;
+      t.same(userInputContainsSQLSyntax(payload, new SQLDialectMySQL()), true);
+      t.same(
+        userInputContainsSQLSyntax(payload, new SQLDialectPostgres()),
+        true
+      );
+      t.same(userInputContainsSQLSyntax(payload, new SQLDialectSQLite()), true);
+    });
+  });
+});
+
+t.test("it does not flag SQL keyword as dangerous", async () => {
+  // They just contain alpha characters
+  SQL_KEYWORDS.forEach((keyword) => {
+    t.same(
+      userInputContainsSQLSyntax(keyword.toLowerCase(), new SQLDialectMySQL()),
+      false
+    );
+    t.same(
+      userInputContainsSQLSyntax(keyword.toUpperCase(), new SQLDialectMySQL()),
+      false
+    );
+  });
+});
+
+t.test("it does flag SQL keyword as dangerous if contains space", async () => {
+  // They just contain alpha characters
+  SQL_KEYWORDS.forEach((keyword) => {
+    const payload = ` ${keyword}`;
+    t.same(
+      userInputContainsSQLSyntax(payload.toLowerCase(), new SQLDialectMySQL()),
+      true
+    );
+    t.same(
+      userInputContainsSQLSyntax(payload.toUpperCase(), new SQLDialectMySQL()),
+      true
+    );
+  });
+});
+
+t.test(
+  "it does flag SQL keyword as dangerous if contains dangerous string",
+  async () => {
+    // They just contain alpha characters
+    SQL_KEYWORDS.forEach((keyword) => {
+      SQL_DANGEROUS_IN_STRING.forEach((string) => {
+        const payload = `${keyword}${string}`;
+        t.same(
+          userInputContainsSQLSyntax(
+            payload.toLowerCase(),
+            new SQLDialectMySQL()
+          ),
+          true
+        );
+        t.same(
+          userInputContainsSQLSyntax(
+            payload.toUpperCase(),
+            new SQLDialectMySQL()
+          ),
+          true
+        );
+      });
+    });
+  }
+);
+
 const files = [
   // Taken from https://github.com/payloadbox/sql-injection-payload-list/tree/master
   join(__dirname, "payloads", "Auth_Bypass.txt"),

library/vulnerabilities/sql-injection/userInputContainsSQLSyntax.ts

@@ -1,13 +1,9 @@
 import { escapeStringRegexp } from "../../helpers/escapeStringRegexp";
-import {
-  COMMON_SQL_KEYWORDS,
-  SQL_DANGEROUS_IN_STRING,
-  SQL_KEYWORDS,
-  SQL_OPERATORS,
-} from "./config";
+import { SQL_DANGEROUS_IN_STRING, SQL_KEYWORDS, SQL_OPERATORS } from "./config";
 import { SQLDialect } from "./dialects/SQLDialect";
 
 const cachedRegexes = new Map<string, RegExp>();
+const alphaNumeric = /^[a-z0-9]+$/i;
 
 /**
  * This function is the first check in order to determine if a SQL injection is happening,
@@ -21,8 +17,8 @@ export function userInputContainsSQLSyntax(
   // e.g. SELECT * FROM table WHERE column = 'value' LIMIT 1
   // If a query parameter is ?LIMIT=1 it would be blocked
   // If the body contains "LIMIT" or "SELECT" it would be blocked
-  // These are common SQL keywords and appear in almost any SQL query
-  if (COMMON_SQL_KEYWORDS.includes(userInput.toUpperCase())) {
+  // If the user input is just alphanumeric, we can safely ignore it
+  if (alphaNumeric.test(userInput)) {
     return false;
   }