Detect XSS involving server-side models and controller handler parameters

javascript/frameworks/ui5/ext/ui5.model.yml

@@ -66,7 +66,6 @@ extensions:
       - ["UI5Control", "Member[getValue].ReturnValue", "remote"]
       - ["UI5CodeEditor", "Member[value]", "remote"]
       - ["UI5CodeEditor", "Member[getCurrentValue].ReturnValue", "remote"]
-      - ["global", "Member[jQuery].Member[sap].Member[getUriParameters].ReturnValue.Member[get].ReturnValue", "remote"]
       - ["global", "Member[jQuery].Member[sap].Member[syncHead,syncGet,syncGetText,syncPost,syncPostText].ReturnValue", "remote"]
       - ["UI5URIParameters", "Member[get].ReturnValue", "remote"]
       - ["UI5URIParameters", "Member[getAll].ReturnValue", "remote"]

javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/Bindings.qll

@@ -193,7 +193,8 @@ abstract private class LateJavaScriptPropertyBinding extends DataFlow::Node {
  */
 private predicate earlyPropertyBinding(
   DataFlow::NewNode newNode, DataFlow::PropWrite bindingTarget, DataFlow::Node binding,
-  DataFlow::Node bindingPath) {
+  DataFlow::Node bindingPath
+) {
   // Property binding via an object literal binding with property `path`.
   // This assumes the value assigned to `path` is a binding, even if we cannot
   // statically determine it is a binding.
@@ -205,9 +206,10 @@ private predicate earlyPropertyBinding(
     if exists(binding.getALocalSource())
     then binding.getALocalSource() = bindingPath
     else binding = bindingPath // e.g., path: "/" + someVar
-  ) and not bindingPath.getStringValue() instanceof BindingString
+  ) and
+  not bindingPath.getStringValue() instanceof BindingString
   or
-  // Propery binding of an arbitrary property for which we can statically determined
+  // Property binding of an arbitrary property for which we can statically determined
   // the value written to the property is a binding path.
   exists(DataFlow::SourceNode objectLiteral |
     newNode.getAnArgument().getALocalSource() = objectLiteral and
@@ -339,9 +341,7 @@ private newtype TBinding =
    * with a property `parts` assigned a value, or
    * an object literal that is assigned a string value that is a binding path.
    */
-  TEarlyJavaScriptPropertyBinding(
-    DataFlow::PropWrite bindingTarget, DataFlow::ValueNode binding
-  ) {
+  TEarlyJavaScriptPropertyBinding(DataFlow::PropWrite bindingTarget, DataFlow::ValueNode binding) {
     earlyPropertyBinding(_, bindingTarget, binding, _)
   } or
   // Property binding via a call to `bindProperty` or `bindValue`.
@@ -415,29 +415,34 @@ private newtype TBindingPath =
     )
   } or
   TDynamicBindingPath(Binding binding, DataFlow::Node dynamicBinding, DataFlow::Node bindingPath) {
-    (exists(DataFlow::PropWrite bindingTarget |
-      binding = TEarlyJavaScriptPropertyBinding(bindingTarget, dynamicBinding) and
-      earlyPropertyBinding(_, bindingTarget, dynamicBinding, bindingPath)
-    )
-    or
-    exists(LateJavaScriptPropertyBinding lateJavaScriptPropertyBinding |
-      // Property binding via a call to `bindProperty` or `bindValue`.
-      binding = TLateJavaScriptPropertyBinding(lateJavaScriptPropertyBinding, dynamicBinding) and
-      latePropertyBinding(lateJavaScriptPropertyBinding, dynamicBinding, bindingPath)
-    )
-    or
-    exists(BindElementMethodCallNode bindElementMethodCall |
-      // Element binding via a call to `bindElement`.
-      binding = TLateJavaScriptContextBinding(bindElementMethodCall, dynamicBinding) and
-      lateContextBinding(bindElementMethodCall, dynamicBinding, bindingPath)
-    )) and
+    (
+      exists(DataFlow::PropWrite bindingTarget |
+        binding = TEarlyJavaScriptPropertyBinding(bindingTarget, dynamicBinding) and
+        earlyPropertyBinding(_, bindingTarget, dynamicBinding, bindingPath)
+      )
+      or
+      exists(LateJavaScriptPropertyBinding lateJavaScriptPropertyBinding |
+        // Property binding via a call to `bindProperty` or `bindValue`.
+        binding = TLateJavaScriptPropertyBinding(lateJavaScriptPropertyBinding, dynamicBinding) and
+        latePropertyBinding(lateJavaScriptPropertyBinding, dynamicBinding, bindingPath)
+      )
+      or
+      exists(BindElementMethodCallNode bindElementMethodCall |
+        // Element binding via a call to `bindElement`.
+        binding = TLateJavaScriptContextBinding(bindElementMethodCall, dynamicBinding) and
+        lateContextBinding(bindElementMethodCall, dynamicBinding, bindingPath)
+      )
+    ) and
     not dynamicBinding.mayHaveStringValue(_)
   }
 
 /**
  * A class representing a binding path.
  */
 class BindingPath extends TBindingPath {
+  /**
+   * For debugging purposes (pretty-printing in result table)
+   */
   string toString() {
     exists(BindingStringParser::BindingPath path |
       this = TStaticBindingPath(_, _, path) and
@@ -460,6 +465,11 @@ class BindingPath extends TBindingPath {
       this = TStaticBindingPath(_, _, path) and
       result = path.toString()
     )
+    or
+    exists(DataFlow::Node pathValue |
+      this = TDynamicBindingPath(_, _, pathValue) and
+      result = pathValue.asExpr().(StringLiteral).getValue().regexpCapture("\\{(.*)\\}", 1)
+    )
   }
 
   Location getLocation() {
@@ -616,8 +626,8 @@ class BindingTarget extends TBindingTarget {
   Binding getBinding() {
     this = TXmlPropertyBindingTarget(_, result) or
     this = TXmlContextBindingTarget(_, result) or
-    this = TLateJavaScriptBindingTarget(_, result) or
     this = TEarlyJavaScriptPropertyBindingTarget(_, result) or
+    this = TLateJavaScriptBindingTarget(_, result) or
     this = TJsonPropertyBindingTarget(_, _, result)
   }
 }
@@ -650,7 +660,9 @@ class Binding extends TBinding {
     or
     exists(DataFlow::PropWrite bindingTarget, DataFlow::Node binding |
       this = TEarlyJavaScriptPropertyBinding(bindingTarget, binding) and
-      result = "Early JavaScript property binding: " + bindingTarget.getPropertyNameExpr() + " to " + binding
+      result =
+        "Early JavaScript property binding: " + bindingTarget.getPropertyNameExpr() + " to " +
+          binding
     )
     or
     exists(LateJavaScriptPropertyBinding lateJavaScriptPropertyBinding, DataFlow::Node binding |
@@ -710,9 +722,7 @@ class Binding extends TBinding {
     )
   }
 
-  BindingPath getBindingPath() {
-    result.getBinding() = this
-  }
+  BindingPath getBindingPath() { result.getBinding() = this }
 
   BindingTarget getBindingTarget() { result.getBinding() = this }
 }

javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/RemoteFlowSources.qll

@@ -0,0 +1,135 @@
+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
+
+private class DataFromRemoteControlReference extends RemoteFlowSource, MethodCallNode {
+  DataFromRemoteControlReference() {
+    exists(UI5Control sourceControl, string typeAlias, ControlReference controlReference |
+      ApiGraphModelsExtensions::typeModel(typeAlias, sourceControl.getImportPath(), _) and
+      ApiGraphModelsExtensions::sourceModel(typeAlias, _, "remote") and
+      sourceControl.getAReference() = controlReference and
+      controlReference.flowsTo(this.getReceiver()) and
+      this.getMethodName() = "getValue"
+    )
+  }
+
+  override string getSourceType() { result = "Data from a remote control" }
+}
+
+class LocalModelContentBoundBidirectionallyToSourceControl extends RemoteFlowSource {
+  UI5BindingPath bindingPath;
+  UI5Control controlDeclaration;
+
+  LocalModelContentBoundBidirectionallyToSourceControl() {
+    exists(UI5InternalModel internalModel |
+      this = bindingPath.getNode() and
+      (
+        this instanceof PropWrite and
+        internalModel.getArgument(0).getALocalSource().asExpr() =
+          this.(PropWrite).getPropertyNameExpr().getParent+()
+        or
+        this.asExpr() instanceof StringLiteral and
+        internalModel.asExpr() = this.asExpr().getParent()
+      ) and
+      any(UI5View view).getASource() = bindingPath and
+      internalModel.(JsonModel).isTwoWayBinding() and
+      controlDeclaration = bindingPath.getControlDeclaration()
+    )
+  }
+
+  override string getSourceType() {
+    result = "Local model bidirectionally bound to a input control"
+  }
+
+  UI5BindingPath getBindingPath() { result = bindingPath }
+
+  UI5Control getControlDeclaration() { result = controlDeclaration }
+}
+
+abstract class UI5ExternalModel extends UI5Model, RemoteFlowSource {
+  abstract string getName();
+}
+
+/** Model which gains content from an SAP OData service. */
+class ODataServiceModel extends UI5ExternalModel {
+  string modelName;
+
+  override string getSourceType() { result = "ODataServiceModel" }
+
+  ODataServiceModel() {
+    /*
+     * e.g. this.getView().setModel(this.getOwnerComponent().getModel("booking_nobatch"))
+     */
+
+    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"))`
+       */
+
+      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 */
+      controller.getOwnerComponent().getExternalModelDef(modelName).getDataSource() instanceof
+        ODataDataSourceManifest
+    )
+    or
+    /*
+     * A constructor call to sap.ui.model.odata.v2.ODataModel.
+     */
+
+    this instanceof NewNode and
+    (
+      exists(RequiredObject oDataModel |
+        oDataModel.flowsTo(this.getCalleeNode()) and
+        oDataModel.getDependencyType() = "sap/ui/model/odata/v2/ODataModel"
+      )
+      or
+      this.getCalleeName() = "ODataModel"
+    ) and
+    modelName = "<no name>"
+  }
+
+  override string getName() { result = modelName }
+}
+
+private class RouteParameterAccess extends RemoteFlowSource instanceof PropRead {
+  override string getSourceType() { result = "RouteParameterAccess" }
+
+  RouteParameterAccess() {
+    exists(
+      ControllerHandler handler, RouteManifest routeManifest, ParameterNode handlerParameter,
+      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
+      (
+        routeManifest.matchesPathString(this.getPropertyName()) and
+        this.getBase().getALocalSource() = getParameterCall
+        or
+        /* TODO: Why does `routeManifest.matchesPathString` not work for propertyName?? */
+        this.getBase().(PropRead).getBase().getALocalSource() = getParameterCall
+      )
+    )
+  }
+}
+
+/**
+ * Method calls that fetch a piece of data either from a library control capable of accepting user input, or from a URI parameter.
+ */
+private class UI5ExtRemoteSource extends RemoteFlowSource {
+  UI5ExtRemoteSource() { this = ModelOutput::getASourceNode("remote").asSource() }
+
+  override string getSourceType() {
+    result = "Remote flow" // Don't discriminate between UI5-specific remote flows and vanilla ones
+  }
+}