From 02c8edadb775e1c4e2101eee64fb600db03cc61e Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Wed, 10 Dec 2025 16:09:28 -0500 Subject: [PATCH 01/15] Add small investigative modelling for fragments - WIP --- .../javascript/frameworks/ui5/UI5View.qll | 55 ++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll index b7b57c7dc..221cb9315 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll @@ -187,6 +187,17 @@ predicate isBuiltInControl(string qualifiedTypeUri) { ) } +/** + * A UI5 Fragment that might include XSS sources and sinks in standard controls. + */ +abstract class UI5Fragment extends File { + abstract UI5Control getControl(); + + abstract UI5BindingPath getASource(); + + abstract UI5BindingPath getAnHtmlISink(); +} + /** * A UI5 View that might include XSS sources and sinks in standard controls. */ @@ -683,8 +694,50 @@ class XmlView extends UI5View instanceof XmlFile { } } +/** + * TODO - consider - if this just copies all predicates - maybe this should be a subtype of XmlView + * and we dont need a separate/parallel type for fragments vs views. this will become clear once + */ +class XmlFragment extends UI5Fragment instanceof XmlFile { + XmlRootElement root; + + XmlFragment() { + root = this.getARootElement() and + ( + root.getNamespace().getUri() = "sap.m" + or + root.getNamespace().getUri() = "sap.ui.core" + ) and + root.hasName("FragmentDefinition") + } + + override UI5Control getControl() { + exists(XmlElement element | + result.asXmlControl() = element and + /* Use getAChild+ because some controls nest other controls inside them as aggregations */ + element = root.getAChild+() and + ( + /* 1. A builtin control provided by UI5 */ + isBuiltInControl(element.getNamespace().getUri()) + or + /* 2. A custom control with implementation code found in the webapp */ + exists(CustomControl control | + control.getName() = element.getNamespace().getUri() + "." + element.getName() and + inSameWebApp(control.getFile(), element.getFile()) + ) + ) + ) + } + + override XmlBindingPath getASource() { none() } + + override XmlBindingPath getAnHtmlISink() { none() } +} + private newtype TUI5Control = - TXmlControl(XmlElement control) or + TXmlControl(XmlElement control) { + control.getFile().getName().matches(["%.view.xml", "%.fragment.xml"]) + } or TJsonControl(JsonObject control) { exists(JsonView view | control.getParent() = view.getRoot().getPropValue("content")) } or From ad42e91becdacaf02074fed878bb48e37783fdb3 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Tue, 16 Dec 2025 10:53:34 -0500 Subject: [PATCH 02/15] Fix controls in xml detection --- .../javascript/frameworks/ui5/UI5View.qll | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll index 221cb9315..72e7d5b71 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll @@ -696,7 +696,7 @@ class XmlView extends UI5View instanceof XmlFile { /** * TODO - consider - if this just copies all predicates - maybe this should be a subtype of XmlView - * and we dont need a separate/parallel type for fragments vs views. this will become clear once + * and we dont need a separate/parallel type for fragments vs views. this will become clear once */ class XmlFragment extends UI5Fragment instanceof XmlFile { XmlRootElement root; @@ -736,7 +736,11 @@ class XmlFragment extends UI5Fragment instanceof XmlFile { private newtype TUI5Control = TXmlControl(XmlElement control) { - control.getFile().getName().matches(["%.view.xml", "%.fragment.xml"]) + control + .(Locatable) + .getFile() + .getBaseName() + .matches(["%.view.xml", "%.view.html", "%.fragment.xml"]) } or TJsonControl(JsonObject control) { exists(JsonView view | control.getParent() = view.getRoot().getPropValue("content")) From 8f807756049af96ac121fddd9ec1975095af4606 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Wed, 17 Dec 2025 12:21:59 -0500 Subject: [PATCH 03/15] Add simple testcase xss fragment in html view --- .../UI5Xss/xss-xml-fragment/UI5Xss.expected | 0 .../UI5Xss/xss-xml-fragment/UI5Xss.qlref | 1 + .../UI5Xss/xss-xml-fragment/package-lock.json | 12 +++++++++++ .../UI5Xss/xss-xml-fragment/package.json | 5 +++++ .../queries/UI5Xss/xss-xml-fragment/ui5.yaml | 7 +++++++ .../webapp/controller/app.controller.js | 21 +++++++++++++++++++ .../UI5Xss/xss-xml-fragment/webapp/index.html | 21 +++++++++++++++++++ .../UI5Xss/xss-xml-fragment/webapp/index.js | 11 ++++++++++ .../xss-xml-fragment/webapp/manifest.json | 5 +++++ .../webapp/view/TestFragment.fragment.xml | 9 ++++++++ .../webapp/view/app.view.html | 6 ++++++ 11 files changed, 98 insertions(+) create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/UI5Xss.expected create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/UI5Xss.qlref create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/package-lock.json create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/package.json create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/ui5.yaml create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/controller/app.controller.js create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/index.html create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/index.js create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/manifest.json create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/view/TestFragment.fragment.xml create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/view/app.view.html diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/UI5Xss.expected b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/UI5Xss.expected new file mode 100644 index 000000000..e69de29bb diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/UI5Xss.qlref b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/UI5Xss.qlref new file mode 100644 index 000000000..ce544f1d0 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/UI5Xss.qlref @@ -0,0 +1 @@ +UI5Xss/UI5Xss.ql \ No newline at end of file diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/package-lock.json b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/package-lock.json new file mode 100644 index 000000000..ba052860f --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/package-lock.json @@ -0,0 +1,12 @@ +{ + "name": "sap-ui5-xss", + "version": "1.0.0", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { + "name": "sap-ui5-xss", + "version": "1.0.0" + } + } +} diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/package.json b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/package.json new file mode 100644 index 000000000..9f0a4560b --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/package.json @@ -0,0 +1,5 @@ +{ + "name": "sap-ui5-xss", + "version": "1.0.0", + "main": "index.js" +} diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/ui5.yaml b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/ui5.yaml new file mode 100644 index 000000000..beb2ff69d --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/ui5.yaml @@ -0,0 +1,7 @@ +specVersion: '3.0' +metadata: + name: sap-ui5-xss +type: application +framework: + name: SAPUI5 + version: "1.115.0" diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/controller/app.controller.js b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/controller/app.controller.js new file mode 100644 index 000000000..db7b40f1f --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/controller/app.controller.js @@ -0,0 +1,21 @@ +sap.ui.define([ + "sap/ui/core/mvc/Controller", + "sap/ui/model/json/JSONModel" +], function (Controller, JSONModel) { + "use strict" + return Controller.extend("codeql-sap-js.controller.app", { + onInit: function () { + var oData = { + input: null + }; + var oModel = new JSONModel(oData); + this.getView().setModel(oModel); + + this.fragment = this.loadFragment({ + name: "codeql-sap-js.view.TestFragment" + }).then(function (oFragment) { + oFragment.placeAt("fragmentContainer"); + }.bind(this)); + } + }); +}) \ No newline at end of file diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/index.html b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/index.html new file mode 100644 index 000000000..0d9daa387 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/index.html @@ -0,0 +1,21 @@ + + + + + + + SAPUI5 XSS + + + + + + + + \ No newline at end of file diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/index.js b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/index.js new file mode 100644 index 000000000..b9774f759 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/index.js @@ -0,0 +1,11 @@ +sap.ui.define([ + "sap/ui/core/mvc/HTMLView" +], function (HTMLView) { + "use strict"; + HTMLView.create({ + viewName: "codeql-sap-js.view.app" + }).then(function (oView) { + oView.placeAt("content"); + }); + +}); \ No newline at end of file diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/manifest.json b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/manifest.json new file mode 100644 index 000000000..65d4de250 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/manifest.json @@ -0,0 +1,5 @@ +{ + "sap.app": { + "id": "sap-ui5-xss" + } +} \ No newline at end of file diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/view/TestFragment.fragment.xml b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/view/TestFragment.fragment.xml new file mode 100644 index 000000000..311fb7c6f --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/view/TestFragment.fragment.xml @@ -0,0 +1,9 @@ + + + + \ No newline at end of file diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/view/app.view.html b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/view/app.view.html new file mode 100644 index 000000000..c835b8ef4 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/view/app.view.html @@ -0,0 +1,6 @@ + \ No newline at end of file From b0c1aef223c78dba51797c4790590b92ee159b0b Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Tue, 30 Dec 2025 12:29:42 -0500 Subject: [PATCH 04/15] Add Fragment load model and test cases for basic xss --- .../javascript/frameworks/ui5/Fragment.qll | 51 +++++++++++++++ .../javascript/frameworks/ui5/UI5View.qll | 65 +++++++++++++++++-- .../xss-xml-fragment-load/UI5Xss.expected | 13 ++++ .../UI5Xss/xss-xml-fragment-load/UI5Xss.qlref | 1 + .../xss-xml-fragment-load/package-lock.json | 12 ++++ .../UI5Xss/xss-xml-fragment-load/package.json | 5 ++ .../UI5Xss/xss-xml-fragment-load/ui5.yaml | 7 ++ .../webapp/controller/app.controller.js | 25 +++++++ .../xss-xml-fragment-load/webapp/index.html | 21 ++++++ .../xss-xml-fragment-load/webapp/index.js | 11 ++++ .../webapp/manifest.json | 5 ++ .../webapp/view/TestFragment.fragment.xml | 9 +++ .../webapp/view/app.view.html | 6 ++ .../UI5Xss/xss-xml-fragment/UI5Xss.expected | 13 ++++ .../webapp/view/TestFragment.fragment.xml | 2 +- .../webapp/view/app.view.html | 4 +- 16 files changed, 241 insertions(+), 9 deletions(-) create mode 100644 javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/Fragment.qll create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/UI5Xss.expected create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/UI5Xss.qlref create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/package-lock.json create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/package.json create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/ui5.yaml create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/controller/app.controller.js create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/index.html create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/index.js create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/manifest.json create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/view/TestFragment.fragment.xml create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/view/app.view.html diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/Fragment.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/Fragment.qll new file mode 100644 index 000000000..86e657e4a --- /dev/null +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/Fragment.qll @@ -0,0 +1,51 @@ +/** + * Provides classes for modeling the sap.ui.core.Fragment API. + */ + +import javascript +import DataFlow +import advanced_security.javascript.frameworks.ui5.UI5 + +/** + * Gets a reference to the Fragment module import. + */ +class FragmentLoad extends InvokeNode, MethodCallNode { + FragmentLoad() { + this = + TypeTrackers::hasDependency(["sap/ui/core/Fragment", "sap.ui.core.Fragment"]) + .getAMemberCall("load") + or + exists(RequiredObject requiredModule, SapDefineModule sapModule | + this = requiredModule.asSourceNode().getAMemberCall("load") and + //ensure it is an sap module define + requiredModule.getEnclosingFunction() = sapModule.getArgument(1) + ) + } + + /** + * Gets the configuration object passed to Fragment.load(). + */ + DataFlow::Node getConfigObject() { result = this.getArgument(0) } + + /** + * Gets the 'name' property value from the config object. + * This specifies the fragment resource to load (e.g., "my.app.fragments.MyFragment"). + */ + DataFlow::Node getNameArgument() { + exists(DataFlow::ObjectLiteralNode config | + config = this.getConfigObject().getALocalSource() and + result = config.getAPropertyWrite("name").getRhs() + ) + } + + /** + * Gets the 'controller' property value from the config object. + * This specifies the fragment's controller, if it has one. + */ + DataFlow::Node getControllerArgument() { + exists(DataFlow::ObjectLiteralNode config | + config = this.getConfigObject().getALocalSource() and + result = config.getAPropertyWrite("controller").getRhs() + ) + } +} diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll index 72e7d5b71..3abf3db77 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll @@ -4,6 +4,7 @@ import advanced_security.javascript.frameworks.ui5.UI5 import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow private import semmle.javascript.frameworks.data.internal.ApiGraphModelsExtensions as ApiGraphModelsExtensions import advanced_security.javascript.frameworks.ui5.Bindings +import advanced_security.javascript.frameworks.ui5.Fragment /** * Gets the immediate supertype of a given type from the extensible predicate `typeModel` provided by @@ -695,10 +696,9 @@ class XmlView extends UI5View instanceof XmlFile { } /** - * TODO - consider - if this just copies all predicates - maybe this should be a subtype of XmlView - * and we dont need a separate/parallel type for fragments vs views. this will become clear once + * An xml fragment. It may or may not have controllers associated. */ -class XmlFragment extends UI5Fragment instanceof XmlFile { +class XmlFragment extends UI5View instanceof XmlFile { XmlRootElement root; XmlFragment() { @@ -711,6 +711,35 @@ class XmlFragment extends UI5Fragment instanceof XmlFile { root.hasName("FragmentDefinition") } + override XmlBindingPath getASource() { + exists(XmlElement control, string type, string path, string property | + type = result.getControlTypeName() and + this = control.getFile() and + ApiGraphModelsExtensions::sourceModel(getASuperType(type), path, "remote", _) and + property = path.replaceAll(" ", "").regexpCapture("Member\\[([^\\]]+)\\]", 1) and + result.getBindingTarget() = control.getAttribute(property) + ) + } + + override XmlBindingPath getAnHtmlISink() { + exists(XmlElement control, string type, string path, string property | + this = control.getFile() and + type = result.getControlTypeName() and + ApiGraphModelsExtensions::sinkModel(getASuperType(type), path, "ui5-html-injection", _) and + property = path.replaceAll(" ", "").regexpCapture("Member\\[([^\\]]+)\\]", 1) and + result.getBindingTarget() = control.getAttribute(property) and + /* If the control is an `sap.ui.core.HTML` then the control should be missing the `sanitizeContent` attribute */ + ( + getASuperType(type) = "HTMLControl" + implies + ( + not exists(control.getAttribute("sanitizeContent")) or + control.getAttribute("sanitizeContent").getValue() = "false" + ) + ) + ) + } + override UI5Control getControl() { exists(XmlElement element | result.asXmlControl() = element and @@ -729,9 +758,33 @@ class XmlFragment extends UI5Fragment instanceof XmlFile { ) } - override XmlBindingPath getASource() { none() } - - override XmlBindingPath getAnHtmlISink() { none() } + /** + * This is either known in the location from which `loadFragment` is called (in a controller's init function) + * OR in the optional controller param of `Fragment.load`. + * This MAY return no value, when the fragment is not associated to any controller. + * When this returns a value it is guaranteed that this xml fragment is instantiated. + */ + override string getControllerName() { + exists(CustomController controller, MethodCallNode loadFragmentCall | + loadFragmentCall.getMethodName() = "loadFragment" and + controller.getAThisNode().flowsTo(loadFragmentCall.getReceiver()) and + controller.getName() = result + ) + or + exists(CustomController controller, FragmentLoad fragmentLoad | + controller.getAThisNode().flowsTo(fragmentLoad.getControllerArgument()) and + /* + * extracting just the base name of the fragment (not the fully qualified) + * otherwise difficult to know which part of absolute path is only for the qualified name + */ + + fragmentLoad + .getNameArgument() + .getStringValue() + .matches("%" + this.getBaseName().replaceAll(".fragment.xml", "")) and + controller.getName() = result + ) + } } private newtype TUI5Control = diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/UI5Xss.expected b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/UI5Xss.expected new file mode 100644 index 000000000..3e379748b --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/UI5Xss.expected @@ -0,0 +1,13 @@ +nodes +| webapp/controller/app.controller.js:10:17:10:27 | input: null | +| webapp/view/TestFragment.fragment.xml:5:1:7:28 | value={/input} | +| webapp/view/app.view.html:4:11:4:33 | data-content={/input} | +edges +| webapp/controller/app.controller.js:10:17:10:27 | input: null | webapp/view/TestFragment.fragment.xml:5:1:7:28 | value={/input} | +| webapp/controller/app.controller.js:10:17:10:27 | input: null | webapp/view/app.view.html:4:11:4:33 | data-content={/input} | +| webapp/controller/app.controller.js:12:26:12:45 | new JSONModel(oData) | webapp/view/app.view.html:4:11:4:33 | data-content={/input} | +| webapp/view/TestFragment.fragment.xml:5:1:7:28 | value={/input} | webapp/controller/app.controller.js:10:17:10:27 | input: null | +| webapp/view/TestFragment.fragment.xml:5:1:7:28 | value={/input} | webapp/controller/app.controller.js:12:26:12:45 | new JSONModel(oData) | +| webapp/view/app.view.html:4:11:4:33 | data-content={/input} | webapp/controller/app.controller.js:10:17:10:27 | input: null | +#select +| webapp/view/app.view.html:4:11:4:33 | data-content={/input} | webapp/view/TestFragment.fragment.xml:5:1:7:28 | value={/input} | webapp/view/app.view.html:4:11:4:33 | data-content={/input} | XSS vulnerability due to $@. | webapp/view/TestFragment.fragment.xml:5:1:7:28 | value={/input} | user-provided value | diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/UI5Xss.qlref b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/UI5Xss.qlref new file mode 100644 index 000000000..ce544f1d0 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/UI5Xss.qlref @@ -0,0 +1 @@ +UI5Xss/UI5Xss.ql \ No newline at end of file diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/package-lock.json b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/package-lock.json new file mode 100644 index 000000000..ba052860f --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/package-lock.json @@ -0,0 +1,12 @@ +{ + "name": "sap-ui5-xss", + "version": "1.0.0", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { + "name": "sap-ui5-xss", + "version": "1.0.0" + } + } +} diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/package.json b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/package.json new file mode 100644 index 000000000..9f0a4560b --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/package.json @@ -0,0 +1,5 @@ +{ + "name": "sap-ui5-xss", + "version": "1.0.0", + "main": "index.js" +} diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/ui5.yaml b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/ui5.yaml new file mode 100644 index 000000000..beb2ff69d --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/ui5.yaml @@ -0,0 +1,7 @@ +specVersion: '3.0' +metadata: + name: sap-ui5-xss +type: application +framework: + name: SAPUI5 + version: "1.115.0" diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/controller/app.controller.js b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/controller/app.controller.js new file mode 100644 index 000000000..5c5ca3d72 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/controller/app.controller.js @@ -0,0 +1,25 @@ +sap.ui.define([ + "sap/ui/core/mvc/Controller", + "sap/ui/model/json/JSONModel", + "sap/ui/core/Fragment" +], function (Controller, JSONModel, Fragment) { + "use strict" + return Controller.extend("codeql-sap-js.controller.app", { + onInit: function () { + var oData = { + input: null + }; + var oModel = new JSONModel(oData); + this.getView().setModel(oModel); + + Fragment.load({ + name: "codeql-sap-js.view.TestFragment", + controller: this, + id: this.getView().getId() + }).then(function (oFragment) { + this.getView().addDependent(oFragment); + oFragment.placeAt("fragmentContainer"); + }.bind(this)); + } + }); +}) \ No newline at end of file diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/index.html b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/index.html new file mode 100644 index 000000000..0d9daa387 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/index.html @@ -0,0 +1,21 @@ + + + + + + + SAPUI5 XSS + + + + + + + + \ No newline at end of file diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/index.js b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/index.js new file mode 100644 index 000000000..b9774f759 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/index.js @@ -0,0 +1,11 @@ +sap.ui.define([ + "sap/ui/core/mvc/HTMLView" +], function (HTMLView) { + "use strict"; + HTMLView.create({ + viewName: "codeql-sap-js.view.app" + }).then(function (oView) { + oView.placeAt("content"); + }); + +}); \ No newline at end of file diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/manifest.json b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/manifest.json new file mode 100644 index 000000000..65d4de250 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/manifest.json @@ -0,0 +1,5 @@ +{ + "sap.app": { + "id": "sap-ui5-xss" + } +} \ No newline at end of file diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/view/TestFragment.fragment.xml b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/view/TestFragment.fragment.xml new file mode 100644 index 000000000..7c84f45e2 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/view/TestFragment.fragment.xml @@ -0,0 +1,9 @@ + + + + \ No newline at end of file diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/view/app.view.html b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/view/app.view.html new file mode 100644 index 000000000..f23bf693f --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment-load/webapp/view/app.view.html @@ -0,0 +1,6 @@ + \ No newline at end of file diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/UI5Xss.expected b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/UI5Xss.expected index e69de29bb..de5a04972 100644 --- a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/UI5Xss.expected +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/UI5Xss.expected @@ -0,0 +1,13 @@ +nodes +| webapp/controller/app.controller.js:9:17:9:27 | input: null | +| webapp/view/TestFragment.fragment.xml:5:1:7:28 | value={/input} | +| webapp/view/app.view.html:4:11:4:33 | data-content={/input} | +edges +| webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/view/TestFragment.fragment.xml:5:1:7:28 | value={/input} | +| webapp/controller/app.controller.js:9:17:9:27 | input: null | webapp/view/app.view.html:4:11:4:33 | data-content={/input} | +| webapp/controller/app.controller.js:11:26:11:45 | new JSONModel(oData) | webapp/view/app.view.html:4:11:4:33 | data-content={/input} | +| webapp/view/TestFragment.fragment.xml:5:1:7:28 | value={/input} | webapp/controller/app.controller.js:9:17:9:27 | input: null | +| webapp/view/TestFragment.fragment.xml:5:1:7:28 | value={/input} | webapp/controller/app.controller.js:11:26:11:45 | new JSONModel(oData) | +| webapp/view/app.view.html:4:11:4:33 | data-content={/input} | webapp/controller/app.controller.js:9:17:9:27 | input: null | +#select +| webapp/view/app.view.html:4:11:4:33 | data-content={/input} | webapp/view/TestFragment.fragment.xml:5:1:7:28 | value={/input} | webapp/view/app.view.html:4:11:4:33 | data-content={/input} | XSS vulnerability due to $@. | webapp/view/TestFragment.fragment.xml:5:1:7:28 | value={/input} | user-provided value | diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/view/TestFragment.fragment.xml b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/view/TestFragment.fragment.xml index 311fb7c6f..7c84f45e2 100644 --- a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/view/TestFragment.fragment.xml +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/view/TestFragment.fragment.xml @@ -4,6 +4,6 @@ > + value="{/input}"> \ No newline at end of file diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/view/app.view.html b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/view/app.view.html index c835b8ef4..f23bf693f 100644 --- a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/view/app.view.html +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-xml-fragment/webapp/view/app.view.html @@ -1,6 +1,6 @@ \ No newline at end of file From 530df03d696292896faaf2ff9c332c63b2959116 Mon Sep 17 00:00:00 2001 From: Nathan Randall Date: Tue, 30 Dec 2025 17:16:01 -0700 Subject: [PATCH 05/15] Improve UI5Xss detection for fragments_samples 1 --- javascript/frameworks/ui5/ext/ui5.model.yml | 4 +++ .../javascript/frameworks/ui5/UI5.qll | 33 +++++++++++++++---- .../javascript/frameworks/ui5/UI5View.qll | 16 +++++++-- .../xss-fragment-static-byid/UI5Xss.expected | 12 +++++++ .../xss-fragment-static-byid/UI5Xss.qlref | 1 + .../xss-fragment-static-byid/package.json | 5 +++ .../UI5Xss/xss-fragment-static-byid/ui5.yaml | 7 ++++ .../webapp/controller/Main.controller.js | 27 +++++++++++++++ .../webapp/index.html | 16 +++++++++ .../xss-fragment-static-byid/webapp/index.js | 10 ++++++ .../webapp/manifest.json | 5 +++ .../webapp/view/Main.view.html | 6 ++++ .../webapp/view/PayloadForm.fragment.xml | 11 +++++++ 13 files changed, 145 insertions(+), 8 deletions(-) create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/UI5Xss.expected create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/UI5Xss.qlref create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/package.json create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/ui5.yaml create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/controller/Main.controller.js create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/index.html create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/index.js create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/manifest.json create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/view/Main.view.html create mode 100644 javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/view/PayloadForm.fragment.xml diff --git a/javascript/frameworks/ui5/ext/ui5.model.yml b/javascript/frameworks/ui5/ext/ui5.model.yml index 19964a4a9..7f6802763 100644 --- a/javascript/frameworks/ui5/ext/ui5.model.yml +++ b/javascript/frameworks/ui5/ext/ui5.model.yml @@ -71,6 +71,10 @@ extensions: - ["UI5ClientStorage", "global", "Member[jQuery].Member[sap].Member[storage]"] - ["UI5ClientStorage", "sap/ui/core/util/File", ""] - ["UI5ClientStorage", "global", "Member[sap].Member[ui].Member[core].Member[util].Member[File]"] + # Fragment API - byId returns a Control (models static Fragment.byId() calls) + - ["Fragment", "Fragment", "Instance"] + - ["Fragment", "sap/ui/core/Fragment", ""] + - ["UI5InputControl", "Fragment", "Member[byId].ReturnValue"] - addsTo: pack: codeql/javascript-all 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 9a110016c..08148c7b8 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 @@ -141,6 +141,16 @@ class SapUiCore extends MethodCallNode { SapUiCore() { this = globalVarRef("sap").getAPropertyRead("ui").getAMethodCall("getCore") } } +/** + * A reference to the Fragment module (`sap/ui/core/Fragment`). + * Used for static methods like `Fragment.byId(viewId, controlId)`. + */ +class FragmentModule extends DataFlow::SourceNode { + FragmentModule() { + this = DataFlow::moduleImport("sap/ui/core/Fragment") + } +} + /** * A user-defined module through `sap.ui.define` or `jQuery.sap.declare`. */ @@ -334,14 +344,25 @@ class ControlReference extends Reference { string controlId; ControlReference() { - this.getArgument(0).getALocalSource().getStringValue() = controlId and ( - exists(CustomController controller | - this = controller.getAViewReference().getAMemberCall("byId") or - this = controller.getAThisNode().getAMemberCall("byId") + // Standard byId patterns: this.byId("id"), this.getView().byId("id"), sap.ui.getCore().byId("id") + this.getArgument(0).getALocalSource().getStringValue() = controlId and + ( + exists(CustomController controller | + this = controller.getAViewReference().getAMemberCall("byId") or + this = controller.getAThisNode().getAMemberCall("byId") + ) + or + exists(SapUiCore sapUiCore | this = sapUiCore.getAMemberCall("byId")) ) - or - exists(SapUiCore sapUiCore | this = sapUiCore.getAMemberCall("byId")) + ) + or + // Fragment.byId(viewId, controlId) - static method with 2 arguments + ( + this.getNumArgument() = 2 and + this.getArgument(1).getALocalSource().getStringValue() = controlId and + this.getMethodName() = "byId" and + exists(FragmentModule fragment | this = fragment.getAMemberCall("byId")) ) } diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll index 3abf3db77..483a30181 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll @@ -890,11 +890,23 @@ class UI5Control extends TUI5Control { /** * Gets a reference to this control. Currently supports only such references made through `byId`. + * Handles both: + * - `this.byId("controlId")` or `this.getView().byId("controlId")` - ID in argument 0 + * - `Fragment.byId(viewId, "controlId")` - ID in argument 1 */ ControlReference getAReference() { result.getMethodName() = "byId" and - result.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() = - this.getProperty("id").getValue() + ( + // Standard byId: ID in first argument + result.getNumArgument() = 1 and + result.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() = + this.getProperty("id").getValue() + or + // Fragment.byId: ID in second argument + result.getNumArgument() = 2 and + result.getArgument(1).getALocalSource().asExpr().(StringLiteral).getValue() = + this.getProperty("id").getValue() + ) } /** Gets a property of this control having the name. */ diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/UI5Xss.expected b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/UI5Xss.expected new file mode 100644 index 000000000..bf9485fe6 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/UI5Xss.expected @@ -0,0 +1,12 @@ +nodes +| webapp/controller/Main.controller.js:20:17:20:29 | sAttackerData | +| webapp/controller/Main.controller.js:20:33:20:97 | Fragmen ... Value() | +| webapp/controller/Main.controller.js:24:34:24:69 | " ... /span>" | +| webapp/controller/Main.controller.js:24:45:24:57 | sAttackerData | +| webapp/view/Main.view.html:4:10:4:34 | data-content={/payload} | +edges +| webapp/controller/Main.controller.js:20:17:20:29 | sAttackerData | webapp/controller/Main.controller.js:24:45:24:57 | sAttackerData | +| webapp/controller/Main.controller.js:20:33:20:97 | Fragmen ... Value() | webapp/controller/Main.controller.js:20:17:20:29 | sAttackerData | +| webapp/controller/Main.controller.js:24:45:24:57 | sAttackerData | webapp/controller/Main.controller.js:24:34:24:69 | " ... /span>" | +#select +| webapp/controller/Main.controller.js:24:34:24:69 | " ... /span>" | webapp/controller/Main.controller.js:20:33:20:97 | Fragmen ... Value() | webapp/controller/Main.controller.js:24:34:24:69 | " ... /span>" | XSS vulnerability due to $@. | webapp/controller/Main.controller.js:20:33:20:97 | Fragmen ... Value() | user-provided value | diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/UI5Xss.qlref b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/UI5Xss.qlref new file mode 100644 index 000000000..ce544f1d0 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/UI5Xss.qlref @@ -0,0 +1 @@ +UI5Xss/UI5Xss.ql \ No newline at end of file diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/package.json b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/package.json new file mode 100644 index 000000000..f07d7baec --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/package.json @@ -0,0 +1,5 @@ +{ + "name": "ui5-xss-fragment-static-byid", + "version": "1.0.0", + "main": "index.js" +} diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/ui5.yaml b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/ui5.yaml new file mode 100644 index 000000000..2c7a22f1e --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/ui5.yaml @@ -0,0 +1,7 @@ +specVersion: '3.0' +metadata: + name: ui5-xss-fragment-static-byid +type: application +framework: + name: SAPUI5 + version: "1.115.0" diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/controller/Main.controller.js b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/controller/Main.controller.js new file mode 100644 index 000000000..fae5eeddd --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/controller/Main.controller.js @@ -0,0 +1,27 @@ +sap.ui.define([ + "sap/ui/core/mvc/Controller", + "sap/ui/core/Fragment" +], function (Controller, Fragment) { + "use strict"; + return Controller.extend("ui5-xss-fragment-static-byid.controller.Main", { + onInit: function () { + // Load fragment with view ID prefix + Fragment.load({ + id: this.getView().getId(), + name: "ui5-xss-fragment-static-byid.view.PayloadForm", + controller: this + }).then(function (oFragment) { + this.byId("fragmentArea").addContent(oFragment); + }.bind(this)); + }, + + onSubmitPayload: function () { + // XSS vulnerability: Fragment.byId() static method to access fragment controls + var sAttackerData = Fragment.byId(this.getView().getId(), "attackerInput").getValue(); + var oHtmlSink = Fragment.byId(this.getView().getId(), "vulnerableOutput"); + + // Sink: setContent with unsanitized user input + oHtmlSink.setContent("" + sAttackerData + ""); + } + }); +}); diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/index.html b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/index.html new file mode 100644 index 000000000..4e9d8e5ea --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/index.html @@ -0,0 +1,16 @@ + + + + + UI5 XSS Fragment Static ById Test + + + + + diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/index.js b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/index.js new file mode 100644 index 000000000..da6657f38 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/index.js @@ -0,0 +1,10 @@ +sap.ui.define([ + "sap/ui/core/mvc/HTMLView" +], function (HTMLView) { + "use strict"; + HTMLView.create({ + viewName: "ui5-xss-fragment-static-byid.view.Main" + }).then(function (oView) { + oView.placeAt("content"); + }); +}); diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/manifest.json b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/manifest.json new file mode 100644 index 000000000..512c47013 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/manifest.json @@ -0,0 +1,5 @@ +{ + "sap.app": { + "id": "ui5-xss-fragment-static-byid" + } +} diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/view/Main.view.html b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/view/Main.view.html new file mode 100644 index 000000000..1ae6a5c77 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/view/Main.view.html @@ -0,0 +1,6 @@ + diff --git a/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/view/PayloadForm.fragment.xml b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/view/PayloadForm.fragment.xml new file mode 100644 index 000000000..8d1866b16 --- /dev/null +++ b/javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-static-byid/webapp/view/PayloadForm.fragment.xml @@ -0,0 +1,11 @@ + + + +