From b1c83023c43330a58251da02ebf499f712fc9311 Mon Sep 17 00:00:00 2001 From: Hans Ott Date: Mon, 29 Jul 2024 17:12:20 +0200 Subject: [PATCH 1/7] Add failing test --- .../sql-injection/detectSQLInjection.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts b/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts index 358919128..e6391ab75 100644 --- a/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts +++ b/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts @@ -343,6 +343,22 @@ 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"); }); const files = [ From 00bae337b1c9580c60d29747bb1f3e52c0e46b30 Mon Sep 17 00:00:00 2001 From: Hans Ott Date: Mon, 29 Jul 2024 17:26:30 +0200 Subject: [PATCH 2/7] Add VIEW to common keywords --- library/vulnerabilities/sql-injection/config.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/library/vulnerabilities/sql-injection/config.ts b/library/vulnerabilities/sql-injection/config.ts index da1f4a9a2..f1dd9cd28 100644 --- a/library/vulnerabilities/sql-injection/config.ts +++ b/library/vulnerabilities/sql-injection/config.ts @@ -115,6 +115,7 @@ export const COMMON_SQL_KEYWORDS = [ "INTO", "KEY", "VALUES", + "VIEW", ]; export const SQL_OPERATORS = [ From 819962231ec56658f66de78c990e9363cc79691b Mon Sep 17 00:00:00 2001 From: Hans Ott Date: Mon, 29 Jul 2024 17:34:31 +0200 Subject: [PATCH 3/7] Ignore user input that is just alphanumeric --- .../sql-injection/config.test.ts | 13 ------ .../vulnerabilities/sql-injection/config.ts | 43 ------------------- .../detectSQLInjection.sqlite.test.ts | 2 +- .../sql-injection/detectSQLInjection.test.ts | 2 +- .../userInputContainsSQLSyntax.ts | 12 ++---- 5 files changed, 6 insertions(+), 66 deletions(-) diff --git a/library/vulnerabilities/sql-injection/config.test.ts b/library/vulnerabilities/sql-injection/config.test.ts index a4fb13516..ca2ead6a6 100644 --- a/library/vulnerabilities/sql-injection/config.test.ts +++ b/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); diff --git a/library/vulnerabilities/sql-injection/config.ts b/library/vulnerabilities/sql-injection/config.ts index f1dd9cd28..9aa5902eb 100644 --- a/library/vulnerabilities/sql-injection/config.ts +++ b/library/vulnerabilities/sql-injection/config.ts @@ -75,49 +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", - "VIEW", -]; - export const SQL_OPERATORS = [ "=", "!", diff --git a/library/vulnerabilities/sql-injection/detectSQLInjection.sqlite.test.ts b/library/vulnerabilities/sql-injection/detectSQLInjection.sqlite.test.ts index e3ba3dabe..0aff2dd11 100644 --- a/library/vulnerabilities/sql-injection/detectSQLInjection.sqlite.test.ts +++ b/library/vulnerabilities/sql-injection/detectSQLInjection.sqlite.test.ts @@ -3,7 +3,7 @@ import { detectSQLInjection } from "./detectSQLInjection"; import { SQLDialectSQLite } from "./dialects/SQLDialectSQLite"; t.test("It flags the VACUUM command as SQL injection", async () => { - isSqlInjection("VACUUM;", "VACUUM"); + isNotSQLInjection("VACUUM;", "VACUUM"); }); t.test("It flags the ATTACH command as SQL injection", async () => { diff --git a/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts b/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts index e6391ab75..cd6ff3749 100644 --- a/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts +++ b/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts @@ -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 (;) ]; diff --git a/library/vulnerabilities/sql-injection/userInputContainsSQLSyntax.ts b/library/vulnerabilities/sql-injection/userInputContainsSQLSyntax.ts index 1b6780a66..60a5b8071 100644 --- a/library/vulnerabilities/sql-injection/userInputContainsSQLSyntax.ts +++ b/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(); +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; } From 3956d95a880ba062d64b0e00b503788b99eb781e Mon Sep 17 00:00:00 2001 From: Hans Ott Date: Tue, 30 Jul 2024 11:56:32 +0200 Subject: [PATCH 4/7] Rename test --- .../sql-injection/detectSQLInjection.sqlite.test.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/library/vulnerabilities/sql-injection/detectSQLInjection.sqlite.test.ts b/library/vulnerabilities/sql-injection/detectSQLInjection.sqlite.test.ts index 0aff2dd11..e5e940f7a 100644 --- a/library/vulnerabilities/sql-injection/detectSQLInjection.sqlite.test.ts +++ b/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 () => { - isNotSQLInjection("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"); }); From 2811e304be0872482b027834c1e88b694771c77e Mon Sep 17 00:00:00 2001 From: Hans Ott Date: Tue, 30 Jul 2024 12:05:23 +0200 Subject: [PATCH 5/7] Add more tests --- .../sql-injection/detectSQLInjection.test.ts | 32 ++++++++++++++++++- .../userInputContainsSQLSyntax.test.ts | 24 ++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts b/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts index cd6ff3749..2b64066b3 100644 --- a/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts +++ b/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"; @@ -361,6 +361,36 @@ t.test("It does not match VIEW keyword", async () => { 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 + ); + }); +}); + const files = [ // Taken from https://github.com/payloadbox/sql-injection-payload-list/tree/master join(__dirname, "payloads", "Auth_Bypass.txt"), diff --git a/library/vulnerabilities/sql-injection/userInputContainsSQLSyntax.test.ts b/library/vulnerabilities/sql-injection/userInputContainsSQLSyntax.test.ts index f5cea9803..12016ee32 100644 --- a/library/vulnerabilities/sql-injection/userInputContainsSQLSyntax.test.ts +++ b/library/vulnerabilities/sql-injection/userInputContainsSQLSyntax.test.ts @@ -1,6 +1,7 @@ import { readFileSync } from "fs"; import { join } from "path"; import * as t from "tap"; +import { SQL_KEYWORDS } from "./config"; import { SQLDialectMySQL } from "./dialects/SQLDialectMySQL"; import { SQLDialectPostgres } from "./dialects/SQLDialectPostgres"; import { userInputContainsSQLSyntax } from "./userInputContainsSQLSyntax"; @@ -13,6 +14,29 @@ t.test("it does not flag common SQL keywords", async () => { t.same(userInputContainsSQLSyntax("SELECT", new SQLDialectMySQL()), false); }); +t.test("it ignores alphanumeric input", async () => { + t.same(userInputContainsSQLSyntax("1", new SQLDialectMySQL()), false); + t.same(userInputContainsSQLSyntax("123", new SQLDialectMySQL()), false); + t.same(userInputContainsSQLSyntax("1313", new SQLDialectMySQL()), false); + t.same(userInputContainsSQLSyntax("0", new SQLDialectMySQL()), false); + t.same(userInputContainsSQLSyntax("abc", new SQLDialectMySQL()), false); + t.same(userInputContainsSQLSyntax("ABC", new SQLDialectMySQL()), false); +}); + +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 + ); + }); +}); + const files = [ // Taken from https://github.com/payloadbox/sql-injection-payload-list/tree/master join(__dirname, "payloads", "Auth_Bypass.txt"), From 5004ed7b155a093383316c756fd53944bd61a05a Mon Sep 17 00:00:00 2001 From: Hans Ott Date: Tue, 30 Jul 2024 13:04:06 +0200 Subject: [PATCH 6/7] Add tests for matching keyword with something else Like a dangerous string or space --- .../sql-injection/detectSQLInjection.test.ts | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts b/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts index 2b64066b3..0a7f3927a 100644 --- a/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts +++ b/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts @@ -391,6 +391,69 @@ t.test("It does not flag SQL keyword if part of another word", async () => { }); }); +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 = [ // Taken from https://github.com/payloadbox/sql-injection-payload-list/tree/master join(__dirname, "payloads", "Auth_Bypass.txt"), From 9a2269ab3e08e1767b1fb1be57ffc5140bbe4486 Mon Sep 17 00:00:00 2001 From: Hans Ott Date: Tue, 30 Jul 2024 13:08:17 +0200 Subject: [PATCH 7/7] Add more tests for combinations --- .../userInputContainsSQLSyntax.test.ts | 71 +++++++++++++++++-- 1 file changed, 64 insertions(+), 7 deletions(-) diff --git a/library/vulnerabilities/sql-injection/userInputContainsSQLSyntax.test.ts b/library/vulnerabilities/sql-injection/userInputContainsSQLSyntax.test.ts index 12016ee32..d893ee391 100644 --- a/library/vulnerabilities/sql-injection/userInputContainsSQLSyntax.test.ts +++ b/library/vulnerabilities/sql-injection/userInputContainsSQLSyntax.test.ts @@ -1,9 +1,10 @@ import { readFileSync } from "fs"; import { join } from "path"; import * as t from "tap"; -import { SQL_KEYWORDS } from "./config"; +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 () => { @@ -14,13 +15,28 @@ 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 () => { - t.same(userInputContainsSQLSyntax("1", new SQLDialectMySQL()), false); - t.same(userInputContainsSQLSyntax("123", new SQLDialectMySQL()), false); - t.same(userInputContainsSQLSyntax("1313", new SQLDialectMySQL()), false); - t.same(userInputContainsSQLSyntax("0", new SQLDialectMySQL()), false); - t.same(userInputContainsSQLSyntax("abc", new SQLDialectMySQL()), false); - t.same(userInputContainsSQLSyntax("ABC", new SQLDialectMySQL()), false); + 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 () => { @@ -37,6 +53,47 @@ t.test("it does not flag SQL keyword as dangerous", async () => { }); }); +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"),