Java: initial update of barrier and test cases to remove FN

Jami Cogswell · Jami Cogswell · commit 911a61df2234 · 2024-03-13T16:28:42.000-04:00
diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll
@@ -64,7 +64,8 @@ private predicate exactPathMatchGuard(Guard g, Expr e, boolean branch) {
   )
 }
 
-private class ExactPathMatchSanitizer extends PathInjectionSanitizer {
+// TODO: switch back to private if possible
+class ExactPathMatchSanitizer extends PathInjectionSanitizer {
   ExactPathMatchSanitizer() {
     this = DataFlow::BarrierGuard<exactPathMatchGuard/3>::getABarrierNode()
     or
@@ -151,7 +152,8 @@ private class DotDotCheckSanitizer extends PathInjectionSanitizer {
   }
 }
 
-private class BlockListGuard extends PathGuard instanceof MethodCall {
+// TODO: switch back to private if possible
+class BlockListGuard extends PathGuard instanceof MethodCall {
   BlockListGuard() {
     (isStringPartialMatch(this) or isPathPrefixMatch(this)) and
     isDisallowedWord(super.getAnArgument())
@@ -228,6 +230,7 @@ private predicate isStringPartialMatch(MethodCall ma) {
   exists(RefType t | t = ma.getMethod().getDeclaringType() |
     t instanceof TypeString or t instanceof StringsKt
   ) and
+  // TODO ! Why not use `StringPartialMatchMethod` for the below?
   getSourceMethod(ma.getMethod())
       .hasName(["contains", "matches", "regionMatches", "indexOf", "lastIndexOf"])
 }
diff --git a/java/ql/lib/semmle/code/java/security/UrlForward.qll b/java/ql/lib/semmle/code/java/security/UrlForward.qll
@@ -4,6 +4,9 @@ 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.dataflow.DataFlow
+private import semmle.code.java.controlflow.Guards
 
 /** A URL forward sink. */
 abstract class UrlForwardSink extends DataFlow::Node { }
@@ -44,16 +47,121 @@ private class PrimitiveBarrier extends UrlForwardBarrier {
   }
 }
 
+// TODO: should this also take URL encoding/decoding into account?
+// TODO: and PathSanitization in general?
 private class FollowsBarrierPrefix extends UrlForwardBarrier {
   FollowsBarrierPrefix() { this.asExpr() = any(BarrierPrefix fp).getAnAppendedExpression() }
 }
 
 private class BarrierPrefix extends InterestingPrefix {
   BarrierPrefix() {
-    // TODO: why not META-INF here as well? (and are `/` correct?)
     not this.getStringValue().matches("/WEB-INF/%") and
     not this.getStringValue() = "forward:"
   }
 
   override int getOffset() { result = 0 }
 }
+
+private class UrlPathBarrier extends UrlForwardBarrier {
+  UrlPathBarrier() {
+    this instanceof PathInjectionSanitizer and
+    (
+      this instanceof ExactPathMatchSanitizer //TODO: still need a better solution for this edge case...
+      or
+      // TODO: these don't enforce order of checks and PathSanitization... make bypass test cases.
+      this instanceof NoEncodingBarrier
+      or
+      this instanceof FullyDecodesBarrier
+    )
+  }
+}
+
+abstract class UrlDecodeCall extends MethodCall { }
+
+private class DefaultUrlDecodeCall extends UrlDecodeCall {
+  DefaultUrlDecodeCall() {
+    this.getMethod().hasQualifiedName("java.net", "URLDecoder", "decode") or // TODO: reuse existing class? Or make this a class?
+    this.getMethod().hasQualifiedName("org.eclipse.jetty.util.URIUtil", "URIUtil", "decodePath")
+  }
+}
+
+// TODO: this can probably be named/designed better...
+abstract class RepeatedStmt extends Stmt { }
+
+private class DefaultRepeatedStmt extends RepeatedStmt {
+  DefaultRepeatedStmt() { this instanceof LoopStmt }
+}
+
+abstract class CheckEncodingCall extends MethodCall { }
+
+private class DefaultCheckEncodingCall extends CheckEncodingCall {
+  DefaultCheckEncodingCall() {
+    // TODO: indexOf?, etc.
+    this.getMethod().hasQualifiedName("java.lang", "String", "contains") and // TODO: reuse existing class? Or make this a class?
+    this.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "%"
+  }
+}
+
+// TODO: better naming?
+// TODO: check if any URL decoding implementations _fully_ decode... or if all need to be called in a loop?
+// TODO: make this extendable instead of `RepeatedStmt`?
+private class RepeatedUrlDecodeCall extends MethodCall {
+  RepeatedUrlDecodeCall() {
+    this instanceof UrlDecodeCall and
+    this.getAnEnclosingStmt() instanceof RepeatedStmt
+  }
+}
+
+private class CheckEncodingGuard extends Guard instanceof MethodCall {
+  CheckEncodingGuard() { this instanceof CheckEncodingCall }
+
+  Expr getCheckedExpr() { result = this.(MethodCall).getQualifier() }
+}
+
+private predicate noEncodingGuard(Guard g, Expr e, boolean branch) {
+  g instanceof CheckEncodingGuard and
+  e = g.(CheckEncodingGuard).getCheckedExpr() and
+  branch = false
+  or
+  // branch = false and
+  // g instanceof AssignExpr and // AssignExpr
+  // exists(CheckEncodingCall call | g.(AssignExpr).getSource() = call | e = call.getQualifier())
+  branch = false and
+  g.(Expr).getType() instanceof BooleanType and // AssignExpr
+  (
+    exists(CheckEncodingCall call, AssignExpr ae |
+      ae.getSource() = call and
+      e = call.getQualifier() and
+      g = ae.getDest()
+    )
+    or
+    exists(CheckEncodingCall call, LocalVariableDeclExpr vde |
+      vde.getInitOrPatternSource() = call and
+      e = call.getQualifier() and
+      g = vde.getAnAccess() //and
+      //vde.getVariable() = g
+    )
+  )
+}
+
+// TODO: check edge case of !contains(%), make sure that behaves as expected at least.
+private class NoEncodingBarrier extends DataFlow::Node {
+  NoEncodingBarrier() { this = DataFlow::BarrierGuard<noEncodingGuard/3>::getABarrierNode() }
+}
+
+private predicate fullyDecodesGuard(Expr e) {
+  exists(CheckEncodingGuard g, RepeatedUrlDecodeCall decodeCall |
+    e = g.getCheckedExpr() and
+    g.controls(decodeCall.getBasicBlock(), true)
+  )
+}
+
+private class FullyDecodesBarrier extends DataFlow::Node {
+  FullyDecodesBarrier() {
+    exists(Variable v, Expr e | this.asExpr() = v.getAnAccess() |
+      fullyDecodesGuard(e) and
+      e = v.getAnAccess() and
+      e.getBasicBlock().bbDominates(this.asExpr().getBasicBlock())
+    )
+  }
+}
diff --git a/java/ql/lib/semmle/code/java/security/UrlForwardQuery.qll b/java/ql/lib/semmle/code/java/security/UrlForwardQuery.qll
@@ -25,10 +25,7 @@ module UrlForwardFlowConfig implements DataFlow::ConfigSig {
 
   predicate isSink(DataFlow::Node sink) { sink instanceof UrlForwardSink }
 
-  predicate isBarrier(DataFlow::Node node) {
-    node instanceof UrlForwardBarrier or
-    node instanceof PathInjectionSanitizer
-  }
+  predicate isBarrier(DataFlow::Node node) { node instanceof UrlForwardBarrier }
 
   DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext }
 }
diff --git a/java/ql/test/query-tests/security/CWE-552/UrlForwardTest.java b/java/ql/test/query-tests/security/CWE-552/UrlForwardTest.java
@@ -112,8 +112,9 @@ public void doFilter2(ServletRequest request, ServletResponse response, FilterCh
 		throws IOException, ServletException {
 		String path = ((HttpServletRequest) request).getServletPath();
 
+		// actually BAD since could potentially bypass with ".." encoded as "%2e%2e"?
 		if (path.startsWith(BASE_PATH) && !path.contains("..")) {
-			request.getRequestDispatcher(path).forward(request, response);
+			request.getRequestDispatcher(path).forward(request, response); // $ hasUrlForward
 		} else {
 			chain.doFilter(request, response);
 		}
@@ -124,6 +125,7 @@ public void doFilter3(ServletRequest request, ServletResponse response, FilterCh
 		throws IOException, ServletException {
 		String path = ((HttpServletRequest) request).getServletPath();
 
+		// this is still good, should not flag here..., url-decoding first doesn't matter if looking for exact match... :(
 		if (path.equals("/comaction")) {
 			request.getRequestDispatcher(path).forward(request, response);
 		} else {
@@ -199,8 +201,9 @@ protected void doHead3(HttpServletRequest request, HttpServletResponse response)
 			throws ServletException, IOException {
 		String path = request.getParameter("path");
 
+		// actually BAD since could potentially bypass with ".." encoded as "%2e%2e"?
 		if (path.startsWith(BASE_PATH) && !path.contains("..")) {
-			request.getServletContext().getRequestDispatcher(path).include(request, response);
+			request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasUrlForward
 		}
 	}
 
@@ -212,45 +215,151 @@ protected void doHead4(HttpServletRequest request, HttpServletResponse response)
 
 		// /pages/welcome.jsp/../../WEB-INF/web.xml becomes /WEB-INF/web.xml
 		// /pages/welcome.jsp/../../%57EB-INF/web.xml becomes /%57EB-INF/web.xml
+		// actually BAD since could potentially bypass with ".." encoded as "%2e%2e": "/pages/welcome.jsp/%2e%2e/%2e%2e/WEB-INF/web.xml" becomes /pages/welcome.jsp/%2e%2e/%2e%2e/WEB-INF/web.xml, which will pass this check and potentially be problematic if decoded later?
 		if (requestedPath.startsWith(BASE_PATH)) {
-			request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response);
+			request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response); // $ hasUrlForward
 		}
 	}
 
-	// FN: Request dispatcher with negation check and path normalization, but without URL decoding
-	// When promoting this query, consider using FlowStates to make `getRequestDispatcher` a sink
-	// only if a URL-decoding step has NOT been crossed (i.e. make URLDecoder.decode change the
-	// state to a different value than the one required at the sink).
-	// TODO: but does this need to take into account URLDecoder.decode in a loop...?
+	// BAD (original FN): Request dispatcher with negation check and path normalization, but without URL decoding
 	protected void doHead5(HttpServletRequest request, HttpServletResponse response)
 			throws ServletException, IOException {
 		String path = request.getParameter("path");
 		Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize();
 
 		if (!requestedPath.startsWith("/WEB-INF") && !requestedPath.startsWith("/META-INF")) {
-			request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response); // $ MISSING: hasUrlForward
+			request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response); // $ hasUrlForward
 		}
 	}
 
-	// GOOD: Request dispatcher with path traversal check and URL decoding
-	protected void doHead6(HttpServletRequest request, HttpServletResponse response)
+	// BAD (I added to test decode with no loop): Request dispatcher with path traversal check and single URL decoding; may be vulnerable to double-encoding
+	protected void doHead7(HttpServletRequest request, HttpServletResponse response)
 			throws ServletException, IOException {
 		String path = request.getParameter("path");
+		path = URLDecoder.decode(path, "UTF-8");
+
+		if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
+			request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasUrlForward
+		}
+	}
+
+	// GOOD: Request dispatcher with path traversal check and URL decoding in a loop to avoid double-encoding bypass
+	protected void doHead6(HttpServletRequest request, HttpServletResponse response)
+			throws ServletException, IOException {
+		String path = request.getParameter("path"); // v
+
+		if (path.contains("%")){ // v.getAnAccess()
+			while (path.contains("%")) {
+				path = URLDecoder.decode(path, "UTF-8");
+			}
+		}
+
+		if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
+			request.getServletContext().getRequestDispatcher(path).include(request, response);
+		}
+	}
+
+	// GOOD: Request dispatcher with path traversal check and URL decoding in a loop to avoid double-encoding bypass
+	protected void doHead8(HttpServletRequest request, HttpServletResponse response)
+			throws ServletException, IOException {
+		String path = request.getParameter("path"); // v
+		while (path.contains("%")) {
+			path = URLDecoder.decode(path, "UTF-8");
+		}
+
+		if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
+			request.getServletContext().getRequestDispatcher(path).include(request, response);
+		}
+	}
+
+	// FP now....
+	// GOOD: Request dispatcher with path traversal check and URL decoding in a loop to avoid double-encoding bypass
+	protected void doHead9(HttpServletRequest request, HttpServletResponse response)
+			throws ServletException, IOException {
+		String path = request.getParameter("path"); // v
 		boolean hasEncoding = path.contains("%");
 		while (hasEncoding) {
 			path = URLDecoder.decode(path, "UTF-8");
 			hasEncoding = path.contains("%");
 		}
 
 		if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
-			request.getServletContext().getRequestDispatcher(path).include(request, response);
+			request.getServletContext().getRequestDispatcher(path).include(request, response); // $ SPURIOUS: hasUrlForward
 		}
 	}
 
