Address incorrect association between frame options and web app

Author: rvermeulen

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

@@ -3,6 +3,7 @@ private import DataFlow
 private import advanced_security.javascript.frameworks.ui5.JsonParser
 private import semmle.javascript.security.dataflow.DomBasedXssCustomizations
 private import advanced_security.javascript.frameworks.ui5.UI5View
+private import advanced_security.javascript.frameworks.ui5.UI5HTML
 
 module UI5 {
   bindingset[this]
@@ -126,6 +127,18 @@ module UI5 {
         result.getAbsolutePath() = resolvedModulePath + ".js"
       )
     }
+
+    FrameOptions getFrameOptions() {
+      exists(HTML::DocumentElement doc | doc.getFile() = this |
+        result.asHtmlFrameOptions() = coreScript.getAnAttribute()
+      )
+      or
+      result.asJsFrameOptions().getFile() = this
+    }
+
+    HTML::DocumentElement getDocument() {
+      result.getFile() = this
+    }
   }
 
   /**

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

@@ -85,10 +85,10 @@ class FrameOptions extends TFrameOptions {
 }
 
 /**
- * Holds if the frame options are left untouched as the default value `trusted`.
+ * Holds if the specified frame options do not prevent click jacking.
  */
-predicate thereIsNoFrameOptionSet(UI5::WebApp webapp) {
-  not exists(FrameOptions frameOptions | webapp.getAResource() = frameOptions.getLocation().getFile() |
+predicate isMissingFrameOptionsToPreventClickjacking(UI5::WebApp webapp) {
+  not exists(FrameOptions frameOptions | webapp.getFrameOptions() = frameOptions |
     frameOptions.allowsSharedOriginEmbedding() or
     frameOptions.deniesEmbedding() or
     frameOptions.allowsAllOriginEmbedding()

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

@@ -15,44 +15,46 @@ import advanced_security.javascript.frameworks.ui5.UI5HTML
 import semmle.javascript.RestrictedLocations
 private import advanced_security.javascript.frameworks.ui5.UI5
 
-class FirstLineOfMainHtml extends HTML::DocumentElement, FirstLineOf {
-  FirstLineOfMainHtml() {
-    exists(UI5::WebApp app | this.getFile().(FirstLineOf).getFile() = app)
+class FirstLineOfDocumentElementWebApp extends HTML::DocumentElement, FirstLineOf {
+  FirstLineOfDocumentElementWebApp() {
+    exists(UI5::WebApp app | app.getDocument() = this)
   }
 }
 
 newtype TAlertLocation =
   TFrameOptions(FrameOptions frameOptions) or
-  TFirstLineOfMainHtml(FirstLineOfMainHtml htmlStartTag)
+  TFirstLineOfDocumentElementWebApp(FirstLineOfDocumentElementWebApp htmlStartTag)
 
 class AlertLocation extends TAlertLocation {
   FrameOptions asFrameOptions() { this = TFrameOptions(result) }
 
-  FirstLineOfMainHtml asFirstLineOfMainHtml() { this = TFirstLineOfMainHtml(result) }
+  FirstLineOfDocumentElementWebApp asFirstLineOfDocumentElementWebApp() { this = TFirstLineOfDocumentElementWebApp(result) }
 
   string toString() {
     result = this.asFrameOptions().toString() or
-    result = this.asFirstLineOfMainHtml().toString()
+    result = this.asFirstLineOfDocumentElementWebApp().toString()
   }
 
   predicate hasLocationInfo(string path, int sl, int sc, int el, int ec) {
     this.asFrameOptions().getLocation().hasLocationInfo(path, sl, sc, el, ec)
     or
-    this.asFirstLineOfMainHtml().hasLocationInfo(path, sl, sc, el, ec)
+    this.asFirstLineOfDocumentElementWebApp().hasLocationInfo(path, sl, sc, el, ec)
   }
 }
 
-from AlertLocation alert, string message
+from AlertLocation alertLocation, string message
 where
-  exists(FrameOptions frameOptions | frameOptions.allowsAllOriginEmbedding() |
-    alert.asFrameOptions() = frameOptions and
-    message =
-      "Possible clickjacking vulnerability due to " + frameOptions.toString() +
-        " being set to `allow`."
-  )
-  or
-  exists(UI5::WebApp app | thereIsNoFrameOptionSet(app) |
-    alert.asFirstLineOfMainHtml().getFile() = app and
+  exists(UI5::WebApp app |
+    exists(FrameOptions frameOptions | app.getFrameOptions() = frameOptions |
+    frameOptions.allowsAllOriginEmbedding() and
+      alertLocation.asFrameOptions() = frameOptions and
+      message =
+        "Possible clickjacking vulnerability due to " + frameOptions.toString() +
+          " being set to `allow`."
+    )
+    or
+    isMissingFrameOptionsToPreventClickjacking(app) and
+    alertLocation.asFirstLineOfDocumentElementWebApp() = app.getDocument() and
     message = "Possible clickjacking vulnerability due to missing frame options."
   )
-select alert, message
+select alertLocation, message