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 da1f4a9a2..9aa5902eb 100644 --- a/library/vulnerabilities/sql-injection/config.ts +++ b/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 = [ "=", "!", diff --git a/library/vulnerabilities/sql-injection/detectSQLInjection.sqlite.test.ts b/library/vulnerabilities/sql-injection/detectSQLInjection.sqlite.test.ts index e3ba3dabe..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 () => { - 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"); }); diff --git a/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts b/library/vulnerabilities/sql-injection/detectSQLInjection.test.ts index 358919128..0a7f3927a 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"; @@ -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 = [ diff --git a/library/vulnerabilities/sql-injection/userInputContainsSQLSyntax.test.ts b/library/vulnerabilities/sql-injection/userInputContainsSQLSyntax.test.ts index f5cea9803..d893ee391 100644 --- a/library/vulnerabilities/sql-injection/userInputContainsSQLSyntax.test.ts +++ b/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"), 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; }