-	// New Tests (i.e. Added by me)
+	// New Tests
 	public void generateResponse(StaplerRequest req, StaplerResponse rsp, Object obj) throws IOException, ServletException {
 		String url = req.getParameter("target");
 		rsp.forward(obj, url, req); // $ hasUrlForward
 	}
 
+	// Other Tests for edge cases:
+	// // GOOD (I added): Request dispatcher with path traversal check and URL decoding in a loop to avoid double-encoding bypass
+	// // testing `if` stmt requirement for BB controlling
+	// protected void doHead12(HttpServletRequest request, HttpServletResponse response)
+	// 		throws ServletException, IOException {
+	// 	String path = request.getParameter("path");
+	// 	if (path.contains("%")) {
+	// 		while (path.contains("%")) {
+	// 			path = URLDecoder.decode(path, "UTF-8");
+	// 		}
+	// 	}
+	// 	if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
+	// 		request.getServletContext().getRequestDispatcher(path).include(request, response);
+	// 	}
+	// }
+	// // BAD (I added): Request dispatcher with path traversal check and single URL decoding; may be vulnerable to double-encoding
+	// // Tests urlEncoding BarrierGuard "a guard that considers a string safe because it is checked for URL encoding sequences,
+    // // having previously been checked against a block-list of forbidden values."
+	// protected void doHead8(HttpServletRequest request, HttpServletResponse response)
+	// 		throws ServletException, IOException {
+	// 	String path = request.getParameter("path");
+
+	// 	if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
+	// 		boolean hasEncoding = path.contains("%"); // BAD: doesn't do anything with the check...
+	// 		request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasUrlForward
+	// 	}
+	// }
+	// // BAD (I added): Request dispatcher with path traversal check and single URL decoding; may be vulnerable to double-encoding
+	// // Tests urlEncoding BarrierGuard "a guard that considers a string safe because it is checked for URL encoding sequences,
+    // // having previously been checked against a block-list of forbidden values."
+	// protected void doHead9(HttpServletRequest request, HttpServletResponse response)
+	// 		throws ServletException, IOException {
+	// 	String path = request.getParameter("path");
+
+	// 	boolean hasEncoding = path.contains("%"); // BAD: doesn't do anything with the check... and check comes BEFORE blocklist so guard should not trigger
+	// 	if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
+	// 		request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasUrlForward
+	// 	}
+	// }
+
+	// // BAD (I added): Request dispatcher with path traversal check and single URL decoding; may be vulnerable to double-encoding
+	// // Tests urlEncoding BarrierGuard "a guard that considers a string safe because it is checked for URL encoding sequences,
+    // // having previously been checked against a block-list of forbidden values."
+	// protected void doHead10(HttpServletRequest request, HttpServletResponse response)
+	// 		throws ServletException, IOException {
+	// 	String path = request.getParameter("path");
+
+	// 	if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
+	// 		if (path.contains("%")){ // BAD: wrong check
+	// 			request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasUrlForward
+	// 		}
+	// 	}
+	// }
+
+	// // "GOOD" (I added): Request dispatcher with path traversal check and single URL decoding; may be vulnerable to double-encoding
+	// // Tests urlEncoding BarrierGuard "a guard that considers a string safe because it is checked for URL encoding sequences,
+    // // having previously been checked against a block-list of forbidden values."
+	// protected void doHead11(HttpServletRequest request, HttpServletResponse response)
+	// 		throws ServletException, IOException {
+	// 	String path = request.getParameter("path");
+
+	// 	if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
+	// 		if (!path.contains("%")){ // GOOD: right check
+	// 			request.getServletContext().getRequestDispatcher(path).include(request, response);
+	// 		}
+	// 	}
+	// }
+
 }

