From 283954d515776da3a7cd4191cfc1101612e49433 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 14 Feb 2025 16:06:43 +0100 Subject: [PATCH 01/16] JS: Do not store into arrays implicitly --- .../javascript/dataflow/internal/TaintTrackingPrivate.qll | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/TaintTrackingPrivate.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/TaintTrackingPrivate.qll index 5f290b557fe1..5ba7cddf0971 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/TaintTrackingPrivate.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/TaintTrackingPrivate.qll @@ -14,13 +14,10 @@ predicate defaultAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) FlowSummaryPrivate::Steps::summaryLocalStep(node1.(FlowSummaryNode).getSummaryNode(), node2.(FlowSummaryNode).getSummaryNode(), false, _) // TODO: preserve 'model' parameter or - // Convert steps into and out of array elements to plain taint steps + // Convert steps out of array elements to plain taint steps FlowSummaryPrivate::Steps::summaryReadStep(node1.(FlowSummaryNode).getSummaryNode(), ContentSet::arrayElement(), node2.(FlowSummaryNode).getSummaryNode()) or - FlowSummaryPrivate::Steps::summaryStoreStep(node1.(FlowSummaryNode).getSummaryNode(), - ContentSet::arrayElement(), node2.(FlowSummaryNode).getSummaryNode()) - or // If the spread argument itself is tainted (not inside a content), store it into the dynamic argument array. exists(InvokeExpr invoke, Content c | node1 = TValueNode(invoke.getAnArgument().stripParens().(SpreadElement).getOperand()) and From d79f4299781a09c74a24bf17952fcd2c3f1c3277 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Feb 2025 10:36:05 +0100 Subject: [PATCH 02/16] JS: Update changes to nodes/edges/subpaths No changes in actual alerts --- .../IndirectCommandInjection.expected | 1 - .../UnsafeShellCommandConstruction.expected | 8 ---- .../Security/CWE-079/DomBasedXss/Xss.expected | 1 - .../XssWithAdditionalSources.expected | 1 - .../ReflectedXss/ReflectedXss.expected | 3 -- .../CWE-079/StoredXss/StoredXss.expected | 46 ------------------- .../CWE-089/untyped/SqlInjection.expected | 5 -- .../ImproperCodeSanitization.expected | 5 -- .../Security/CWE-730/RegExpInjection.expected | 5 -- 9 files changed, 75 deletions(-) diff --git a/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/IndirectCommandInjection.expected b/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/IndirectCommandInjection.expected index b8ce07f4ca8e..af0b8090ff7b 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/IndirectCommandInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/IndirectCommandInjection.expected @@ -27,7 +27,6 @@ edges | command-line-parameter-command-injection.js:14:6:14:30 | fewerArgs [ArrayElement] | command-line-parameter-command-injection.js:18:13:18:21 | fewerArgs [ArrayElement] | provenance | | | command-line-parameter-command-injection.js:14:18:14:21 | args | command-line-parameter-command-injection.js:14:18:14:30 | args.slice(1) | provenance | | | command-line-parameter-command-injection.js:14:18:14:21 | args | command-line-parameter-command-injection.js:14:18:14:30 | args.slice(1) [ArrayElement] | provenance | | -| command-line-parameter-command-injection.js:14:18:14:21 | args [ArrayElement] | command-line-parameter-command-injection.js:14:18:14:30 | args.slice(1) | provenance | | | command-line-parameter-command-injection.js:14:18:14:21 | args [ArrayElement] | command-line-parameter-command-injection.js:14:18:14:30 | args.slice(1) [ArrayElement] | provenance | | | command-line-parameter-command-injection.js:14:18:14:30 | args.slice(1) | command-line-parameter-command-injection.js:14:6:14:30 | fewerArgs | provenance | | | command-line-parameter-command-injection.js:14:18:14:30 | args.slice(1) [ArrayElement] | command-line-parameter-command-injection.js:14:6:14:30 | fewerArgs [ArrayElement] | provenance | | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected index 0e0cf297fa29..482c3cfff1b4 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected @@ -89,18 +89,14 @@ edges | lib/lib.js:414:40:414:43 | name | lib/lib.js:426:11:426:14 | name | provenance | | | lib/lib.js:414:40:414:43 | name | lib/lib.js:426:11:426:14 | name | provenance | | | lib/lib.js:414:40:414:43 | name | lib/lib.js:428:36:428:39 | name | provenance | | -| lib/lib.js:426:2:426:4 | [post update] arr | lib/lib.js:427:14:427:16 | arr | provenance | | | lib/lib.js:426:2:426:4 | [post update] arr [ArrayElement] | lib/lib.js:427:14:427:16 | arr | provenance | | -| lib/lib.js:426:11:426:14 | name | lib/lib.js:426:2:426:4 | [post update] arr | provenance | | | lib/lib.js:426:11:426:14 | name | lib/lib.js:426:2:426:4 | [post update] arr [ArrayElement] | provenance | | | lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | lib/lib.js:428:14:428:58 | build(" ... + '-') | provenance | | | lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | lib/lib.js:431:23:431:26 | last | provenance | | | lib/lib.js:428:36:428:39 | name | lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | provenance | | | lib/lib.js:431:23:431:26 | last | lib/lib.js:436:19:436:22 | last | provenance | | | lib/lib.js:431:23:431:26 | last | lib/lib.js:436:19:436:22 | last | provenance | | -| lib/lib.js:436:10:436:12 | [post update] arr | lib/lib.js:437:9:437:11 | arr | provenance | | | lib/lib.js:436:10:436:12 | [post update] arr [ArrayElement] | lib/lib.js:437:9:437:11 | arr [ArrayElement] | provenance | | -| lib/lib.js:436:19:436:22 | last | lib/lib.js:436:10:436:12 | [post update] arr | provenance | | | lib/lib.js:436:19:436:22 | last | lib/lib.js:436:10:436:12 | [post update] arr [ArrayElement] | provenance | | | lib/lib.js:441:39:441:42 | name | lib/lib.js:442:24:442:27 | name | provenance | | | lib/lib.js:446:20:446:23 | name | lib/lib.js:447:25:447:28 | name | provenance | | @@ -273,7 +269,6 @@ nodes | lib/lib.js:419:32:419:35 | name | semmle.label | name | | lib/lib.js:420:29:420:32 | name | semmle.label | name | | lib/lib.js:424:24:424:27 | name | semmle.label | name | -| lib/lib.js:426:2:426:4 | [post update] arr | semmle.label | [post update] arr | | lib/lib.js:426:2:426:4 | [post update] arr [ArrayElement] | semmle.label | [post update] arr [ArrayElement] | | lib/lib.js:426:11:426:14 | name | semmle.label | name | | lib/lib.js:426:11:426:14 | name | semmle.label | name | @@ -282,11 +277,9 @@ nodes | lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | semmle.label | (name ? ... ) + '-' | | lib/lib.js:428:36:428:39 | name | semmle.label | name | | lib/lib.js:431:23:431:26 | last | semmle.label | last | -| lib/lib.js:436:10:436:12 | [post update] arr | semmle.label | [post update] arr | | lib/lib.js:436:10:436:12 | [post update] arr [ArrayElement] | semmle.label | [post update] arr [ArrayElement] | | lib/lib.js:436:19:436:22 | last | semmle.label | last | | lib/lib.js:436:19:436:22 | last | semmle.label | last | -| lib/lib.js:437:9:437:11 | arr | semmle.label | arr | | lib/lib.js:437:9:437:11 | arr [ArrayElement] | semmle.label | arr [ArrayElement] | | lib/lib.js:441:39:441:42 | name | semmle.label | name | | lib/lib.js:442:24:442:27 | name | semmle.label | name | @@ -354,7 +347,6 @@ nodes subpaths | lib/lib.js:251:27:251:30 | name | lib/lib.js:239:28:239:28 | s | lib/lib.js:245:9:245:9 | s | lib/lib.js:251:16:251:31 | cleanInput(name) | | lib/lib.js:340:25:340:25 | n | lib/lib.js:329:13:329:13 | x | lib/lib.js:330:9:330:9 | x | lib/lib.js:340:22:340:26 | id(n) | -| lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | lib/lib.js:431:23:431:26 | last | lib/lib.js:437:9:437:11 | arr | lib/lib.js:428:14:428:58 | build(" ... + '-') | | lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | lib/lib.js:431:23:431:26 | last | lib/lib.js:437:9:437:11 | arr [ArrayElement] | lib/lib.js:428:14:428:58 | build(" ... + '-') | #select | lib/isImported.js:6:10:6:25 | "rm -rf " + name | lib/isImported.js:5:49:5:52 | name | lib/isImported.js:6:22:6:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/isImported.js:5:49:5:52 | name | library input | lib/isImported.js:6:2:6:26 | cp.exec ... + name) | shell command | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected index 7d067911c197..443e8c18d2c3 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected @@ -1145,7 +1145,6 @@ edges | various-concat-obfuscations.js:18:10:18:59 | '
') | provenance | | | various-concat-obfuscations.js:18:10:18:88 | '
') [ArrayElement] | provenance | | -| various-concat-obfuscations.js:18:10:18:88 | '
') | provenance | | | various-concat-obfuscations.js:18:10:18:88 | '
') [ArrayElement] | provenance | | | various-concat-obfuscations.js:18:32:18:36 | attrs | various-concat-obfuscations.js:18:32:18:58 | attrs.d ... 'left' | provenance | | | various-concat-obfuscations.js:18:32:18:58 | attrs.d ... 'left' | various-concat-obfuscations.js:18:10:18:59 | '
') | provenance | | | various-concat-obfuscations.js:18:10:18:88 | '
') [ArrayElement] | provenance | | -| various-concat-obfuscations.js:18:10:18:88 | '
') | provenance | | | various-concat-obfuscations.js:18:10:18:88 | '
') [ArrayElement] | provenance | | | various-concat-obfuscations.js:18:32:18:36 | attrs | various-concat-obfuscations.js:18:32:18:58 | attrs.d ... 'left' | provenance | | | various-concat-obfuscations.js:18:32:18:58 | attrs.d ... 'left' | various-concat-obfuscations.js:18:10:18:59 | '
' ... '' | xss-through-filenames.js:20:13:20:18 | [post update] files3 | provenance | | | xss-through-filenames.js:20:25:20:47 | '
  • ' ... '
  • ' | xss-through-filenames.js:20:13:20:18 | [post update] files3 [ArrayElement] | provenance | | | xss-through-filenames.js:20:34:20:37 | file | xss-through-filenames.js:20:25:20:47 | '
  • ' ... '
  • ' | provenance | | -| xss-through-filenames.js:22:16:22:21 | files3 | xss-through-filenames.js:22:16:22:30 | files3.join('') | provenance | | -| xss-through-filenames.js:22:16:22:21 | files3 | xss-through-filenames.js:22:16:22:30 | files3.join('') | provenance | | -| xss-through-filenames.js:22:16:22:21 | files3 [ArrayElement] | xss-through-filenames.js:22:16:22:30 | files3.join('') | provenance | | | xss-through-filenames.js:22:16:22:21 | files3 [ArrayElement] | xss-through-filenames.js:22:16:22:30 | files3.join('') | provenance | | | xss-through-filenames.js:25:43:25:48 | files1 | xss-through-filenames.js:26:19:26:24 | files1 | provenance | | | xss-through-filenames.js:25:43:25:48 | files1 | xss-through-filenames.js:30:9:30:14 | files1 | provenance | | | xss-through-filenames.js:30:9:30:14 | files1 | xss-through-filenames.js:30:34:30:37 | file | provenance | | | xss-through-filenames.js:30:9:30:14 | files1 | xss-through-filenames.js:33:19:33:24 | files2 | provenance | | -| xss-through-filenames.js:30:9:30:14 | files1 | xss-through-filenames.js:33:19:33:24 | files2 | provenance | | | xss-through-filenames.js:30:9:30:14 | files1 | xss-through-filenames.js:33:19:33:24 | files2 [ArrayElement] | provenance | | | xss-through-filenames.js:30:34:30:37 | file | xss-through-filenames.js:31:25:31:28 | file | provenance | | -| xss-through-filenames.js:31:25:31:28 | file | xss-through-filenames.js:31:13:31:18 | [post update] files2 | provenance | | | xss-through-filenames.js:31:25:31:28 | file | xss-through-filenames.js:31:13:31:18 | [post update] files2 [ArrayElement] | provenance | | -| xss-through-filenames.js:33:19:33:24 | files2 | xss-through-filenames.js:35:29:35:34 | files2 | provenance | | | xss-through-filenames.js:33:19:33:24 | files2 [ArrayElement] | xss-through-filenames.js:35:29:35:34 | files2 [ArrayElement] | provenance | | | xss-through-filenames.js:35:13:35:35 | files3 | xss-through-filenames.js:37:19:37:24 | files3 | provenance | | | xss-through-filenames.js:35:22:35:35 | format(files2) | xss-through-filenames.js:35:13:35:35 | files3 | provenance | | -| xss-through-filenames.js:35:29:35:34 | files2 | xss-through-filenames.js:17:21:17:26 | files2 | provenance | | -| xss-through-filenames.js:35:29:35:34 | files2 | xss-through-filenames.js:35:22:35:35 | format(files2) | provenance | | | xss-through-filenames.js:35:29:35:34 | files2 [ArrayElement] | xss-through-filenames.js:17:21:17:26 | files2 [ArrayElement] | provenance | | | xss-through-filenames.js:35:29:35:34 | files2 [ArrayElement] | xss-through-filenames.js:35:22:35:35 | format(files2) | provenance | | | xss-through-torrent.js:6:6:6:24 | name | xss-through-torrent.js:7:11:7:14 | name | provenance | | @@ -48,57 +25,34 @@ edges nodes | xss-through-filenames.js:7:43:7:48 | files1 | semmle.label | files1 | | xss-through-filenames.js:8:18:8:23 | files1 | semmle.label | files1 | -| xss-through-filenames.js:17:21:17:26 | files2 | semmle.label | files2 | | xss-through-filenames.js:17:21:17:26 | files2 [ArrayElement] | semmle.label | files2 [ArrayElement] | -| xss-through-filenames.js:19:9:19:14 | files2 | semmle.label | files2 | | xss-through-filenames.js:19:9:19:14 | files2 [ArrayElement] | semmle.label | files2 [ArrayElement] | -| xss-through-filenames.js:19:9:19:25 | files2.sort(sort) | semmle.label | files2.sort(sort) | -| xss-through-filenames.js:19:9:19:25 | files2.sort(sort) | semmle.label | files2.sort(sort) | -| xss-through-filenames.js:19:9:19:25 | files2.sort(sort) [ArrayElement] | semmle.label | files2.sort(sort) [ArrayElement] | | xss-through-filenames.js:19:9:19:25 | files2.sort(sort) [ArrayElement] | semmle.label | files2.sort(sort) [ArrayElement] | | xss-through-filenames.js:19:45:19:48 | file | semmle.label | file | -| xss-through-filenames.js:20:13:20:18 | [post update] files3 | semmle.label | [post update] files3 | | xss-through-filenames.js:20:13:20:18 | [post update] files3 [ArrayElement] | semmle.label | [post update] files3 [ArrayElement] | | xss-through-filenames.js:20:25:20:47 | '
  • ' ... '
  • ' | semmle.label | '
  • ' ... '
  • ' | | xss-through-filenames.js:20:34:20:37 | file | semmle.label | file | -| xss-through-filenames.js:22:16:22:21 | files3 | semmle.label | files3 | -| xss-through-filenames.js:22:16:22:21 | files3 | semmle.label | files3 | | xss-through-filenames.js:22:16:22:21 | files3 [ArrayElement] | semmle.label | files3 [ArrayElement] | -| xss-through-filenames.js:22:16:22:21 | files3 [ArrayElement] | semmle.label | files3 [ArrayElement] | -| xss-through-filenames.js:22:16:22:30 | files3.join('') | semmle.label | files3.join('') | | xss-through-filenames.js:22:16:22:30 | files3.join('') | semmle.label | files3.join('') | | xss-through-filenames.js:25:43:25:48 | files1 | semmle.label | files1 | | xss-through-filenames.js:26:19:26:24 | files1 | semmle.label | files1 | | xss-through-filenames.js:30:9:30:14 | files1 | semmle.label | files1 | | xss-through-filenames.js:30:34:30:37 | file | semmle.label | file | -| xss-through-filenames.js:31:13:31:18 | [post update] files2 | semmle.label | [post update] files2 | | xss-through-filenames.js:31:13:31:18 | [post update] files2 [ArrayElement] | semmle.label | [post update] files2 [ArrayElement] | | xss-through-filenames.js:31:25:31:28 | file | semmle.label | file | | xss-through-filenames.js:33:19:33:24 | files2 | semmle.label | files2 | -| xss-through-filenames.js:33:19:33:24 | files2 | semmle.label | files2 | | xss-through-filenames.js:33:19:33:24 | files2 [ArrayElement] | semmle.label | files2 [ArrayElement] | | xss-through-filenames.js:35:13:35:35 | files3 | semmle.label | files3 | | xss-through-filenames.js:35:22:35:35 | format(files2) | semmle.label | format(files2) | -| xss-through-filenames.js:35:29:35:34 | files2 | semmle.label | files2 | | xss-through-filenames.js:35:29:35:34 | files2 [ArrayElement] | semmle.label | files2 [ArrayElement] | | xss-through-filenames.js:37:19:37:24 | files3 | semmle.label | files3 | | xss-through-torrent.js:6:6:6:24 | name | semmle.label | name | | xss-through-torrent.js:6:13:6:24 | torrent.name | semmle.label | torrent.name | | xss-through-torrent.js:7:11:7:14 | name | semmle.label | name | subpaths -| xss-through-filenames.js:19:9:19:25 | files2.sort(sort) | xss-through-filenames.js:19:45:19:48 | file | xss-through-filenames.js:20:13:20:18 | [post update] files3 | xss-through-filenames.js:22:16:22:21 | files3 | -| xss-through-filenames.js:19:9:19:25 | files2.sort(sort) | xss-through-filenames.js:19:45:19:48 | file | xss-through-filenames.js:20:13:20:18 | [post update] files3 | xss-through-filenames.js:22:16:22:21 | files3 | -| xss-through-filenames.js:19:9:19:25 | files2.sort(sort) | xss-through-filenames.js:19:45:19:48 | file | xss-through-filenames.js:20:13:20:18 | [post update] files3 [ArrayElement] | xss-through-filenames.js:22:16:22:21 | files3 [ArrayElement] | -| xss-through-filenames.js:19:9:19:25 | files2.sort(sort) | xss-through-filenames.js:19:45:19:48 | file | xss-through-filenames.js:20:13:20:18 | [post update] files3 [ArrayElement] | xss-through-filenames.js:22:16:22:21 | files3 [ArrayElement] | -| xss-through-filenames.js:19:9:19:25 | files2.sort(sort) [ArrayElement] | xss-through-filenames.js:19:45:19:48 | file | xss-through-filenames.js:20:13:20:18 | [post update] files3 | xss-through-filenames.js:22:16:22:21 | files3 | -| xss-through-filenames.js:19:9:19:25 | files2.sort(sort) [ArrayElement] | xss-through-filenames.js:19:45:19:48 | file | xss-through-filenames.js:20:13:20:18 | [post update] files3 | xss-through-filenames.js:22:16:22:21 | files3 | -| xss-through-filenames.js:19:9:19:25 | files2.sort(sort) [ArrayElement] | xss-through-filenames.js:19:45:19:48 | file | xss-through-filenames.js:20:13:20:18 | [post update] files3 [ArrayElement] | xss-through-filenames.js:22:16:22:21 | files3 [ArrayElement] | | xss-through-filenames.js:19:9:19:25 | files2.sort(sort) [ArrayElement] | xss-through-filenames.js:19:45:19:48 | file | xss-through-filenames.js:20:13:20:18 | [post update] files3 [ArrayElement] | xss-through-filenames.js:22:16:22:21 | files3 [ArrayElement] | -| xss-through-filenames.js:30:9:30:14 | files1 | xss-through-filenames.js:30:34:30:37 | file | xss-through-filenames.js:31:13:31:18 | [post update] files2 | xss-through-filenames.js:33:19:33:24 | files2 | -| xss-through-filenames.js:30:9:30:14 | files1 | xss-through-filenames.js:30:34:30:37 | file | xss-through-filenames.js:31:13:31:18 | [post update] files2 | xss-through-filenames.js:33:19:33:24 | files2 | | xss-through-filenames.js:30:9:30:14 | files1 | xss-through-filenames.js:30:34:30:37 | file | xss-through-filenames.js:31:13:31:18 | [post update] files2 [ArrayElement] | xss-through-filenames.js:33:19:33:24 | files2 | | xss-through-filenames.js:30:9:30:14 | files1 | xss-through-filenames.js:30:34:30:37 | file | xss-through-filenames.js:31:13:31:18 | [post update] files2 [ArrayElement] | xss-through-filenames.js:33:19:33:24 | files2 [ArrayElement] | -| xss-through-filenames.js:35:29:35:34 | files2 | xss-through-filenames.js:17:21:17:26 | files2 | xss-through-filenames.js:22:16:22:30 | files3.join('') | xss-through-filenames.js:35:22:35:35 | format(files2) | | xss-through-filenames.js:35:29:35:34 | files2 [ArrayElement] | xss-through-filenames.js:17:21:17:26 | files2 [ArrayElement] | xss-through-filenames.js:22:16:22:30 | files3.join('') | xss-through-filenames.js:35:22:35:35 | format(files2) | #select | xss-through-filenames.js:8:18:8:23 | files1 | xss-through-filenames.js:7:43:7:48 | files1 | xss-through-filenames.js:8:18:8:23 | files1 | Stored cross-site scripting vulnerability due to $@. | xss-through-filenames.js:7:43:7:48 | files1 | stored value | diff --git a/javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected b/javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected index 3664d7db8286..5ae279288cf8 100644 --- a/javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected @@ -52,12 +52,10 @@ nodes | json-schema-validator.js:61:22:61:26 | query | semmle.label | query | | koarouter.js:5:11:5:33 | version | semmle.label | version | | koarouter.js:5:13:5:19 | version | semmle.label | version | -| koarouter.js:14:9:14:18 | [post update] conditions | semmle.label | [post update] conditions | | koarouter.js:14:9:14:18 | [post update] conditions [ArrayElement] | semmle.label | [post update] conditions [ArrayElement] | | koarouter.js:14:25:14:46 | `versio ... rsion}` | semmle.label | `versio ... rsion}` | | koarouter.js:14:38:14:44 | version | semmle.label | version | | koarouter.js:17:27:17:77 | `SELECT ... nd ')}` | semmle.label | `SELECT ... nd ')}` | -| koarouter.js:17:52:17:61 | conditions | semmle.label | conditions | | koarouter.js:17:52:17:61 | conditions [ArrayElement] | semmle.label | conditions [ArrayElement] | | koarouter.js:17:52:17:75 | conditi ... and ') | semmle.label | conditi ... and ') | | ldap.js:20:7:20:34 | q | semmle.label | q | @@ -324,12 +322,9 @@ edges | json-schema-validator.js:50:34:50:47 | req.query.data | json-schema-validator.js:50:23:50:48 | JSON.pa ... y.data) | provenance | Config | | koarouter.js:5:11:5:33 | version | koarouter.js:14:38:14:44 | version | provenance | | | koarouter.js:5:13:5:19 | version | koarouter.js:5:11:5:33 | version | provenance | | -| koarouter.js:14:9:14:18 | [post update] conditions | koarouter.js:17:52:17:61 | conditions | provenance | | | koarouter.js:14:9:14:18 | [post update] conditions [ArrayElement] | koarouter.js:17:52:17:61 | conditions [ArrayElement] | provenance | | -| koarouter.js:14:25:14:46 | `versio ... rsion}` | koarouter.js:14:9:14:18 | [post update] conditions | provenance | | | koarouter.js:14:25:14:46 | `versio ... rsion}` | koarouter.js:14:9:14:18 | [post update] conditions [ArrayElement] | provenance | | | koarouter.js:14:38:14:44 | version | koarouter.js:14:25:14:46 | `versio ... rsion}` | provenance | | -| koarouter.js:17:52:17:61 | conditions | koarouter.js:17:52:17:75 | conditi ... and ') | provenance | | | koarouter.js:17:52:17:61 | conditions [ArrayElement] | koarouter.js:17:52:17:75 | conditi ... and ') | provenance | | | koarouter.js:17:52:17:75 | conditi ... and ') | koarouter.js:17:27:17:77 | `SELECT ... nd ')}` | provenance | | | ldap.js:20:7:20:34 | q | ldap.js:22:18:22:18 | q | provenance | | diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/ImproperCodeSanitization.expected b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/ImproperCodeSanitization.expected index 3b86bfb074db..48fa61317807 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/ImproperCodeSanitization.expected +++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/ImproperCodeSanitization.expected @@ -1,23 +1,18 @@ edges | bad-code-sanitization.js:2:12:2:90 | /^[_$a- ... key)}]` | bad-code-sanitization.js:7:31:7:43 | safeProp(key) | provenance | | | bad-code-sanitization.js:2:69:2:87 | JSON.stringify(key) | bad-code-sanitization.js:2:12:2:90 | /^[_$a- ... key)}]` | provenance | | -| bad-code-sanitization.js:7:5:7:14 | [post update] statements | bad-code-sanitization.js:8:27:8:36 | statements | provenance | | | bad-code-sanitization.js:7:5:7:14 | [post update] statements [ArrayElement] | bad-code-sanitization.js:8:27:8:36 | statements [ArrayElement] | provenance | | -| bad-code-sanitization.js:7:21:7:70 | `${name ... key])}` | bad-code-sanitization.js:7:5:7:14 | [post update] statements | provenance | | | bad-code-sanitization.js:7:21:7:70 | `${name ... key])}` | bad-code-sanitization.js:7:5:7:14 | [post update] statements [ArrayElement] | provenance | | | bad-code-sanitization.js:7:31:7:43 | safeProp(key) | bad-code-sanitization.js:7:21:7:70 | `${name ... key])}` | provenance | | -| bad-code-sanitization.js:8:27:8:36 | statements | bad-code-sanitization.js:8:27:8:46 | statements.join(';') | provenance | | | bad-code-sanitization.js:8:27:8:36 | statements [ArrayElement] | bad-code-sanitization.js:8:27:8:46 | statements.join(';') | provenance | | | bad-code-sanitization.js:63:11:63:55 | assignment | bad-code-sanitization.js:64:27:64:36 | assignment | provenance | | | bad-code-sanitization.js:63:31:63:49 | JSON.stringify(key) | bad-code-sanitization.js:63:11:63:55 | assignment | provenance | | nodes | bad-code-sanitization.js:2:12:2:90 | /^[_$a- ... key)}]` | semmle.label | /^[_$a- ... key)}]` | | bad-code-sanitization.js:2:69:2:87 | JSON.stringify(key) | semmle.label | JSON.stringify(key) | -| bad-code-sanitization.js:7:5:7:14 | [post update] statements | semmle.label | [post update] statements | | bad-code-sanitization.js:7:5:7:14 | [post update] statements [ArrayElement] | semmle.label | [post update] statements [ArrayElement] | | bad-code-sanitization.js:7:21:7:70 | `${name ... key])}` | semmle.label | `${name ... key])}` | | bad-code-sanitization.js:7:31:7:43 | safeProp(key) | semmle.label | safeProp(key) | -| bad-code-sanitization.js:8:27:8:36 | statements | semmle.label | statements | | bad-code-sanitization.js:8:27:8:36 | statements [ArrayElement] | semmle.label | statements [ArrayElement] | | bad-code-sanitization.js:8:27:8:46 | statements.join(';') | semmle.label | statements.join(';') | | bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) | semmle.label | htmlescape(pathname) | diff --git a/javascript/ql/test/query-tests/Security/CWE-730/RegExpInjection.expected b/javascript/ql/test/query-tests/Security/CWE-730/RegExpInjection.expected index 86ac47a8e16f..ef614088e2cc 100644 --- a/javascript/ql/test/query-tests/Security/CWE-730/RegExpInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-730/RegExpInjection.expected @@ -28,11 +28,8 @@ edges | RegExpInjection.js:29:21:29:21 | s | RegExpInjection.js:31:23:31:23 | s | provenance | | | RegExpInjection.js:33:12:33:14 | key | RegExpInjection.js:29:21:29:21 | s | provenance | | | RegExpInjection.js:34:12:34:19 | getKey() | RegExpInjection.js:29:21:29:21 | s | provenance | | -| RegExpInjection.js:54:14:54:16 | key | RegExpInjection.js:54:14:54:27 | key.split(".") | provenance | | | RegExpInjection.js:54:14:54:16 | key | RegExpInjection.js:54:14:54:27 | key.split(".") [ArrayElement] | provenance | | -| RegExpInjection.js:54:14:54:27 | key.split(".") | RegExpInjection.js:54:14:54:42 | key.spl ... x => x) | provenance | | | RegExpInjection.js:54:14:54:27 | key.split(".") [ArrayElement] | RegExpInjection.js:54:14:54:42 | key.spl ... x => x) [ArrayElement] | provenance | | -| RegExpInjection.js:54:14:54:42 | key.spl ... x => x) | RegExpInjection.js:54:14:54:52 | key.spl ... in("-") | provenance | | | RegExpInjection.js:54:14:54:42 | key.spl ... x => x) [ArrayElement] | RegExpInjection.js:54:14:54:52 | key.spl ... in("-") | provenance | | | RegExpInjection.js:60:31:60:56 | input | RegExpInjection.js:64:14:64:18 | input | provenance | | | RegExpInjection.js:60:39:60:56 | req.param("input") | RegExpInjection.js:60:31:60:56 | input | provenance | | @@ -81,9 +78,7 @@ nodes | RegExpInjection.js:46:27:46:31 | input | semmle.label | input | | RegExpInjection.js:47:26:47:30 | input | semmle.label | input | | RegExpInjection.js:54:14:54:16 | key | semmle.label | key | -| RegExpInjection.js:54:14:54:27 | key.split(".") | semmle.label | key.split(".") | | RegExpInjection.js:54:14:54:27 | key.split(".") [ArrayElement] | semmle.label | key.split(".") [ArrayElement] | -| RegExpInjection.js:54:14:54:42 | key.spl ... x => x) | semmle.label | key.spl ... x => x) | | RegExpInjection.js:54:14:54:42 | key.spl ... x => x) [ArrayElement] | semmle.label | key.spl ... x => x) [ArrayElement] | | RegExpInjection.js:54:14:54:52 | key.spl ... in("-") | semmle.label | key.spl ... in("-") | | RegExpInjection.js:60:31:60:56 | input | semmle.label | input | From e8d170322490f1927d273c873510cb0c13ab30f3 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Feb 2025 11:12:33 +0100 Subject: [PATCH 03/16] JS: Add test for flow through Buffer.concat This flow was lost since the existing model of concat() boxes its return value in ArrayElement. There is no explicit model of Buffer.concat. --- javascript/ql/test/library-tests/TripleDot/buffer.js | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 javascript/ql/test/library-tests/TripleDot/buffer.js diff --git a/javascript/ql/test/library-tests/TripleDot/buffer.js b/javascript/ql/test/library-tests/TripleDot/buffer.js new file mode 100644 index 000000000000..71369546b7e7 --- /dev/null +++ b/javascript/ql/test/library-tests/TripleDot/buffer.js @@ -0,0 +1,7 @@ +import 'dummy'; + +function t1() { + const b1 = Buffer.from(source("t1.1")); + const b2 = Buffer.from(source("t1.2")); + sink(Buffer.concat([b1, b2]).toString("utf8")); // $ MISSING: hasTaintFlow=t1.1 hasTaintFlow=t1.2 +} From d87534c7d0b9c407b8229605bb4626b6870c77c7 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Feb 2025 11:10:51 +0100 Subject: [PATCH 04/16] JS: Model Array#toString --- .../flow_summaries/AmbiguousCoreMethods.qll | 19 ++++++++++++++++++- .../ql/test/library-tests/TripleDot/buffer.js | 2 +- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/internal/flow_summaries/AmbiguousCoreMethods.qll b/javascript/ql/lib/semmle/javascript/internal/flow_summaries/AmbiguousCoreMethods.qll index 95981a6fb95d..660ee4389f25 100644 --- a/javascript/ql/lib/semmle/javascript/internal/flow_summaries/AmbiguousCoreMethods.qll +++ b/javascript/ql/lib/semmle/javascript/internal/flow_summaries/AmbiguousCoreMethods.qll @@ -24,6 +24,7 @@ private import javascript private import semmle.javascript.dataflow.internal.DataFlowNode private import semmle.javascript.dataflow.FlowSummary +private import Arrays private import FlowSummaryUtil class At extends SummarizedCallable { @@ -41,15 +42,18 @@ class At extends SummarizedCallable { } class Concat extends SummarizedCallable { - Concat() { this = "Array#concat / String#concat" } + Concat() { this = "Array#concat / String#concat / Buffer.concat" } override InstanceCall getACallSimple() { result.getMethodName() = "concat" } override predicate propagatesFlow(string input, string output, boolean preservesValue) { + // Array#concat. + // Also models Buffer.concat as this happens to out work well with our toString() model. preservesValue = true and input = "Argument[this,0..].ArrayElement" and output = "ReturnValue.ArrayElement" or + // String#concat preservesValue = false and input = "Argument[this,0..]" and output = "ReturnValue" @@ -149,3 +153,16 @@ class Values extends SummarizedCallable { output = "ReturnValue.IteratorElement" } } + +class ToString extends SummarizedCallable { + ToString() { this = "Object#toString / Array#toString" } + + override DataFlow::MethodCallNode getACallSimple() { result.getMethodName() = "toString" } + + override predicate propagatesFlow(string input, string output, boolean preservesValue) { + preservesValue = false and + // Arrays stringify their contents and joins by "," + input = "Argument[this].ArrayElementDeep" and + output = "ReturnValue" + } +} diff --git a/javascript/ql/test/library-tests/TripleDot/buffer.js b/javascript/ql/test/library-tests/TripleDot/buffer.js index 71369546b7e7..db32a5d99159 100644 --- a/javascript/ql/test/library-tests/TripleDot/buffer.js +++ b/javascript/ql/test/library-tests/TripleDot/buffer.js @@ -3,5 +3,5 @@ import 'dummy'; function t1() { const b1 = Buffer.from(source("t1.1")); const b2 = Buffer.from(source("t1.2")); - sink(Buffer.concat([b1, b2]).toString("utf8")); // $ MISSING: hasTaintFlow=t1.1 hasTaintFlow=t1.2 + sink(Buffer.concat([b1, b2]).toString("utf8")); // $ hasTaintFlow=t1.1 hasTaintFlow=t1.2 } From a74b203c868e2e681c1e85fc69306fa1bead5ca0 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Feb 2025 11:21:46 +0100 Subject: [PATCH 05/16] JS: Add test with implicit array stringification --- .../ql/test/library-tests/TripleDot/arrays.js | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/javascript/ql/test/library-tests/TripleDot/arrays.js b/javascript/ql/test/library-tests/TripleDot/arrays.js index 0a18066eb767..ca5732da5af2 100644 --- a/javascript/ql/test/library-tests/TripleDot/arrays.js +++ b/javascript/ql/test/library-tests/TripleDot/arrays.js @@ -20,3 +20,26 @@ function shiftTaint() { sink(array.shift()); // $ hasTaintFlow=shift.directly-tainted sink(array.shift()); // $ hasTaintFlow=shift.directly-tainted } + +function implicitToString() { + const array = [source('implicitToString.1')]; + array.push(source('implicitToString.2')) + + sink(array + "foo"); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink("foo" + array); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink("" + array); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink(array + 1); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink(1 + array); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink(unknown() + array); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink(array + unknown()); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + + sink(`${array}`); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink(`${array} foo`); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + + sink(String(array)); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + + sink(array.toString()); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink(array.toString("utf8")); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + + sink(Array.prototype.toString.call(array)); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 +} From 33ab7db98a313ec37f83b8e005ecf15e1fb56be7 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Feb 2025 11:25:03 +0100 Subject: [PATCH 06/16] JS: Handle Array.prototype.toString calls --- .../internal/flow_summaries/AmbiguousCoreMethods.qll | 6 +++++- javascript/ql/test/library-tests/TripleDot/arrays.js | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/internal/flow_summaries/AmbiguousCoreMethods.qll b/javascript/ql/lib/semmle/javascript/internal/flow_summaries/AmbiguousCoreMethods.qll index 660ee4389f25..45ad9cf7a9c9 100644 --- a/javascript/ql/lib/semmle/javascript/internal/flow_summaries/AmbiguousCoreMethods.qll +++ b/javascript/ql/lib/semmle/javascript/internal/flow_summaries/AmbiguousCoreMethods.qll @@ -157,7 +157,11 @@ class Values extends SummarizedCallable { class ToString extends SummarizedCallable { ToString() { this = "Object#toString / Array#toString" } - override DataFlow::MethodCallNode getACallSimple() { result.getMethodName() = "toString" } + override InstanceCall getACallSimple() { + result.(DataFlow::MethodCallNode).getMethodName() = "toString" + or + result = arrayConstructorRef().getAPropertyRead("prototype").getAMemberCall("toString") + } override predicate propagatesFlow(string input, string output, boolean preservesValue) { preservesValue = false and diff --git a/javascript/ql/test/library-tests/TripleDot/arrays.js b/javascript/ql/test/library-tests/TripleDot/arrays.js index ca5732da5af2..76c8f4ffb340 100644 --- a/javascript/ql/test/library-tests/TripleDot/arrays.js +++ b/javascript/ql/test/library-tests/TripleDot/arrays.js @@ -41,5 +41,5 @@ function implicitToString() { sink(array.toString()); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 sink(array.toString("utf8")); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 - sink(Array.prototype.toString.call(array)); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink(Array.prototype.toString.call(array)); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 } From 352924fb8c3548ab0f9542164fa09c0745eb6554 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Feb 2025 11:36:28 +0100 Subject: [PATCH 07/16] JS: Handle a few other stringification contexts --- .../dataflow/internal/DataFlowPrivate.qll | 17 +++++++++++++ .../ql/test/library-tests/TripleDot/arrays.js | 24 +++++++++---------- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll index 0e95d3511559..e25f93bca3f5 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll @@ -1432,6 +1432,23 @@ predicate readStep(Node node1, ContentSet c, Node node2) { c = ContentSet::arrayElementLowerBound(pos.asPositionalLowerBound()) ) ) + or + // Implicitly read array elements before stringification + stringifiedNode(node1) and + node2 = node1 and + c = ContentSet::arrayElement() +} + +private predicate stringifiedNode(Node node) { + exists(Expr e | node = TValueNode(e) | + e = any(AddExpr add).getAnOperand() and + not e instanceof StringLiteral + or + e = any(TemplateLiteral t).getAnElement() and + not e instanceof TemplateElement + ) + or + node = DataFlow::globalVarRef("String").getAnInvocation().getArgument(0) } /** Gets the post-update node for which `node` is the corresponding pre-update node. */ diff --git a/javascript/ql/test/library-tests/TripleDot/arrays.js b/javascript/ql/test/library-tests/TripleDot/arrays.js index 76c8f4ffb340..e0847cbefb13 100644 --- a/javascript/ql/test/library-tests/TripleDot/arrays.js +++ b/javascript/ql/test/library-tests/TripleDot/arrays.js @@ -25,18 +25,18 @@ function implicitToString() { const array = [source('implicitToString.1')]; array.push(source('implicitToString.2')) - sink(array + "foo"); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 - sink("foo" + array); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 - sink("" + array); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 - sink(array + 1); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 - sink(1 + array); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 - sink(unknown() + array); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 - sink(array + unknown()); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 - - sink(`${array}`); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 - sink(`${array} foo`); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 - - sink(String(array)); // $ MISSING: hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink(array + "foo"); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink("foo" + array); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink("" + array); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink(array + 1); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink(1 + array); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink(unknown() + array); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink(array + unknown()); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + + sink(`${array}`); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink(`${array} foo`); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + + sink(String(array)); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 sink(array.toString()); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 sink(array.toString("utf8")); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 From 08b9d934c0a6076a7f65a90fba519ff95fddf3d0 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Feb 2025 11:37:44 +0100 Subject: [PATCH 08/16] JS: Add a negative test --- javascript/ql/test/library-tests/TripleDot/arrays.js | 1 + 1 file changed, 1 insertion(+) diff --git a/javascript/ql/test/library-tests/TripleDot/arrays.js b/javascript/ql/test/library-tests/TripleDot/arrays.js index e0847cbefb13..9376d4aba6c8 100644 --- a/javascript/ql/test/library-tests/TripleDot/arrays.js +++ b/javascript/ql/test/library-tests/TripleDot/arrays.js @@ -42,4 +42,5 @@ function implicitToString() { sink(array.toString("utf8")); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 sink(Array.prototype.toString.call(array)); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2 + sink(Object.prototype.toString.call(array)); // OK - returns "[object Array]" } From 4e325d9f1c3c29ac833e54a1921062db4ad45ea5 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Feb 2025 11:53:50 +0100 Subject: [PATCH 09/16] JS: Convert some exception steps to legacy --- .../ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll b/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll index dcf361f6734e..a49cf639c928 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll @@ -96,7 +96,7 @@ module LodashUnderscore { /** * A data flow step propagating an exception thrown from a callback to a Lodash/Underscore function. */ - private class ExceptionStep extends DataFlow::SharedFlowStep { + private class ExceptionStep extends DataFlow::LegacyFlowStep { override predicate step(DataFlow::Node pred, DataFlow::Node succ) { exists(DataFlow::CallNode call, string name | // Members ending with By, With, or While indicate that they are a variant of From 6e074c301fde57cd889b4854b129dd686d5f4bcf Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Feb 2025 14:54:45 +0100 Subject: [PATCH 10/16] JS: Port lodash callback steps to flow summaries Not all of lodash, just the callbacks we already modeled plus a few easy ones --- .../frameworks/LodashUnderscore.qll | 171 +++++++++++++++++- 1 file changed, 170 insertions(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll b/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll index a49cf639c928..e23729bcb3c1 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll @@ -144,7 +144,13 @@ module LodashUnderscore { name = ["union", "zip"] and pred = call.getAnArgument() and succ = call - or + ) + } + + private predicate underscoreTaintStepLegacy(DataFlow::Node pred, DataFlow::Node succ) { + exists(string name, DataFlow::CallNode call | + call = any(Member member | member.getName() = name).getACall() + | name = ["each", "map", "every", "some", "max", "min", "sortBy", "partition", "mapObject", "tap"] and pred = call.getArgument(0) and @@ -168,6 +174,169 @@ module LodashUnderscore { underscoreTaintStep(pred, succ) } } + + private class UnderscoreTaintStepLegacy extends TaintTracking::LegacyTaintStep { + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + underscoreTaintStepLegacy(pred, succ) + } + } + + private class LodashEach extends DataFlow::SummarizedCallable { + LodashEach() { this = "_.each-like" } + + override DataFlow::CallNode getACall() { + result = member(["each", "eachRight", "forEach", "forEachRight", "every", "some"]).getACall() + } + + override predicate propagatesFlow(string input, string output, boolean preservesValue) { + preservesValue = true and + input = "Argument[0].ArrayElement" and + output = "Argument[1].Parameter[0]" + } + } + + private class LodashMap extends DataFlow::SummarizedCallable { + LodashMap() { this = "_.map" } + + override DataFlow::CallNode getACall() { result = member("map").getACall() } + + override predicate propagatesFlow(string input, string output, boolean preservesValue) { + ( + input = "Argument[0].ArrayElement" and + output = "Argument[1].Parameter[0]" + or + input = "Argument[1].ReturnValue" and + output = "ReturnValue.ArrayElement" + ) and + preservesValue = true + } + } + + private class LodashFlatMap extends DataFlow::SummarizedCallable { + LodashFlatMap() { this = "_.flatMap" } + + override DataFlow::CallNode getACall() { result = member("flatMap").getACall() } + + override predicate propagatesFlow(string input, string output, boolean preservesValue) { + ( + input = "Argument[0].ArrayElement" and + output = "Argument[1].Parameter[0]" + or + input = "Argument[1].ReturnValue.WithoutArrayElement" and + output = "ReturnValue.ArrayElement" + or + input = "Argument[1].ReturnValue.ArrayElement" and + output = "ReturnValue.ArrayElement" + ) and + preservesValue = true + } + } + + private class LodashFlatMapDeep extends DataFlow::SummarizedCallable { + LodashFlatMapDeep() { this = "_.flatMapDeep" } + + override DataFlow::CallNode getACall() { + result = member(["flatMapDeep", "flatMapDepth"]).getACall() + } + + override predicate propagatesFlow(string input, string output, boolean preservesValue) { + ( + input = "Argument[0].ArrayElement" and + output = "Argument[1].Parameter[0]" + or + input = "Argument[1].ReturnValue.WithoutArrayElement" and + output = "ReturnValue.ArrayElement" + or + input = "Argument[1].ReturnValue.ArrayElementDeep" and + output = "ReturnValue.ArrayElement" + ) and + preservesValue = true + } + } + + private class LodashReduce extends DataFlow::SummarizedCallable { + LodashReduce() { this = "_.reduce-like" } + + override DataFlow::CallNode getACall() { result = member(["reduce", "reduceRight"]).getACall() } + + override predicate propagatesFlow(string input, string output, boolean preservesValue) { + ( + input = "Argument[0].ArrayElement" and + output = "Argument[1].Parameter[1]" + or + input = ["Argument[1].ReturnValue", "Argument[2]"] and + output = ["ReturnValue", "Argument[1].Parameter[0]"] + ) and + preservesValue = true + } + } + + private class LoashSortBy extends DataFlow::SummarizedCallable { + LoashSortBy() { this = "_.sortBy-like" } + + override DataFlow::CallNode getACall() { result = member(["sortBy", "orderBy"]).getACall() } + + override predicate propagatesFlow(string input, string output, boolean preservesValue) { + input = "Argument[0].ArrayElement" and + output = ["Argument[1].Parameter[1]", "ReturnValue.ArrayElement"] and + preservesValue = true + } + } + + private class LodashMinMaxBy extends DataFlow::SummarizedCallable { + LodashMinMaxBy() { this = "_.minBy / _.maxBy" } + + override DataFlow::CallNode getACall() { result = member(["minBy", "maxBy"]).getACall() } + + override predicate propagatesFlow(string input, string output, boolean preservesValue) { + input = "Argument[0].ArrayElement" and + output = ["Argument[1].Parameter[1]", "ReturnValue"] and + preservesValue = true + } + } + + private class LodashPartition extends DataFlow::SummarizedCallable { + LodashPartition() { this = "_.partition" } + + override DataFlow::CallNode getACall() { result = member(["partition"]).getACall() } + + override predicate propagatesFlow(string input, string output, boolean preservesValue) { + input = "Argument[0].ArrayElement" and + output = ["Argument[1].Parameter[1]", "ReturnValue.ArrayElement.ArrayElement"] and + preservesValue = true + } + } + + private class UnderscoreMapObject extends DataFlow::SummarizedCallable { + UnderscoreMapObject() { this = "_.mapObject" } + + override DataFlow::CallNode getACall() { result = member("mapObject").getACall() } + + override predicate propagatesFlow(string input, string output, boolean preservesValue) { + // Just collapse all properties with AnyMember. We could be more precise by generating a summary + // for each property name, but for a rarely-used method like this it dosn't seem worth it. + ( + input = "Argument[0].AnyMember" and + output = "Argument[1].Parameter[1]" + or + input = "Argument[1].ReturnValue" and + output = "ReturnValue.AnyMember" + ) and + preservesValue = true + } + } + + private class LodashTap extends DataFlow::SummarizedCallable { + LodashTap() { this = "_.tap" } + + override DataFlow::CallNode getACall() { result = member("tap").getACall() } + + override predicate propagatesFlow(string input, string output, boolean preservesValue) { + input = "Argument[0]" and + output = ["Argument[1].Parameter[0]", "ReturnValue"] and + preservesValue = true + } + } } /** From a54f0a74f116d121310aea15a456abcef0775be2 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Feb 2025 15:00:02 +0100 Subject: [PATCH 11/16] JS: Target post-update node instead of getALocalSource getAPropertyWrite() contains getALocalSource() under the the hood. Don't rely on that to find the successor of a mutation. --- javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll index 237c7c45dd66..01d2b10a14ea 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll @@ -409,7 +409,7 @@ module TaintTracking { not assgn.getWriteNode() instanceof Property and // not a write inside an object literal pred = assgn.getRhs() and assgn = obj.getAPropertyWrite() and - succ = obj + succ = assgn.getBase().getPostUpdateNode() | obj instanceof DataFlow::ObjectLiteralNode or From c9587028305f8649439458052dcb037ea1066d80 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Feb 2025 20:30:07 +0100 Subject: [PATCH 12/16] JS: Accept some unproblematic consistency warnings --- .../test/library-tests/FlowSummary/DataFlowConsistency.expected | 2 ++ 1 file changed, 2 insertions(+) diff --git a/javascript/ql/test/library-tests/FlowSummary/DataFlowConsistency.expected b/javascript/ql/test/library-tests/FlowSummary/DataFlowConsistency.expected index da22cf7e7786..f3b3ca527c3b 100644 --- a/javascript/ql/test/library-tests/FlowSummary/DataFlowConsistency.expected +++ b/javascript/ql/test/library-tests/FlowSummary/DataFlowConsistency.expected @@ -20,6 +20,7 @@ reverseRead | tst.js:267:28:267:31 | map3 | Origin of readStep is missing a PostUpdateNode. | argHasPostUpdate postWithInFlow +| file://:0:0:0:0 | [summary] to write: Argument[0] in _.tap | PostUpdateNode should not be the target of local flow. | | file://:0:0:0:0 | [summary] to write: Argument[1] in Array method with flow into callback | PostUpdateNode should not be the target of local flow. | | file://:0:0:0:0 | [summary] to write: Argument[1] in Array#filter | PostUpdateNode should not be the target of local flow. | | file://:0:0:0:0 | [summary] to write: Argument[1] in Array#find / Array#findLast | PostUpdateNode should not be the target of local flow. | @@ -29,6 +30,7 @@ postWithInFlow | file://:0:0:0:0 | [summary] to write: Argument[1] in Array#reduce / Array#reduceRight | PostUpdateNode should not be the target of local flow. | | file://:0:0:0:0 | [summary] to write: Argument[2] in 'array.prototype.find' / 'array-find' | PostUpdateNode should not be the target of local flow. | | file://:0:0:0:0 | [summary] to write: Argument[2] in Array.from(arg, callback, [thisArg]) | PostUpdateNode should not be the target of local flow. | +| file://:0:0:0:0 | [summary] to write: Argument[2] in _.reduce-like | PostUpdateNode should not be the target of local flow. | | file://:0:0:0:0 | [summary] to write: Argument[this] in Array#flatMap | PostUpdateNode should not be the target of local flow. | | file://:0:0:0:0 | [summary] to write: Argument[this] in Array#forEach / Map#forEach / Set#forEach | PostUpdateNode should not be the target of local flow. | | file://:0:0:0:0 | [summary] to write: Argument[this] in Array#map | PostUpdateNode should not be the target of local flow. | From e61068337789f382965b58f88904369656eebe94 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 18 Feb 2025 09:25:23 +0100 Subject: [PATCH 13/16] JS: Linter fix --- .../ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll b/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll index e23729bcb3c1..b9feb8aab8fb 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll @@ -298,7 +298,7 @@ module LodashUnderscore { private class LodashPartition extends DataFlow::SummarizedCallable { LodashPartition() { this = "_.partition" } - override DataFlow::CallNode getACall() { result = member(["partition"]).getACall() } + override DataFlow::CallNode getACall() { result = member("partition").getACall() } override predicate propagatesFlow(string input, string output, boolean preservesValue) { input = "Argument[0].ArrayElement" and From 82a4b172183326e19876a75420ba00f02ba192c5 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 18 Feb 2025 09:43:08 +0100 Subject: [PATCH 14/16] JS: Change note --- .../src/change-notes/2025-02-18-no-implicit-array-taint.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 javascript/ql/src/change-notes/2025-02-18-no-implicit-array-taint.md diff --git a/javascript/ql/src/change-notes/2025-02-18-no-implicit-array-taint.md b/javascript/ql/src/change-notes/2025-02-18-no-implicit-array-taint.md new file mode 100644 index 000000000000..444ffb30905f --- /dev/null +++ b/javascript/ql/src/change-notes/2025-02-18-no-implicit-array-taint.md @@ -0,0 +1,5 @@ +--- +category: majorAnalysis +--- +* Improved precision of data flow through arrays, fixing some spurious flows + that would sometimes cause the `length` property of an array to be seen as tainted. From 7486742c37533ac9e4309f5cf6b95948a9ec4707 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 18 Feb 2025 16:53:40 +0100 Subject: [PATCH 15/16] JS: Fix model of _.sortBy --- .../ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll b/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll index b9feb8aab8fb..67ddfaadf91b 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll @@ -278,7 +278,7 @@ module LodashUnderscore { override predicate propagatesFlow(string input, string output, boolean preservesValue) { input = "Argument[0].ArrayElement" and - output = ["Argument[1].Parameter[1]", "ReturnValue.ArrayElement"] and + output = ["Argument[1].Parameter[0]", "ReturnValue.ArrayElement"] and preservesValue = true } } From 804a1a6cb086f7532e4d9259570fdb5d91886d69 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 18 Feb 2025 16:58:04 +0100 Subject: [PATCH 16/16] JS: Handle array of sorting criteria --- .../lib/semmle/javascript/frameworks/LodashUnderscore.qll | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll b/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll index 67ddfaadf91b..7c2e6aa37a58 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll @@ -278,7 +278,11 @@ module LodashUnderscore { override predicate propagatesFlow(string input, string output, boolean preservesValue) { input = "Argument[0].ArrayElement" and - output = ["Argument[1].Parameter[0]", "ReturnValue.ArrayElement"] and + output = + [ + "Argument[1].Parameter[0]", "Argument[1].ArrayElement.Parameter[0]", + "ReturnValue.ArrayElement" + ] and preservesValue = true } }