Address deprecation of PathExpr and port ZipSlipQuery

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

@@ -4,6 +4,7 @@ import advanced_security.javascript.frameworks.ui5.JsonParser
 import semmle.javascript.security.dataflow.DomBasedXssCustomizations
 import advanced_security.javascript.frameworks.ui5.UI5View
 import advanced_security.javascript.frameworks.ui5.UI5HTML
+import codeql.util.FileSystem
 
 private module WebAppResourceRootJsonReader implements JsonParser::MakeJsonReaderSig<WebApp> {
   class JsonReader extends WebApp {
@@ -33,12 +34,13 @@ private predicate isAnUnResolvedResourceRoot(WebApp webApp, string name, string
   )
 }
 
-class ResourceRootPathString extends PathString {
-  WebApp webApp;
-
-  ResourceRootPathString() { isAnUnResolvedResourceRoot(webApp, _, this) }
-
-  override Folder getARootFolder() { result = webApp.getWebAppFolder() }
+private module UI5WebAppResolverConfig implements Folder::ResolveSig {
+  predicate shouldResolve(Container f, string relativePath) {
+    exists(WebApp webApp |
+      f = webApp.getWebAppFolder() and
+      isAnUnResolvedResourceRoot(webApp, _, relativePath)
+    )
+  }
 }
 
 class ResourceRoot extends Container {
@@ -48,7 +50,7 @@ class ResourceRoot extends Container {
 
   ResourceRoot() {
     isAnUnResolvedResourceRoot(webApp, name, path) and
-    path.(PathString).resolve(webApp.getWebAppFolder()).getContainer() = this
+    Folder::Resolve<UI5WebAppResolverConfig>::resolve(webApp.getWebAppFolder(), path) = this
   }
 
   string getName() { result = name }
@@ -168,7 +170,9 @@ class SapDefineModule extends AmdModuleDefinition::Range, MethodCallExpr, UserMo
     )
   }
 
-  string getDependency(int i) { result = this.(AmdModuleDefinition).getDependencyExpr(i).getStringValue() }
+  string getDependency(int i) {
+    result = this.(AmdModuleDefinition).getDependencyExpr(i).getStringValue()
+  }
 
   override string getADependency() { result = this.getDependency(_) }
 

javascript/frameworks/xsjs/ext/xsjs.model.yml

@@ -35,19 +35,14 @@ extensions:
       extensible: sourceModel
     data:
       # ========== 1. Retrieving Web Request Body ==========
-      - [WebRequestBody, "Member[asArrayBuffer].ReturnValue", remote]
-      - [WebRequestBody, "Member[asString].ReturnValue", remote]
-      - [WebRequestBody, "Member[asWebRequest].ReturnValue", remote]
+      - [WebRequestBody, "", remote]
 
       # ========== 2. Retrieving Web Request Parameter Value ==========
       - [WebRequestParameters, "Member[get].ReturnValue", remote]
       - [WebRequestParameters, AnyMember, remote]
 
       # ========== 3. Receiving Response through HTTPClient ==========
       - [HTTPClient, "InboundResponse.Member[body]", remote]
-      - [HTTPClient, "InboundResponse.Member[body].Member[asArrayBuffer].ReturnValue", remote]
-      - [HTTPClient, "InboundResponse.Member[body].Member[asString].ReturnValue", remote]
-      - [HTTPClient, "InboundResponse.Member[body].Member[asWebRequest].ReturnValue", remote]
       - [HTTPClient, "InboundResponse.Member[cacheControl]", remote]
       - [HTTPClient, "InboundResponse.Member[contentType]", remote]
       - [HTTPClient, "InboundResponse.Member[cookies]", remote]
@@ -60,12 +55,15 @@ extensions:
       extensible: sinkModel
     data:
       - [WebResponse, "Member[setBody].Argument[0]", html-injection]
-      # - [Mail, "Member[send].Argument[this]", "???"]
-      # - [SMTPConnection, "Member[send].Argument[0]", "???"]
-      # - ["HTTPClient", "Member[request].Argument[0]", "???"]
 
   - addsTo:
       pack: codeql/javascript-all
       extensible: summaryModel
     data:
-      - [global, "Member[JSON].Member[parse]", "Argument[0]", "ReturnValue", taint]
+      - [global, "Member[JSON].Member[parse]", "Argument[0]", "ReturnValue", taint]
+      - [WebRequestBody, "Member[asArrayBuffer]", "Argument[this]", "ReturnValue", taint]
+      - [WebRequestBody, "Member[asString]", "Argument[this]", "ReturnValue", taint]
+      - [WebRequestBody, "Member[asWebRequest]", "Argument[this]", "ReturnValue", taint]
+      - [InboundResponse, "Member[body].Member[asArrayBuffer]", "Argument[this]", "ReturnValue", taint]
+      - [InboundResponse, "Member[body].Member[asString]", "Argument[this]", "ReturnValue", taint]
+      - [InboundResponse, "Member[body].Member[asWebRequest]", "Argument[this]", "ReturnValue", taint]

javascript/frameworks/xsjs/lib/advanced_security/javascript/frameworks/xsjs/XSJSZipSlipQuery.qll

@@ -4,45 +4,15 @@ import semmle.javascript.security.dataflow.ZipSlipQuery as ZipSlip
 import semmle.javascript.security.dataflow.TaintedPathCustomizations::TaintedPath as TaintedPath
 
 /**
- * An instance of `$.util.Zip`, but the argument to the constructor call is reachable from a remote flow source.
- */
-class XSJSZipInstanceDependingOnRemoteFlowSource extends XSJSZipInstance {
-  RemoteFlowSource remoteArgument;
-
-  XSJSZipInstanceDependingOnRemoteFlowSource() {
-    this.getAnArgument().getALocalSource() = remoteArgument
-  }
-
-  RemoteFlowSource getRemoteArgument() { result = remoteArgument }
-}
-
-class XSJSRemoteFlowSourceToZipInstanceStep extends DataFlow::SharedFlowStep {
-  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-    exists(XSJSZipInstanceDependingOnRemoteFlowSource dollarUtilZip |
-      pred = dollarUtilZip.getRemoteArgument() and
-      succ = dollarUtilZip
-    )
-  }
-}
-
-class ForInLoopDomainToVariableStep extends DataFlow::SharedFlowStep {
-  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-    exists(ForInStmt forLoop |
-      pred = forLoop.getIterationDomain().flow() and
-      succ = forLoop.getAnIterationVariable().getAnAccess().flow()
-    )
-  }
-}
-
-/**
- * A node that checks if the path that is being extracted is indeed the prefix of the entry, e.g.
+ * A node that checks if the path that is being extracted is indeed the prefix of the entry.
+ * e.g.
  * ``` javascript
  * if (entryPath.indexOf("SomePrefix") === 0) {
  *   // extract the file with the path.
  * }
  * ```
  */
