diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript_sap_cap_all/Customizations.qll b/javascript/frameworks/cap/lib/advanced_security/javascript_sap_cap_all/Customizations.qll index 81f31da81..ef0de740f 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript_sap_cap_all/Customizations.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript_sap_cap_all/Customizations.qll @@ -1,6 +1,8 @@ -// This file is included for use in custom CodeQL bundles (https://github.com/advanced-security/codeql-bundle). -// The contents of this file will be included in the standard library `Customizations.qll`, and will therefore -// be included in the out-of-the-box security queries. -// -// We import under alias to avoid any potential naming conflicts +/** + * This file is included for use in custom CodeQL bundles (https://github.com/advanced-security/codeql-bundle). + * The contents of this file will be included in the standard library `Customizations.qll`, and will therefore + * be included in the out-of-the-box security queries. + */ + +/* We import under alias to avoid any potential naming conflicts */ import advanced_security.javascript.frameworks.cap.RemoteFlowSources as CAPRemoteFlowSources diff --git a/javascript/frameworks/ui5/ext/ui5.model.yml b/javascript/frameworks/ui5/ext/ui5.model.yml index 11da5231b..0f34f1aaf 100644 --- a/javascript/frameworks/ui5/ext/ui5.model.yml +++ b/javascript/frameworks/ui5/ext/ui5.model.yml @@ -15,9 +15,9 @@ extensions: - ["RenderManager", "Renderer", "Member[render].Parameter[0]"] - ["Patcher", "Patcher", "Instance"] - ["Patcher", "sap/ui/core/Patcher", ""] - - ["HTMLControl", "HTMLControl", "Instance"] - - ["HTMLControl", "sap/ui/core/HTML", ""] - - ["HTMLControl", "global", "Member[sap].Member[ui].Member[core].Member[HTML]"] + - ["UI5HTMLControl", "UI5HTMLControl", "Instance"] + - ["UI5HTMLControl", "sap/ui/core/HTML", ""] + - ["UI5HTMLControl", "global", "Member[sap].Member[ui].Member[core].Member[HTML]"] - ["Assert", "global", "Member[sap].Member[base].Member[assert]"] - ["Assert", "global", "Member[jQuery].Member[sap].Member[assert]"] - ["SapLogger", "sap/base/Log", ""] @@ -41,27 +41,27 @@ extensions: - ["SapUIDom", "sap/ui/dom", ""] - ["SapUIDom", "global", "Member[sap].Member[ui].Member[dom]"] # model Controls inheritance - - ["UI5Control", "UI5Control", "Instance"] - - ["UI5Control", "sap/m/InputBase", ""] - - ["UI5Control", "sap/m/Input", ""] - - ["UI5Control", "sap/ui/webc/main/Input", ""] - - ["UI5Control", "sap/ui/commons/TextField", ""] - - ["UI5Control", "sap/ui/commons/PasswordField", ""] - - ["UI5Control", "sap/ui/commons/ValueHelpField", ""] - - ["UI5Control", "sap/ui/commons/SearchField", ""] - - ["UI5Control", "sap/ui/commons/ComboBox", ""] - - ["UI5Control", "sap/ui/commons/TextArea", ""] - - ["UI5Control", "sap/ui/webc/main/TextArea", ""] - - ["UI5Control", "sap/m/ComboBoxTextField", ""] - - ["UI5Control", "sap/m/MaskEnabler", ""] - - ["UI5Control", "sap/m/MaskInput", ""] - - ["UI5Control", "sap/m/TextArea", ""] - - ["UI5Control", "sap/m/ComboBoxBase", ""] - - ["UI5Control", "sap/m/MultiInput", ""] - - ["UI5Control", "sap/ui/webc/main/MultiInput", ""] - - ["UI5Control", "sap/m/SearchField", ""] - - ["UI5Control", "sap/m/FeedInput", ""] - - ["UI5Control", "sap/ui/richtexteditor/RichTextEditor", ""] + - ["UI5InputControl", "UI5InputControl", "Instance"] + - ["UI5InputControl", "sap/m/InputBase", ""] + - ["UI5InputControl", "sap/m/Input", ""] + - ["UI5InputControl", "sap/ui/webc/main/Input", ""] + - ["UI5InputControl", "sap/ui/commons/TextField", ""] + - ["UI5InputControl", "sap/ui/commons/PasswordField", ""] + - ["UI5InputControl", "sap/ui/commons/ValueHelpField", ""] + - ["UI5InputControl", "sap/ui/commons/SearchField", ""] + - ["UI5InputControl", "sap/ui/commons/ComboBox", ""] + - ["UI5InputControl", "sap/ui/commons/TextArea", ""] + - ["UI5InputControl", "sap/ui/webc/main/TextArea", ""] + - ["UI5InputControl", "sap/m/ComboBoxTextField", ""] + - ["UI5InputControl", "sap/m/MaskEnabler", ""] + - ["UI5InputControl", "sap/m/MaskInput", ""] + - ["UI5InputControl", "sap/m/TextArea", ""] + - ["UI5InputControl", "sap/m/ComboBoxBase", ""] + - ["UI5InputControl", "sap/m/MultiInput", ""] + - ["UI5InputControl", "sap/ui/webc/main/MultiInput", ""] + - ["UI5InputControl", "sap/m/SearchField", ""] + - ["UI5InputControl", "sap/m/FeedInput", ""] + - ["UI5InputControl", "sap/ui/richtexteditor/RichTextEditor", ""] - ["UI5CodeEditor", "UI5CodeEditor", "Instance"] - ["UI5CodeEditor", "sap/ui/codeeditor/CodeEditor", ""] # Classes that provide Path injection APIs @@ -75,8 +75,8 @@ extensions: pack: codeql/javascript-all extensible: "sourceModel" data: - - ["UI5Control", "Member[value]", "remote"] - - ["UI5Control", "Member[getValue].ReturnValue", "remote"] + - ["UI5InputControl", "Member[value]", "remote"] + - ["UI5InputControl", "Member[getValue].ReturnValue", "remote"] - ["UI5CodeEditor", "Member[value]", "remote"] - ["UI5CodeEditor", "Member[getCurrentValue].ReturnValue", "remote"] - ["global", "Member[jQuery].Member[sap].Member[syncHead,syncGet,syncGetText,syncPost,syncPostText].ReturnValue", "remote"] @@ -89,9 +89,9 @@ extensions: data: # html-injection - ["global", "Member[jQuery].Member[sap].Member[globalEval].Argument[0]", "ui5-html-injection"] - - ["HTMLControl", "Argument[0].Member[content]", "ui5-html-injection"] - - ["HTMLControl", "Member[content]", "ui5-html-injection"] - - ["HTMLControl", "Member[setContent].Argument[0]", "ui5-html-injection"] + - ["UI5HTMLControl", "Argument[0].Member[content]", "ui5-html-injection"] + - ["UI5HTMLControl", "Member[content]", "ui5-html-injection"] + - ["UI5HTMLControl", "Member[setContent].Argument[0]", "ui5-html-injection"] - ["Patcher", "Member[unsafeHtml].Argument[0..]", "ui5-html-injection"] - ["RenderManager", "Member[write,unsafeHtml].Argument[0..]", "ui5-html-injection"] - ["RenderManager", "Member[writeAttribute,addStyle].Argument[0..1]", "ui5-html-injection"] @@ -137,3 +137,5 @@ extensions: - ["sap/base/security/encodeURL", "", "Argument[0]", "ReturnValue", "taint"] - ["sap/base/security/encodeURLParameters", "", "Argument[0]", "ReturnValue", "taint"] - ["sap/base/security/encodeXML", "", "Argument[0]", "ReturnValue", "taint"] + - ["UI5HTMLControl", "", "Argument[0].Member[content]", "ReturnValue.Member[content]", "taint"] + - ["UI5HTMLControl", "Instance.Member[getContent]", "Argument[this].Member[content]", "ReturnValue", "taint"] diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/RemoteFlowSources.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/RemoteFlowSources.qll index a367d919a..b12dcf430 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/RemoteFlowSources.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/RemoteFlowSources.qll @@ -1,22 +1,54 @@ import javascript import advanced_security.javascript.frameworks.ui5.UI5 import advanced_security.javascript.frameworks.ui5.UI5View -private import semmle.javascript.frameworks.data.internal.ApiGraphModelsExtensions as ApiGraphModelsExtensions +import semmle.javascript.security.dataflow.XssThroughDomCustomizations +private import semmle.javascript.frameworks.data.internal.ApiGraphModelsExtensions -private class DataFromRemoteControlReference extends RemoteFlowSource, MethodCallNode { +private class DataFromRemoteControlReference extends RemoteFlowSource { DataFromRemoteControlReference() { exists(UI5Control sourceControl, string typeAlias, ControlReference controlReference | - ApiGraphModelsExtensions::typeModel(typeAlias, sourceControl.getImportPath(), _) and - ApiGraphModelsExtensions::sourceModel(typeAlias, _, "remote", _) and + typeModel(typeAlias, sourceControl.getImportPath(), _) and + sourceModel(typeAlias, _, "remote", _) and sourceControl.getAReference() = controlReference and - controlReference.flowsTo(this.getReceiver()) and - this.getMethodName() = "getValue" + ( + this = controlReference.getAMemberCall("getValue") or + this = controlReference.getAPropertyRead("value") + ) ) } override string getSourceType() { result = "Data from a remote control" } } +private class InputControlInstantiation extends ElementInstantiation { + InputControlInstantiation() { typeModel("UI5InputControl", this.getImportPath(), _) } +} + +private module TrackPlaceAtCallConfigFlow = TaintTracking::Global; + +class DataFromInstantiatedAndPlacedAtControl extends RemoteFlowSource, XssThroughDom::Source { + InputControlInstantiation controlInstantiation; + ControlPlaceAtCall placeAtCall; + + DataFromInstantiatedAndPlacedAtControl() { + exists(string typeAlias, ControlReference controlReference | + /* Double check that the type derives a remote flow source. */ + typeModel(typeAlias, controlInstantiation.getImportPath(), _) and + sourceModel(typeAlias, _, "remote", _) and + controlInstantiation.getId() = controlReference.getId() and + ( + this = controlReference.getAMemberCall("getValue") or + this = controlReference.getAPropertyRead("value") + ) + ) and + TrackPlaceAtCallConfigFlow::flow(controlInstantiation, placeAtCall) + } + + override string getSourceType() { + result = "Data from an instantiated control placed in a DOM tree" + } +} + class LocalModelContentBoundBidirectionallyToSourceControl extends RemoteFlowSource { UI5BindingPath bindingPath; UI5Control controlDeclaration; @@ -60,18 +92,22 @@ class ODataServiceModel extends UI5ExternalModel { ODataServiceModel() { exists(MethodCallNode setModelCall, CustomController controller | /* - * 1. This flows from a DF node corresponding to the parent component's model to the `this.setModel` call - * i.e. Aims to capture something like `this.getOwnerComponent().getModel("someModelName")` as in - * `this.getView().setModel(this.getOwnerComponent().getModel("someModelName"))` + * 1. This flows from a DF node corresponding to the parent component's model + * to the `this.setModel` call. e.g. + * + * `this.getOwnerComponent().getModel("someModelName")` as in + * `this.getView().setModel(this.getOwnerComponent().getModel("someModelName"))`. */ modelName = this.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() and this.getCalleeName() = "getModel" and controller.getOwnerComponentRef().flowsTo(this.(MethodCallNode).getReceiver()) and this.flowsTo(setModelCall.getArgument(0)) and - setModelCall.getMethodName() = "setModel" and - setModelCall.getReceiver() = controller.getAViewReference() and - /* 2. The component's manifest.json declares the DataSource as being of OData type */ + setModelCall = controller.getAViewReference().getAMemberCall("setModel") and + /* + * 2. The component's `manifest.json` declares the DataSource as being of OData type. + */ + controller.getOwnerComponent().getExternalModelDef(modelName).getDataSource() instanceof ODataDataSourceManifest ) @@ -101,21 +137,17 @@ private class RouteParameterAccess extends RemoteFlowSource instanceof PropRead override string getSourceType() { result = "RouteParameterAccess" } RouteParameterAccess() { - exists( - ControllerHandler handler, RouteManifest routeManifest, ParameterNode handlerParameter, - MethodCallNode getParameterCall - | + exists(ControllerHandler handler, RouteManifest routeManifest, MethodCallNode getParameterCall | handler.isAttachedToRoute(routeManifest.getName()) and this.asExpr().getEnclosingFunction() = handler.getFunction() and - handlerParameter = handler.getParameter(0) and - getParameterCall.getMethodName() = "getParameter" and - getParameterCall.getReceiver().getALocalSource() = handlerParameter and + getParameterCall = handler.getParameter(0).getAMemberCall("getParameter") and ( - routeManifest.matchesPathString(this.getPropertyName()) and - this.getBase().getALocalSource() = getParameterCall + exists(string path | + this = getParameterCall.getAPropertyRead(path) and + routeManifest.matchesPathString(path) + ) or - /* TODO: Why does `routeManifest.matchesPathString` not work for propertyName?? */ - this.getBase().(PropRead).getBase().getALocalSource() = getParameterCall + this = getParameterCall.getAPropertyRead().getAPropertyRead() ) ) } @@ -125,10 +157,8 @@ private class DisplayEventHandlerParameterAccess extends RemoteFlowSource instan override string getSourceType() { result = "DisplayEventHandlerParameterAccess" } DisplayEventHandlerParameterAccess() { - exists(DisplayEventHandler handler, MethodCallNode getParameterCall | - getParameterCall.getMethodName() = "getParameter" and - this.getBase().getALocalSource() = getParameterCall and - handler.getParameter(0) = getParameterCall.getReceiver().getALocalSource() + exists(DisplayEventHandler handler | + this = handler.getParameter(0).getAMemberCall("getParameter").getAPropertyRead() ) } } 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 be274c2e7..9a110016c 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 @@ -5,6 +5,7 @@ import semmle.javascript.security.dataflow.DomBasedXssCustomizations import advanced_security.javascript.frameworks.ui5.UI5View import advanced_security.javascript.frameworks.ui5.UI5HTML import codeql.util.FileSystem +private import semmle.javascript.frameworks.data.internal.ApiGraphModelsExtensions as ApiGraphModelsExtensions private module WebAppResourceRootJsonReader implements JsonParser::MakeJsonReaderSig { class JsonReader extends WebApp { @@ -133,10 +134,11 @@ class WebApp extends HTML::HtmlFile { } /** - * https://sapui5.hana.ondemand.com/sdk/#/api/sap.ui.loader%23methods/sap.ui.loader.config + * The global instance of `sap.ui.Core` that represents this UI5 application, as retrieved + * by a call to `sap.ui.getCore()`. */ -class Loader extends CallNode { - Loader() { this = globalVarRef("sap").getAPropertyRead("ui").getAMethodCall("loader") } +class SapUiCore extends MethodCallNode { + SapUiCore() { this = globalVarRef("sap").getAPropertyRead("ui").getAMethodCall("getCore") } } /** @@ -212,7 +214,9 @@ class JQuerySap extends DataFlow::SourceNode { * A user-defined module through `jQuery.sap.declare`. */ class JQueryDefineModule extends UserModule, MethodCallExpr { - JQueryDefineModule() { exists(JQuerySap jquerySap | jquerySap.asExpr() = this.getReceiver()) } + JQueryDefineModule() { + exists(JQuerySap jQuerySap | this = jQuerySap.getAMemberCall(["declare", "require"]).asExpr()) + } override string getADependency() { result = this.getArgument(0).getStringValue() } @@ -227,8 +231,9 @@ class JQueryDefineModule extends UserModule, MethodCallExpr { class Renderer extends SapExtendCall { Renderer() { - this.getReceiver().getALocalSource() = + this = TypeTrackers::hasDependency(["sap/ui/core/Renderer", "sap.ui.core.Renderer"]) + .getAMemberCall("extend") } FunctionNode getRenderer() { @@ -242,8 +247,9 @@ class Renderer extends SapExtendCall { class CustomControl extends SapExtendCall { CustomControl() { - this.getReceiver().getALocalSource() = - TypeTrackers::hasDependency(["sap/ui/core/Control", "sap.ui.core.Control"]) or + this = + TypeTrackers::hasDependency(["sap/ui/core/Control", "sap.ui.core.Control"]) + .getAMemberCall("extend") or exists(SapDefineModule sapModule | this.getDefine() = sapModule.getExtendingModule()) } @@ -293,22 +299,49 @@ class CustomControl extends SapExtendCall { } } +class ControlPlaceAtCall extends MethodCallNode { + ControlPlaceAtCall() { + /* 1. `this.placeAt(...)` in a custom control definition. */ + exists(CustomControl control | this = control.getAThisNode().getAMemberCall("placeAt")) + or + /* + * 2. `new SomeControl(...).placeAt(...)` where `SomeControl` may be UI5 + * library control or a custom control. + */ + + exists(ElementInstantiation controlInstantiation | + this = controlInstantiation.getAMemberCall("placeAt") + ) + or + /* + * 3. `oController.getView().byId(...).placeAt(...)` where + * `oController.getView().byId(...)` is a reference to a library control + * or a custom control. + */ + + exists(ControlReference controlReference | this = controlReference.getAMemberCall("placeAt")) + } + + string getDomElementId() { result = this.getArgument(0).getStringValue() } +} + abstract class Reference extends MethodCallNode { } /** - * A JS reference to a `UI5Control`, commonly obtained via `View.byId(controlId)`. + * A JS reference to a `UI5Control`, commonly obtained via its ID. */ class ControlReference extends Reference { string controlId; ControlReference() { - exists(CustomController controller | - ( - controller.getAViewReference().flowsTo(this.getReceiver()) or - controller.getAThisNode() = this.getReceiver() - ) and - this.getMethodName() = "byId" and - this.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() = controlId + 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")) ) } @@ -319,6 +352,14 @@ class ControlReference extends Reference { ) } + predicate isLibraryControlReference(string importPath) { + exists(XmlView xml, UI5Control control | + control = xml.getControl() and + control.getQualifiedType() = importPath and + controlId = control.getProperty("id").getValue() + ) + } + string getId() { result = controlId } MethodCallNode getARead(string propertyName) { @@ -389,17 +430,11 @@ class ControlReference extends Reference { class ViewReference extends Reference { CustomController controller; - ViewReference() { - this.getMethodName() = "getView" and - controller.getAThisNode() = this.getReceiver() - } + ViewReference() { this = controller.getAThisNode().getAMemberCall("getView") } UI5View getDefinition() { result = controller.getView() } - MethodCallNode getABindElementCall() { - result.getMethodName() = "bindElement" and - this.flowsTo(result.getReceiver()) - } + MethodCallNode getABindElementCall() { result = this.getAMemberCall("bindElement") } } /** @@ -408,7 +443,7 @@ class ViewReference extends Reference { class ControllerReference extends Reference { ViewReference viewReference; - ControllerReference() { viewReference.flowsTo(this.getReceiver()) } + ControllerReference() { this = viewReference.getAMemberCall("getController") } CustomController getDefinition() { result = viewReference.getDefinition().getController() } } @@ -417,8 +452,9 @@ class CustomController extends SapExtendCall { string name; CustomController() { - this.getReceiver().getALocalSource() = - TypeTrackers::hasDependency(["sap/ui/core/mvc/Controller", "sap.ui.core.mvc.Controller"]) and + this = + TypeTrackers::hasDependency(["sap/ui/core/mvc/Controller", "sap.ui.core.mvc.Controller"]) + .getAMemberCall("extend") and name = this.getFile().getBaseName().regexpCapture("([a-zA-Z0-9]+).[cC]ontroller.js", 1) } @@ -438,68 +474,51 @@ class CustomController extends SapExtendCall { } MethodCallNode getOwnerComponentRef() { - this.getAThisNode() = result.getReceiver() and - result.getMethodName() = "getOwnerComponent" + result = this.getAThisNode().getAMemberCall("getOwnerComponent") } /** * Gets a reference to a view object that can be accessed from one of the methods of this controller. */ - ViewReference getAViewReference() { - result.getMethodName() = "getView" and - result.(MethodCallNode).getReceiver() = this.getAThisNode() - } + ViewReference getAViewReference() { result = this.getAThisNode().getAMemberCall("getView") } UI5View getView() { this = result.getController() } ControlReference getAControlReference() { - exists(MethodCallNode viewRef | - viewRef = this.getAViewReference() and - /* There is a view */ - viewRef.flowsTo(result.(MethodCallNode).getReceiver()) and - /* The result is a member of this view */ - result.(MethodCallNode).getMethodName() = "byId" - ) + result = this.getAViewReference().getAMemberCall("byId") } - ValueNode getAThisNode() { - exists(ThisNode thisNode | thisNode.getBinder() = this.getAMethod() | - /* ========== 1. `this` referring to the binder ========== */ - thisNode.flowsTo(result) - or - /* 2. ========== `this` bound to an outside `this` ========== */ - /* - * 2-1. The DisplayEventHandler's `this` bound to an outside `this` via - * `.attachDisplay` or `.detachDisplay` - */ + override ThisNode getAThisNode() { + /* ========== 1. `this` referring to the binder ========== */ + result = super.getAThisNode() + or + /* 2. ========== `this` bound to a callback's `this` ========== */ + /* + * 2-1. The this node of `.attachDisplay` or `.detachDisplay` also represents this + * controller. + */ - exists(DisplayEventHandler handler, ThisNode handlerThis | handlerThis.getBinder() = handler | - thisNode.flowsTo(handler.getAssociatedContextObject()) and - handlerThis.flowsTo(result) - ) + exists(DisplayEventHandler handler | handler = result.getBinder() | + handler.getAssociatedContextObject().getALocalSource() = this.getAThisNode() ) } UI5Model getModel() { - exists(MethodCallNode setModelCall | - this.getAViewReference().flowsTo(setModelCall.getReceiver()) and - setModelCall.getMethodName() = "setModel" and - result.flowsTo(setModelCall.getAnArgument()) - ) + result = this.getAViewReference().getAMemberCall("setModel").getAnArgument().getALocalSource() } ModelReference getModelReference(string modelName) { - this.getAViewReference().flowsTo(result.getReceiver()) and - result.getModelName() = modelName + result = this.getAModelReference() and + result.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() = modelName } - ModelReference getAModelReference() { this.getAViewReference().flowsTo(result.getReceiver()) } + ModelReference getAModelReference() { + result = this.getAViewReference().getAMemberCall("getModel") + } RouterReference getARouterReference() { - result.getMethodName() = "getRouter" and - exists(ThisNode controllerThis | - result.(MethodCallNode).getReceiver() = controllerThis.getALocalUse() and - controllerThis.getBinder() = this.getAMethod() + exists(ThisNode controllerThis | controllerThis.getBinder() = this.getAMethod() | + result = controllerThis.getAMemberCall("getRouter") ) } @@ -514,9 +533,10 @@ class RouteReference extends MethodCallNode { string name; RouteReference() { - this.getMethodName() = "getRoute" and - this.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() = name and - exists(RouterReference routerReference | routerReference.flowsTo(this.getReceiver())) + exists(RouterReference routerReference | + this = routerReference.getAMemberCall("getRoute") and + this.getArgument(0).getALocalSource().getStringValue() = name + ) } string getName() { result = name } @@ -544,10 +564,9 @@ class ControllerHandler extends EventHandler { class RouterReference extends MethodCallNode { RouterReference() { - this.getMethodName() = "getRouter" and exists(CustomController controller | - controller.getAThisNode() = this.getReceiver() or - controller.getOwnerComponentRef().flowsTo(this.getReceiver()) + this = controller.getAThisNode().getAMemberCall("getRouter") or + this = controller.getOwnerComponentRef().getAMemberCall("getRouter") ) } @@ -564,9 +583,8 @@ class RoutingTarget extends MethodCallNode { RouterReference routerReference; RoutingTarget() { - this.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() = name and - routerReference = this.getReceiver().getALocalSource() and - this.getMethodName() = "getTarget" + this.getArgument(0).getALocalSource().getStringValue() = name and + this = routerReference.getAMemberCall("getTarget") } RouterReference getRouterReference() { result = routerReference } @@ -579,13 +597,11 @@ class RoutingTarget extends MethodCallNode { } MethodCallNode getAnAttachDisplayCall() { - result.getReceiver().getALocalSource() = routerReference.getATarget() and - result.getMethodName() = "attachDisplay" + result = routerReference.getATarget().getAMemberCall("attachDisplay") } MethodCallNode getADetachDisplayCall() { - result.getReceiver().getALocalSource() = routerReference.getATarget() and - result.getMethodName() = "detachDisplay" + result = routerReference.getATarget().getAMemberCall("detachDisplay") } } @@ -614,17 +630,14 @@ class DisplayEventHandler extends EventHandler { */ class ModelReference extends MethodCallNode { ModelReference() { - this.getMethodName() = "getModel" and - ( - exists(ViewReference view | view.flowsTo(this.getReceiver())) - or - exists(CustomController controller | - controller.getAViewReference().flowsTo(this.getReceiver()) or - controller.getOwnerComponentRef().flowsTo(this.getReceiver()) - ) - or - exists(Component component | component.getAThisNode().flowsTo(this.getReceiver())) + exists(ViewReference view | this = view.getAMemberCall("getModel")) + or + exists(CustomController controller | + this = controller.getAViewReference().getAMemberCall("getModel") or + this = controller.getOwnerComponentRef().getAMemberCall("getModel") ) + or + exists(Component component | this = component.getAThisNode().getAMemberCall("getModel")) } predicate isDefaultModelReference() { this.getNumArgument() = 0 } @@ -632,9 +645,7 @@ class ModelReference extends MethodCallNode { /** * Gets the models' name being referred to, given that it can be statically determined. */ - string getModelName() { - result = this.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() - } + string getModelName() { result = this.getArgument(0).getALocalSource().getStringValue() } predicate isLocalModelReference() { exists(InternalModelManifest internalModelManifest | @@ -703,10 +714,7 @@ class ModelReference extends MethodCallNode { /** * Gets a `getProperty` or `getObject` method call on this `ModelReference`. These methods read from a single property of the model this refers to. */ - MethodCallNode getARead() { - result.getMethodName() = ["getProperty", "getObject"] and - result.getReceiver().getALocalSource() = this - } + MethodCallNode getARead() { result = this.getAMemberCall(["getProperty", "getObject"]) } /** * Gets the resolved model of this `ModelReference` by looking for a matching `setModel` call. @@ -723,10 +731,7 @@ abstract class UI5Model extends InvokeNode { /** * A `getProperty` or `getObject` method call on this `UI5Model`. These methods read from a single property of this model. */ - MethodCallNode getARead() { - result.getMethodName() = ["getProperty", "getObject"] and - result.getReceiver().getALocalSource() = this - } + MethodCallNode getARead() { result = this.getAMemberCall(["getProperty", "getObject"]) } } /** @@ -747,16 +752,16 @@ import ManifestJson */ class Component extends SapExtendCall { Component() { - this.getReceiver().getALocalSource() = - /* - * Represents models that are loaded from an external source, e.g. OData service. - * It is the value flowing to a `setModel` call in a handler of a `CustomController` (which is represented by `ControllerHandler`), since it is the closest we can get to the actual model itself. - */ + /* + * Represents models that are loaded from an external source, e.g. OData service. + * It is the value flowing to a `setModel` call in a handler of a `CustomController` (which is represented by `ControllerHandler`), since it is the closest we can get to the actual model itself. + */ + this = TypeTrackers::hasDependency([ "sap/ui/core/mvc/Component", "sap.ui.core.mvc.Component", "sap/ui/core/UIComponent", "sap.ui.core.UIComponent" - ]) + ]).getAMemberCall("extend") } string getId() { result = this.getName().regexpCapture("([a-zA-Z0-9.]+).Component", 1) } @@ -787,8 +792,6 @@ class Component extends SapExtendCall { } ExternalModelManifest getAnExternalModelDef() { result = this.getExternalModelDef(_) } - - ThisNode getAThisNode() { result.getBinder() = this.getAMethod() } } module ManifestJson { @@ -1184,20 +1187,17 @@ class ResourceModel extends UI5Model, ModelReference { /* A model reference obtained from this.getOwnerComponent().getModel("i18n") */ exists(CustomController controller, ResourceModelManifest manifest | ( - controller.getAThisNode() = this.getReceiver() or - controller.getOwnerComponentRef().flowsTo(this.(ModelReference).getReceiver()) + this = controller.getAThisNode().getAMemberCall("getModel") or + this = controller.getOwnerComponentRef().getAMemberCall("getModel") ) and modelName = this.getModelName() and manifest.getName() = modelName ) } - override MethodCallNode getARead() { result = this.(ModelReference).getARead() } + override MethodCallNode getARead() { result = ModelReference.super.getARead() } - MethodCallNode getResourceBundle() { - result.getMethodName() = "getResourceBundle" and - this = result.getReceiver().getALocalSource() - } + MethodCallNode getResourceBundle() { result = this.getAMemberCall("getResourceBundle") } } class BindingMode extends RequiredObject { @@ -1238,21 +1238,18 @@ class RequiredObject extends Expr { */ class SapExtendCall extends InvokeNode, MethodCallNode { SapExtendCall() { - /* 1. The receiver object is an imported one */ exists(RequiredObject requiredModule | - requiredModule.asSourceNode().flowsTo(this.getReceiver()) - ) and - /* 2. The method name is `extend` */ - this.(MethodCallNode).getMethodName() = "extend" + this = requiredModule.asSourceNode().getAMemberCall("extend") + ) } FunctionNode getMethod(string methodName) { - result = this.getContent().(ObjectLiteralNode).getAPropertySource(methodName).(FunctionNode) + result = this.getContent().(ObjectLiteralNode).getAPropertySource(methodName) } FunctionNode getAMethod() { result = this.getMethod(_) } - string getName() { result = this.getArgument(0).asExpr().(StringLiteral).getValue() } + string getName() { result = this.getArgument(0).getALocalSource().getStringValue() } ObjectLiteralNode getContent() { result = this.getArgument(1) } @@ -1267,39 +1264,25 @@ class SapExtendCall extends InvokeNode, MethodCallNode { /** Gets the `sap.ui.define` call that wraps this extension. */ SapDefineModule getDefine() { this.getEnclosingFunction() = result.getArgument(1) } -} -private newtype TSapElement = - DefinitionOfElement(SapExtendCall extension) or - ReferenceOfElement(Reference reference) - -class SapElement extends TSapElement { - SapExtendCall asDefinition() { this = DefinitionOfElement(result) } + ThisNode getAThisNode() { result.getBinder() = this.getAMethod() } +} - Reference asReference() { this = ReferenceOfElement(result) } +class ElementInstantiation extends NewNode { + string importPath; - SapElement getParentElement() { - result.asReference() = this.asDefinition().(CustomControl).getController().getAViewReference() or - result.asReference() = - this.asReference().(ControlReference).getDefinition().getController().getAViewReference() or - result.asDefinition() = this.asReference().(ViewReference).getDefinition().getController() or - result.asDefinition() = this.asDefinition().(CustomController).getOwnerComponent() or - result.asDefinition() = - this.asReference().(ControllerReference).getDefinition().getOwnerComponent() + ElementInstantiation() { + exists(RequiredObject requiredObject | + this = requiredObject.asSourceNode().getAnInstantiation() and + importPath = requiredObject.getDependency() + ) } - string toString() { - result = this.asDefinition().toString() or - result = this.asReference().toString() + string getId() { + result = this.getArgument(0).(SourceNode).getAPropertyWrite("id").getRhs().getStringValue() } - predicate hasLocationInfo( - string filepath, int startline, int startcolumn, int endline, int endcolumn - ) { - this.asDefinition().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) - or - this.asReference().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) - } + string getImportPath() { result = importPath } } /** @@ -1312,11 +1295,8 @@ class Metadata extends ObjectLiteralNode { Metadata() { this = extension.getContent().getAPropertySource("metadata") } - SourceNode getProperty(string name) { - result = - any(PropertyMetadata property | - property.getParentMetadata() = this and property.getName() = name - ) + PropertyMetadata getProperty(string name) { + result.getParentMetadata() = this and result.getName() = name } } @@ -1368,10 +1348,10 @@ class PropertyMetadata extends ObjectLiteralNode { */ predicate isUnrestrictedStringType() { /* text : "string" */ - this.asExpr().(StringLiteral).getValue() = "string" + this.getStringValue() = "string" or /* text: { type: "string" } */ - this.getAPropertySource("type").asExpr().(StringLiteral).getValue() = "string" + this.getAPropertySource("type").getStringValue() = "string" or /* text: { someOther: "someOtherVal", ... } */ not exists(this.getAPropertySource("type")) 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 0a309b2cc..b7b57c7dc 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 @@ -190,7 +190,6 @@ predicate isBuiltInControl(string qualifiedTypeUri) { /** * A UI5 View that might include XSS sources and sinks in standard controls. */ -/* TODO: Update docstring */ abstract class UI5View extends File { abstract string getControllerName(); 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 71d83278a..947b3d7de 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,6 +1,7 @@ import javascript import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow as UI5DataFlow import advanced_security.javascript.frameworks.ui5.UI5View +private import semmle.javascript.frameworks.data.internal.ApiGraphModelsExtensions private import semmle.javascript.security.dataflow.DomBasedXssQuery as DomBasedXss module UI5Xss implements DataFlow::ConfigSig { @@ -34,7 +35,8 @@ module UI5Xss implements DataFlow::ConfigSig { predicate isSink(DataFlow::Node node) { node instanceof UI5ExtHtmlISink or - node instanceof UI5ModelHtmlISink + node instanceof UI5ModelHtmlISink or + node instanceof DynamicallySetElementValueOfInstantiatedHTMLControlPlacedAtDom } predicate isAdditionalFlowStep(DataFlow::Node start, DataFlow::Node end) { @@ -67,5 +69,61 @@ class UI5ModelHtmlISink extends DataFlow::Node { * An HTML injection sink typically for custom controls whose RenderManager calls acting as sinks. */ private class UI5ExtHtmlISink extends DataFlow::Node { - UI5ExtHtmlISink() { this = ModelOutput::getASinkNode("ui5-html-injection").asSink() } + UI5ExtHtmlISink() { + this = ModelOutput::getASinkNode("ui5-html-injection").asSink() and + /* Exclude property writes to HTML controls; they are covered in a separate class below. */ + not this instanceof DynamicallySetElementValueOfInstantiatedHTMLControlPlacedAtDom + } +} + +private class HTMLControlInstantiation extends ElementInstantiation { + HTMLControlInstantiation() { typeModel("UI5HTMLControl", this.getImportPath(), _) } +} + +private module TrackPlaceAtCallConfigFlow = TaintTracking::Global; + +abstract class DynamicallySetElementValueOfHTML extends DataFlow::Node { } + +/** + * The DOM value of a UI5 control that is dynamically generated then placed at + * a certain position in a DOM. + */ +class DynamicallySetElementValueOfInstantiatedHTMLControlPlacedAtDom extends DynamicallySetElementValueOfHTML +{ + DataFlow::Node root; + ControlPlaceAtCall placeAtCall; + + DynamicallySetElementValueOfInstantiatedHTMLControlPlacedAtDom() { + exists(NewNode new | root = new | + new = ModelOutput::getATypeNode("UI5HTMLControl").getAnInstantiation() and + ( + this = new.getAnArgument().(ObjectLiteralNode).getAPropertyWrite("content").getRhs() + or + this = new.getAPropertyWrite("content").getRhs() + or + this = new.getAMemberCall("setContent").getAnArgument() + ) + ) and + /* Ensure that this is placed somewhere in the DOM. */ + TrackPlaceAtCallConfigFlow::flow(root, placeAtCall) + } +} + +class DynamicallySetElementValueOfHTMLControlReference extends DynamicallySetElementValueOfHTML { + DynamicallySetElementValueOfHTMLControlReference() { + /* 2. The content is written to the reference of the control. */ + exists(ControlReference controlReference | + controlReference.isLibraryControlReference("sap.m.HTML") + | + /* + * 2-1. The content is written directly to the `content` property of the control + * reference. + */ + + this = controlReference.getAPropertyWrite("content") + or + /* 2-2. The content is written using the `setContent` setter. */ + this = controlReference.getAMemberCall("setContent").getArgument(0) + ) + } } 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 14cd8947a..d89626f32 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 @@ -7,9 +7,11 @@ import advanced_security.javascript.frameworks.ui5.dataflow.FlowSteps private import PatchDataFlow /** - * A statically visible part of a local model's content that has a binding path referring to it in a control declaration acting as an HTML injection sink. + * A statically visible part of a local model's content that has a binding path referring to it in a + * control declaration acting as an HTML injection sink. * - * e.g.1. Given a JSON model `oModel` declared in a controller handler and an HTML injection sink `SomeSinkControl` as: + * e.g.1. Given a JSON model `oModel` declared in a controller handler and an HTML injection sink + * `SomeSinkControl` as: * ```javascript * let oModel = new JSONModel({ y: null }); * ``` @@ -19,7 +21,8 @@ private import PatchDataFlow * ``` * The content `y: null` of `oModel` is recognized as an instance of this class. * - * e.g.2. Given a JSON model `oModel` which gains its content from a JSON file and an HTML injection sink `SomeSinkControl` as: + * e.g.2. Given a JSON model `oModel` which gains its content from a JSON file and an HTML injection + * sink `SomeSinkControl` as: * ```javascript * let oModel = new JSONModel("controller/contents.json"); * ``` @@ -155,8 +158,11 @@ module UI5PathGraph Con } /** - * An edge from a content of an internal model to the corresponding binding path in a view, which makes it an edge in the opposite direction as of `bindingPathToInternalModelContent` above. - * In order to ensure that the edge indeed holds, we check if the model's binding mode is declared as two-way. + * An edge from a content of an internal model to the corresponding binding path in a view, + * which makes it an edge in the opposite direction as of `bindingPathToInternalModelContent` + * above. + * In order to ensure that the edge indeed holds, we check if the model's binding mode is + * declared as two-way. * * c.f. `FlowSteps::InternalModelContentToCustomMetadataPropertyStep`. */ @@ -212,3 +218,60 @@ module UI5PathGraph Con ui5PathNodeSucc.asUI5BindingPathNode() = any(UI5View view).getAnHtmlISink() } } + +module TrackPlaceAtCallConfig implements DataFlow::ConfigSig { + /** + * A child element instantiation. + */ + predicate isSource(DataFlow::Node node) { node instanceof ElementInstantiation } + + /** + * An "extension point" exposed from a parent element instantiation to + * register a child to itself. + */ + predicate isSink(DataFlow::Node node) { node instanceof ControlPlaceAtCall } + + /** + * Step from data being written and the property that is being written to. + * Needed to express the fact that the child control is usually written to + * a property of a parent control to establish the child-parent relationship. + * + * NOTE: The "extension point" exposed by the parent control can come in the + * form of: + * + * 1. `new Parent({ children: [ new Child( ... ) ] })` + * 2. `new Parent(...).children[0] = new Child( ... )` + * 3. `new Parent(...).addChild(new Child( ... ))` + * + * The first and second cases are (nested) `PropWrite`s to `getArgument(0)` + * and are easily covered. But the third case poses a new problem of identifying + * methods that writes to the `children` property, which CodeQL can't verify + * because its definition is in the library. + * + * - [X] Mark all method calls: overgeneralizes, can infer dumb things + * (`sap.m.FlexBox.removeItem` comes to mind), but would still work for all + * true cases + * - [ ] Heuristic by prefix: not an attractive option since heuristics can fail + */ + predicate isAdditionalFlowStep(DataFlow::Node start, DataFlow::Node end) { + exists(DataFlow::PropWrite propWrite | + start = propWrite.getRhs() and + end = propWrite.getBase() + ) + or + exists(DataFlow::MethodCallNode maybeAddingChildAPICall | + start = maybeAddingChildAPICall.getAnArgument() and + end = maybeAddingChildAPICall.getReceiver() + ) + or + exists(DataFlow::MethodCallNode call | + start = call.getReceiver() and + end = call + ) + or + exists(DataFlow::NewNode new | + start = new.getAnArgument() and + end = new + ) + } +} diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript_sap_ui5_all/Customizations.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript_sap_ui5_all/Customizations.qll index e6f989590..b3849262a 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript_sap_ui5_all/Customizations.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript_sap_ui5_all/Customizations.qll @@ -1,6 +1,8 @@ -// This file is included for use in custom CodeQL bundles (https://github.com/advanced-security/codeql-bundle). -// The contents of this file will be included in the standard library `Customizations.qll`, and will therefore -// be included in the out-of-the-box security queries. -// -// We import under alias to avoid any potential naming conflicts +/** + * This file is included for use in custom CodeQL bundles (https://github.com/advanced-security/codeql-bundle). + * The contents of this file will be included in the standard library `Customizations.qll`, and will therefore + * be included in the out-of-the-box security queries. + */ + +/* We import under alias to avoid any potential naming conflicts */ import advanced_security.javascript.frameworks.ui5.RemoteFlowSources as UI5RemoteFlowSources diff --git a/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/dynamicWriteToHtmlContent.expected b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/dynamicWriteToHtmlContent.expected new file mode 100644 index 000000000..1dca6e0ca --- /dev/null +++ b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/dynamicWriteToHtmlContent.expected @@ -0,0 +1,5 @@ +| webapp/controller/app.controller.js:12:9:12:27 | htmlControl.content | Dynamic write to content of an HTML control without the use of a binding path. | +| webapp/controller/app.controller.js:15:32:15:56 | inputRe ... Value() | Dynamic write to content of an HTML control without the use of a binding path. | +| webapp/controller/app.controller.js:24:20:24:60 | `
$ ...
` | Dynamic write to content of an HTML control without the use of a binding path. | +| webapp/controller/app.controller.js:30:32:30:56 | inputRe ... Value() | Dynamic write to content of an HTML control without the use of a binding path. | +| webapp/controller/app.controller.js:35:33:35:57 | inputRe ... Value() | Dynamic write to content of an HTML control without the use of a binding path. | diff --git a/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/dynamicWriteToHtmlContent.ql b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/dynamicWriteToHtmlContent.ql new file mode 100644 index 000000000..06816b22f --- /dev/null +++ b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/dynamicWriteToHtmlContent.ql @@ -0,0 +1,5 @@ +import javascript +import advanced_security.javascript.frameworks.ui5.UI5XssQuery + +from DynamicallySetElementValueOfHTML htmlContent +select htmlContent, "Dynamic write to content of an HTML control without the use of a binding path." diff --git a/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/package-lock.json b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/package-lock.json new file mode 100644 index 000000000..ba052860f --- /dev/null +++ b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/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/models/dynamic_write_to_html_content/package.json b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/package.json new file mode 100644 index 000000000..9f0a4560b --- /dev/null +++ b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/package.json @@ -0,0 +1,5 @@ +{ + "name": "sap-ui5-xss", + "version": "1.0.0", + "main": "index.js" +} diff --git a/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/ui5.yaml b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/ui5.yaml new file mode 100644 index 000000000..beb2ff69d --- /dev/null +++ b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/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/models/dynamic_write_to_html_content/webapp/controller/app.controller.js b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/webapp/controller/app.controller.js new file mode 100644 index 000000000..222973749 --- /dev/null +++ b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/webapp/controller/app.controller.js @@ -0,0 +1,55 @@ +sap.ui.define( + ["sap/ui/core/mvc/Controller", "sap/ui/core/HTML"], + function (Controller, HTML) { + "use strict"; + return Controller.extend("codeql-sap-js.controller.app", { + onInit: function () { + let inputReference = this.getView().byId("unit-test-target1"); + let htmlControl = this.getView().byId("htmlControl"); + + /* ========== 1. UNSAFE: Input value piped into a reference to a static HTML, via a reference ========== */ + /* 1-1. Value directly set to `HTML.content` */ + htmlControl.content = inputReference.getValue(); // UNSAFE: property `content` set with an input value of a reference to a static value + + /* 1-2. Value set by `HTML.setContent(content)` */ + htmlControl.setContent(inputReference.getValue()); // UNSAFE: property `content` set with an input value of a reference to a static value + }, + + doSomething1: function () { + let inputReference = this.getView().byId("unit-test-target1"); + + /* ========== 2. UNSAFE: Input value piped into dynamic HTML, instantiated and placed on-demand ========== */ + /* 2-1. Value passed to the argument of the constructor call */ + let htmlControl1 = new HTML({ + content: `
${inputReference.getValue()}
`, // UNSAFE: property `content` set with an input value, control later placed at DOM + }); + htmlControl1.placeAt("HTMLPlaceholder"); + + /* 2-2. Value directly set to `HTML.content` */ + let htmlControl2 = new HTML(); + htmlControl2.content = inputReference.getValue(); // UNSAFE: property `content` set with an input value, control later placed at DOM + htmlControl2.placeAt("HTMLPlaceholder"); + + /* 2-3. Value set by `HTML.setContent(content)` */ + let htmlControl3 = new HTML(); + htmlControl3.setContent(inputReference.getValue()); // UNSAFE: property `content` set with an input value, control later placed at DOM + htmlControl3.placeAt("HTMLPlaceholder"); + }, + + doSomething2: function () { + let inputReference = this.getView().byId("unit-test-target1"); + + /* ========== 3. SAFE: Input value piped into dynamic HTML, instantiated but not placed anywhere in the DOM ========== */ + let htmlControl1 = new HTML({ + content: `
${inputReference.getValue()}
`, // SAFE: property `content` set with an input value but control not placed anywhere + }); + + let htmlControl2 = new HTML(); + htmlControl2.content = inputReference.getValue(); // SAFE: property `content` set with an input value but control not placed anywhere + + let htmlControl3 = new HTML(); + htmlControl3.setContent(inputReference.getValue()); // SAFE: property `content` set with an input value but control not placed anywhere + }, + }); + } +); diff --git a/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/webapp/index.html b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/webapp/index.html new file mode 100644 index 000000000..c523033ec --- /dev/null +++ b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/webapp/index.html @@ -0,0 +1,18 @@ + + + + + SAPUI5 XSS + + + + +
+ diff --git a/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/webapp/index.js b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/webapp/index.js new file mode 100644 index 000000000..7a66697fd --- /dev/null +++ b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/webapp/index.js @@ -0,0 +1,11 @@ +sap.ui.define([ + "sap/ui/core/mvc/XMLView" +], function (XMLView) { + "use strict"; + XMLView.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/models/dynamic_write_to_html_content/webapp/manifest.json b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/webapp/manifest.json new file mode 100644 index 000000000..65d4de250 --- /dev/null +++ b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/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/models/dynamic_write_to_html_content/webapp/view/app.view.xml b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/webapp/view/app.view.xml new file mode 100644 index 000000000..5f809cc7a --- /dev/null +++ b/javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/webapp/view/app.view.xml @@ -0,0 +1,7 @@ + + +