Java: remove path-injection related models and tests for now

Author: Jami Cogswell

java/ql/lib/ext/unsafeUrlForwardExperimentalMove.model.yml

@@ -6,56 +6,18 @@ extensions:
       - ["jakarta.servlet.http", "HttpServletRequest", True, "getServletPath", "", "", "ReturnValue", "remote", "manual"]
       - ["javax.servlet.http", "HttpServletRequest", True, "getServletPath", "", "", "ReturnValue", "remote", "manual"]
 
-      # # ! below added by me when debugging CVEs:
-      # - ["org.springframework.cloud.config.server.resource", "ResourceController", True, "retrieve", "(String,String,String,ServletWebRequest,boolean)", "", "Parameter[3]", "remote", "manual"]
-      # - ["org.springframework.web.context.request", "ServletWebRequest", True, "getContextPath", "()", "", "ReturnValue", "remote", "manual"]
-
   - addsTo:
       pack: codeql/java-all
       extensible: sinkModel
     data:
       - ["java.util.concurrent", "TimeUnit", True, "sleep", "", "", "Argument[0]", "thread-pause", "manual"] # ! this seems like a typo; doesn't look like it's used in the query at all
 
-      - ["org.springframework.core.io", "ClassPathResource", True, "getFilename", "", "", "Argument[this]", "get-resource", "manual"] # ! Note: `ClassPathResource` implements `Resource`, so it might make more sense to model some of these as `Resource` with subtype True.
-      - ["org.springframework.core.io", "ClassPathResource", True, "getPath", "", "", "Argument[this]", "get-resource", "manual"]
-      - ["org.springframework.core.io", "ClassPathResource", True, "getURL", "", "", "Argument[this]", "get-resource", "manual"]
-      - ["org.springframework.core.io", "ClassPathResource", True, "resolveURL", "", "", "Argument[this]", "get-resource", "manual"]
-      # # ! below added by me when debugging CVEs:
-      # - ["org.springframework.cloud.config.server.resource", "ResourceController", True, "retrieve", "", "", "Argument[0]", "get-resource", "manual"] # don't need
-      # # - ["org.springframework.cloud.config.server.resource", "ResourceController", True, "getFilePath", "", "", "Argument[0..3]", "get-resource", "manual"] # don't need
-      # # - ["org.springframework.cloud.config.server.resource", "ResourceRepository", True, "findOne", "", "", "Argument[0..3]", "get-resource", "manual"] # convert to summary
-      # # - ["org.springframework.core.io", "InputStreamSource", True, "getInputStream", "", "", "Argument[this]", "get-resource", "manual"] # convert to summary
-      # - ["org.springframework.util", "StreamUtils", True, "copyToString", "", "", "Argument[0]", "get-resource", "manual"] # * public class with good docs
-      # # - ["org.springframework.cloud.config.server.environment", "SearchPathLocator", True, "getLocations", "(String,String,String)", "", "Argument[0..2]", "get-resource", "manual"] # convert to summary
-      # # - ["org.springframework.cloud.config.server.environment", "SearchPathLocator$Locations", True, "getLocations", "()", "", "Argument[this]", "get-resource", "manual"] # convert to summary
-      # - ["org.springframework.core.io", "ResourceLoader", True, "getResource", "", "", "Argument[0]", "get-resource", "manual"] # * public interface with good docs, might be problematic for FPs based on fact that the ext contributor changed this to a taint step to avoid "exists" FPS (maybe there's another way to exclude those FPs though).
-      # - ["javax.servlet", "ServletContext", True, "getResource", "", "", "Argument[0]", "get-resource", "manual"]
-      # - ["javax.servlet", "ServletContext", True, "getResourceAsStream", "", "", "Argument[0]", "get-resource", "manual"]
-      # - ["javax.servlet", "ServletContext", True, "getResourcePaths", "", "", "Argument[0]", "get-resource", "manual"]
-      # - ["javax.servlet", "ServletContext", True, "getResource", "", "", "Argument[this]", "get-resource", "manual"]
-      # - ["javax.servlet", "ServletContext", True, "getResourceAsStream", "", "", "Argument[this]", "get-resource", "manual"]
-      # - ["javax.servlet", "ServletContext", True, "getResourcePaths", "", "", "Argument[this]", "get-resource", "manual"]
-      # # - ["org.apache.tomcat.util.http", "RequestUtil", True, "normalize", "", "", "Argument[0]", "get-resource", "manual"]
-
   - addsTo:
       pack: codeql/java-all
       extensible: summaryModel
     data:
