Merge pull request #257 from advanced-security/mbaluda/sanitize-content

Ensure sanitizeContent attribute is respected

Author: jeongsoolee09

.github/workflows/code_scanning.yml

@@ -42,7 +42,7 @@ jobs:
             
     - name: Initialize CodeQL
       id: initialize-codeql
-      uses: github/codeql-action/init@v3
+      uses: github/codeql-action/init@v4
       env:
         # Add our custom extractor to the CodeQL search path
         CODEQL_ACTION_EXTRA_OPTIONS: '{"database":{"init":["--search-path","${{ github.workspace }}/extractors"]}}'
@@ -62,7 +62,7 @@ jobs:
 
     - name: Perform CodeQL Analysis
       id: analyze
-      uses: github/codeql-action/analyze@v3
+      uses: github/codeql-action/analyze@v4
       env:
         LGTM_INDEX_XML_MODE: all
         LGTM_INDEX_FILETYPES: ".json:JSON"

.github/workflows/javascript.sarif.expected

@@ -93,6 +93,8 @@ extensions:
       - ["UI5HTMLControl", "Argument[0].Member[content]", "ui5-html-injection"]
       - ["UI5HTMLControl", "Member[content]", "ui5-html-injection"]
       - ["UI5HTMLControl", "Member[setContent].Argument[0]", "ui5-html-injection"]
+      - ["sap/ui/richtexteditor/RichTextEditor", "Member[value]", "ui5-html-injection"]
+      - ["sap/ui/richtexteditor/RichTextEditor", "Member[setValue].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"]

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

@@ -1,5 +1,3 @@
-import javascript
-import DataFlow
 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
@@ -648,16 +646,7 @@ class XmlView extends UI5View instanceof XmlFile {
       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"
-        )
-      )
+      result.getBindingTarget() = control.getAttribute(property)
     )
   }
 
@@ -823,7 +812,7 @@ class UI5Control extends TUI5Control {
   }
 
   /** Holds if this control reads from or writes to a model. */
