From 5fad828296fcdac178afc4bc9772948228fe95e0 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Wed, 2 Jul 2025 12:34:57 +0200 Subject: [PATCH 1/4] JS:Fix double curly brace issue in incomplete sanitization alerts --- .../CWE-116/IncompleteSanitization.ql | 8 +- .../IncompleteSanitization.expected | 74 +++++++++---------- 2 files changed, 42 insertions(+), 40 deletions(-) diff --git a/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql b/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql index 7d0dc71a2a84..11a683600514 100644 --- a/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql +++ b/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql @@ -152,12 +152,13 @@ string getPatternOrValueString(DataFlow::Node node) { else result = node.toString() } -from StringReplaceCall repl, DataFlow::Node old, string msg +from StringReplaceCall repl, DataFlow::Node old, string msg, string pattern where (old = repl.getArgument(0) or old = repl.getRegExp()) and ( not repl.maybeGlobal() and - msg = "This replaces only the first occurrence of " + getPatternOrValueString(old) + "." and + pattern = getPatternOrValueString(old) and + msg = "This replaces only the first occurrence of $@." and // only flag if this is likely to be a sanitizer or URL encoder or decoder exists(string m | m = getAMatchedString(old) | // sanitizer @@ -184,6 +185,7 @@ where or isBackslashEscape(repl, _) and not allBackslashesEscaped(repl) and + pattern = "" and msg = "This does not escape backslash characters in the input." ) -select repl.getCalleeNode(), msg +select repl.getCalleeNode(), msg, old, pattern diff --git a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected index bfee25f1c254..b809a8a0a80a 100644 --- a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected +++ b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected @@ -1,37 +1,37 @@ -| tst.js:5:10:5:18 | s.replace | This replaces only the first occurrence of "'". | -| tst.js:9:10:9:18 | s.replace | This replaces only the first occurrence of /'/. | -| tst.js:13:10:13:18 | s.replace | This does not escape backslash characters in the input. | -| tst.js:17:10:17:18 | s.replace | This does not escape backslash characters in the input. | -| tst.js:21:10:21:18 | s.replace | This does not escape backslash characters in the input. | -| tst.js:25:10:25:18 | s.replace | This does not escape backslash characters in the input. | -| tst.js:29:10:29:18 | s.replace | This does not escape backslash characters in the input. | -| tst.js:33:10:33:18 | s.replace | This replaces only the first occurrence of '\|'. | -| tst.js:37:10:37:18 | s.replace | This does not escape backslash characters in the input. | -| tst.js:41:10:41:18 | s.replace | This replaces only the first occurrence of "/". | -| tst.js:45:10:45:18 | s.replace | This replaces only the first occurrence of "%25". | -| tst.js:49:10:49:18 | s.replace | This replaces only the first occurrence of `'`. | -| tst.js:53:10:53:18 | s.replace | This replaces only the first occurrence of "'". | -| tst.js:57:10:57:18 | s.replace | This replaces only the first occurrence of `'`. | -| tst.js:61:10:61:18 | s.replace | This replaces only the first occurrence of "'" + "". | -| tst.js:65:10:65:18 | s.replace | This replaces only the first occurrence of "'". | -| tst.js:69:10:69:18 | s.replace | This replaces only the first occurrence of "'" + "". | -| tst.js:133:2:133:10 | s.replace | This replaces only the first occurrence of '<'. | -| tst.js:133:2:133:27 | s.repla ... replace | This replaces only the first occurrence of '>'. | -| tst.js:135:2:135:10 | s.replace | This replaces only the first occurrence of '['. | -| tst.js:135:2:135:30 | s.repla ... replace | This replaces only the first occurrence of ']'. | -| tst.js:136:2:136:10 | s.replace | This replaces only the first occurrence of '{'. | -| tst.js:136:2:136:30 | s.repla ... replace | This replaces only the first occurrence of '}'. | -| tst.js:140:2:140:10 | s.replace | This replaces only the first occurrence of /{/. | -| tst.js:140:2:140:27 | s.repla ... replace | This replaces only the first occurrence of /}/. | -| tst.js:141:2:141:10 | s.replace | This replaces only the first occurrence of ']'. | -| tst.js:141:2:141:27 | s.repla ... replace | This replaces only the first occurrence of '['. | -| tst.js:148:2:148:10 | x.replace | This replaces only the first occurrence of "\\n". | -| tst.js:149:2:149:24 | x.repla ... replace | This replaces only the first occurrence of "\\n". | -| tst.js:193:9:193:17 | s.replace | This replaces only the first occurrence of /'/. | -| tst.js:202:10:202:18 | p.replace | This replaces only the first occurrence of "/../". | -| tst.js:341:9:341:17 | p.replace | This replaces only the first occurrence of /\\.\\.//. | -| tst.js:345:9:345:17 | s.replace | This does not escape backslash characters in the input. | -| tst.js:349:9:349:17 | s.replace | This replaces only the first occurrence of /'/. | -| tst.js:353:9:353:17 | s.replace | This does not escape backslash characters in the input. | -| tst.js:362:2:362:10 | x.replace | This replaces only the first occurrence of /\n/. | -| tst.js:363:2:363:24 | x.repla ... replace | This replaces only the first occurrence of /\n/. | +| tst.js:5:10:5:18 | s.replace | This replaces only the first occurrence of $@. | tst.js:5:20:5:22 | "'" | "'" | +| tst.js:9:10:9:18 | s.replace | This replaces only the first occurrence of $@. | tst.js:9:20:9:22 | /'/ | /'/ | +| tst.js:13:10:13:18 | s.replace | This does not escape backslash characters in the input. | tst.js:13:20:13:23 | /'/g | | +| tst.js:17:10:17:18 | s.replace | This does not escape backslash characters in the input. | tst.js:17:20:17:23 | /'/g | | +| tst.js:21:10:21:18 | s.replace | This does not escape backslash characters in the input. | tst.js:21:20:21:26 | /['"]/g | | +| tst.js:25:10:25:18 | s.replace | This does not escape backslash characters in the input. | tst.js:25:20:25:28 | /(['"])/g | | +| tst.js:29:10:29:18 | s.replace | This does not escape backslash characters in the input. | tst.js:29:20:29:27 | /('\|")/g | | +| tst.js:33:10:33:18 | s.replace | This replaces only the first occurrence of $@. | tst.js:33:20:33:22 | '\|' | '\|' | +| tst.js:37:10:37:18 | s.replace | This does not escape backslash characters in the input. | tst.js:37:20:37:23 | /"/g | | +| tst.js:41:10:41:18 | s.replace | This replaces only the first occurrence of $@. | tst.js:41:20:41:22 | "/" | "/" | +| tst.js:45:10:45:18 | s.replace | This replaces only the first occurrence of $@. | tst.js:45:20:45:24 | "%25" | "%25" | +| tst.js:49:10:49:18 | s.replace | This replaces only the first occurrence of $@. | tst.js:49:20:49:22 | `'` | `'` | +| tst.js:53:10:53:18 | s.replace | This replaces only the first occurrence of $@. | tst.js:53:20:53:22 | "'" | "'" | +| tst.js:57:10:57:18 | s.replace | This replaces only the first occurrence of $@. | tst.js:57:20:57:22 | `'` | `'` | +| tst.js:61:10:61:18 | s.replace | This replaces only the first occurrence of $@. | tst.js:61:20:61:27 | "'" + "" | "'" + "" | +| tst.js:65:10:65:18 | s.replace | This replaces only the first occurrence of $@. | tst.js:65:20:65:22 | "'" | "'" | +| tst.js:69:10:69:18 | s.replace | This replaces only the first occurrence of $@. | tst.js:69:20:69:27 | "'" + "" | "'" + "" | +| tst.js:133:2:133:10 | s.replace | This replaces only the first occurrence of $@. | tst.js:133:12:133:14 | '<' | '<' | +| tst.js:133:2:133:27 | s.repla ... replace | This replaces only the first occurrence of $@. | tst.js:133:29:133:31 | '>' | '>' | +| tst.js:135:2:135:10 | s.replace | This replaces only the first occurrence of $@. | tst.js:135:12:135:14 | '[' | '[' | +| tst.js:135:2:135:30 | s.repla ... replace | This replaces only the first occurrence of $@. | tst.js:135:32:135:34 | ']' | ']' | +| tst.js:136:2:136:10 | s.replace | This replaces only the first occurrence of $@. | tst.js:136:12:136:14 | '{' | '{' | +| tst.js:136:2:136:30 | s.repla ... replace | This replaces only the first occurrence of $@. | tst.js:136:32:136:34 | '}' | '}' | +| tst.js:140:2:140:10 | s.replace | This replaces only the first occurrence of $@. | tst.js:140:12:140:14 | /{/ | /{/ | +| tst.js:140:2:140:27 | s.repla ... replace | This replaces only the first occurrence of $@. | tst.js:140:29:140:31 | /}/ | /}/ | +| tst.js:141:2:141:10 | s.replace | This replaces only the first occurrence of $@. | tst.js:141:12:141:14 | ']' | ']' | +| tst.js:141:2:141:27 | s.repla ... replace | This replaces only the first occurrence of $@. | tst.js:141:29:141:31 | '[' | '[' | +| tst.js:148:2:148:10 | x.replace | This replaces only the first occurrence of $@. | tst.js:148:12:148:15 | "\\n" | "\\n" | +| tst.js:149:2:149:24 | x.repla ... replace | This replaces only the first occurrence of $@. | tst.js:149:26:149:29 | "\\n" | "\\n" | +| tst.js:193:9:193:17 | s.replace | This replaces only the first occurrence of $@. | tst.js:192:17:192:19 | /'/ | /'/ | +| tst.js:202:10:202:18 | p.replace | This replaces only the first occurrence of $@. | tst.js:202:20:202:25 | "/../" | "/../" | +| tst.js:341:9:341:17 | p.replace | This replaces only the first occurrence of $@. | tst.js:341:19:341:39 | new Reg ... .\\\\./") | /\\.\\.// | +| tst.js:345:9:345:17 | s.replace | This does not escape backslash characters in the input. | tst.js:345:19:345:38 | new RegExp("\\'","g") | | +| tst.js:349:9:349:17 | s.replace | This replaces only the first occurrence of $@. | tst.js:349:19:349:34 | new RegExp("\\'") | /'/ | +| tst.js:353:9:353:17 | s.replace | This does not escape backslash characters in the input. | tst.js:353:19:353:50 | new Reg ... lags()) | | +| tst.js:362:2:362:10 | x.replace | This replaces only the first occurrence of $@. | tst.js:362:12:362:27 | new RegExp("\\n") | /\n/ | +| tst.js:363:2:363:24 | x.repla ... replace | This replaces only the first occurrence of $@. | tst.js:363:26:363:41 | new RegExp("\\n") | /\n/ | From c7e5ede62ee0ed937dc13b2c7dcf18ecb87719f7 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Wed, 2 Jul 2025 13:13:14 +0200 Subject: [PATCH 2/4] JS: add change note --- .../ql/src/change-notes/2025-07-02-incomplete-sanitization.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 javascript/ql/src/change-notes/2025-07-02-incomplete-sanitization.md diff --git a/javascript/ql/src/change-notes/2025-07-02-incomplete-sanitization.md b/javascript/ql/src/change-notes/2025-07-02-incomplete-sanitization.md new file mode 100644 index 000000000000..35a2ad2dae52 --- /dev/null +++ b/javascript/ql/src/change-notes/2025-07-02-incomplete-sanitization.md @@ -0,0 +1,4 @@ +--- +category: fix +--- +* Fixed incorrect display of some special characters in `js/incomplete-sanitization` alert messages. From 953bd905f99dae3eb9e68757322b309871a125e6 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Wed, 2 Jul 2025 13:20:00 +0200 Subject: [PATCH 3/4] JS: display `this node` if the current node does not have string value. --- .../Security/CWE-116/IncompleteSanitization.ql | 4 +--- .../IncompleteSanitization.expected | 16 ++++++++-------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql b/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql index 11a683600514..abbd45787c41 100644 --- a/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql +++ b/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql @@ -147,9 +147,7 @@ predicate whitelistedRemoval(StringReplaceCall repl) { * Gets a nice string representation of the pattern or value of the node. */ string getPatternOrValueString(DataFlow::Node node) { - if node instanceof DataFlow::RegExpConstructorInvokeNode - then result = "/" + node.(DataFlow::RegExpConstructorInvokeNode).getRoot() + "/" - else result = node.toString() + if exists(node.getStringValue()) then result = node.toString() else result = "this node" } from StringReplaceCall repl, DataFlow::Node old, string msg, string pattern diff --git a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected index b809a8a0a80a..f1a69d3b8fdd 100644 --- a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected +++ b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected @@ -1,5 +1,5 @@ | tst.js:5:10:5:18 | s.replace | This replaces only the first occurrence of $@. | tst.js:5:20:5:22 | "'" | "'" | -| tst.js:9:10:9:18 | s.replace | This replaces only the first occurrence of $@. | tst.js:9:20:9:22 | /'/ | /'/ | +| tst.js:9:10:9:18 | s.replace | This replaces only the first occurrence of $@. | tst.js:9:20:9:22 | /'/ | this node | | tst.js:13:10:13:18 | s.replace | This does not escape backslash characters in the input. | tst.js:13:20:13:23 | /'/g | | | tst.js:17:10:17:18 | s.replace | This does not escape backslash characters in the input. | tst.js:17:20:17:23 | /'/g | | | tst.js:21:10:21:18 | s.replace | This does not escape backslash characters in the input. | tst.js:21:20:21:26 | /['"]/g | | @@ -21,17 +21,17 @@ | tst.js:135:2:135:30 | s.repla ... replace | This replaces only the first occurrence of $@. | tst.js:135:32:135:34 | ']' | ']' | | tst.js:136:2:136:10 | s.replace | This replaces only the first occurrence of $@. | tst.js:136:12:136:14 | '{' | '{' | | tst.js:136:2:136:30 | s.repla ... replace | This replaces only the first occurrence of $@. | tst.js:136:32:136:34 | '}' | '}' | -| tst.js:140:2:140:10 | s.replace | This replaces only the first occurrence of $@. | tst.js:140:12:140:14 | /{/ | /{/ | -| tst.js:140:2:140:27 | s.repla ... replace | This replaces only the first occurrence of $@. | tst.js:140:29:140:31 | /}/ | /}/ | +| tst.js:140:2:140:10 | s.replace | This replaces only the first occurrence of $@. | tst.js:140:12:140:14 | /{/ | this node | +| tst.js:140:2:140:27 | s.repla ... replace | This replaces only the first occurrence of $@. | tst.js:140:29:140:31 | /}/ | this node | | tst.js:141:2:141:10 | s.replace | This replaces only the first occurrence of $@. | tst.js:141:12:141:14 | ']' | ']' | | tst.js:141:2:141:27 | s.repla ... replace | This replaces only the first occurrence of $@. | tst.js:141:29:141:31 | '[' | '[' | | tst.js:148:2:148:10 | x.replace | This replaces only the first occurrence of $@. | tst.js:148:12:148:15 | "\\n" | "\\n" | | tst.js:149:2:149:24 | x.repla ... replace | This replaces only the first occurrence of $@. | tst.js:149:26:149:29 | "\\n" | "\\n" | -| tst.js:193:9:193:17 | s.replace | This replaces only the first occurrence of $@. | tst.js:192:17:192:19 | /'/ | /'/ | +| tst.js:193:9:193:17 | s.replace | This replaces only the first occurrence of $@. | tst.js:192:17:192:19 | /'/ | this node | | tst.js:202:10:202:18 | p.replace | This replaces only the first occurrence of $@. | tst.js:202:20:202:25 | "/../" | "/../" | -| tst.js:341:9:341:17 | p.replace | This replaces only the first occurrence of $@. | tst.js:341:19:341:39 | new Reg ... .\\\\./") | /\\.\\.// | +| tst.js:341:9:341:17 | p.replace | This replaces only the first occurrence of $@. | tst.js:341:19:341:39 | new Reg ... .\\\\./") | this node | | tst.js:345:9:345:17 | s.replace | This does not escape backslash characters in the input. | tst.js:345:19:345:38 | new RegExp("\\'","g") | | -| tst.js:349:9:349:17 | s.replace | This replaces only the first occurrence of $@. | tst.js:349:19:349:34 | new RegExp("\\'") | /'/ | +| tst.js:349:9:349:17 | s.replace | This replaces only the first occurrence of $@. | tst.js:349:19:349:34 | new RegExp("\\'") | this node | | tst.js:353:9:353:17 | s.replace | This does not escape backslash characters in the input. | tst.js:353:19:353:50 | new Reg ... lags()) | | -| tst.js:362:2:362:10 | x.replace | This replaces only the first occurrence of $@. | tst.js:362:12:362:27 | new RegExp("\\n") | /\n/ | -| tst.js:363:2:363:24 | x.repla ... replace | This replaces only the first occurrence of $@. | tst.js:363:26:363:41 | new RegExp("\\n") | /\n/ | +| tst.js:362:2:362:10 | x.replace | This replaces only the first occurrence of $@. | tst.js:362:12:362:27 | new RegExp("\\n") | this node | +| tst.js:363:2:363:24 | x.repla ... replace | This replaces only the first occurrence of $@. | tst.js:363:26:363:41 | new RegExp("\\n") | this node | From cc1d2c4473dac63fcdb4b0626ec2e83dc8f6ae51 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Wed, 2 Jul 2025 18:03:52 +0200 Subject: [PATCH 4/4] JS: add extra test case with `}` --- .../IncompleteSanitization/IncompleteSanitization.expected | 1 + .../query-tests/Security/CWE-116/IncompleteSanitization/tst.js | 1 + 2 files changed, 2 insertions(+) diff --git a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected index f1a69d3b8fdd..31596ce61635 100644 --- a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected +++ b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected @@ -35,3 +35,4 @@ | tst.js:353:9:353:17 | s.replace | This does not escape backslash characters in the input. | tst.js:353:19:353:50 | new Reg ... lags()) | | | tst.js:362:2:362:10 | x.replace | This replaces only the first occurrence of $@. | tst.js:362:12:362:27 | new RegExp("\\n") | this node | | tst.js:363:2:363:24 | x.repla ... replace | This replaces only the first occurrence of $@. | tst.js:363:26:363:41 | new RegExp("\\n") | this node | +| tst.js:367:2:367:24 | x.repla ... replace | This replaces only the first occurrence of $@. | tst.js:367:26:367:28 | '}' | '}' | diff --git a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js index d457af27f143..610294a66429 100644 --- a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js @@ -364,4 +364,5 @@ function newlinesNewReGexp(s) { x.replace(new RegExp("\n", unknownFlags()), "").replace(x, y); x.replace(x, y).replace(new RegExp("\n", unknownFlags()), ""); + x.replace(x, y).replace('}', ""); // $ Alert[js/incomplete-sanitization] }