-      - ["io.undertow.server.handlers.resource", "Resource", True, "getFile", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
-      - ["io.undertow.server.handlers.resource", "Resource", True, "getFilePath", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
-      - ["io.undertow.server.handlers.resource", "Resource", True, "getPath", "", "", "Argument[this]", "ReturnValue", "taint", "manual"] # ! this as a taint step seems to contradict the fact that they did `ClassPathResource.getPath` as a sink for Spring...
-      - ["java.nio.file", "Path", True, "normalize", "", "", "Argument[this]", "ReturnValue", "taint", "manual"] # ! shouldn't this be a sanitizer instead??? Or no because WEB-INF ones don't care about normalization?
+      - ["java.nio.file", "Path", True, "normalize", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
       - ["java.nio.file", "Path", True, "resolve", "", "", "Argument[this]", "ReturnValue", "taint", "manual"] # ! check if this and the below are already in the default models
       - ["java.nio.file", "Path", True, "resolve", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
       - ["java.nio.file", "Path", True, "toString", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
       - ["java.nio.file", "Paths", True, "get", "", "", "Argument[0..1]", "ReturnValue", "taint", "manual"]
-
-      - ["org.springframework.core.io", "ClassPathResource", False, "ClassPathResource", "", "", "Argument[0]", "Argument[this]", "taint", "manual"]
-      - ["org.springframework.core.io", "Resource", True, "createRelative", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
-      # # ! below added/modified by me when debugging CVEs:
-      - ["org.springframework.core.io", "ResourceLoader", True, "getResource", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
-      # - ["org.springframework.cloud.config.server.resource", "ResourceRepository", True, "findOne", "", "", "Argument[0..3]", "ReturnValue", "taint", "manual"] # * public interface, but might be too specific, no easily findable docs...
-      # - ["org.springframework.core.io", "InputStreamSource", True, "getInputStream", "", "", "Argument[this]", "ReturnValue", "taint", "manual"] # * public interface with good docs, Note: other `getInputStream`s are remote source and/or taint step, so this as taint step versus sink probably is more consistent
-      # - ["org.springframework.cloud.config.server.environment", "SearchPathLocator", True, "getLocations", "(String,String,String)", "", "Argument[0..2]", "ReturnValue", "taint", "manual"] # * public interface with docs: https://www.javadoc.io/static/org.springframework.cloud/spring-cloud-config-server/2.1.0.RELEASE/org/springframework/cloud/config/server/environment/SearchPathLocator.html
-      # - ["org.springframework.cloud.config.server.environment", "SearchPathLocator$Locations", True, "getLocations", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"] # ! is the `Locations` class package-private? Or does it inherit public from it's enclosing interface?
-      # - ["org.springframework.cloud.config.server.resource", "ResourceController", True, "getFilePath", "", "", "Argument[0..3]", "ReturnValue", "taint", "manual"] # don't need

java/ql/lib/semmle/code/java/security/UnsafeUrlForward.qll

@@ -22,96 +22,7 @@ private class RequestDispatcherSink extends UnsafeUrlForwardSink {
   }
 }
 
-/** The `getResource` method of `Class`. */
-class GetClassResourceMethod extends Method {
-  GetClassResourceMethod() {
-    this.getDeclaringType() instanceof TypeClass and
-    this.hasName("getResource")
-  }
-}
-
-/** The `getResourceAsStream` method of `Class`. */
-class GetClassResourceAsStreamMethod extends Method {
-  GetClassResourceAsStreamMethod() {
-    this.getDeclaringType() instanceof TypeClass and
-    this.hasName("getResourceAsStream")
-  }
-}
-
-/** The `getResource` method of `ClassLoader`. */
-class GetClassLoaderResourceMethod extends Method {
-  GetClassLoaderResourceMethod() {
-    this.getDeclaringType() instanceof ClassLoaderClass and
-    this.hasName("getResource")
-  }
-}
-
-/** The `getResourceAsStream` method of `ClassLoader`. */
-class GetClassLoaderResourceAsStreamMethod extends Method {
-  GetClassLoaderResourceAsStreamMethod() {
-    this.getDeclaringType() instanceof ClassLoaderClass and
-    this.hasName("getResourceAsStream")
-  }
-}
-
-/** The JBoss class `FileResourceManager`. */
-class FileResourceManager extends RefType {
-  FileResourceManager() {
-    this.hasQualifiedName("io.undertow.server.handlers.resource", "FileResourceManager")
-  }
-}
-
-/** The JBoss method `getResource` of `FileResourceManager`. */
-class GetWildflyResourceMethod extends Method {
-  GetWildflyResourceMethod() {
-    this.getDeclaringType().getASupertype*() instanceof FileResourceManager and
-    this.hasName("getResource")
-  }
-}
-
-/** The JBoss class `VirtualFile`. */
-class VirtualFile extends RefType {
-  VirtualFile() { this.hasQualifiedName("org.jboss.vfs", "VirtualFile") }
-}
-
-/** The JBoss method `getChild` of `FileResourceManager`. */
-class GetVirtualFileChildMethod extends Method {
-  GetVirtualFileChildMethod() {
-    this.getDeclaringType().getASupertype*() instanceof VirtualFile and
-    this.hasName("getChild")
-  }
-}
-
-/** An argument to `getResource()` or `getResourceAsStream()`. */
-private class GetResourceSink extends UnsafeUrlForwardSink {
-  GetResourceSink() {
-    sinkNode(this, "request-forgery")
-    or
-    sinkNode(this, "get-resource")
-    or
-    exists(MethodCall ma |
-      (
-        ma.getMethod() instanceof GetServletResourceAsStreamMethod or
-        ma.getMethod() instanceof GetFacesResourceAsStreamMethod or
-        ma.getMethod() instanceof GetClassResourceAsStreamMethod or
-        ma.getMethod() instanceof GetClassLoaderResourceAsStreamMethod or
-        ma.getMethod() instanceof GetVirtualFileChildMethod
-      ) and
-      ma.getArgument(0) = this.asExpr()
-    )
-  }
-}
-
-/** A sink for methods that load Spring resources. */
-private class SpringResourceSink extends UnsafeUrlForwardSink {
-  SpringResourceSink() {
-    exists(MethodCall ma |
-      ma.getMethod() instanceof GetResourceUtilsMethod and
-      ma.getArgument(0) = this.asExpr()
-    )
-  }
-}
-
+// TODO: look into `StaplerResponse.forward`, etc., and think about re-adding the MaD "request-forgery" sinks as a result
 /** An argument to `new ModelAndView` or `ModelAndView.setViewName`. */
 private class SpringModelAndViewSink extends UnsafeUrlForwardSink {
   SpringModelAndViewSink() {

java/ql/lib/semmle/code/java/security/UnsafeUrlForwardQuery.qll

@@ -1,13 +1,19 @@
+/** Provides configurations to be used in queries related to unsafe URL forwarding. */
+
 import java
 import semmle.code.java.security.UnsafeUrlForward
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.Jsf
 import semmle.code.java.security.PathSanitizer
 
+/**
+ * A taint-tracking configuration for untrusted user input in a URL forward or include.
+ */
 module UnsafeUrlForwardFlowConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
     source instanceof ThreatModelFlowSource and
+    // TODO: move below logic to class in UnsafeUrlForward.qll?
     not exists(MethodCall ma, Method m | ma.getMethod() = m |
       (
         m instanceof HttpServletRequestGetRequestUriMethod or
@@ -25,21 +31,11 @@ module UnsafeUrlForwardFlowConfig implements DataFlow::ConfigSig {
     node instanceof PathInjectionSanitizer
   }
 
+  // TODO: check if the below is still needed after removing path-injection related sinks.
   DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext }
-
-  predicate isAdditionalFlowStep(DataFlow::Node prev, DataFlow::Node succ) {
-    exists(MethodCall ma |
-      (
-        ma.getMethod() instanceof GetServletResourceMethod or
-        ma.getMethod() instanceof GetFacesResourceMethod or
-        ma.getMethod() instanceof GetClassResourceMethod or
-        ma.getMethod() instanceof GetClassLoaderResourceMethod or
-        ma.getMethod() instanceof GetWildflyResourceMethod
-      ) and
-      ma.getArgument(0) = prev.asExpr() and
-      ma = succ.asExpr()
-    )
-  }
 }
 
+/**
+ * Taint-tracking flow for untrusted user input in a URL forward or include.
+ */
 module UnsafeUrlForwardFlow = TaintTracking::Global<UnsafeUrlForwardFlowConfig>;