-  predicate accessesModel(UI5Model model) { accessesModel(model, _) }
+  predicate accessesModel(UI5Model model) { this.accessesModel(model, _) }
 
   /** Holds if this control reads from or writes to a model with regards to a binding path. */
   predicate accessesModel(UI5Model model, XmlBindingPath bindingPath) {
@@ -842,6 +831,43 @@ class UI5Control extends TUI5Control {
 
   /** Get the controller that manages this control. */
   CustomController getController() { result = this.getView().getController() }
+
+  /**
+   * Gets the full import path of the associated control.
+   */
+  string getControlTypeName() { result = this.getQualifiedType().replaceAll(".", "/") }
+
+  /**
+   * Holds if the control content is sanitized for HTML
+   * 'sap/ui/core/HTML' sanitized using the property 'sanitizeContent'
+   * 'sap/ui/richttexteditor/RichTextEditor' sanitized using the property 'sanitizeValue'
+   */
+  predicate isHTMLSanitized() {
+    this.getControlTypeName() = "sap/ui/richtexteditor/RichTextEditor" and
+    not this.isSanitizePropertySetTo("sanitizeValue", false)
+    or
+    this.getControlTypeName() = "sap/ui/core/HTML" and
+    this.isSanitizePropertySetTo("sanitizeContent", true) and
+    not this.isSanitizePropertySetTo("sanitizeContent", false)
+  }
+
+  bindingset[propName, val]
+  private predicate isSanitizePropertySetTo(string propName, boolean val) {
+    /* 1. `sanitizeContent` attribute is set declaratively. */
+    this.getProperty(propName).toString() = val.toString()
+    or
+    /* 2. `sanitizeContent` attribute is set programmatically using setProperty(). */
+    exists(CallNode node | node = this.getAReference().getAMemberCall("setProperty") |
+      node.getArgument(0).getStringValue() = propName and
+      not node.getArgument(1).mayHaveBooleanValue(val.booleanNot())
+    )
+    or
+    /* 3. `sanitizeContent` attribute is set programmatically using a setter. */
+    exists(CallNode node |
+      node = this.getAReference().getAMemberCall("setS" + propName.suffix(1)) and
+      not node.getArgument(0).mayHaveBooleanValue(val.booleanNot())
+    )
+  }
 }
 
 private newtype TUI5ControlProperty =
@@ -857,7 +883,7 @@ class UI5ControlProperty extends TUI5ControlProperty {
   ValueNode asJsControlProperty() { this = TJsControlProperty(result) }
 
   string toString() {
-    result = this.asXmlControlProperty().toString() or
+    result = this.asXmlControlProperty().getValue().toString() or
     result = this.asJsonControlProperty().toString() or
     result = this.asJsControlProperty().toString()
   }

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

@@ -1,7 +1,6 @@
 import javascript
 import semmle.javascript.dataflow.DataFlow as StdLibDataFlow
 import advanced_security.javascript.frameworks.ui5.UI5
-import advanced_security.javascript.frameworks.ui5.UI5View
 import advanced_security.javascript.frameworks.ui5.RemoteFlowSources
 import advanced_security.javascript.frameworks.ui5.dataflow.FlowSteps
 private import PatchDataFlow
@@ -107,6 +106,7 @@ module UI5PathGraph<PathNodeSig ConfigPathNode, PathGraphSig<ConfigPathNode> Con
     }
 
     UI5PathNode getAPrimaryHtmlISink() {
+      not result.asUI5BindingPathNode().getControlDeclaration().isHTMLSanitized() and
       if
         this.asDataFlowNode() instanceof LocalModelContentBoundBidirectionallyToHtmlISinkControl or
         this.asDataFlowNode() instanceof UI5ExternalModel // TODO: Narrow it down to ExternalModelBoundToHtmlISinkControl

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

@@ -42,6 +42,8 @@ User input flows to XSS sinks via event handlers in 4 different ways:
 
 ### [xss-html-control](queries/UI5Xss/xss-html-control)
 - `sap.ui.core.HTML` Control
+- sanitization using the `sanitizeContent` property
+- sanitization disabled by programmatically setting the `sanitizeContent` property to false 
 
 ### [xss-html-control-df](queries/UI5Xss/xss-html-control-df)
 - `sap.ui.core.HTML` Control
@@ -57,15 +59,17 @@ User input flows to XSS sinks via event handlers in 4 different ways:
 
 ### [xss-html-view](queries/UI5Xss/xss-html-view)
 - `sap.ui.core.mvc.HTMLView` View
-- 
+
 ### [xss-indirect-control](queries/UI5Xss/xss-indirect-control)
 - control accessed indirectly
 
 ### [xss-js-view](queries/UI5Xss/xss-js-view)
 - `sap.ui.core.mvc.JSView` View
+- sanitization using the `sanitizeContent` property
 
 ### [xss-json-view](queries/UI5Xss/xss-json-view)
 - `sap.ui.core.mvc.JSONView` View
+- sanitization using the `sanitizeContent` property
 
 ### [xss-separate-renderer](queries/UI5Xss/xss-separate-renderer)
 - `renderer` property is set to a class name (a string)

javascript/frameworks/ui5/test/README.md

@@ -1 +1,3 @@
-| sink1.xml:5:5:5:44 | content={path: '/input'} | The binding path `content={path: '/input'}` is an HTML injection sink. |
+| sink1.xml:7:5:7:68 | content={path: '/input'} | The binding path `content={path: '/input'}` is an HTML injection sink. |
+| sink1.xml:8:5:8:44 | content={path: '/input'} | The binding path `content={path: '/input'}` is an HTML injection sink. |
+| sink1.xml:10:5:10:73 | value={path: '/input'} | The binding path `value={path: '/input'}` is an HTML injection sink. |

javascript/frameworks/ui5/test/models/sink/UI5ViewSinkTest.expected

@@ -9,5 +9,7 @@ import javascript
 import advanced_security.javascript.frameworks.ui5.UI5View
 
 from UI5BindingPath bp
-where bp = any(UI5View ui5v).getAnHtmlISink()
+where
+  bp = any(UI5View ui5v).getAnHtmlISink() and
+  not bp.getControlDeclaration().isHTMLSanitized()
 select bp, "The binding path `" + bp.toString() + "` is an HTML injection sink."

javascript/frameworks/ui5/test/models/sink/UI5ViewSinkTest.ql

@@ -1,12 +1,12 @@
-| sink.js:24:38:24:42 | code0 | SAP UI5 log injection sink with kind: ui5-log-injection |
-| sink.js:24:45:24:49 | code1 | SAP UI5 log injection sink with kind: ui5-log-injection |
-| sink.js:24:52:24:56 | code2 | SAP UI5 log injection sink with kind: ui5-log-injection |
-| sink.js:25:38:25:42 | code0 | SAP UI5 log injection sink with kind: ui5-log-injection |
-| sink.js:25:45:25:49 | code1 | SAP UI5 log injection sink with kind: ui5-log-injection |
-| sink.js:25:52:25:56 | code2 | SAP UI5 log injection sink with kind: ui5-log-injection |
-| sink.js:27:40:27:44 | code0 | SAP UI5 log injection sink with kind: ui5-log-injection |
-| sink.js:27:47:27:51 | code1 | SAP UI5 log injection sink with kind: ui5-log-injection |
-| sink.js:27:54:27:58 | code2 | SAP UI5 log injection sink with kind: ui5-log-injection |
+| sink.js:26:38:26:42 | code0 | SAP UI5 log injection sink with kind: ui5-log-injection |
+| sink.js:26:45:26:49 | code1 | SAP UI5 log injection sink with kind: ui5-log-injection |
+| sink.js:26:52:26:56 | code2 | SAP UI5 log injection sink with kind: ui5-log-injection |
+| sink.js:27:38:27:42 | code0 | SAP UI5 log injection sink with kind: ui5-log-injection |
+| sink.js:27:45:27:49 | code1 | SAP UI5 log injection sink with kind: ui5-log-injection |
+| sink.js:27:52:27:56 | code2 | SAP UI5 log injection sink with kind: ui5-log-injection |
+| sink.js:28:40:28:44 | code0 | SAP UI5 log injection sink with kind: ui5-log-injection |
+| sink.js:28:47:28:51 | code1 | SAP UI5 log injection sink with kind: ui5-log-injection |
+| sink.js:28:54:28:58 | code2 | SAP UI5 log injection sink with kind: ui5-log-injection |
 | sink.js:29:37:29:41 | code0 | SAP UI5 log injection sink with kind: ui5-log-injection |
 | sink.js:29:44:29:48 | code1 | SAP UI5 log injection sink with kind: ui5-log-injection |
 | sink.js:29:51:29:55 | code2 | SAP UI5 log injection sink with kind: ui5-log-injection |

javascript/frameworks/ui5/test/models/sink/logSinkTest.expected

@@ -8,7 +8,8 @@ sap.ui.define(
     "sap/base/util/Properties",
     "sap/ui/core/RenderManager",
     "sap/ui/util/Storage",
-    "sap/ui/core/util/File"
+    "sap/ui/core/util/File",
+    "sap/ui/richtexteditor/RichTextEditor"
   ],
   function (
     LoaderExtensions,
@@ -19,13 +20,12 @@ sap.ui.define(
     Properties,
     RenderManager,
     Storage,
-    File
+    File,
+    RichTextEditor
   ) {
     var value = jQuery.sap.log.fatal(code0, code1, code2);
     var value = jQuery.sap.log.error(code0, code1, code2);
-
     var value = jQuery.sap.log.warning(code0, code1, code2);
-
     var value = jQuery.sap.log.info(code0, code1, code2);
 
     var value = jQuery.sap.log.debug(code0, code1, code2);
@@ -94,7 +94,7 @@ sap.ui.define(
     var value = obj.registerResourcePath(code0, code1);
     var obj = new Properties();
     var value = obj.create(code0);
-    var obj = new HTML({content: code0});
+    var obj = new HTML({ content: code0 }); // UNSAFE: Content is not sanitized
     obj.content = code0;
     obj.setContent(code0);
     var obj = new Patcher();
@@ -125,5 +125,12 @@ sap.ui.define(
     var value = sap.ui.core.util.File.save(code0, code1, "csv", "text/csv", code4, code5);
     var value = sap.ui.core.util.File.save(code0, code1, "csv", "text/plain", code4, code5);
     var value = sap.ui.core.util.File.save(code0, code1, code2, code3, code4, code5);
+
+    var obj = new HTML({ content: code0, sanitizeContent: true }); // SAFE: Content is sanitized
+    var obj = new HTML({ content: code0, sanitizeContent: false }); // UNSAFE: Content is explicitly not sanitized
+
+    var obj = new RichTextEditor({ value: code0 }); // SAFE: Content is sanitized by default
+    var obj = new RichTextEditor({ value: code0, sanitizeValue: true }); // SAFE: Content is sanitized
+    var obj = new RichTextEditor({ value: code0, sanitizeValue: false }); // UNSAFE: Content is explicitly not sanitized
   },
 );