Merge pull request #14854 from jcogs33/jcogs33/unsafe-url-forward-promotion

Java: Promote Unsafe URL Forward query from experimental

Author: jcogs33

java/ql/lib/ext/experimental/io.undertow.server.handlers.resource.model.yml

@@ -4,4 +4,3 @@ extensions:
       extensible: experimentalSinkModel
     data:
       - ["java.util.concurrent", "TimeUnit", True, "sleep", "", "", "Argument[0]", "thread-pause", "manual", "thread-resource-abuse"]
-      - ["java.util.concurrent", "TimeUnit", True, "sleep", "", "", "Argument[0]", "thread-pause", "manual", "unsafe-url-forward"]

java/ql/lib/ext/experimental/java.nio.file.model.yml

@@ -1,9 +1,4 @@
 extensions:
-  - addsTo:
-      pack: codeql/java-all
-      extensible: experimentalSourceModel
-    data:
-      - ["javax.servlet.http", "HttpServletRequest", True, "getServletPath", "", "", "ReturnValue", "remote", "manual", "unsafe-url-forward"]
   - addsTo:
       pack: codeql/java-all
       extensible: experimentalSourceModel
@@ -13,4 +8,3 @@ extensions:
       - ["javax.servlet.http", "HttpServletRequest", False, "getRequestURI", "()", "", "ReturnValue", "uri-path", "manual", "permissive-dot-regex-query"]
       - ["javax.servlet.http", "HttpServletRequest", False, "getRequestURL", "()", "", "ReturnValue", "uri-path", "manual", "permissive-dot-regex-query"]
       - ["javax.servlet.http", "HttpServletRequest", False, "getServletPath", "()", "", "ReturnValue", "uri-path", "manual", "permissive-dot-regex-query"]
-

java/ql/lib/ext/experimental/java.util.concurrent.model.yml

@@ -1,6 +1,6 @@
 extensions:
   - addsTo:
       pack: codeql/java-all
-      extensible: experimentalSourceModel
+      extensible: sourceModel
     data:
-      - ["jakarta.servlet.http", "HttpServletRequest", True, "getServletPath", "", "", "ReturnValue", "remote", "manual", "unsafe-url-forward"]
+      - ["jakarta.servlet.http", "HttpServletRequest", True, "getServletPath", "", "", "ReturnValue", "remote", "manual"]

java/ql/lib/ext/experimental/javax.servlet.http.model.yml

@@ -0,0 +1,7 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sinkModel
+    data:
+      - ["jakarta.servlet", "ServletContext", True, "getRequestDispatcher", "(String)", "", "Argument[0]", "url-forward", "manual"]
+      - ["jakarta.servlet", "ServletRequest", True, "getRequestDispatcher", "(String)", "", "Argument[0]", "url-forward", "manual"]

java/ql/lib/ext/experimental/org.springframework.core.io.model.yml

@@ -0,0 +1,6 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sinkModel
+    data:
+      - ["javax.portlet", "PortletContext", True, "getRequestDispatcher", "(String)", "", "Argument[0]", "url-forward", "manual"]

java/ql/lib/ext/experimental/jakarta.servlet.http.model.yml renamed to java/ql/lib/ext/jakarta.servlet.http.model.yml

@@ -18,6 +18,8 @@ extensions:
       - ["javax.servlet.http", "HttpServletRequest", False, "getRemoteUser", "()", "", "ReturnValue", "remote", "manual"]
       - ["javax.servlet.http", "HttpServletRequest", False, "getRequestURI", "()", "", "ReturnValue", "remote", "manual"]
       - ["javax.servlet.http", "HttpServletRequest", False, "getRequestURL", "()", "", "ReturnValue", "remote", "manual"]
+      - ["javax.servlet.http", "HttpServletRequest", False, "getServletPath", "()", "", "ReturnValue", "remote", "manual"]
+
   - addsTo:
       pack: codeql/java-all
       extensible: sinkModel

java/ql/lib/ext/jakarta.servlet.model.yml

@@ -14,6 +14,8 @@ extensions:
       extensible: sinkModel
     data:
       - ["javax.servlet", "ServletContext", True, "getResourceAsStream", "(String)", "", "Argument[0]", "path-injection", "ai-manual"]
+      - ["javax.servlet", "ServletContext", True, "getRequestDispatcher", "(String)", "", "Argument[0]", "url-forward", "manual"]
+      - ["javax.servlet", "ServletRequest", True, "getRequestDispatcher", "(String)", "", "Argument[0]", "url-forward", "manual"]
   - addsTo:
       pack: codeql/java-all
       extensible: summaryModel

