Java: update UrlPathBarrier to include FollowsBarrierPrefix

Author: Jami Cogswell

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

@@ -41,8 +41,7 @@ abstract class UrlForwardBarrier extends DataFlow::Node { }
 
 private class PrimitiveBarrier extends UrlForwardBarrier instanceof SimpleTypeSanitizer { }
 
-/** A barrier for URLs appended to a prefix. */
-private class FollowsBarrierPrefix extends UrlForwardBarrier {
+private class FollowsBarrierPrefix extends DataFlow::Node {
   FollowsBarrierPrefix() { this.asExpr() = any(BarrierPrefix fp).getAnAppendedExpression() }
 }
 
@@ -55,14 +54,16 @@ private class BarrierPrefix extends InterestingPrefix {
   override int getOffset() { result = 0 }
 }
 
-/** A barrier that protects against path injection vulnerabilities while accounting for URL encoding. */
+/**
+ * A barrier that protects against path injection vulnerabilities while accounting
+ * for URL encoding and concatenated prefixes.
+ */
 private class UrlPathBarrier extends UrlForwardBarrier instanceof PathInjectionSanitizer {
   UrlPathBarrier() {
-    this instanceof ExactPathMatchSanitizer
-    or
-    this instanceof NoUrlEncodingBarrier
-    or
-    this instanceof FullyDecodesUrlBarrier
+    this instanceof ExactPathMatchSanitizer or
+    this instanceof NoUrlEncodingBarrier or
+    this instanceof FullyDecodesUrlBarrier or
+    this instanceof FollowsBarrierPrefix
   }
 }
 

java/ql/test/query-tests/security/CWE-552/UrlForwardTest.java

@@ -85,7 +85,41 @@ public void bad7(String url, HttpServletRequest request, HttpServletResponse res
 	@GetMapping("/good1")
 	public void good1(String url, HttpServletRequest request, HttpServletResponse response) {
 		try {
-			request.getRequestDispatcher("/index.jsp?token=" + url).forward(request, response);
+			request.getRequestDispatcher("/index.jsp?token=" + url).forward(request, response); // $ SPURIOUS: hasUrlForward
+		} catch (ServletException e) {
+			e.printStackTrace();
+		} catch (IOException e) {
+			e.printStackTrace();
+		}
+	}
+
+	// BAD: appended to a prefix without path sanitization
+	@GetMapping("/bad8")
+	public void bad8(String urlPath, HttpServletRequest request, HttpServletResponse response) {
+		try {
+			String url = "/pages" + urlPath;
+			request.getRequestDispatcher(url).forward(request, response); // $ hasUrlForward
+		} catch (ServletException e) {
+			e.printStackTrace();
+		} catch (IOException e) {
+			e.printStackTrace();
+		}
+	}
+
+	// GOOD: appended to a prefix with path sanitization
+	@GetMapping("/good2")
+	public void good2(String urlPath, HttpServletRequest request, HttpServletResponse response) {
+		try {
+			while (urlPath.contains("%")) {
+				urlPath = URLDecoder.decode(urlPath, "UTF-8");
+			}
+
+			if (!urlPath.contains("..") && !urlPath.startsWith("/WEB-INF")) {
+				// Note: path injection sanitizer does not account for string concatenation instead of a `startswith` check
+				String url = "/pages" + urlPath;
+				request.getRequestDispatcher(url).forward(request, response);
+			}
+
 		} catch (ServletException e) {
 			e.printStackTrace();
 		} catch (IOException e) {