Java: some updates to test cases

Author: Jami Cogswell

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

@@ -64,7 +64,10 @@ private predicate exactPathMatchGuard(Guard g, Expr e, boolean branch) {
   )
 }
 
-// TODO: switch back to private if possible
+/**
+ * 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()
@@ -152,8 +155,7 @@ private class DotDotCheckSanitizer extends PathInjectionSanitizer {
   }
 }
 
-// TODO: switch back to private if possible
-class BlockListGuard extends PathGuard instanceof MethodCall {
+private class BlockListGuard extends PathGuard instanceof MethodCall {
   BlockListGuard() {
     (isStringPartialMatch(this) or isPathPrefixMatch(this)) and
     isDisallowedWord(super.getAnArgument())
@@ -230,7 +232,6 @@ 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"])
 }

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

@@ -50,23 +50,20 @@ private class FollowsBarrierPrefix extends UrlForwardBarrier {
 private class BarrierPrefix extends InterestingPrefix {
   BarrierPrefix() {
     not this.getStringValue().matches("/WEB-INF/%") and
-    not this.getStringValue() = "forward:"
+    not this instanceof ForwardPrefix
   }
 
   override int getOffset() { result = 0 }
 }
 
-private class UrlPathBarrier extends UrlForwardBarrier {
+private class UrlPathBarrier extends UrlForwardBarrier instanceof PathInjectionSanitizer {
   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
-    )
+    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
   }
 }
 

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

@@ -23,7 +23,7 @@
 @Controller
 public class UrlForwardTest extends HttpServlet implements Filter {
 
-	// (1) ORIGINAL
+	// Spring-related test cases
 	@GetMapping("/bad1")
 	public ModelAndView bad1(String url) {
 		return new ModelAndView(url); // $ hasUrlForward
@@ -91,7 +91,7 @@ public void good1(String url, HttpServletRequest request, HttpServletResponse re
 		}
 	}
 
-	// (2) UnsafeRequestPath
+	// Non-Spring test cases (UnsafeRequest*Path*)
 	private static final String BASE_PATH = "/pages";
 
 	@Override
@@ -107,12 +107,12 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
 		}
 	}
 
-	// GOOD: Request dispatcher from servlet path with check
+	// BAD: Request dispatcher from servlet path with check that does not decode
+	// the user-supplied path; could bypass check with ".." encoded as "%2e%2e".
 	public void doFilter2(ServletRequest request, ServletResponse response, FilterChain chain)
 		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); // $ hasUrlForward
 		} else {
@@ -125,15 +125,14 @@ 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 {
 			chain.doFilter(request, response);
 		}
 	}
 
-	// (3) UnsafeServletRequestDispatch
+	// Non-Spring test cases (UnsafeServletRequest*Dispatch*)
 	@Override
 	// BAD: Request dispatcher constructed from `ServletContext` without input validation
 	protected void doGet(HttpServletRequest request, HttpServletResponse response)
@@ -190,49 +189,49 @@ protected void doHead2(HttpServletRequest request, HttpServletResponse response)
 		String path = request.getParameter("path");
 
 		// A sample payload "/pages/welcome.jsp/../WEB-INF/web.xml" can bypass the `startsWith` check
-		// The payload "/pages/welcome.jsp/../../%57EB-INF/web.xml" can bypass the check as well since RequestDispatcher will decode `%57` as `W`
 		if (path.startsWith(BASE_PATH)) {
 			request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasUrlForward
 		}
 	}
 
-	// GOOD: Request dispatcher with path traversal check
+	// BAD: Request dispatcher with path traversal check that does not decode
+	// the user-supplied path; could bypass check with ".." encoded as "%2e%2e".
 	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); // $ hasUrlForward
 		}
 	}
 
-	// GOOD: Request dispatcher with path normalization and comparison
+	// BAD: Request dispatcher with path normalization and comparison, but
+	// does not decode before normalization.
 	protected void doHead4(HttpServletRequest request, HttpServletResponse response)
 			throws ServletException, IOException {
 		String path = request.getParameter("path");
+
+		// Since not decoded before normalization, "%2e%2e" can remain in the path
 		Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize();
 
-		// /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); // $ hasUrlForward
 		}
 	}
 
-	// BAD (original FN): Request dispatcher with negation check and path normalization, but without URL decoding
+	// BAD: 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");
+		// Since not decoded before normalization, "/%57EB-INF" can remain in the path and pass the `startsWith` check.
 		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); // $ hasUrlForward
 		}
 	}
 
-	// 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
+	// BAD: 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");
@@ -246,9 +245,9 @@ protected void doHead7(HttpServletRequest request, HttpServletResponse response)
 	// 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
+		String path = request.getParameter("path"); // TODO: remove this debugging comment: // v
 
-		if (path.contains("%")){ // v.getAnAccess()
+		if (path.contains("%")){ // TODO: remove this debugging comment: // v.getAnAccess()
 			while (path.contains("%")) {
 				path = URLDecoder.decode(path, "UTF-8");
 			}
@@ -259,10 +258,53 @@ protected void doHead6(HttpServletRequest request, HttpServletResponse response)
 		}
 	}
 
+	// GOOD: Request dispatcher with URL encoding check and path traversal check
+	protected void doHead16(HttpServletRequest request, HttpServletResponse response)
+			throws ServletException, IOException {
+		String path = request.getParameter("path");
+
+		if (!path.contains("%")){
+			if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
+				request.getServletContext().getRequestDispatcher(path).include(request, response);
+			}
+		}
+	}
+
+	// TODO: clean-up
+	// 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.contains("%")){ // BAD: wrong check
+		if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
+			// if (path.contains("%")){ // BAD: wrong check
+				request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasUrlForward
+			// }
+		}
+	}
+	}
+
+	// TODO: clean-up
+	// "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);
+			}
+		}
+	}
+
 	// 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
+		String path = request.getParameter("path"); // TODO: remove this debugging comment: // v
 		while (path.contains("%")) {
 			path = URLDecoder.decode(path, "UTF-8");
 		}
@@ -272,6 +314,7 @@ protected void doHead8(HttpServletRequest request, HttpServletResponse response)
 		}
 	}
 
+	// TODO: see if can fix?
 	// 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)
@@ -288,78 +331,26 @@ protected void doHead9(HttpServletRequest request, HttpServletResponse response)
 		}
 	}
 
-	// New Tests
+	// BAD: `StaplerResponse.forward` without any checks
 	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);
-	// 		}
-	// 	}
-	// }
+	// QHelp example
+	private static final String VALID_FORWARD = "https://cwe.mitre.org/data/definitions/552.html";
+
+	protected void doGet2(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); // $ hasUrlForward
+
+		// 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);
+		}
+	}
 }