java/ql/lib/ext/javax.portlet.model.yml

@@ -9,7 +9,7 @@ extensions:
       - ["org.kohsuke.stapler", "HttpResponses", True, "staticResource", "(URL,long)", "", "Argument[0]", "request-forgery", "manual"]
       - ["org.kohsuke.stapler", "HttpResponses", True, "html", "(String)", "", "Argument[0]", "html-injection", "manual"]
       - ["org.kohsuke.stapler", "HttpResponses", True, "literalHtml", "(String)", "", "Argument[0]", "html-injection", "manual"]
-      - ["org.kohsuke.stapler", "StaplerResponse", True, "forward", "(Object,String,StaplerRequest)", "", "Argument[1]", "request-forgery", "manual"]
+      - ["org.kohsuke.stapler", "StaplerResponse", True, "forward", "(Object,String,StaplerRequest)", "", "Argument[1]", "url-forward", "manual"]
       - ["org.kohsuke.stapler", "StaplerResponse", True, "sendRedirect2", "(String)", "", "Argument[0]", "url-redirection", "manual"]
       - ["org.kohsuke.stapler", "StaplerResponse", True, "sendRedirect", "(int,String)", "", "Argument[1]", "url-redirection", "manual"]
       - ["org.kohsuke.stapler", "StaplerResponse", True, "sendRedirect", "(String)", "", "Argument[0]", "url-redirection", "manual"]

java/ql/lib/ext/javax.servlet.http.model.yml

@@ -0,0 +1,7 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sinkModel
+    data:
+      - ["org.springframework.web.portlet", "ModelAndView", False, "ModelAndView", "", "", "Argument[0]", "url-forward", "manual"]
+      - ["org.springframework.web.portlet", "ModelAndView", False, "setViewName", "", "", "Argument[0]", "url-forward", "manual"]

java/ql/lib/ext/javax.servlet.model.yml

@@ -0,0 +1,7 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sinkModel
+    data:
+      - ["org.springframework.web.servlet", "ModelAndView", False, "ModelAndView", "", "", "Argument[0]", "url-forward", "manual"]
+      - ["org.springframework.web.servlet", "ModelAndView", False, "setViewName", "", "", "Argument[0]", "url-forward", "manual"]

java/ql/lib/ext/org.kohsuke.stapler.model.yml

@@ -38,6 +38,13 @@ class StringLengthMethod extends Method {
   StringLengthMethod() { this.hasName("length") and this.getDeclaringType() instanceof TypeString }
 }
 
+/** The `contains()` method of the class `java.lang.String`. */
+class StringContainsMethod extends Method {
+  StringContainsMethod() {
+    this.hasName("contains") and this.getDeclaringType() instanceof TypeString
+  }
+}
+
 /**
  * The methods on the class `java.lang.String` that are used to perform partial matches with a specified substring or char.
  */

java/ql/lib/ext/org.springframework.web.portlet.model.yml

@@ -24,6 +24,11 @@ class TypeUrl extends RefType {
   TypeUrl() { this.hasQualifiedName("java.net", "URL") }
 }
 
+/** The type `java.net.URLDecoder`. */
+class TypeUrlDecoder extends RefType {
+  TypeUrlDecoder() { this.hasQualifiedName("java.net", "URLDecoder") }
+}
+
 /** The type `java.net.URI`. */
 class TypeUri extends RefType {
   TypeUri() { this.hasQualifiedName("java.net", "URI") }
@@ -157,6 +162,14 @@ class UrlOpenConnectionMethod extends Method {
   }
 }
 
