UI5 client side log-injection improvements

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

@@ -25,6 +25,8 @@ extensions:
       - ["SapLogger", "global", "Member[jQuery].Member[sap].Member[log]"]
       # Logging functions as well as `getLogger` also serves as a constructor
       - ["SapLogger", "SapLogger", "Member[debug,error,fatal,info,setLevel,trace,warning,getLogger].ReturnValue"]
+      - ["SapLogEntries", "SapLogger", "Member[addLogListener].Parameter[0].Member[onLogEntry].Argument[0]"]
+      - ["SapLogEntries", "SapLogger", "Member[getLogEntries].ReturnValue"]
       - ["ResourceBundle", "ResourceBundle", "Instance"]
       - ["ResourceBundle", "sap/base/i18n/ResourceBundle", ""]
       - ["Properties", "Properties", "Instance"]
@@ -64,7 +66,6 @@ extensions:
       - ["UI5ClientStorage", "sap/ui/core/util/File", ""]
       - ["UI5ClientStorage", "global", "Member[sap].Member[ui].Member[core].Member[util].Member[File]"]
 
-
   - addsTo:
       pack: codeql/javascript-all
       extensible: "sourceModel"

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

@@ -78,6 +78,10 @@ class WebAppManifest extends File {
   WebApp getWebapp() { result = webapp }
 }
 
+predicate inSameWebApp(File f1, File f2) {
+  exists(WebApp webApp | webApp.getAResource() = f1 and webApp.getAResource() = f2)
+}
+
 /** A UI5 bootstrapped web application. */
 class WebApp extends HTML::HtmlFile {
   SapUiCoreScriptElement coreScript;
@@ -368,9 +372,7 @@ class ControlReference extends Reference {
         result.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() = propertyName
       )
     ) and
-    exists(WebApp webApp |
-      webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()
-    )
+    inSameWebApp(this.getFile(), result.getFile())
   }
 
   MethodCallNode getAWrite(string propertyName) {
@@ -400,9 +402,7 @@ class ControlReference extends Reference {
         result.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() = propertyName
       )
     ) and
-    exists(WebApp webApp |
-      webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()
-    )
+    inSameWebApp(this.getFile(), result.getFile())
   }
 }
 
@@ -1437,9 +1437,7 @@ class PropertyMetadata extends ObjectLiteralNode {
       result.getMethodName() = "setProperty" and
       result.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() = name
     ) and
-    exists(WebApp webApp |
-      webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()
-    )
+    inSameWebApp(this.getFile(), result.getFile())
   }
 
   MethodCallNode getARead() {
@@ -1474,8 +1472,6 @@ class PropertyMetadata extends ObjectLiteralNode {
       result.getMethodName() = "getProperty" and
       result.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() = name
     ) and
-    exists(WebApp webApp |
-      webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()
-    )
+    inSameWebApp(this.getFile(), result.getFile())
   }
 }

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

@@ -136,9 +136,7 @@ abstract class UI5BindingPath extends BindingPath {
     )
     // and
     // /* This binding path and the resulting model should live inside the same webapp */
-    // exists(WebApp webApp |
-    //   webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()
-    // )
+    // inSameWebApp(this.getFile(), result.getFile())
   }
 
   /**
@@ -151,10 +149,7 @@ abstract class UI5BindingPath extends BindingPath {
       result.(DataFlow::PropWrite).getPropertyNameExpr() = p.getNameExpr() and
       this.getAbsolutePath() = model.getPathString(p) and
       /* Restrict search to inside the same webapp. */
-      exists(WebApp webApp |
-        webApp.getAResource() = this.getLocation().getFile() and
-        webApp.getAResource() = result.getFile()
-      )
+      inSameWebApp(this.getLocation().getFile(), result.getFile())
     )
     or
     /* 1-2. Internal (Client-side) model, model loaded from JSON file */
@@ -164,19 +159,13 @@ abstract class UI5BindingPath extends BindingPath {
       this.getPath() = model.getPathStringPropName(propName) and
       exists(JsonObject obj, JsonValue val | val = obj.getPropValue(propName)) and
       /* Restrict search to inside the same webapp. */
-      exists(WebApp webApp |
-        webApp.getAResource() = this.getLocation().getFile() and
-        webApp.getAResource() = result.getFile()
-      )
+      inSameWebApp(this.getLocation().getFile(), result.getFile())
     )
     or
     /* 2. External (Server-side) model */
     result = this.getModel().(UI5ExternalModel) and
     /* Restrict search to inside the same webapp. */