Original file line number	Diff line number	Diff line change
`@@ -64,7 +64,8 @@ private predicate exactPathMatchGuard(Guard g, Expr e, boolean branch) {`
`64`	`64`	`)`
`65`	`65`	`}`
`66`	`66`
`67`		`-private class ExactPathMatchSanitizer extends PathInjectionSanitizer {`
	`67`	`+// TODO: switch back to private if possible`
	`68`	`+class ExactPathMatchSanitizer extends PathInjectionSanitizer {`
`68`	`69`	`ExactPathMatchSanitizer() {`
`69`	`70`	`this = DataFlow::BarrierGuard<exactPathMatchGuard/3>::getABarrierNode()`
`70`	`71`	`or`
`@@ -151,7 +152,8 @@ private class DotDotCheckSanitizer extends PathInjectionSanitizer {`
`151`	`152`	`}`
`152`	`153`	`}`
`153`	`154`
`154`		`-private class BlockListGuard extends PathGuard instanceof MethodCall {`
	`155`	`+// TODO: switch back to private if possible`
	`156`	`+class BlockListGuard extends PathGuard instanceof MethodCall {`
`155`	`157`	`BlockListGuard() {`
`156`	`158`	`(isStringPartialMatch(this) or isPathPrefixMatch(this)) and`
`157`	`159`	`isDisallowedWord(super.getAnArgument())`
`@@ -228,6 +230,7 @@ private predicate isStringPartialMatch(MethodCall ma) {`
`228`	`230`	`exists(RefType t \| t = ma.getMethod().getDeclaringType() \|`
`229`	`231`	`t instanceof TypeString or t instanceof StringsKt`
`230`	`232`	`) and`
	`233`	+ // TODO ! Why not use `StringPartialMatchMethod` for the below?
`231`	`234`	`getSourceMethod(ma.getMethod())`
`232`	`235`	`.hasName(["contains", "matches", "regionMatches", "indexOf", "lastIndexOf"])`
`233`	`236`	`}`
Original file line number	Diff line number	Diff line change
`@@ -25,10 +25,7 @@ module UrlForwardFlowConfig implements DataFlow::ConfigSig {`
`25`	`25`
`26`	`26`	`predicate isSink(DataFlow::Node sink) { sink instanceof UrlForwardSink }`
`27`	`27`
`28`		`- predicate isBarrier(DataFlow::Node node) {`
`29`		`- node instanceof UrlForwardBarrier or`
`30`		`- node instanceof PathInjectionSanitizer`
`31`		`- }`
	`28`	`+ predicate isBarrier(DataFlow::Node node) { node instanceof UrlForwardBarrier }`
`32`	`29`
`33`	`30`	`DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext }`
`34`	`31`	`}`