-class ZipEntryPathIndexOfCallEqualsZeroGuard extends TaintTracking::SanitizerGuardNode {
+class ZipEntryPathIndexOfCallEqualsZeroGuard extends DataFlow::Node {
   EqualityTest equalityTest;
   MethodCallNode indexOfCall;
   ForInStmt forLoop;
@@ -55,53 +25,69 @@ class ZipEntryPathIndexOfCallEqualsZeroGuard extends TaintTracking::SanitizerGua
     equalityTest.getRightOperand().getIntValue() = 0
   }
 
-  override predicate sanitizes(boolean outcome, Expr receiver) {
+  predicate blocksExpr(boolean outcome, Expr receiver) {
     exists(DataFlow::Node targetFilePath, DataFlow::Node forLoopVariable |
       receiver = targetFilePath.asExpr() and
       targetFilePath = indexOfCall.getReceiver() and
       forLoopVariable = forLoop.getAnIterationVariable().getAnAccess().flow() and
-      TaintedPath::isAdditionalTaintedPathFlowStep(forLoopVariable,
-        targetFilePath.getALocalSource(), _, _) and
+      TaintedPath::isAdditionalFlowStep(forLoopVariable, _, targetFilePath.getALocalSource(), _) and
       outcome = equalityTest.getPolarity()
     )
   }
 }
 
-/**
- * A class wraps `TaintedPath::BarrierGuardNode` by delegating its `sanitizes/0` to the `blocks/0` predicate.
- * The characteristic predicate of this class is deliberately left out.
- */
-class TaintedPathSanitizerGuard extends TaintTracking::SanitizerGuardNode {
-  TaintedPathSanitizerGuard() { this = this }
-
-  override predicate sanitizes(boolean outcome, Expr receiver) {
-    exists(TaintedPath::BarrierGuard node | node.blocksExpr(outcome, receiver))
+module XSJSZipSlip implements DataFlow::StateConfigSig {
+  class FlowState extends string {
+    FlowState() { this in ["$.util.Zip uninitialized", "$.util.Zip initialized"] }
   }
-}
 
-class Configuration extends TaintTracking::Configuration {
-  Configuration() { this = "XSJS Zip Slip Query" }
+  predicate isSource(DataFlow::Node node, FlowState state) {
+    node instanceof RemoteFlowSource and
+    state = "$.util.Zip uninitialized"
+  }
 
-  override predicate isSource(DataFlow::Node start) {
-    super.isSource(start)
-    or
-    exists(XSJSZipInstanceDependingOnRemoteFlowSource dollarUtilZip |
-      start = dollarUtilZip.getRemoteArgument()
-    )
+  predicate isSink(DataFlow::Node node, FlowState state) {
+    node instanceof ZipSlip::Sink and
+    state = "$.util.Zip initialized"
   }
 
-  override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node dst) {
-    TaintedPath::isAdditionalTaintedPathFlowStep(src, dst, _, _)
+  predicate isBarrier(DataFlow::Node node, FlowState state) {
+    (
+      node = DataFlow::MakeBarrierGuard<ZipEntryPathIndexOfCallEqualsZeroGuard>::getABarrierNode()
+      or
+      node = DataFlow::MakeBarrierGuard<TaintedPath::BarrierGuard>::getABarrierNode()
+    ) and
+    state = state
   }
 
-  override predicate isSink(DataFlow::Node end) {
-    super.isSink(end)
+  predicate isAdditionalFlowStep(
+    DataFlow::Node start, FlowState preState, DataFlow::Node end, FlowState postState
+  ) {
+    /* 1. `$.util.Zip` initialized */
+    start = start and
+    preState = "$.util.Zip uninitialized" and
+    end instanceof XSJSZipInstance and
+    // start.getBasicBlock().getASuccessor+() = end.getBasicBlock() and
+    postState = "$.util.Zip initialized"
     or
-    end instanceof ZipSlip::Sink
-  }
+    /*
+     * 2. Jump from a domain of a for-in statement to an access of the iteration variable.
+     * e.g.
+     * ``` javascript
+     * for (var x in y) {
+     *   var z = x;
+     * }
+     * ```
+     * This step jumps from `y` to `x` in the body of the for-in loop.
+     */
 
-  override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode node) {
-    node instanceof ZipEntryPathIndexOfCallEqualsZeroGuard or
-    node instanceof TaintedPathSanitizerGuard
+    exists(ForInStmt forLoop |
+      start = forLoop.getIterationDomain().flow() and
+      end = forLoop.getAnIterationVariable().getAnAccess().flow() and
+      preState = postState
+    )
+    or
+    TaintedPath::isAdditionalFlowStep(start, _, end, _) and
+    preState = postState
   }
 }

javascript/frameworks/xsjs/src/XSJSZipSlip/XSJSZipSlip.ql

@@ -12,8 +12,13 @@
 
 import javascript
 import advanced_security.javascript.frameworks.xsjs.XSJSZipSlipQuery
-import DataFlow::PathGraph
+import semmle.javascript.frameworks.data.ModelsAsData
 
-from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
-where config.hasFlowPath(source, sink)
-select sink, source, sink, "The path of $@ being saved depends on a $@.", sink, "this zip file", source, "user-provided value"
+module XSJSZipSlipFlow = DataFlow::GlobalWithState<XSJSZipSlip>;
+
+import XSJSZipSlipFlow::PathGraph
+
+from XSJSZipSlipFlow::PathNode source, XSJSZipSlipFlow::PathNode sink
+where XSJSZipSlipFlow::flowPath(source, sink)
+select sink, source, sink, "The path of $@ being saved depends on a $@.", sink, "this zip file",
+  source, "user-provided value"

javascript/frameworks/xsjs/test/models/source/source.expected

@@ -1,15 +1,7 @@
-| source.xsjs:42:24:42:54 | webRequ ... uffer() | Remote flow source of type: Remote flow |
-| source.xsjs:42:24:42:54 | webRequ ... uffer() | Remote flow source of type: Source node (remote) [from data-extension] |
-| source.xsjs:43:24:43:49 | webRequ ... tring() | Remote flow source of type: Remote flow |
-| source.xsjs:43:24:43:49 | webRequ ... tring() | Remote flow source of type: Source node (remote) [from data-extension] |
-| source.xsjs:44:24:44:53 | webRequ ... quest() | Remote flow source of type: Remote flow |
-| source.xsjs:44:24:44:53 | webRequ ... quest() | Remote flow source of type: Source node (remote) [from data-extension] |
-| source.xsjs:46:24:46:54 | webRequ ... uffer() | Remote flow source of type: Remote flow |
-| source.xsjs:46:24:46:54 | webRequ ... uffer() | Remote flow source of type: Source node (remote) [from data-extension] |
-| source.xsjs:47:24:47:49 | webRequ ... tring() | Remote flow source of type: Remote flow |
-| source.xsjs:47:24:47:49 | webRequ ... tring() | Remote flow source of type: Source node (remote) [from data-extension] |
-| source.xsjs:48:24:48:53 | webRequ ... quest() | Remote flow source of type: Remote flow |
-| source.xsjs:48:24:48:53 | webRequ ... quest() | Remote flow source of type: Source node (remote) [from data-extension] |
+| source.xsjs:8:23:8:38 | webRequest1.body | Remote flow source of type: Remote flow |
+| source.xsjs:8:23:8:38 | webRequest1.body | Remote flow source of type: Source node (remote) [from data-extension] |
+| source.xsjs:9:23:9:38 | webRequest2.body | Remote flow source of type: Remote flow |
+| source.xsjs:9:23:9:38 | webRequest2.body | Remote flow source of type: Source node (remote) [from data-extension] |
 | source.xsjs:52:25:52:44 | webRequestParam1.get | Remote flow source of type: Remote flow |
 | source.xsjs:52:25:52:44 | webRequestParam1.get | Remote flow source of type: Source node (remote) [from data-extension] |
 | source.xsjs:52:25:52:51 | webRequ ... ("key") | Remote flow source of type: Remote flow |