-    exists(WebApp webApp |
-      webApp.getAResource() = this.getLocation().getFile() and
-      webApp.getAResource() = result.getFile()
-    )
+    inSameWebApp(this.getLocation().getFile(), result.getFile())
   }
 }
 
@@ -212,9 +201,7 @@ abstract class UI5View extends File {
     /* The controller name should match between the view and the controller definition. */
     result.getName() = this.getControllerName() and
     /* The View and the Controller are in a same webapp. */
-    exists(WebApp webApp |
-      webApp.getAResource() = this and webApp.getAResource() = result.getFile()
-    )
+    inSameWebApp(this, result.getFile())
   }
 
   abstract UI5Control getControl();
@@ -304,10 +291,7 @@ class JsView extends UI5View {
         /* 2. A custom control with implementation code found in the webapp */
         exists(CustomControl control |
           control.getName() = node.asExpr().getAChildExpr().(DotExpr).getQualifiedName() and
-          exists(WebApp webApp |
-            webApp.getAResource() = control.getFile() and
-            webApp.getAResource() = node.getFile()
-          )
+          inSameWebApp(control.getFile(), node.getFile())
         )
       )
     )
@@ -367,10 +351,7 @@ class JsonView extends UI5View {
         /* 2. A custom control with implementation code found in the webapp */
         exists(CustomControl control |
           control.getName() = object.getPropStringValue("Type") and
-          exists(WebApp webApp |
-            webApp.getAResource() = control.getFile() and
-            webApp.getAResource() = object.getFile()
-          )
+          inSameWebApp(control.getFile(), object.getFile())
         )
       )
     )
@@ -516,10 +497,7 @@ class HtmlView extends UI5View, HTML::HtmlFile {
         /* 2. A custom control with implementation code found in the webapp */
         exists(CustomControl control |
           control.getName() = element.getAttributeByName("sap-ui-type").getValue() and
-          exists(WebApp webApp |
-            webApp.getAResource() = control.getFile() and
-            webApp.getAResource() = element.getFile()
-          )
+          inSameWebApp(control.getFile(), element.getFile())
         )
       )
     )
@@ -699,10 +677,7 @@ class XmlView extends UI5View instanceof XmlFile {
         /* 2. A custom control with implementation code found in the webapp */
         exists(CustomControl control |
           control.getName() = element.getNamespace().getUri() + "." + element.getName() and
-          exists(WebApp webApp |
-            webApp.getAResource() = control.getFile() and
-            webApp.getAResource() = element.getFile()
-          )
+          inSameWebApp(control.getFile(), element.getFile())
         )
       )
     )