+/** The method `java.net.URLDecoder::decode`. */
+class UrlDecodeMethod extends Method {
+  UrlDecodeMethod() {
+    this.getDeclaringType() instanceof TypeUrlDecoder and
+    this.getName() = "decode"
+  }
+}
+
 /** The method `javax.net.SocketFactory::createSocket`. */
 class CreateSocketMethod extends Method {
   CreateSocketMethod() {

java/ql/lib/ext/org.springframework.web.servlet.model.yml

@@ -64,7 +64,11 @@ private predicate exactPathMatchGuard(Guard g, Expr e, boolean branch) {
   )
 }
 
-private class ExactPathMatchSanitizer extends PathInjectionSanitizer {
+/**
+ * A sanitizer that protects against path injection vulnerabilities
+ * by checking for a matching path.
+ */
+class ExactPathMatchSanitizer extends PathInjectionSanitizer {
   ExactPathMatchSanitizer() {
     this = DataFlow::BarrierGuard<exactPathMatchGuard/3>::getABarrierNode()
     or

java/ql/lib/semmle/code/java/JDK.qll

@@ -0,0 +1,203 @@
+/** Provides classes and a taint-tracking configuration to reason about unsafe URL forwarding. */
+
+import java
+private import semmle.code.java.dataflow.ExternalFlow
+private import semmle.code.java.dataflow.FlowSources
+private import semmle.code.java.dataflow.StringPrefixes
+private import semmle.code.java.security.PathSanitizer
+private import semmle.code.java.controlflow.Guards
+private import semmle.code.java.security.Sanitizers
+
+/** A URL forward sink. */
+abstract class UrlForwardSink extends DataFlow::Node { }
+
+/**
+ * A default sink representing methods susceptible to URL
+ * forwarding attacks.
+ */
+private class DefaultUrlForwardSink extends UrlForwardSink {
+  DefaultUrlForwardSink() { sinkNode(this, "url-forward") }
+}
+
+/**
+ * An expression appended (perhaps indirectly) to `"forward:"`
+ * and reachable from a Spring entry point.
+ */
+private class SpringUrlForwardPrefixSink extends UrlForwardSink {
+  SpringUrlForwardPrefixSink() {
+    any(SpringRequestMappingMethod srmm).polyCalls*(this.getEnclosingCallable()) and
+    appendedToForwardPrefix(this)
+  }
+}
+
+pragma[nomagic]
+private predicate appendedToForwardPrefix(DataFlow::ExprNode exprNode) {
+  exists(ForwardPrefix fp | exprNode.asExpr() = fp.getAnAppendedExpression())
+}
+
+private class ForwardPrefix extends InterestingPrefix {
+  ForwardPrefix() { this.getStringValue() = "forward:" }
+
+  override int getOffset() { result = 0 }
+}
+
+/** A URL forward barrier. */
+abstract class UrlForwardBarrier extends DataFlow::Node { }
+
+private class PrimitiveBarrier extends UrlForwardBarrier instanceof SimpleTypeSanitizer { }
+
+/**
+ * A barrier for values appended to a "redirect:" prefix.
+ * These results are excluded because they should be handled
+ * by the `java/unvalidated-url-redirection` query instead.
+ */
+private class RedirectPrefixBarrier extends UrlForwardBarrier {
+  RedirectPrefixBarrier() { this.asExpr() = any(RedirectPrefix fp).getAnAppendedExpression() }
+}
+
+private class RedirectPrefix extends InterestingPrefix {
+  RedirectPrefix() { this.getStringValue() = "redirect:" }
+
+  override int getOffset() { result = 0 }
+}
+
+/**
+ * A value that is the result of prepending a string that prevents
+ * any value from controlling the path of a URL.
+ */
+private class FollowsBarrierPrefix extends UrlForwardBarrier {
+  FollowsBarrierPrefix() { this.asExpr() = any(BarrierPrefix fp).getAnAppendedExpression() }
+}
+
+private class BarrierPrefix extends InterestingPrefix {
+  int offset;
+
+  BarrierPrefix() {
+    // Matches strings that look like when prepended to untrusted input, they will restrict
+    // the path of a URL: for example, anything containing `?` or `#`.
+    exists(this.getStringValue().regexpFind("[?#]", 0, offset))
+    or
+    this.(CharacterLiteral).getValue() = ["?", "#"] and offset = 0
+  }
+
+  override int getOffset() { result = offset }
+}
+
+/**
+ * A barrier that protects against path injection vulnerabilities
+ * while accounting for URL encoding.
+ */
+private class UrlPathBarrier extends UrlForwardBarrier instanceof PathInjectionSanitizer {
+  UrlPathBarrier() {
+    this instanceof ExactPathMatchSanitizer or
+    this instanceof NoUrlEncodingBarrier or
+    this instanceof FullyDecodesUrlBarrier
+  }
+}
+
+/** A call to a method that decodes a URL. */
+abstract class UrlDecodeCall extends MethodCall { }
+
+private class DefaultUrlDecodeCall extends UrlDecodeCall {
+  DefaultUrlDecodeCall() {
+    this.getMethod() instanceof UrlDecodeMethod or
+    this.getMethod().hasQualifiedName("org.eclipse.jetty.util.URIUtil", "URIUtil", "decodePath")
+  }
+}
+
+/** A repeated call to a method that decodes a URL. */
+abstract class RepeatedUrlDecodeCall extends MethodCall { }
+
+private class DefaultRepeatedUrlDecodeCall extends RepeatedUrlDecodeCall instanceof UrlDecodeCall {
+  DefaultRepeatedUrlDecodeCall() { this.getAnEnclosingStmt() instanceof LoopStmt }
+}
+
+/** A method call that checks a string for URL encoding. */
+abstract class CheckUrlEncodingCall extends MethodCall { }
+
+private class DefaultCheckUrlEncodingCall extends CheckUrlEncodingCall {
+  DefaultCheckUrlEncodingCall() {
+    this.getMethod() instanceof StringContainsMethod and
+    this.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "%"
+  }
+}
+
+/** A guard that looks for a method call that checks for URL encoding. */
+private class CheckUrlEncodingGuard extends Guard instanceof CheckUrlEncodingCall {
+  Expr getCheckedExpr() { result = this.(MethodCall).getQualifier() }
+}
+
+/** Holds if `g` is guard for a URL that does not contain URL encoding. */
+private predicate noUrlEncodingGuard(Guard g, Expr e, boolean branch) {
+  e = g.(CheckUrlEncodingGuard).getCheckedExpr() and
+  branch = false
+  or
+  branch = false and
+  g.(Expr).getType() instanceof BooleanType and
+  (
+    exists(CheckUrlEncodingCall call, AssignExpr ae |
+      ae.getSource() = call and
+      e = call.getQualifier() and
+      g = ae.getDest()
+    )
+    or
+    exists(CheckUrlEncodingCall call, LocalVariableDeclExpr vde |
+      vde.getInitOrPatternSource() = call and
+      e = call.getQualifier() and
+      g = vde.getAnAccess()
+    )
+  )
+}
+
+/** A barrier for URLs that do not contain URL encoding. */
+private class NoUrlEncodingBarrier extends DataFlow::Node {
+  NoUrlEncodingBarrier() { this = DataFlow::BarrierGuard<noUrlEncodingGuard/3>::getABarrierNode() }
+}
+
+/** Holds if `g` is guard for a URL that is fully decoded. */
+private predicate fullyDecodesUrlGuard(Expr e) {
+  exists(CheckUrlEncodingGuard g, RepeatedUrlDecodeCall decodeCall |
+    e = g.getCheckedExpr() and
+    g.controls(decodeCall.getBasicBlock(), true)
+  )
+}
+
+/** A barrier for URLs that are fully decoded. */
+private class FullyDecodesUrlBarrier extends DataFlow::Node {
+  FullyDecodesUrlBarrier() {
+    exists(Variable v, Expr e | this.asExpr() = v.getAnAccess() |
+      fullyDecodesUrlGuard(e) and
+      e = v.getAnAccess() and
+      e.getBasicBlock().bbDominates(this.asExpr().getBasicBlock())
+    )
+  }
+}
+
+/**
+ * A taint-tracking configuration for reasoning about URL forwarding.
+ */
+module UrlForwardFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source instanceof ThreatModelFlowSource and
+    // excluded due to FPs
+    not exists(MethodCall mc, Method m |
+      m instanceof HttpServletRequestGetRequestUriMethod or
+      m instanceof HttpServletRequestGetRequestUrlMethod or
+      m instanceof HttpServletRequestGetPathMethod
+    |
+      mc.getMethod() = m and
+      mc = source.asExpr()
+    )
+  }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof UrlForwardSink }
+
+  predicate isBarrier(DataFlow::Node node) { node instanceof UrlForwardBarrier }
+
+  DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext }
+}
+
+/**
+ * Taint-tracking flow for URL forwarding.
+ */
+module UrlForwardFlow = TaintTracking::Global<UrlForwardFlowConfig>;

java/ql/lib/semmle/code/java/frameworks/Networking.qll

@@ -0,0 +1,17 @@
+public class UrlForward extends HttpServlet {
+	private static final String VALID_FORWARD = "https://cwe.mitre.org/data/definitions/552.html";
+
+	protected void doGet(HttpServletRequest request, HttpServletResponse response)
+			throws ServletException, IOException {
+		ServletConfig cfg = getServletConfig();
+		ServletContext sc = cfg.getServletContext();
+
+		// BAD: a request parameter is incorporated without validation into a URL forward
+		sc.getRequestDispatcher(request.getParameter("target")).forward(request, response);
+
+		// GOOD: the request parameter is validated against a known fixed string
+		if (VALID_FORWARD.equals(request.getParameter("target"))) {
+			sc.getRequestDispatcher(VALID_FORWARD).forward(request, response);
+		}
+	}
+}