From a97afee6b3f4e75dfe26dbccd2c249c0bf9a7119 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 13 Aug 2025 16:20:03 -0400 Subject: [PATCH 01/18] Parameterize UI5PathGraph on `PathNodeSig` and `PathGraphSig` --- .../frameworks/ui5/dataflow/DataFlow.qll | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/DataFlow.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/DataFlow.qll index 94675378d..dbbfb8bab 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/DataFlow.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/DataFlow.qll @@ -4,7 +4,7 @@ import advanced_security.javascript.frameworks.ui5.UI5 import advanced_security.javascript.frameworks.ui5.UI5View import advanced_security.javascript.frameworks.ui5.RemoteFlowSources import advanced_security.javascript.frameworks.ui5.dataflow.FlowSteps -private import StdLibDataFlow::DataFlow::PathGraph as DataFlowPathGraph +// private import StdLibDataFlow::DataFlow::PathGraph as DataFlowPathGraph private import PatchDataFlow /** @@ -60,7 +60,7 @@ class LocalModelContentBoundBidirectionallyToHtmlISinkControl extends DomBasedXs UI5Control getControlDeclaration() { result = controlDeclaration } } -module UI5PathGraph { +module UI5PathGraph ConfigModule> { private newtype TNode = TUI5BindingPathNode(UI5BindingPath path) or TDataFlowNode(DataFlow::Node node) @@ -92,7 +92,7 @@ module UI5PathGraph { .hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) } - DataFlow::PathNode getPathNode() { result.getNode() = this.asDataFlowNode() } + ConfigPathNode getPathNode() { result.getNode() = this.asDataFlowNode() } UI5PathNode getAPrimarySource() { if this.asDataFlowNode() instanceof LocalModelContentBoundBidirectionallyToSourceControl @@ -124,9 +124,9 @@ module UI5PathGraph { query predicate nodes(UI5PathNode ui5PathNode) { exists(ui5PathNode.asUI5BindingPathNode()) or - exists(DataFlow::PathNode pathNode | + exists(ConfigPathNode pathNode | pathNode.getNode() = ui5PathNode.asDataFlowNode() and - DataFlowPathGraph::nodes(pathNode) + ConfigModule::nodes(pathNode, _, _) ) } @@ -182,10 +182,10 @@ module UI5PathGraph { query predicate edges(UI5PathNode ui5PathNodePred, UI5PathNode ui5PathNodeSucc) { /* Include all existing dataflow edges */ - exists(DataFlow::PathNode pathNodeFrom, DataFlow::PathNode pathNodeTo | + exists(ConfigPathNode pathNodeFrom, ConfigPathNode pathNodeTo | pathNodeFrom.getNode() = ui5PathNodePred.asDataFlowNode() and pathNodeTo.getNode() = ui5PathNodeSucc.asDataFlowNode() and - DataFlowPathGraph::edges(pathNodeFrom, pathNodeTo) + ConfigModule::edges(pathNodeFrom, pathNodeTo, _, _) ) and /* ========= TODO: Legacy code ========= */ /* Exclude duplicate edge from model to handler parameter */ From 17f28976c8392b7a2256215d84a9ab5be1a5e8c0 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 13 Aug 2025 16:48:38 -0400 Subject: [PATCH 02/18] Port over `UI5Xss` --- .../javascript/frameworks/ui5/UI5XssQuery.qll | 53 +++++++++---------- .../frameworks/ui5/src/UI5Xss/UI5Xss.ql | 19 ++++--- 2 files changed, 37 insertions(+), 35 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5XssQuery.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5XssQuery.qll index 7261e4587..71d83278a 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5XssQuery.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5XssQuery.qll @@ -1,39 +1,18 @@ import javascript import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow as UI5DataFlow import advanced_security.javascript.frameworks.ui5.UI5View -import semmle.javascript.security.dataflow.DomBasedXssQuery as DomBasedXss +private import semmle.javascript.security.dataflow.DomBasedXssQuery as DomBasedXss -class Configuration extends DomBasedXss::Configuration { - override predicate isSource(DataFlow::Node start) { - super.isSource(start) +module UI5Xss implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node start) { + DomBasedXss::DomBasedXssConfig::isSource(start, _) or start instanceof RemoteFlowSource } - override predicate isAdditionalFlowStep( - DataFlow::Node start, DataFlow::Node end, DataFlow::FlowLabel inLabel, - DataFlow::FlowLabel outLabel - ) { - /* Already an additional flow step defined in `DomBasedXssQuery::Configuration` */ - super.isAdditionalFlowStep(start, end, inLabel, outLabel) - or - /* TODO: Legacy code */ - /* Handler argument node to handler parameter */ - exists(UI5Handler h | - start = h.getBindingPath().getNode() and - /* - * Ideally we would like to show an intermediate node where - * the handler is bound to a control, but there is no sourceNode there - * `end = h.getBindingPath() or start = h.getBindingPath()` - */ - - end = h.getParameter(0) - ) - } - - override predicate isBarrier(DataFlow::Node node) { + predicate isBarrier(DataFlow::Node node) { /* 1. Already a sanitizer defined in `DomBasedXssQuery::Configuration` */ - super.isSanitizer(node) + DomBasedXss::DomBasedXssConfig::isBarrier(node) or /* 2. Value read from a non-string control property */ exists(PropertyMetadata m | not m.isUnrestrictedStringType() | node = m) @@ -53,10 +32,28 @@ class Configuration extends DomBasedXss::Configuration { ["encodeCSS", "encodeJS", "encodeURL", "encodeURLParameters", "encodeXML", "encodeHTML"] } - override predicate isSink(DataFlow::Node node) { + predicate isSink(DataFlow::Node node) { node instanceof UI5ExtHtmlISink or node instanceof UI5ModelHtmlISink } + + predicate isAdditionalFlowStep(DataFlow::Node start, DataFlow::Node end) { + /* Already an additional flow step defined in `DomBasedXssQuery::Configuration` */ + DomBasedXss::DomBasedXssConfig::isAdditionalFlowStep(start, _, end, _) + or + /* TODO: Legacy code */ + /* Handler argument node to handler parameter */ + exists(UI5Handler h | + start = h.getBindingPath().getNode() and + /* + * Ideally we would like to show an intermediate node where + * the handler is bound to a control, but there is no sourceNode there + * `end = h.getBindingPath() or start = h.getBindingPath()` + */ + + end = h.getParameter(0) + ) + } } /** diff --git a/javascript/frameworks/ui5/src/UI5Xss/UI5Xss.ql b/javascript/frameworks/ui5/src/UI5Xss/UI5Xss.ql index 01cee5e5a..1bd1db5c9 100644 --- a/javascript/frameworks/ui5/src/UI5Xss/UI5Xss.ql +++ b/javascript/frameworks/ui5/src/UI5Xss/UI5Xss.ql @@ -13,17 +13,22 @@ */ import javascript -import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow as UI5DataFlow -import UI5DataFlow::UI5PathGraph +import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow import advanced_security.javascript.frameworks.ui5.UI5XssQuery +module UI5XssFlow = TaintTracking::Global; + +module UI5XssUI5PathGraph = UI5PathGraph; + +import UI5XssUI5PathGraph + from - Configuration config, UI5PathGraph::UI5PathNode source, UI5PathGraph::UI5PathNode sink, - UI5PathGraph::UI5PathNode primarySource, UI5PathGraph::UI5PathNode primarySink + UI5XssUI5PathGraph::UI5PathNode source, UI5XssUI5PathGraph::UI5PathNode sink, + UI5XssUI5PathGraph::UI5PathNode primarySource, UI5XssUI5PathGraph::UI5PathNode primarySink where - config.hasFlowPath(source.getPathNode(), sink.getPathNode()) and - config.isSource(source.asDataFlowNode()) and - config.isSink(sink.asDataFlowNode()) and + UI5XssFlow::flowPath(source.getPathNode(), sink.getPathNode()) and + UI5Xss::isSource(source.asDataFlowNode()) and + UI5Xss::isSink(sink.asDataFlowNode()) and primarySource = source.getAPrimarySource() and primarySink = sink.getAPrimaryHtmlISink() select primarySink, primarySource, primarySink, "XSS vulnerability due to $@.", primarySource, From c62ef21c78fac70fe5ce5648a6e733c954cc7d99 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 13 Aug 2025 16:49:13 -0400 Subject: [PATCH 03/18] Update expected results of UI5Xss All of the disappeared edges are circular ones that "circles back" on itself and does not influence the analysis at all. --- .../ui5/test/queries/UI5Xss/xss-html-control/UI5Xss.expected | 1 - .../test/queries/UI5Xss/xss-html-external-model/UI5Xss.expected | 1 - .../ui5/test/queries/UI5Xss/xss-html-view/UI5Xss.expected | 1 - .../ui5/test/queries/UI5Xss/xss-js-view/UI5Xss.expected | 1 - .../ui5/test/queries/UI5Xss/xss-json-view/UI5Xss.expected | 1 - .../ui5/test/queries/UI5Xss/xss-webc-control/UI5Xss.expected | 1 - 6 files changed, 6 deletions(-) diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-html-control/UI5Xss.expected b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-html-control/UI5Xss.expected index 42d5ecc13..3730e21d3 100644 --- a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-html-control/UI5Xss.expected +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-html-control/UI5Xss.expected @@ -3,7 +3,6 @@ nodes | webapp/view/app.view.xml:5:5:7:28 | value={/input} | | webapp/view/app.view.xml:8:5:8:36 | content={/input} | edges -| webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/controller/app.controller.js:9:17:9:27 | input: null | | webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/view/app.view.xml:5:5:7:28 | value={/input} | | webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/view/app.view.xml:8:5:8:36 | content={/input} | | webapp/controller/app.controller.js:11:26:11:45 | new JSONModel(oData) | webapp/view/app.view.xml:8:5:8:36 | content={/input} | diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-html-external-model/UI5Xss.expected b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-html-external-model/UI5Xss.expected index 64079365b..0fcd96cae 100644 --- a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-html-external-model/UI5Xss.expected +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-html-external-model/UI5Xss.expected @@ -4,7 +4,6 @@ nodes | webapp/view/app.view.xml:8:5:8:36 | content={/input} | edges | webapp/controller/app.controller.js:8:26:8:63 | new JSO ... .json") | webapp/view/app.view.xml:8:5:8:36 | content={/input} | -| webapp/controller/app.controller.js:8:40:8:62 | "contro ... l.json" | webapp/controller/app.controller.js:8:40:8:62 | "contro ... l.json" | | webapp/controller/app.controller.js:8:40:8:62 | "contro ... l.json" | webapp/view/app.view.xml:5:5:7:28 | value={/input} | | webapp/controller/app.controller.js:8:40:8:62 | "contro ... l.json" | webapp/view/app.view.xml:8:5:8:36 | content={/input} | | webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/controller/app.controller.js:8:26:8:63 | new JSO ... .json") | diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-html-view/UI5Xss.expected b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-html-view/UI5Xss.expected index 76d479a27..a80edc7f7 100644 --- a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-html-view/UI5Xss.expected +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-html-view/UI5Xss.expected @@ -3,7 +3,6 @@ nodes | webapp/view/app.view.html:5:11:5:31 | data-value={/input} | | webapp/view/app.view.html:8:11:8:33 | data-content={/input} | edges -| webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/controller/app.controller.js:9:17:9:27 | input: null | | webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/view/app.view.html:5:11:5:31 | data-value={/input} | | webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/view/app.view.html:8:11:8:33 | data-content={/input} | | webapp/controller/app.controller.js:11:26:11:45 | new JSONModel(oData) | webapp/view/app.view.html:8:11:8:33 | data-content={/input} | diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-js-view/UI5Xss.expected b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-js-view/UI5Xss.expected index cbbdaa38e..eca9c570c 100644 --- a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-js-view/UI5Xss.expected +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-js-view/UI5Xss.expected @@ -3,7 +3,6 @@ nodes | webapp/view/app.view.js:18:20:18:29 | /input | | webapp/view/app.view.js:21:22:21:31 | /input | edges -| webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/controller/app.controller.js:9:17:9:27 | input: null | | webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/view/app.view.js:18:20:18:29 | /input | | webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/view/app.view.js:21:22:21:31 | /input | | webapp/controller/app.controller.js:11:26:11:45 | new JSONModel(oData) | webapp/view/app.view.js:21:22:21:31 | /input | diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-json-view/UI5Xss.expected b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-json-view/UI5Xss.expected index 962978f48..f6b7bb7ec 100644 --- a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-json-view/UI5Xss.expected +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-json-view/UI5Xss.expected @@ -3,7 +3,6 @@ nodes | webapp/view/app.view.json:9:13:9:22 | "value": "{/input}" | | webapp/view/app.view.json:13:15:13:24 | "content": "{/input}" | edges -| webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/controller/app.controller.js:9:17:9:27 | input: null | | webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/view/app.view.json:9:13:9:22 | "value": "{/input}" | | webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/view/app.view.json:13:15:13:24 | "content": "{/input}" | | webapp/controller/app.controller.js:11:26:11:45 | new JSONModel(oData) | webapp/view/app.view.json:13:15:13:24 | "content": "{/input}" | diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-webc-control/UI5Xss.expected b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-webc-control/UI5Xss.expected index 81b30aefe..92890f272 100644 --- a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-webc-control/UI5Xss.expected +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-webc-control/UI5Xss.expected @@ -5,7 +5,6 @@ nodes | webapp/view/app.view.xml:15:5:18:28 | value={/input} | | webapp/view/app.view.xml:22:5:22:37 | content={/input} | edges -| webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/controller/app.controller.js:9:17:9:27 | input: null | | webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/view/app.view.xml:8:5:10:26 | value={/input} | | webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/view/app.view.xml:12:13:12:37 | text={/input} | | webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/view/app.view.xml:15:5:18:28 | value={/input} | From 7a9f335e6ae52932afc00708016b2ff071116ce8 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 13 Aug 2025 16:56:55 -0400 Subject: [PATCH 04/18] Rename the second parameter to `ConfigPathGraph` --- .../javascript/frameworks/ui5/dataflow/DataFlow.qll | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/DataFlow.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/DataFlow.qll index dbbfb8bab..964f945ba 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/DataFlow.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/DataFlow.qll @@ -60,7 +60,7 @@ class LocalModelContentBoundBidirectionallyToHtmlISinkControl extends DomBasedXs UI5Control getControlDeclaration() { result = controlDeclaration } } -module UI5PathGraph ConfigModule> { +module UI5PathGraph ConfigPathGraph> { private newtype TNode = TUI5BindingPathNode(UI5BindingPath path) or TDataFlowNode(DataFlow::Node node) @@ -126,7 +126,7 @@ module UI5PathGraph Con or exists(ConfigPathNode pathNode | pathNode.getNode() = ui5PathNode.asDataFlowNode() and - ConfigModule::nodes(pathNode, _, _) + ConfigPathGraph::nodes(pathNode, _, _) ) } @@ -185,7 +185,7 @@ module UI5PathGraph Con exists(ConfigPathNode pathNodeFrom, ConfigPathNode pathNodeTo | pathNodeFrom.getNode() = ui5PathNodePred.asDataFlowNode() and pathNodeTo.getNode() = ui5PathNodeSucc.asDataFlowNode() and - ConfigModule::edges(pathNodeFrom, pathNodeTo, _, _) + ConfigPathGraph::edges(pathNodeFrom, pathNodeTo, _, _) ) and /* ========= TODO: Legacy code ========= */ /* Exclude duplicate edge from model to handler parameter */ From 781ae31ff05e8f57a76c939bfb3d2e9f93dcf1bd Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 13 Aug 2025 17:13:32 -0400 Subject: [PATCH 05/18] Port over UI5PathInjection - Port over UI5PathInjection. - Create a UI5PathInjectionQuery file and move the configuration there. - Update expected results of the query. --- .../frameworks/ui5/UI5PathInjectionQuery.qll | 9 +++++++++ .../src/UI5PathInjection/UI5PathInjection.ql | 20 ++++++++----------- .../UI5PathInjection.expected | 1 - .../UI5PathInjection.expected | 1 - .../UI5PathInjection.expected | 1 - 5 files changed, 17 insertions(+), 15 deletions(-) create mode 100644 javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5PathInjectionQuery.qll diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5PathInjectionQuery.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5PathInjectionQuery.qll new file mode 100644 index 000000000..18d6e27c1 --- /dev/null +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5PathInjectionQuery.qll @@ -0,0 +1,9 @@ +import javascript + +module UI5PathInjection implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource } + + predicate isSink(DataFlow::Node node) { + node = ModelOutput::getASinkNode("ui5-path-injection").asSink() + } +} diff --git a/javascript/frameworks/ui5/src/UI5PathInjection/UI5PathInjection.ql b/javascript/frameworks/ui5/src/UI5PathInjection/UI5PathInjection.ql index 2d370208d..cdea08ff6 100644 --- a/javascript/frameworks/ui5/src/UI5PathInjection/UI5PathInjection.ql +++ b/javascript/frameworks/ui5/src/UI5PathInjection/UI5PathInjection.ql @@ -14,24 +14,20 @@ import javascript import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow -import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow::UI5PathGraph +import advanced_security.javascript.frameworks.ui5.UI5PathInjectionQuery -// import semmle.javascript.security.dataflow.TaintedPathQuery as TaintedPathQuery -class UI5PathInjectionConfiguration extends TaintTracking::Configuration { - UI5PathInjectionConfiguration() { this = "UI5 Path Injection" } +module UI5PathInjectionFlow = TaintTracking::Global; - override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource } +module UI5PathInjectionPathGraph = + UI5PathGraph; - override predicate isSink(DataFlow::Node node) { - node = ModelOutput::getASinkNode("ui5-path-injection").asSink() - } -} +import UI5PathInjectionPathGraph from - UI5PathInjectionConfiguration config, UI5PathNode source, UI5PathNode sink, - UI5PathNode primarySource + UI5PathInjectionPathGraph::UI5PathNode source, UI5PathInjectionPathGraph::UI5PathNode sink, + UI5PathInjectionPathGraph::UI5PathNode primarySource where - config.hasFlowPath(source.getPathNode(), sink.getPathNode()) and + UI5PathInjectionFlow::flowPath(source.getPathNode(), sink.getPathNode()) and primarySource = source.getAPrimarySource() select sink, primarySource, sink, "The path of a saved file depends on a $@.", primarySource, "user-provided value" diff --git a/javascript/frameworks/ui5/test/queries/UI5PathInjection/path-custom-control-property-sanitized/UI5PathInjection.expected b/javascript/frameworks/ui5/test/queries/UI5PathInjection/path-custom-control-property-sanitized/UI5PathInjection.expected index 89d898adc..6a0a93b2d 100644 --- a/javascript/frameworks/ui5/test/queries/UI5PathInjection/path-custom-control-property-sanitized/UI5PathInjection.expected +++ b/javascript/frameworks/ui5/test/queries/UI5PathInjection/path-custom-control-property-sanitized/UI5PathInjection.expected @@ -1,4 +1,3 @@ -WARNING: type 'Configuration' has been deprecated and may be removed in future (UI5PathInjection.ql:20,45-73) nodes | webapp/control/xss.js:8:23:8:37 | { type: "int" } | | webapp/control/xss.js:17:43:17:60 | oControl.getText() | diff --git a/javascript/frameworks/ui5/test/queries/UI5PathInjection/path-custom-control-sanitized/UI5PathInjection.expected b/javascript/frameworks/ui5/test/queries/UI5PathInjection/path-custom-control-sanitized/UI5PathInjection.expected index c0a394b18..14fd36fd5 100644 --- a/javascript/frameworks/ui5/test/queries/UI5PathInjection/path-custom-control-sanitized/UI5PathInjection.expected +++ b/javascript/frameworks/ui5/test/queries/UI5PathInjection/path-custom-control-sanitized/UI5PathInjection.expected @@ -1,4 +1,3 @@ -WARNING: type 'Configuration' has been deprecated and may be removed in future (UI5PathInjection.ql:20,45-73) nodes | webapp/control/xss.js:9:23:9:40 | { type: "string" } | | webapp/control/xss.js:15:21:15:46 | value | diff --git a/javascript/frameworks/ui5/test/queries/UI5PathInjection/path-html-control-df/UI5PathInjection.expected b/javascript/frameworks/ui5/test/queries/UI5PathInjection/path-html-control-df/UI5PathInjection.expected index 52662564d..0d871e07d 100644 --- a/javascript/frameworks/ui5/test/queries/UI5PathInjection/path-html-control-df/UI5PathInjection.expected +++ b/javascript/frameworks/ui5/test/queries/UI5PathInjection/path-html-control-df/UI5PathInjection.expected @@ -1,4 +1,3 @@ -WARNING: type 'Configuration' has been deprecated and may be removed in future (UI5PathInjection.ql:20,45-73) nodes | webapp/controller/app.controller.js:10:17:10:27 | input: null | | webapp/controller/app.controller.js:16:39:16:66 | oModel. ... input') | From 56346829d3a5cd88f6e3bc698ac092da48a8d6a3 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Fri, 15 Aug 2025 17:38:12 -0400 Subject: [PATCH 06/18] Port over `UI5LogInjection` NOTE: The end-to-end results of the queries are okay (the #select portion is unchanged), but there are difference in `nodes` and `edges` that probably needs attention. --- .../frameworks/ui5/UI5LogInjectionQuery.qll | 11 ++++++----- .../ui5/src/UI5LogInjection/UI5LogInjection.ql | 14 +++++++++++--- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogInjectionQuery.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogInjectionQuery.qll index d4139a4db..94523cd27 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogInjectionQuery.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogInjectionQuery.qll @@ -1,12 +1,13 @@ import javascript import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow -private import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow::UI5PathGraph -import semmle.javascript.security.dataflow.LogInjectionQuery as LogInjection +import semmle.javascript.security.dataflow.LogInjectionQuery -class UI5LogInjectionConfiguration extends LogInjection::LogInjectionConfiguration { - override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource } +module UI5LogInjection implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource } - override predicate isSink(DataFlow::Node node) { + predicate isBarrier(DataFlow::Node node) { LogInjectionConfig::isBarrier(node) } + + predicate isSink(DataFlow::Node node) { node = ModelOutput::getASinkNode("ui5-log-injection").asSink() } } diff --git a/javascript/frameworks/ui5/src/UI5LogInjection/UI5LogInjection.ql b/javascript/frameworks/ui5/src/UI5LogInjection/UI5LogInjection.ql index cd0fa4cca..22602be21 100644 --- a/javascript/frameworks/ui5/src/UI5LogInjection/UI5LogInjection.ql +++ b/javascript/frameworks/ui5/src/UI5LogInjection/UI5LogInjection.ql @@ -13,11 +13,19 @@ import javascript import advanced_security.javascript.frameworks.ui5.UI5LogInjectionQuery -import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow::UI5PathGraph +import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow + +module UI5LogInjectionFlow = TaintTracking::Global; + +module UI5LogInjectionUI5PathGraph = + UI5PathGraph; + +import UI5LogInjectionUI5PathGraph from - UI5LogInjectionConfiguration cfg, UI5PathNode source, UI5PathNode sink, UI5PathNode primarySource + UI5LogInjectionUI5PathGraph::UI5PathNode source, UI5LogInjectionUI5PathGraph::UI5PathNode sink, + UI5LogInjectionUI5PathGraph::UI5PathNode primarySource where - cfg.hasFlowPath(source.getPathNode(), sink.getPathNode()) and + UI5LogInjectionFlow::flowPath(source.getPathNode(), sink.getPathNode()) and primarySource = source.getAPrimarySource() select sink, primarySource, sink, "Log entry depends on a $@.", primarySource, "user-provided value" From 4c5b7f68e04d8734178ce3262789464358215e8c Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Fri, 15 Aug 2025 17:39:19 -0400 Subject: [PATCH 07/18] Remove commented out code --- .../javascript/frameworks/ui5/dataflow/DataFlow.qll | 1 - 1 file changed, 1 deletion(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/DataFlow.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/DataFlow.qll index 964f945ba..14cd8947a 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/DataFlow.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/DataFlow.qll @@ -4,7 +4,6 @@ import advanced_security.javascript.frameworks.ui5.UI5 import advanced_security.javascript.frameworks.ui5.UI5View import advanced_security.javascript.frameworks.ui5.RemoteFlowSources import advanced_security.javascript.frameworks.ui5.dataflow.FlowSteps -// private import StdLibDataFlow::DataFlow::PathGraph as DataFlowPathGraph private import PatchDataFlow /** From 32aba1ceae1874852872e02d2a85d5f0aec667dd Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Fri, 15 Aug 2025 18:21:23 -0400 Subject: [PATCH 08/18] Port over `UI5LogsToHttp` NOTE: The end-to-end results of the queries are okay (the `#select` portion is unchanged), but there are difference in `nodes` and `edges` that probably needs attention. --- .../frameworks/ui5/UI5LogsToHttpQuery.qll | 55 ++++++++++++++++ .../ui5/src/UI5LogInjection/UI5LogsToHttp.ql | 66 +++---------------- 2 files changed, 64 insertions(+), 57 deletions(-) create mode 100644 javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogsToHttpQuery.qll diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogsToHttpQuery.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogsToHttpQuery.qll new file mode 100644 index 000000000..c8a80ead1 --- /dev/null +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogsToHttpQuery.qll @@ -0,0 +1,55 @@ +import javascript +import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow +import advanced_security.javascript.frameworks.ui5.UI5LogInjectionQuery + +class ClientRequestInjectionVector extends DataFlow::Node { + ClientRequestInjectionVector() { + exists(ClientRequest req | + this = req.getUrl() or + this = req.getADataNode() + ) + } +} + +class UI5LogEntryFlowState extends string { + UI5LogEntryFlowState() { this = ["not-logged-not-accessed", "logged-and-accessed"] } +} + +module UI5LogEntryToHttp implements DataFlow::StateConfigSig { + class FlowState = UI5LogEntryFlowState; + + predicate isSource(DataFlow::Node node, FlowState state) { + node instanceof RemoteFlowSource and + state = "not-logged-not-accessed" + } + + predicate isAdditionalFlowStep( + DataFlow::Node start, FlowState preState, DataFlow::Node end, FlowState postState + ) { + UI5LogInjection::isAdditionalFlowStep(start, end) and + preState = postState + or + /* + * NOTE: This disjunct is a labeled version of LogArgumentToListener in + * FlowSteps.qll, a DataFlow::SharedFlowStep. As the class is considered + * legacy on version 2.4.0, we leave the two here (labeled) and there + * (unlabeled). This is something we should also tidy up when we migrate + * to the newer APIs. + */ + + inSameWebApp(start.getFile(), end.getFile()) and + start = + ModelOutput::getATypeNode("SapLogger") + .getMember(["debug", "error", "fatal", "info", "trace", "warning"]) + .getACall() + .getAnArgument() and + end = ModelOutput::getATypeNode("SapLogEntries").asSource() and + preState = "not-logged-not-accessed" and + postState = "logged-and-accessed" + } + + predicate isSink(DataFlow::Node node, FlowState state) { + node instanceof ClientRequestInjectionVector and + state = "logged-and-accessed" + } +} diff --git a/javascript/frameworks/ui5/src/UI5LogInjection/UI5LogsToHttp.ql b/javascript/frameworks/ui5/src/UI5LogInjection/UI5LogsToHttp.ql index 5cd48ca78..d95dbf013 100644 --- a/javascript/frameworks/ui5/src/UI5LogInjection/UI5LogsToHttp.ql +++ b/javascript/frameworks/ui5/src/UI5LogInjection/UI5LogsToHttp.ql @@ -12,69 +12,21 @@ */ import javascript +import advanced_security.javascript.frameworks.ui5.UI5LogsToHttpQuery import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow -import semmle.javascript.frameworks.data.internal.ApiGraphModels -import advanced_security.javascript.frameworks.ui5.UI5LogInjectionQuery -import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow::UI5PathGraph -class ClientRequestInjectionVector extends DataFlow::Node { - ClientRequestInjectionVector() { - exists(ClientRequest req | - this = req.getUrl() or - this = req.getADataNode() - ) - } -} +module UI5LogsToHttpFlow = TaintTracking::GlobalWithState; -class UI5LogEntryFlowState extends DataFlow::FlowLabel { - UI5LogEntryFlowState() { this = ["not-logged-not-accessed", "logged-and-accessed"] } -} +module UI5LogsToHttpUI5PathGraph = + UI5PathGraph; -class UI5LogEntryToHttp extends TaintTracking::Configuration { - UI5LogEntryToHttp() { this = "UI5 Log Entry included in an outbound HTTP request" } +import UI5LogsToHttpUI5PathGraph - override predicate isSource(DataFlow::Node node, DataFlow::FlowLabel state) { - node instanceof RemoteFlowSource and - state = "not-logged-not-accessed" - } - - override predicate isAdditionalFlowStep( - DataFlow::Node start, DataFlow::Node end, DataFlow::FlowLabel preState, - DataFlow::FlowLabel postState - ) { - exists(UI5LogInjectionConfiguration cfg | - cfg.isAdditionalFlowStep(start, end) and - preState = postState - ) - or - /* - * NOTE: This disjunct is a labeled version of LogArgumentToListener in - * FlowSteps.qll, a DataFlow::SharedFlowStep. As the class is considered - * legacy on version 2.4.0, we leave the two here (labeled) and there - * (unlabeled). This is something we should also tidy up when we migrate - * to the newer APIs. - */ - - inSameWebApp(start.getFile(), end.getFile()) and - start = - ModelOutput::getATypeNode("SapLogger") - .getMember(["debug", "error", "fatal", "info", "trace", "warning"]) - .getACall() - .getAnArgument() and - end = ModelOutput::getATypeNode("SapLogEntries").asSource() and - preState = "not-logged-not-accessed" and - postState = "logged-and-accessed" - } - - override predicate isSink(DataFlow::Node node, DataFlow::FlowLabel state) { - node instanceof ClientRequestInjectionVector and - state = "logged-and-accessed" - } -} - -from UI5LogEntryToHttp cfg, UI5PathNode source, UI5PathNode sink, UI5PathNode primarySource +from + UI5LogsToHttpUI5PathGraph::UI5PathNode source, UI5LogsToHttpUI5PathGraph::UI5PathNode sink, + UI5LogsToHttpUI5PathGraph::UI5PathNode primarySource where - cfg.hasFlowPath(source.getPathNode(), sink.getPathNode()) and + UI5LogsToHttpFlow::flowPath(source.getPathNode(), sink.getPathNode()) and primarySource = source.getAPrimarySource() select sink, primarySource, sink, "Outbound network request depends on $@ log data.", primarySource, "user-provided" From dbcdcbf4dc0ebbeb4833541d3dfee03a86330564 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Fri, 15 Aug 2025 18:27:43 -0400 Subject: [PATCH 09/18] Port over `UI5UnsafeLogAccess` --- .../ui5/UI5UnsafeLogAccessQuery.qll | 45 ++++++++++++++++ .../src/UI5LogInjection/UI5UnsafeLogAccess.ql | 51 ++++--------------- .../UI5UnsafeLogAccess.expected | 1 - 3 files changed, 54 insertions(+), 43 deletions(-) create mode 100644 javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5UnsafeLogAccessQuery.qll diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5UnsafeLogAccessQuery.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5UnsafeLogAccessQuery.qll new file mode 100644 index 000000000..650094478 --- /dev/null +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5UnsafeLogAccessQuery.qll @@ -0,0 +1,45 @@ +import javascript +import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow +import semmle.javascript.security.dataflow.LogInjectionQuery + +module UI5UnsafeLogAccess implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource } + + predicate isBarrier(DataFlow::Node node) { LogInjectionConfig::isBarrier(node) } + + predicate isSink(DataFlow::Node node) { + node = ModelOutput::getASinkNode("ui5-log-injection").asSink() + } +} + +private newtype TLogEntriesNode = + TDataFlowNode(DataFlow::Node node) { + node = ModelOutput::getATypeNode("SapLogEntries").getInducingNode() + } or + TUI5ControlNode(UI5Control control) { control.getImportPath() = "sap/ui/vk/Notifications" } + +class LogEntriesNode extends TLogEntriesNode { + DataFlow::Node asDataFlowNode() { this = TDataFlowNode(result) } + + UI5Control asUI5ControlNode() { this = TUI5ControlNode(result) } + + File getFile() { + result = this.asDataFlowNode().getFile() + or + result = this.asUI5ControlNode().getView() + } + + string toString() { + result = this.asDataFlowNode().toString() + or + result = this.asUI5ControlNode().toString() + } + + predicate hasLocationInfo( + string filepath, int startline, int startcolumn, int endline, int endcolumn + ) { + this.asDataFlowNode().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) + or + this.asUI5ControlNode().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) + } +} \ No newline at end of file diff --git a/javascript/frameworks/ui5/src/UI5LogInjection/UI5UnsafeLogAccess.ql b/javascript/frameworks/ui5/src/UI5LogInjection/UI5UnsafeLogAccess.ql index 510eff483..23340cac2 100644 --- a/javascript/frameworks/ui5/src/UI5LogInjection/UI5UnsafeLogAccess.ql +++ b/javascript/frameworks/ui5/src/UI5LogInjection/UI5UnsafeLogAccess.ql @@ -12,54 +12,21 @@ import javascript import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow -import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow::UI5PathGraph -import semmle.javascript.security.dataflow.LogInjectionQuery as LogInjection +import advanced_security.javascript.frameworks.ui5.UI5UnsafeLogAccessQuery -class UI5LogInjectionConfiguration extends LogInjection::LogInjectionConfiguration { - override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource } +module UI5UnsafeLogAccessFlow = TaintTracking::Global; - override predicate isSink(DataFlow::Node node) { - node = ModelOutput::getASinkNode("ui5-log-injection").asSink() - } -} +module UI5UnsafeLogAccessFlowUI5PathGraph = + UI5PathGraph; -private newtype TLogEntriesNode = - TDataFlowNode(DataFlow::Node node) { - node = ModelOutput::getATypeNode("SapLogEntries").getInducingNode() - } or - TUI5ControlNode(UI5Control control) { control.getImportPath() = "sap/ui/vk/Notifications" } - -class LogEntriesNode extends TLogEntriesNode { - DataFlow::Node asDataFlowNode() { this = TDataFlowNode(result) } - - UI5Control asUI5ControlNode() { this = TUI5ControlNode(result) } - - File getFile() { - result = this.asDataFlowNode().getFile() - or - result = this.asUI5ControlNode().getView() - } - - string toString() { - result = this.asDataFlowNode().toString() - or - result = this.asUI5ControlNode().toString() - } - - predicate hasLocationInfo( - string filepath, int startline, int startcolumn, int endline, int endcolumn - ) { - this.asDataFlowNode().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) - or - this.asUI5ControlNode().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) - } -} +import UI5UnsafeLogAccessFlowUI5PathGraph from - UI5LogInjectionConfiguration cfg, UI5PathNode source, UI5PathNode sink, UI5PathNode primarySource, - LogEntriesNode logEntries + UI5UnsafeLogAccessFlowUI5PathGraph::UI5PathNode source, + UI5UnsafeLogAccessFlowUI5PathGraph::UI5PathNode sink, + UI5UnsafeLogAccessFlowUI5PathGraph::UI5PathNode primarySource, LogEntriesNode logEntries where - cfg.hasFlowPath(source.getPathNode(), sink.getPathNode()) and + UI5UnsafeLogAccessFlow::flowPath(source.getPathNode(), sink.getPathNode()) and primarySource = source.getAPrimarySource() and inSameWebApp(source.getFile(), logEntries.getFile()) select logEntries, primarySource, sink, "Accessed log entries depend on $@.", primarySource, diff --git a/javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/UI5UnsafeLogAccess.expected b/javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/UI5UnsafeLogAccess.expected index fda48aa60..d032912bd 100644 --- a/javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/UI5UnsafeLogAccess.expected +++ b/javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/UI5UnsafeLogAccess.expected @@ -1,4 +1,3 @@ -WARNING: type 'LogInjectionConfiguration' has been deprecated and may be removed in future (UI5UnsafeLogAccess.ql:18,44-83) nodes | webapp/controller/app.controller.js:11:11:11:21 | input: null | | webapp/controller/app.controller.js:17:13:17:48 | input | From fbc03f51be960c411d42238248321990654f6351 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 18 Aug 2025 17:41:51 -0400 Subject: [PATCH 10/18] Port over `UI5FormulaInjection` --- .../frameworks/ui5/UI5FormulaInjectionQuery.qll | 12 +++++------- .../UI5FormulaInjection/UI5FormulaInjection.ql | 15 +++++++++++---- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5FormulaInjectionQuery.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5FormulaInjectionQuery.qll index bbfd1acde..0a9e87bda 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5FormulaInjectionQuery.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5FormulaInjectionQuery.qll @@ -8,13 +8,13 @@ import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow private class StoragePutCall extends CallNode { StoragePutCall() { /* 1. This is a call to `sap.ui.util.Storage.put` */ - // 1-1. Required from `sap/ui/util/Storage` + /* 1-1. Required from `sap/ui/util/Storage` */ exists(RequiredObject storageClass | this.getReceiver().getALocalSource() = storageClass.asSourceNode() and this.getCalleeName() = "put" ) or - // 1-2. Direct call to `sap.ui.util.Storage.put` + /* 1-2. Direct call to `sap.ui.util.Storage.put` */ this = globalVarRef("sap") .getAPropertyRead("ui") @@ -109,12 +109,10 @@ private class FileSaveCall extends CallNode { } } -class UI5FormulaInjectionConfiguration extends TaintTracking::Configuration { - UI5FormulaInjectionConfiguration() { this = "UI5 Formula Injection" } +module UI5FormulaInjection implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource } - override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource } - - override predicate isSink(DataFlow::Node node) { + predicate isSink(DataFlow::Node node) { exists(StoragePutCall storagePutCall | node = storagePutCall.getArgument(1)) or exists(FileSaveCall fileSaveCall | diff --git a/javascript/frameworks/ui5/src/UI5FormulaInjection/UI5FormulaInjection.ql b/javascript/frameworks/ui5/src/UI5FormulaInjection/UI5FormulaInjection.ql index 1a8fb1fd0..ca60a3f73 100644 --- a/javascript/frameworks/ui5/src/UI5FormulaInjection/UI5FormulaInjection.ql +++ b/javascript/frameworks/ui5/src/UI5FormulaInjection/UI5FormulaInjection.ql @@ -13,14 +13,21 @@ import javascript import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow -import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow::UI5PathGraph import advanced_security.javascript.frameworks.ui5.UI5FormulaInjectionQuery +module UI5FormulaInjectionFlow = TaintTracking::Global; + +module UI5FormulaInjectionUI5PathGraph = + UI5PathGraph; + +import UI5FormulaInjectionUI5PathGraph + from - UI5FormulaInjectionConfiguration config, UI5PathNode source, UI5PathNode sink, - UI5PathNode primarySource + UI5FormulaInjectionUI5PathGraph::UI5PathNode source, + UI5FormulaInjectionUI5PathGraph::UI5PathNode sink, + UI5FormulaInjectionUI5PathGraph::UI5PathNode primarySource where - config.hasFlowPath(source.getPathNode(), sink.getPathNode()) and + UI5FormulaInjectionFlow::flowPath(source.getPathNode(), sink.getPathNode()) and primarySource = source.getAPrimarySource() select sink, primarySource, sink, "The content of a saved file depends on a $@.", primarySource, "user-provided value" From eae4c93865aad6d8447220a358a496c633ce9060 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 18 Aug 2025 17:42:01 -0400 Subject: [PATCH 11/18] Update expected results of `UI5UnsafeLogAccess` --- .../log-entry-flows-to-notifications/UI5UnsafeLogAccess.expected | 1 - 1 file changed, 1 deletion(-) diff --git a/javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-notifications/UI5UnsafeLogAccess.expected b/javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-notifications/UI5UnsafeLogAccess.expected index 680bdd867..f05e5f1f0 100644 --- a/javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-notifications/UI5UnsafeLogAccess.expected +++ b/javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-notifications/UI5UnsafeLogAccess.expected @@ -1,4 +1,3 @@ -WARNING: type 'LogInjectionConfiguration' has been deprecated and may be removed in future (UI5UnsafeLogAccess.ql:18,44-83) nodes | webapp/controller/app.controller.js:9:17:9:27 | input: null | | webapp/controller/app.controller.js:15:17:15:52 | input | From 9b09c02d59ab167551da115fd0822283af9f1ebd Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 19 Aug 2025 10:33:35 -0400 Subject: [PATCH 12/18] Update expected results These are results with changes in only nodes and edges. We accept it because it means there are no changes in the overall behavior. --- .../avoid-duplicate-alerts/UI5LogInjection.expected | 12 ++---------- .../log-entry-flows-to-sinks/UI5Xss.expected | 4 ---- .../UI5Xss/avoid-duplicate-alerts/UI5Xss.expected | 6 ------ 3 files changed, 2 insertions(+), 20 deletions(-) diff --git a/javascript/frameworks/ui5/test/queries/UI5LogInjection/avoid-duplicate-alerts/UI5LogInjection.expected b/javascript/frameworks/ui5/test/queries/UI5LogInjection/avoid-duplicate-alerts/UI5LogInjection.expected index a9669444b..4ead0a63a 100644 --- a/javascript/frameworks/ui5/test/queries/UI5LogInjection/avoid-duplicate-alerts/UI5LogInjection.expected +++ b/javascript/frameworks/ui5/test/queries/UI5LogInjection/avoid-duplicate-alerts/UI5LogInjection.expected @@ -7,16 +7,12 @@ nodes | LogInjectionTest.js:13:23:13:29 | req.url | | LogInjectionTest.js:14:9:14:32 | value | | LogInjectionTest.js:14:17:14:17 | q | -| LogInjectionTest.js:14:17:14:23 | q.query | -| LogInjectionTest.js:14:17:14:32 | q.query.username | | LogInjectionTest.js:16:26:16:30 | value | | LogInjectionTest.js:21:9:21:36 | q | | LogInjectionTest.js:21:13:21:36 | url.par ... , true) | | LogInjectionTest.js:21:23:21:29 | req.url | | LogInjectionTest.js:22:9:22:32 | value | | LogInjectionTest.js:22:17:22:17 | q | -| LogInjectionTest.js:22:17:22:23 | q.query | -| LogInjectionTest.js:22:17:22:32 | q.query.username | | LogInjectionTest.js:23:9:23:44 | value1 | | LogInjectionTest.js:23:18:23:44 | jQuery. ... (value) | | LogInjectionTest.js:23:39:23:43 | value | @@ -28,16 +24,12 @@ edges | LogInjectionTest.js:13:13:13:36 | url.par ... , true) | LogInjectionTest.js:13:9:13:36 | q | | LogInjectionTest.js:13:23:13:29 | req.url | LogInjectionTest.js:13:13:13:36 | url.par ... , true) | | LogInjectionTest.js:14:9:14:32 | value | LogInjectionTest.js:16:26:16:30 | value | -| LogInjectionTest.js:14:17:14:17 | q | LogInjectionTest.js:14:17:14:23 | q.query | -| LogInjectionTest.js:14:17:14:23 | q.query | LogInjectionTest.js:14:17:14:32 | q.query.username | -| LogInjectionTest.js:14:17:14:32 | q.query.username | LogInjectionTest.js:14:9:14:32 | value | +| LogInjectionTest.js:14:17:14:17 | q | LogInjectionTest.js:14:9:14:32 | value | | LogInjectionTest.js:21:9:21:36 | q | LogInjectionTest.js:22:17:22:17 | q | | LogInjectionTest.js:21:13:21:36 | url.par ... , true) | LogInjectionTest.js:21:9:21:36 | q | | LogInjectionTest.js:21:23:21:29 | req.url | LogInjectionTest.js:21:13:21:36 | url.par ... , true) | | LogInjectionTest.js:22:9:22:32 | value | LogInjectionTest.js:23:39:23:43 | value | -| LogInjectionTest.js:22:17:22:17 | q | LogInjectionTest.js:22:17:22:23 | q.query | -| LogInjectionTest.js:22:17:22:23 | q.query | LogInjectionTest.js:22:17:22:32 | q.query.username | -| LogInjectionTest.js:22:17:22:32 | q.query.username | LogInjectionTest.js:22:9:22:32 | value | +| LogInjectionTest.js:22:17:22:17 | q | LogInjectionTest.js:22:9:22:32 | value | | LogInjectionTest.js:23:9:23:44 | value1 | LogInjectionTest.js:25:26:25:31 | value1 | | LogInjectionTest.js:23:18:23:44 | jQuery. ... (value) | LogInjectionTest.js:23:9:23:44 | value1 | | LogInjectionTest.js:23:39:23:43 | value | LogInjectionTest.js:23:18:23:44 | jQuery. ... (value) | diff --git a/javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/UI5Xss.expected b/javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/UI5Xss.expected index 9f8ee558e..c7d2a3ec1 100644 --- a/javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/UI5Xss.expected +++ b/javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/UI5Xss.expected @@ -4,8 +4,6 @@ nodes | webapp/controller/app.controller.js:17:21:17:48 | oModel. ... input") | | webapp/controller/app.controller.js:18:30:18:34 | input | | webapp/utils/CustomLogListener.js:9:29:9:34 | oEvent | -| webapp/utils/CustomLogListener.js:15:24:15:29 | oEvent | -| webapp/utils/CustomLogListener.js:15:24:15:37 | oEvent.message | | webapp/utils/CustomLogListener.js:16:31:16:36 | oEvent | | webapp/utils/CustomLogListener.js:16:31:16:44 | oEvent.message | | webapp/view/app.view.xml:5:5:7:28 | value={/input} | @@ -18,9 +16,7 @@ edges | webapp/controller/app.controller.js:17:13:17:48 | input | webapp/controller/app.controller.js:18:30:18:34 | input | | webapp/controller/app.controller.js:17:21:17:48 | oModel. ... input") | webapp/controller/app.controller.js:17:13:17:48 | input | | webapp/controller/app.controller.js:18:30:18:34 | input | webapp/utils/CustomLogListener.js:9:29:9:34 | oEvent | -| webapp/utils/CustomLogListener.js:9:29:9:34 | oEvent | webapp/utils/CustomLogListener.js:15:24:15:29 | oEvent | | webapp/utils/CustomLogListener.js:9:29:9:34 | oEvent | webapp/utils/CustomLogListener.js:16:31:16:36 | oEvent | -| webapp/utils/CustomLogListener.js:15:24:15:29 | oEvent | webapp/utils/CustomLogListener.js:15:24:15:37 | oEvent.message | | webapp/utils/CustomLogListener.js:16:31:16:36 | oEvent | webapp/utils/CustomLogListener.js:16:31:16:44 | oEvent.message | | webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/controller/app.controller.js:11:11:11:21 | input: null | | webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/controller/app.controller.js:14:22:14:41 | new JSONModel(oData) | diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/avoid-duplicate-alerts/UI5Xss.expected b/javascript/frameworks/ui5/test/queries/UI5Xss/avoid-duplicate-alerts/UI5Xss.expected index 2081b0e31..428d109bf 100644 --- a/javascript/frameworks/ui5/test/queries/UI5Xss/avoid-duplicate-alerts/UI5Xss.expected +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/avoid-duplicate-alerts/UI5Xss.expected @@ -1,29 +1,23 @@ nodes | XssTest.js:3:9:3:50 | value | | XssTest.js:3:17:3:50 | jQuery. ... param") | -| XssTest.js:4:20:4:24 | value | | XssTest.js:5:27:5:31 | value | | XssTest.js:10:9:10:40 | value | | XssTest.js:10:17:10:40 | documen ... .search | -| XssTest.js:11:20:11:24 | value | | XssTest.js:12:27:12:31 | value | | XssTest.js:17:9:17:40 | value | | XssTest.js:17:17:17:40 | documen ... .search | | XssTest.js:18:9:18:44 | value1 | | XssTest.js:18:18:18:44 | jQuery. ... (value) | | XssTest.js:18:39:18:43 | value | -| XssTest.js:19:20:19:25 | value1 | | XssTest.js:20:27:20:32 | value1 | edges -| XssTest.js:3:9:3:50 | value | XssTest.js:4:20:4:24 | value | | XssTest.js:3:9:3:50 | value | XssTest.js:5:27:5:31 | value | | XssTest.js:3:17:3:50 | jQuery. ... param") | XssTest.js:3:9:3:50 | value | -| XssTest.js:10:9:10:40 | value | XssTest.js:11:20:11:24 | value | | XssTest.js:10:9:10:40 | value | XssTest.js:12:27:12:31 | value | | XssTest.js:10:17:10:40 | documen ... .search | XssTest.js:10:9:10:40 | value | | XssTest.js:17:9:17:40 | value | XssTest.js:18:39:18:43 | value | | XssTest.js:17:17:17:40 | documen ... .search | XssTest.js:17:9:17:40 | value | -| XssTest.js:18:9:18:44 | value1 | XssTest.js:19:20:19:25 | value1 | | XssTest.js:18:9:18:44 | value1 | XssTest.js:20:27:20:32 | value1 | | XssTest.js:18:18:18:44 | jQuery. ... (value) | XssTest.js:18:9:18:44 | value1 | | XssTest.js:18:39:18:43 | value | XssTest.js:18:18:18:44 | jQuery. ... (value) | From 6df3a9ef2a0a3716d8edd509eebf54d0b8194a74 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 19 Aug 2025 10:46:15 -0400 Subject: [PATCH 13/18] Update expected results These are results with changes in only nodes and edges. We accept it because it means there are no changes in the overall behavior. --- .../log-entry-flows-to-sinks/UI5LogsToHttp.expected | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/UI5LogsToHttp.expected b/javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/UI5LogsToHttp.expected index 84c95d294..f6ab248ca 100644 --- a/javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/UI5LogsToHttp.expected +++ b/javascript/frameworks/ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/UI5LogsToHttp.expected @@ -1,9 +1,3 @@ -WARNING: type 'Configuration' has been deprecated and may be removed in future (UI5LogsToHttp.ql:33,33-61) -WARNING: type 'FlowLabel' has been deprecated and may be removed in future (UI5LogsToHttp.ql:29,36-55) -WARNING: type 'FlowLabel' has been deprecated and may be removed in future (UI5LogsToHttp.ql:36,52-71) -WARNING: type 'FlowLabel' has been deprecated and may be removed in future (UI5LogsToHttp.ql:42,47-66) -WARNING: type 'FlowLabel' has been deprecated and may be removed in future (UI5LogsToHttp.ql:43,5-24) -WARNING: type 'FlowLabel' has been deprecated and may be removed in future (UI5LogsToHttp.ql:69,50-69) nodes | webapp/controller/app.controller.js:11:11:11:21 | input: null | | webapp/controller/app.controller.js:17:13:17:48 | input | @@ -14,8 +8,6 @@ nodes | webapp/utils/CustomLogListener.js:13:19:13:32 | oEvent.message | | webapp/utils/LogEntriesToHttp.js:7:13:7:52 | message | | webapp/utils/LogEntriesToHttp.js:7:23:7:41 | Log.getLogEntries() | -| webapp/utils/LogEntriesToHttp.js:7:23:7:44 | Log.get ... es()[0] | -| webapp/utils/LogEntriesToHttp.js:7:23:7:52 | Log.get ... message | | webapp/utils/LogEntriesToHttp.js:11:19:11:25 | message | | webapp/view/app.view.xml:5:5:7:28 | value={/input} | | webapp/view/app.view.xml:8:5:8:37 | content={/output} | @@ -31,9 +23,7 @@ edges | webapp/utils/CustomLogListener.js:9:29:9:34 | oEvent | webapp/utils/CustomLogListener.js:13:19:13:24 | oEvent | | webapp/utils/CustomLogListener.js:13:19:13:24 | oEvent | webapp/utils/CustomLogListener.js:13:19:13:32 | oEvent.message | | webapp/utils/LogEntriesToHttp.js:7:13:7:52 | message | webapp/utils/LogEntriesToHttp.js:11:19:11:25 | message | -| webapp/utils/LogEntriesToHttp.js:7:23:7:41 | Log.getLogEntries() | webapp/utils/LogEntriesToHttp.js:7:23:7:44 | Log.get ... es()[0] | -| webapp/utils/LogEntriesToHttp.js:7:23:7:44 | Log.get ... es()[0] | webapp/utils/LogEntriesToHttp.js:7:23:7:52 | Log.get ... message | -| webapp/utils/LogEntriesToHttp.js:7:23:7:52 | Log.get ... message | webapp/utils/LogEntriesToHttp.js:7:13:7:52 | message | +| webapp/utils/LogEntriesToHttp.js:7:23:7:41 | Log.getLogEntries() | webapp/utils/LogEntriesToHttp.js:7:13:7:52 | message | | webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/controller/app.controller.js:11:11:11:21 | input: null | | webapp/view/app.view.xml:5:5:7:28 | value={/input} | webapp/controller/app.controller.js:14:22:14:41 | new JSONModel(oData) | | webapp/view/app.view.xml:8:5:8:37 | content={/output} | webapp/controller/app.controller.js:12:11:12:22 | output: null | From 638a1ccf110bf775605dbfc960dd62ada39872d6 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 19 Aug 2025 14:59:34 -0400 Subject: [PATCH 14/18] Port over `formulaSinkTest` and expected results --- .../ui5/test/models/sink/formulaSinkTest.expected | 14 +++++++------- .../ui5/test/models/sink/formulaSinkTest.ql | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/javascript/frameworks/ui5/test/models/sink/formulaSinkTest.expected b/javascript/frameworks/ui5/test/models/sink/formulaSinkTest.expected index 80218973a..7b5d53e5c 100644 --- a/javascript/frameworks/ui5/test/models/sink/formulaSinkTest.expected +++ b/javascript/frameworks/ui5/test/models/sink/formulaSinkTest.expected @@ -1,7 +1,7 @@ -| sink.js:118:36:118:40 | code1 | code1 | -| sink.js:119:48:119:52 | code1 | code1 | -| sink.js:120:47:120:51 | code1 | code1 | -| sink.js:122:27:122:31 | code0 | code0 | -| sink.js:123:27:123:31 | code0 | code0 | -| sink.js:125:44:125:48 | code0 | code0 | -| sink.js:126:44:126:48 | code0 | code0 | +| sink.js:118:36:118:40 | code1 | sink.js:118:36:118:40 | code1 | +| sink.js:119:48:119:52 | code1 | sink.js:119:48:119:52 | code1 | +| sink.js:120:47:120:51 | code1 | sink.js:120:47:120:51 | code1 | +| sink.js:122:27:122:31 | code0 | sink.js:122:27:122:31 | code0 | +| sink.js:123:27:123:31 | code0 | sink.js:123:27:123:31 | code0 | +| sink.js:125:44:125:48 | code0 | sink.js:125:44:125:48 | code0 | +| sink.js:126:44:126:48 | code0 | sink.js:126:44:126:48 | code0 | diff --git a/javascript/frameworks/ui5/test/models/sink/formulaSinkTest.ql b/javascript/frameworks/ui5/test/models/sink/formulaSinkTest.ql index 12eb7d9a9..fa7e1c426 100644 --- a/javascript/frameworks/ui5/test/models/sink/formulaSinkTest.ql +++ b/javascript/frameworks/ui5/test/models/sink/formulaSinkTest.ql @@ -7,8 +7,8 @@ import javascript import advanced_security.javascript.frameworks.ui5.UI5FormulaInjectionQuery -import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow as UI5DataFlow +import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow -from UI5FormulaInjectionConfiguration config, DataFlow::Node sink -where config.isSink(sink) +from DataFlow::Node sink +where UI5FormulaInjection::isSink(sink) select sink, sink.toString() From 615c1b9d188472eec8496379815169426b0d91b9 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 19 Aug 2025 15:04:16 -0400 Subject: [PATCH 15/18] Update expected results of `formulaSinkTest` --- .../ui5/test/models/sink/formulaSinkTest.expected | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/javascript/frameworks/ui5/test/models/sink/formulaSinkTest.expected b/javascript/frameworks/ui5/test/models/sink/formulaSinkTest.expected index 7b5d53e5c..80218973a 100644 --- a/javascript/frameworks/ui5/test/models/sink/formulaSinkTest.expected +++ b/javascript/frameworks/ui5/test/models/sink/formulaSinkTest.expected @@ -1,7 +1,7 @@ -| sink.js:118:36:118:40 | code1 | sink.js:118:36:118:40 | code1 | -| sink.js:119:48:119:52 | code1 | sink.js:119:48:119:52 | code1 | -| sink.js:120:47:120:51 | code1 | sink.js:120:47:120:51 | code1 | -| sink.js:122:27:122:31 | code0 | sink.js:122:27:122:31 | code0 | -| sink.js:123:27:123:31 | code0 | sink.js:123:27:123:31 | code0 | -| sink.js:125:44:125:48 | code0 | sink.js:125:44:125:48 | code0 | -| sink.js:126:44:126:48 | code0 | sink.js:126:44:126:48 | code0 | +| sink.js:118:36:118:40 | code1 | code1 | +| sink.js:119:48:119:52 | code1 | code1 | +| sink.js:120:47:120:51 | code1 | code1 | +| sink.js:122:27:122:31 | code0 | code0 | +| sink.js:123:27:123:31 | code0 | code0 | +| sink.js:125:44:125:48 | code0 | code0 | +| sink.js:126:44:126:48 | code0 | code0 | From ab4bc8a61e58268c5a1bb06c60d9d82cfb914c59 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 19 Aug 2025 15:07:27 -0400 Subject: [PATCH 16/18] Replace `AmdModuleDefinition.getDependency/1` --- .../ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll index 752e9eed7..df6a93682 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll @@ -168,7 +168,7 @@ class SapDefineModule extends AmdModuleDefinition::Range, MethodCallExpr, UserMo ) } - string getDependency(int i) { result = this.(AmdModuleDefinition).getDependency(i).getValue() } + string getDependency(int i) { result = this.(AmdModuleDefinition).getDependencyExpr(i).getStringValue() } override string getADependency() { result = this.getDependency(_) } From f541012cd9f3a2142fca3f28ec4d9abadf50f4db Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 8 Sep 2025 10:16:21 -0400 Subject: [PATCH 17/18] Demote `LogArgumentToListener` to a query-dependent flow step --- .../frameworks/ui5/UI5LogsToHttpQuery.qll | 7 ++----- .../frameworks/ui5/dataflow/FlowSteps.qll | 18 +----------------- 2 files changed, 3 insertions(+), 22 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogsToHttpQuery.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogsToHttpQuery.qll index c8a80ead1..25aa22fa4 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogsToHttpQuery.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogsToHttpQuery.qll @@ -30,11 +30,8 @@ module UI5LogEntryToHttp implements DataFlow::StateConfigSig { preState = postState or /* - * NOTE: This disjunct is a labeled version of LogArgumentToListener in - * FlowSteps.qll, a DataFlow::SharedFlowStep. As the class is considered - * legacy on version 2.4.0, we leave the two here (labeled) and there - * (unlabeled). This is something we should also tidy up when we migrate - * to the newer APIs. + * Jump from any argument of a SAP logging function to the `onLogEntry` + * method of a custom log listener in the same application. */ inSameWebApp(start.getFile(), end.getFile()) and diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/FlowSteps.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/FlowSteps.qll index 3fde64d21..504d257e1 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/FlowSteps.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/FlowSteps.qll @@ -341,20 +341,4 @@ class ResourceBundleGetTextCallArgToReturnValueStep extends DataFlow::SharedFlow end = getTextCall ) } -} - -/** - * A step from any argument of a SAP logging function to the `onLogEntry` - * method of a custom log listener in the same application. - */ -class LogArgumentToListener extends DataFlow::SharedFlowStep { - override predicate step(DataFlow::Node start, DataFlow::Node end) { - inSameWebApp(start.getFile(), end.getFile()) and - start = - ModelOutput::getATypeNode("SapLogger") - .getMember(["debug", "error", "fatal", "info", "trace", "warning"]) - .getACall() - .getAnArgument() and - end = ModelOutput::getATypeNode("SapLogEntries").asSource() - } -} +} \ No newline at end of file From 909ce393dc426e4eeef33a8d46be7b9c78a21321 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 8 Sep 2025 10:51:37 -0400 Subject: [PATCH 18/18] Put back `LogArgumentToListener` to `FlowSteps.qll` and reference it in the query --- .../frameworks/ui5/UI5LogsToHttpQuery.qll | 19 +++++-------------- .../frameworks/ui5/dataflow/FlowSteps.qll | 18 +++++++++++++++++- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogsToHttpQuery.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogsToHttpQuery.qll index 25aa22fa4..8a6b6452c 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogsToHttpQuery.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5LogsToHttpQuery.qll @@ -29,20 +29,11 @@ module UI5LogEntryToHttp implements DataFlow::StateConfigSig { UI5LogInjection::isAdditionalFlowStep(start, end) and preState = postState or - /* - * Jump from any argument of a SAP logging function to the `onLogEntry` - * method of a custom log listener in the same application. - */ - - inSameWebApp(start.getFile(), end.getFile()) and - start = - ModelOutput::getATypeNode("SapLogger") - .getMember(["debug", "error", "fatal", "info", "trace", "warning"]) - .getACall() - .getAnArgument() and - end = ModelOutput::getATypeNode("SapLogEntries").asSource() and - preState = "not-logged-not-accessed" and - postState = "logged-and-accessed" + exists(LogArgumentToListener logArgumentToListener | + logArgumentToListener.step(start, end) and + preState = "not-logged-not-accessed" and + postState = "logged-and-accessed" + ) } predicate isSink(DataFlow::Node node, FlowState state) { diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/FlowSteps.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/FlowSteps.qll index 504d257e1..3fde64d21 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/FlowSteps.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/FlowSteps.qll @@ -341,4 +341,20 @@ class ResourceBundleGetTextCallArgToReturnValueStep extends DataFlow::SharedFlow end = getTextCall ) } -} \ No newline at end of file +} + +/** + * A step from any argument of a SAP logging function to the `onLogEntry` + * method of a custom log listener in the same application. + */ +class LogArgumentToListener extends DataFlow::SharedFlowStep { + override predicate step(DataFlow::Node start, DataFlow::Node end) { + inSameWebApp(start.getFile(), end.getFile()) and + start = + ModelOutput::getATypeNode("SapLogger") + .getMember(["debug", "error", "fatal", "info", "trace", "warning"]) + .getACall() + .getAnArgument() and + end = ModelOutput::getATypeNode("SapLogEntries").asSource() + } +}