@@ -801,10 +776,7 @@ class UI5Control extends TUI5Control {
    */
   CustomControl getDefinition() {
     result.getName() = this.getQualifiedType() and
-    exists(WebApp webApp |
-      webApp.getAResource() = this.getFile() and
-      webApp.getAResource() = result.getFile()
-    )
+    inSameWebApp(this.getFile(), result.getFile())
   }
 
   /**
@@ -840,20 +812,14 @@ class UI5Control extends TUI5Control {
   bindingset[propName]
   MethodCallNode getARead(string propName) {
     // TODO: in same view
-    exists(WebApp webApp |
-      webApp.getAResource() = this.getFile() and
-      webApp.getAResource() = result.getFile()
-    ) and
+    inSameWebApp(this.getFile(), result.getFile()) and
     result.getMethodName() = "get" + capitalize(propName)
   }
 
   bindingset[propName]
   MethodCallNode getAWrite(string propName) {
     // TODO: in same view
-    exists(WebApp webApp |
-      webApp.getAResource() = this.getFile() and
-      webApp.getAResource() = result.getFile()
-    ) and
+    inSameWebApp(this.getFile(), result.getFile()) and
     result.getMethodName() = "set" + capitalize(propName)
   }
 

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

@@ -76,6 +76,12 @@ module UI5PathGraph {
       result = this.asUI5BindingPathNode().toString()
     }
 
+    File getFile() {
+      result = this.asDataFlowNode().getFile()
+      or
+      result = this.asUI5BindingPathNode().getView()
+    }
+
     predicate hasLocationInfo(
       string filepath, int startline, int startcolumn, int endline, int endcolumn
     ) {

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

@@ -1,5 +1,6 @@
 import javascript
 import advanced_security.javascript.frameworks.ui5.UI5
+private import semmle.javascript.frameworks.data.internal.ApiGraphModelsExtensions as ApiGraphModelsExtensions
 
 /**
  * Step from a part of internal model to a relevant control property.
@@ -25,7 +26,8 @@ class InternalModelContentToCustomMetadataPropertyStep extends DataFlow::SharedF
 }
 
 /**
- * This is a step in the opposite direction of the `InternalModelContentToCustomMetadataPropertyStep` above. In order to ensure that this indeed holds, we check if the internal model is set to a two-way binding mode.
+ * This is a step in the opposite direction of the `InternalModelContentToCustomMetadataPropertyStep` above.
+ * In order to ensure that this indeed holds, we check if the internal model is set to a two-way binding mode.
  */
 class CustomMetadataPropertyStepToInternalModelContent extends DataFlow::SharedFlowStep {
   override predicate step(DataFlow::Node start, DataFlow::Node end) {
@@ -81,7 +83,8 @@ class CustomMetadataPropertyReadStep extends DataFlow::SharedFlowStep {
 }
 
 /**
- * Step from the second argument of `setProperty` method call on a local model or a reference to that model, that is, the receiver. If the contents of the model is statically visible, then get the relevant portion of the content instead.
+ * Step from the second argument of `setProperty` method call on a local model or a reference to that model, that is, the receiver.
+ * If the contents of the model is statically visible, then get the relevant portion of the content instead.
  *
  * e.g. Given these methods in a same controller,
  *
@@ -96,7 +99,8 @@ class CustomMetadataPropertyReadStep extends DataFlow::SharedFlowStep {
  * }
  * ```
  *
- * Create an edge from `someValue` to `this.getView().getModel()`. As the content of `oModel` can be statically determined, also create an edge from `someValue` to `x: null`.
+ * Create an edge from `someValue` to `this.getView().getModel()`.
+ * As the content of `oModel` can be statically determined, also create an edge from `someValue` to `x: null`.
  */
 class LocalModelSetPropertyStep extends DataFlow::SharedFlowStep {
   override predicate step(DataFlow::Node start, DataFlow::Node end) {
@@ -170,7 +174,9 @@ class LocalModelSetPropertyStep extends DataFlow::SharedFlowStep {
  * }
  * ```
  *
- * Establish an edge from `this.getView().getModel("someModel")` in `someHandler2` to the entire `getProperty` call in `someHandler1`. Note that `modelRefFrom` and `modelRefTo` may refer to the same `ModelReference`.
+ * Establish an edge from `this.getView().getModel("someModel")` in `someHandler2` to
+ * the entire `getProperty` call in `someHandler1`.
+ * Note that `modelRefFrom` and `modelRefTo` may refer to the same `ModelReference`.
  *
  * This is a dual of `LocalModelSetPropertyStep` above.
  */
@@ -232,7 +238,8 @@ class LocalModelGetPropertyStep extends DataFlow::SharedFlowStep {
 }
 
 /**
- * Step from a local model or a reference to it, to a `getProperty` method call on it. This assumes a corresponding `setProperty` call exists on the same model and its same property.
+ * Step from a local model or a reference to it, to a `getProperty` method call on it.
+ * This assumes a corresponding `setProperty` call exists on the same model and its same property.
  *
  * e.g. Given these methods in a same controller,
  * ```javascript
@@ -250,7 +257,9 @@ class LocalModelGetPropertyStep extends DataFlow::SharedFlowStep {
  * }
  * ```
  *
- * Establish an edge from `this.getView().getModel("someModel")` in `someHandler2` to the entire `getProperty` call in `someHandler1`. Note that `modelRefFrom` and `modelRefTo` may refer to the same `ModelReference`.
+ * Establish an edge from `this.getView().getModel("someModel")` in `someHandler2` to
+ * the entire `getProperty` call in `someHandler1`.
+ * Note that `modelRefFrom` and `modelRefTo` may refer to the same `ModelReference`.
  */
 /*
  * TODO:
@@ -334,3 +343,19 @@ class ResourceBundleGetTextCallArgToReturnValueStep extends DataFlow::SharedFlow
     )
   }
 }
+
+/**
+ * A step from any argument of a SAP logging function to the `onLogEntry`
+ * method of a custom log listener in the same application.
+ */
+class LogArgumentToListener extends DataFlow::SharedFlowStep {
+  override predicate step(DataFlow::Node start, DataFlow::Node end) {
+    inSameWebApp(start.getFile(), end.getFile()) and
+    start =
+      ModelOutput::getATypeNode("SapLogger")
+          .getMember(["debug", "error", "fatal", "info", "trace", "warning"])
+          .getACall()
+          .getAnArgument() and
+    end = ModelOutput::getATypeNode("SapLogEntries").asSource()
+  }
+}

javascript/frameworks/ui5/src/UI5Clickjacking/UI5Clickjacking.ql

@@ -29,7 +29,9 @@ newtype TAlertLocation =
 class AlertLocation extends TAlertLocation {
   FrameOptions asFrameOptions() { this = TFrameOptions(result) }
 
-  FirstLineOfDocumentElementWebApp asFirstLineOfDocumentElementWebApp() { this = TFirstLineOfDocumentElementWebApp(result) }
+  FirstLineOfDocumentElementWebApp asFirstLineOfDocumentElementWebApp() {
+    this = TFirstLineOfDocumentElementWebApp(result)
+  }
 
   string toString() {
     result = this.asFrameOptions().toString() or
@@ -47,7 +49,7 @@ from AlertLocation alertLocation, string message
 where
   exists(WebApp app |
     exists(FrameOptions frameOptions | app.getFrameOptions() = frameOptions |
-    frameOptions.allowsAllOriginEmbedding() and
+      frameOptions.allowsAllOriginEmbedding() and
       alertLocation.asFrameOptions() = frameOptions and
       message =
         "Possible clickjacking vulnerability due to " + frameOptions.toString() +

javascript/frameworks/ui5/src/UI5LogInjection/UI5LogInjection.ql

@@ -1,10 +1,10 @@
 /**
- * @name UI5 Log injection
+ * @name UI5 client-side Log injection
  * @description Building log entries from user-controlled sources is vulnerable to
  *              insertion of forged log entries by a malicious user.
  * @kind path-problem
- * @problem.severity error
- * @security-severity 7.8
+ * @problem.severity recommendation
+ * @security-severity 3.5
  * @precision medium
  * @id js/ui5-log-injection
  * @tags security

javascript/frameworks/ui5/src/UI5LogInjection/UI5LogsToHttp.ql

@@ -0,0 +1,36 @@
+/**
+ * @name UI5 Log injection in outbound network request
+ * @description Building log entries from user-controlled sources is vulnerable to
+ *              insertion of forged log entries by a malicious user.
+ * @kind path-problem
+ * @problem.severity warning
+ * @security-severity 6.5
+ * @precision medium
+ * @id js/ui5-log-injection-to-http
+ * @tags security
+ *       external/cwe/cwe-117
+ */
+
+import javascript
+import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow
+import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow::UI5PathGraph
+import semmle.javascript.security.dataflow.LogInjectionQuery as LogInjection
+
+class UI5LogInjectionConfiguration extends LogInjection::LogInjectionConfiguration {
+  override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node node) {
+    exists(ClientRequest req |
+      node = req.getUrl() or
+      node = req.getADataNode()
+    )
+  }
+}
+
+from
+  UI5LogInjectionConfiguration cfg, UI5PathNode source, UI5PathNode sink, UI5PathNode primarySource
+where
+  cfg.hasFlowPath(source.getPathNode(), sink.getPathNode()) and
+  primarySource = source.getAPrimarySource()
+select sink, primarySource, sink, "Outbound network request depends on $@ log data.", primarySource,
+  "user-provided"