Enable precise report of instantiated controls with placeAt

Author: jeongsoolee09

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

@@ -24,13 +24,15 @@ private class InputControlInstantiation extends ElementInstantiation {
   InputControlInstantiation() { typeModel("UI5InputControl", this.getImportPath(), _) }
 }
 
-private class DataFromInstantiatedAndPlacedAtControl extends RemoteFlowSource, XssThroughDom::Source
+private module TrackPlaceAtCallConfigFlow = TaintTracking::Global<TrackPlaceAtCallConfig>;
+
+class DataFromInstantiatedAndPlacedAtControl extends RemoteFlowSource, XssThroughDom::Source
 {
+  InputControlInstantiation controlInstantiation;
+  ControlPlaceAtCall placeAtCall;
+
   DataFromInstantiatedAndPlacedAtControl() {
-    exists(
-      InputControlInstantiation controlInstantiation, string typeAlias,
-      ControlReference controlReference
-    |
+    exists(string typeAlias, ControlReference controlReference |
       /* Double check that the type derives a remote flow source. */
       typeModel(typeAlias, controlInstantiation.getImportPath(), _) and
       sourceModel(typeAlias, _, "remote", _) and
@@ -39,12 +41,18 @@ private class DataFromInstantiatedAndPlacedAtControl extends RemoteFlowSource, X
         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"
   }
+
+  ControlPlaceAtCall getPlaceAtCall() {
+    // result = "TODO"
+    none() // TODO
+  }
 }
 
 class LocalModelContentBoundBidirectionallyToSourceControl extends RemoteFlowSource {

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

@@ -302,26 +302,27 @@ class CustomControl extends SapExtendCall {
 class ControlPlaceAtCall extends MethodCallNode {
   ControlPlaceAtCall() {
     exists(SapElement ui5Control |
-      // /* 1. `this.placeAt(...)` in a custom control definition */
-      // this = ui5Control.asDefinition().getAThisNode()
-      // or
-      // /*
-      // * 2. `new SomeControl(...).placeAt(...)` where `SomeControl`
-      // *    may be UI5 library control or a custom control
-      // */
-      // ui5Control.asInstantiation().
-      // or
-      // /* 3. `.byId(...).placeAt(...)` */
-      // ui5Control.asReference().
-      none() // TODO
+      /* 1. `this.placeAt(...)` in a custom control definition */
+      this = ui5Control.asDefinition().getAThisNode().getAMemberCall("placeAt")
+      or
+      /*
+       * 2. `new SomeControl(...).placeAt(...)` where
+       * `SomeControl` may be UI5 library control or a custom control
+       */
+
+      this = ui5Control.asInstantiation().getAMemberCall("placeAt")
+      or
+      // this = ui5Control.getParentElement*().asInstantiation().getAMemberCall("placeAt") or
+      /* 3. `.byId(...).placeAt(...)` */
+      this = ui5Control.asReference().getAMemberCall("placeAt")
     )
   }
+
+  string getDomElementId() { result = this.getArgument(0).getStringValue() }
 }
 
 abstract class Reference extends MethodCallNode { }
 
-private predicate byId(MethodCallNode byId) { byId.getMethodName() = "byId" }
-
 /**
  * A JS reference to a `UI5Control`, commonly obtained via its ID.
  */
@@ -476,10 +477,10 @@ class CustomController extends SapExtendCall {
   }
 
   override ThisNode getAThisNode() {
-    /* 1. `this` referring to the binder */
+    /* ========== 1. `this` referring to the binder ========== */
     result = super.getAThisNode()
     or
-    /* 2. `this` bound to a callback's `this` */
+    /* 2. ========== `this` bound to a callback's `this` ========== */
     /*
      * 2-1. The this node of `.attachDisplay` or `.detachDisplay` also represents this
      * controller.
@@ -1289,8 +1290,19 @@ class SapElement extends TSapElement {
     result.asDefinition() = this.asReference().(ViewReference).getDefinition().getController() or
     result.asDefinition() = this.asDefinition().(CustomController).getOwnerComponent() or
     result.asDefinition() =
-      this.asReference().(ControllerReference).getDefinition().getOwnerComponent()
-    /* TODO: Implement branch for `asInstantiation` */
+      this.asReference().(ControllerReference).getDefinition().getOwnerComponent() or
+    /* ==================== exists(result.asInstantiation()) branches ==================== */
+    result.asInstantiation() =
+      this.asReference().(ControlReference).getAMemberCall(_).getAnArgument().getALocalSource() or
+    result.asInstantiation() =
+      this.asReference().(ControlReference).getAPropertyWrite().getRhs().getALocalSource()
+    // or
+    // result.asInstantiation() =
+    //   this.asInstantiation().getAMemberCall(_).getAnArgument().getALocalSource() or
+    // result.asInstantiation() = this.asInstantiation().getAPropertyWrite().getRhs().getALocalSource() or
+    // result.asInstantiation() = this.asInstantiation().getAnArgument()
+    // TrackParentControlConfig::flow(this.asInstantiation())
+    /* =================================================================================== */
   }
 
   string getId() {
@@ -1313,15 +1325,14 @@ class SapElement extends TSapElement {
 
   string toString() {
     result = this.asDefinition().toString() or
-    result = this.asReference().toString()
+    result = this.asReference().toString() or
+    result = this.asInstantiation().toString()
   }
 
-  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)
+  Location getLocation() {
+    result = this.asDefinition().getLocation() or
+    result = this.asReference().getLocation() or
+    result = this.asInstantiation().getLocation()
   }
 }
 

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

@@ -79,15 +79,24 @@ private class HTMLControlInstantiation extends ElementInstantiation {
   HTMLControlInstantiation() { typeModel("UI5HTMLControl", this.getImportPath(), _) }
 }
 
+private module TrackPlaceAtCallConfigFlow = TaintTracking::Global<TrackPlaceAtCallConfig>;
+
 /**
  * The DOM value of a UI5 control that is dynamically generated then placed at
  * a certain position in a DOM.
  */
-/* TODO: Needs testing of each branch */
-class DangerouslySetElementValueOfInstantiatedHTMLControlPlacedAtDom extends DataFlow::Node {
+/*
+ * TODO 1: Needs testing of each branch
+ * TODO 2: Model the `placeAt`:
+ */
+
+private class DangerouslySetElementValueOfInstantiatedHTMLControlPlacedAtDom extends DataFlow::Node {
+  HTMLControlInstantiation htmlControlInstantiation;
+  ControlPlaceAtCall placeAtCall;
+
   DangerouslySetElementValueOfInstantiatedHTMLControlPlacedAtDom() {
-    /* 1. The content is set via the first argument of the constructor. */
-    exists(HTMLControlInstantiation htmlControlInstantiation |
+    (
+      /* 1. The content is set via the first argument of the constructor. */
       exists(string typeAlias |
         typeModel(typeAlias, htmlControlInstantiation.getImportPath(), _) and
         /* Double check that the type derives a UI5-XSS sink. */
@@ -113,6 +122,7 @@ class DangerouslySetElementValueOfInstantiatedHTMLControlPlacedAtDom extends Dat
         /* 2-2. The content is written using the `setContent` setter.  */
         this = controlReference.getAMemberCall("content").getArgument(0)
       )
-    )
+    ) and
+    TrackPlaceAtCallConfigFlow::flow(htmlControlInstantiation, placeAtCall)
   }
 }

javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/DataFlow.qll

@@ -72,6 +72,10 @@ module UI5PathGraph<PathNodeSig ConfigPathNode, PathGraphSig<ConfigPathNode> Con
 
     UI5BindingPath asUI5BindingPathNode() { this = TUI5BindingPathNode(result) }
 
+    predicate isDataFlowNode() { exists(this.asDataFlowNode()) }
+
+    predicate isUI5BindingPathNode() { exists(this.asUI5BindingPathNode()) }
+
     string toString() {
       result = this.asDataFlowNode().toString()
       or
@@ -218,3 +222,64 @@ module UI5PathGraph<PathNodeSig ConfigPathNode, PathGraphSig<ConfigPathNode> Con
     ui5PathNodeSucc.asUI5BindingPathNode() = any(UI5View view).getAnHtmlISink()
   }
 }
+
+module TrackPlaceAtCallConfig implements DataFlow::ConfigSig {
+  /**
+   * A child element instantiation.
+   */
+  predicate isSource(DataFlow::Node node) { node instanceof ElementInstantiation }
+
+  additional predicate isSinkWithPlaceAtCall(DataFlow::Node node, ControlPlaceAtCall placeAtCall) {
+    node = placeAtCall
+  }
+
+  /**
+   * An "extension point" exposed from a parent element instantiation to
+   * register a child to itself.
+   */
+  predicate isSink(DataFlow::Node node) { isSinkWithPlaceAtCall(node, _) }
+
+  /**
+   * 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
+    )
+  }
+}