Update use cases and qldoc

Author: luchua-bc

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java

@@ -5,78 +5,51 @@ public class UnsafeServletRequestDispatch extends HttpServlet {
 	protected void doGet(HttpServletRequest request, HttpServletResponse response)
 			throws ServletException, IOException {
 		{
-			// GOOD: whitelisted URI
+			ServletConfig cfg = getServletConfig();
+			ServletContext sc = cfg.getServletContext();
+
+			// GOOD: check for an explicitly permitted URI
+			String action = request.getParameter("action");
 			if (action.equals("Login")) {
-				ServletContext sc = cfg.getServletContext();
 				RequestDispatcher rd = sc.getRequestDispatcher("/Login.jsp");
 				rd.forward(request, response);
-			} 
-		}
+			}
 
-		{
-			// BAD: Request dispatcher constructed from `ServletContext` without input validation
+			// BAD: no URI validation
 			String returnURL = request.getParameter("returnURL");
-			ServletConfig cfg = getServletConfig();
-
-			ServletContext sc = cfg.getServletContext();
 			RequestDispatcher rd = sc.getRequestDispatcher(returnURL);
 			rd.forward(request, response);
-		}
 
-		{
-			// BAD: Request dispatcher without path traversal check 
+			// 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`
 			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`  
+			// BAD: no check for path traversal
 			if (path.startsWith(BASE_PATH)) {
 				request.getServletContext().getRequestDispatcher(path).include(request, response);
 			}
-		}
-	}
-
-	{
-		// GOOD: Request dispatcher with path traversal check 
-		String path = request.getParameter("path");
-
-		if (path.startsWith(BASE_PATH) && !path.contains("..")) {
-			request.getServletContext().getRequestDispatcher(path).include(request, response);
-		}
-	}
-
-	{
-		// GOOD: Request dispatcher with path normalization 
-		String path = request.getParameter("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
-		if (requestedPath.startsWith(BASE_PATH)) {
-			request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response);
-		}
-	}
 
-	{
-		// BAD: Request dispatcher with improper negation check and without url decoding 
-		String path = request.getParameter("path");
-		Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize();
+			// GOOD: To check there won't be unexpected path traversal, we can check for any `..` sequences and whether the URI starts with a given web root path.
+			if (path.startsWith(BASE_PATH) && !path.contains("..")) {
+				request.getServletContext().getRequestDispatcher(path).include(request, response);
+			}
 
-		if (!requestedPath.startsWith("/WEB-INF") && !requestedPath.startsWith("/META-INF")) {
-			request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response);
-		}
-	}
+			// GOOD: Or alternatively we can use Path.normalize and check whether the URI starts with a given web root path.
+			Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize();
+			if (requestedPath.startsWith(BASE_PATH)) {
+				request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response);
+			}
 
-	{
-		// GOOD: Request dispatcher with path traversal check and url decoding
-		String path = request.getParameter("path");
-		boolean hasEncoding = path.contains("%");
-		while (hasEncoding) {
-			path = URLDecoder.decode(path, "UTF-8");
-			hasEncoding = path.contains("%");
-		}
+			// GOOD: Or alternatively ensure URL encoding is removed and then check for any `..` sequences.
+			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);
+			if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
+				request.getServletContext().getRequestDispatcher(path).include(request, response);
+			}
 		}
 	}
 }

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp

@@ -13,6 +13,9 @@
 
 <p>Unsanitized user provided data must not be used to construct the path for URL forwarding. In order to prevent
 untrusted URL forwarding, it is recommended to avoid concatenating user input directly into the forwarding URL.
+Instead, user input should be checked for path traversal using <code>..</code> sequences or the user input should
+be normalized using <code>Path.normalize</code>, any URL encoding should be removed, and user input should be
+checked against a permitted URI.
 </p>
 
 </recommendation>