diff --git a/java/ql/lib/semmle/code/java/security/UnsafeUrlForward.qll b/java/ql/lib/semmle/code/java/security/UrlForward.qll similarity index 66% rename from java/ql/lib/semmle/code/java/security/UnsafeUrlForward.qll rename to java/ql/lib/semmle/code/java/security/UrlForward.qll index 4a529896f86..aa21ed48aba 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeUrlForward.qll +++ b/java/ql/lib/semmle/code/java/security/UrlForward.qll @@ -1,23 +1,23 @@ -/** Provides classes related to unsafe URL forwarding in Java. */ +/** Provides classes to reason about URL forward attacks. */ import java private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.FlowSources private import semmle.code.java.dataflow.StringPrefixes -/** A sink for unsafe URL forward vulnerabilities. */ -abstract class UnsafeUrlForwardSink extends DataFlow::Node { } +/** A URL forward sink. */ +abstract class UrlForwardSink extends DataFlow::Node { } -/** A default sink representing methods susceptible to unsafe URL forwarding. */ -private class DefaultUnsafeUrlForwardSink extends UnsafeUrlForwardSink { - DefaultUnsafeUrlForwardSink() { sinkNode(this, "url-forward") } +/** 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 which * is reachable from a Spring entry point. */ -private class SpringUrlForwardSink extends UnsafeUrlForwardSink { +private class SpringUrlForwardSink extends UrlForwardSink { SpringUrlForwardSink() { // TODO: check if can use MaD "Annotated" for `SpringRequestMappingMethod` or if too complicated for MaD (probably too complicated). any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable()) and @@ -32,10 +32,10 @@ private class ForwardPrefix extends InterestingPrefix { override int getOffset() { result = 0 } } -/** A sanitizer for unsafe URL forward vulnerabilities. */ -abstract class UnsafeUrlForwardSanitizer extends DataFlow::Node { } +/** A URL forward sanitizer. */ +abstract class UrlForwardSanitizer extends DataFlow::Node { } -private class PrimitiveSanitizer extends UnsafeUrlForwardSanitizer { +private class PrimitiveSanitizer extends UrlForwardSanitizer { PrimitiveSanitizer() { this.getType() instanceof PrimitiveType or this.getType() instanceof BoxedType or @@ -44,7 +44,7 @@ private class PrimitiveSanitizer extends UnsafeUrlForwardSanitizer { } // TODO: double-check this sanitizer (and should I switch all "sanitizer" naming to "barrier" instead?) -private class FollowsSanitizingPrefix extends UnsafeUrlForwardSanitizer { +private class FollowsSanitizingPrefix extends UrlForwardSanitizer { FollowsSanitizingPrefix() { this.asExpr() = any(SanitizingPrefix fp).getAnAppendedExpression() } } diff --git a/java/ql/lib/semmle/code/java/security/UnsafeUrlForwardQuery.qll b/java/ql/lib/semmle/code/java/security/UrlForwardQuery.qll similarity index 55% rename from java/ql/lib/semmle/code/java/security/UnsafeUrlForwardQuery.qll rename to java/ql/lib/semmle/code/java/security/UrlForwardQuery.qll index 49670234582..dadf4be612f 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeUrlForwardQuery.qll +++ b/java/ql/lib/semmle/code/java/security/UrlForwardQuery.qll @@ -1,18 +1,18 @@ -/** Provides configurations to be used in queries related to unsafe URL forwarding. */ +/** Provides a taint-tracking configuration for reasoning about URL forwarding. */ import java -import semmle.code.java.security.UnsafeUrlForward +import semmle.code.java.security.UrlForward import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.TaintTracking import semmle.code.java.security.PathSanitizer /** - * A taint-tracking configuration for untrusted user input in a URL forward or include. + * A taint-tracking configuration for reasoning about URL forwarding. */ -module UnsafeUrlForwardFlowConfig implements DataFlow::ConfigSig { +module UrlForwardFlowConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source instanceof ThreatModelFlowSource and - // TODO: move below logic to class in UnsafeUrlForward.qll? And check exactly why these were excluded. + // TODO: move below logic to class in UrlForward.qll? And check exactly why these were excluded. not exists(MethodCall ma, Method m | ma.getMethod() = m | ( m instanceof HttpServletRequestGetRequestUriMethod or @@ -23,10 +23,10 @@ module UnsafeUrlForwardFlowConfig implements DataFlow::ConfigSig { ) } - predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeUrlForwardSink } + predicate isSink(DataFlow::Node sink) { sink instanceof UrlForwardSink } predicate isBarrier(DataFlow::Node node) { - node instanceof UnsafeUrlForwardSanitizer or + node instanceof UrlForwardSanitizer or node instanceof PathInjectionSanitizer } @@ -35,6 +35,6 @@ module UnsafeUrlForwardFlowConfig implements DataFlow::ConfigSig { } /** - * Taint-tracking flow for untrusted user input in a URL forward or include. + * Taint-tracking flow for URL forwarding. */ -module UnsafeUrlForwardFlow = TaintTracking::Global; +module UrlForwardFlow = TaintTracking::Global; diff --git a/java/ql/src/Security/CWE/CWE-552/UnsafeUrlForward.java b/java/ql/src/Security/CWE/CWE-552/UrlForward.java similarity index 97% rename from java/ql/src/Security/CWE/CWE-552/UnsafeUrlForward.java rename to java/ql/src/Security/CWE/CWE-552/UrlForward.java index d159c405736..53b959bb889 100644 --- a/java/ql/src/Security/CWE/CWE-552/UnsafeUrlForward.java +++ b/java/ql/src/Security/CWE/CWE-552/UrlForward.java @@ -7,7 +7,7 @@ import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.servlet.ModelAndView; @Controller -public class UnsafeUrlForward { +public class UrlForward { @GetMapping("/bad1") public ModelAndView bad1(String url) { diff --git a/java/ql/src/Security/CWE/CWE-552/UnsafeUrlForward.qhelp b/java/ql/src/Security/CWE/CWE-552/UrlForward.qhelp similarity index 98% rename from java/ql/src/Security/CWE/CWE-552/UnsafeUrlForward.qhelp rename to java/ql/src/Security/CWE/CWE-552/UrlForward.qhelp index 2e425952edc..fa9ffd45c10 100644 --- a/java/ql/src/Security/CWE/CWE-552/UnsafeUrlForward.qhelp +++ b/java/ql/src/Security/CWE/CWE-552/UrlForward.qhelp @@ -27,7 +27,7 @@ without validating the input, which may cause file leakage. In the good1 - +

The following examples show an HTTP request parameter or request path being used directly in a request dispatcher of Java EE without validating the input, which allows sensitive file exposure diff --git a/java/ql/src/Security/CWE/CWE-552/UnsafeUrlForward.ql b/java/ql/src/Security/CWE/CWE-552/UrlForward.ql similarity index 71% rename from java/ql/src/Security/CWE/CWE-552/UnsafeUrlForward.ql rename to java/ql/src/Security/CWE/CWE-552/UrlForward.ql index 06686d5e0bd..66d3d0dd1ca 100644 --- a/java/ql/src/Security/CWE/CWE-552/UnsafeUrlForward.ql +++ b/java/ql/src/Security/CWE/CWE-552/UrlForward.ql @@ -14,10 +14,10 @@ */ import java -import semmle.code.java.security.UnsafeUrlForwardQuery -import UnsafeUrlForwardFlow::PathGraph +import semmle.code.java.security.UrlForwardQuery +import UrlForwardFlow::PathGraph -from UnsafeUrlForwardFlow::PathNode source, UnsafeUrlForwardFlow::PathNode sink -where UnsafeUrlForwardFlow::flowPath(source, sink) +from UrlForwardFlow::PathNode source, UrlForwardFlow::PathNode sink +where UrlForwardFlow::flowPath(source, sink) select sink.getNode(), source, sink, "Untrusted URL forward depends on a $@.", source.getNode(), "user-provided value" diff --git a/java/ql/test/query-tests/security/CWE-552/UnsafeRequestPath.java b/java/ql/test/query-tests/security/CWE-552/UnsafeRequestPath.java index 55afe84bc19..3d82e7d783e 100644 --- a/java/ql/test/query-tests/security/CWE-552/UnsafeRequestPath.java +++ b/java/ql/test/query-tests/security/CWE-552/UnsafeRequestPath.java @@ -20,7 +20,7 @@ public class UnsafeRequestPath implements Filter { String path = ((HttpServletRequest) request).getServletPath(); // A sample payload "/%57EB-INF/web.xml" can bypass this `startsWith` check if (path != null && !path.startsWith("/WEB-INF")) { - request.getRequestDispatcher(path).forward(request, response); // $ hasUnsafeUrlForward + request.getRequestDispatcher(path).forward(request, response); // $ hasUrlForward } else { chain.doFilter(request, response); } diff --git a/java/ql/test/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java b/java/ql/test/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java index 9d501f2ec0d..66521c5897b 100644 --- a/java/ql/test/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java +++ b/java/ql/test/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java @@ -29,7 +29,7 @@ public class UnsafeServletRequestDispatch extends HttpServlet { rd.forward(request, response); } else { ServletContext sc = cfg.getServletContext(); - RequestDispatcher rd = sc.getRequestDispatcher(returnURL); // $ hasUnsafeUrlForward + RequestDispatcher rd = sc.getRequestDispatcher(returnURL); // $ hasUrlForward rd.forward(request, response); } } @@ -45,7 +45,7 @@ public class UnsafeServletRequestDispatch extends HttpServlet { RequestDispatcher rd = request.getRequestDispatcher("/Login.jsp"); rd.forward(request, response); } else { - RequestDispatcher rd = request.getRequestDispatcher(returnURL); // $ hasUnsafeUrlForward + RequestDispatcher rd = request.getRequestDispatcher(returnURL); // $ hasUrlForward rd.forward(request, response); } } @@ -73,7 +73,7 @@ public class UnsafeServletRequestDispatch extends HttpServlet { // 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); // $ hasUnsafeUrlForward + request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasUrlForward } } @@ -110,7 +110,7 @@ public class UnsafeServletRequestDispatch extends HttpServlet { 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: hasUnsafeUrlForward + request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response); // $ MISSING: hasUrlForward } } diff --git a/java/ql/test/query-tests/security/CWE-552/UnsafeUrlForwardTest.ql b/java/ql/test/query-tests/security/CWE-552/UnsafeUrlForwardTest.ql deleted file mode 100644 index cf77edcf12a..00000000000 --- a/java/ql/test/query-tests/security/CWE-552/UnsafeUrlForwardTest.ql +++ /dev/null @@ -1,18 +0,0 @@ -import java -import TestUtilities.InlineExpectationsTest -import semmle.code.java.security.UnsafeUrlForwardQuery - -module UnsafeUrlForwardTest implements TestSig { - string getARelevantTag() { result = "hasUnsafeUrlForward" } - - predicate hasActualResult(Location location, string element, string tag, string value) { - tag = "hasUnsafeUrlForward" and - exists(UnsafeUrlForwardFlow::PathNode sink | UnsafeUrlForwardFlow::flowPath(_, sink) | - location = sink.getNode().getLocation() and - element = sink.getNode().toString() and - value = "" - ) - } -} - -import MakeTest diff --git a/java/ql/test/query-tests/security/CWE-552/UnsafeUrlForwardTest.expected b/java/ql/test/query-tests/security/CWE-552/UrlForwardTest.expected similarity index 100% rename from java/ql/test/query-tests/security/CWE-552/UnsafeUrlForwardTest.expected rename to java/ql/test/query-tests/security/CWE-552/UrlForwardTest.expected diff --git a/java/ql/test/query-tests/security/CWE-552/UnsafeUrlForward.java b/java/ql/test/query-tests/security/CWE-552/UrlForwardTest.java similarity index 83% rename from java/ql/test/query-tests/security/CWE-552/UnsafeUrlForward.java rename to java/ql/test/query-tests/security/CWE-552/UrlForwardTest.java index 0a00637cd44..2db1e812fb6 100644 --- a/java/ql/test/query-tests/security/CWE-552/UnsafeUrlForward.java +++ b/java/ql/test/query-tests/security/CWE-552/UrlForwardTest.java @@ -7,35 +7,35 @@ import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.servlet.ModelAndView; @Controller -public class UnsafeUrlForward { +public class UrlForwardTest { @GetMapping("/bad1") public ModelAndView bad1(String url) { - return new ModelAndView(url); // $ hasUnsafeUrlForward + return new ModelAndView(url); // $ hasUrlForward } @GetMapping("/bad2") public ModelAndView bad2(String url) { ModelAndView modelAndView = new ModelAndView(); - modelAndView.setViewName(url); // $ hasUnsafeUrlForward + modelAndView.setViewName(url); // $ hasUrlForward return modelAndView; } @GetMapping("/bad3") public String bad3(String url) { - return "forward:" + url + "/swagger-ui/index.html"; // $ hasUnsafeUrlForward + return "forward:" + url + "/swagger-ui/index.html"; // $ hasUrlForward } @GetMapping("/bad4") public ModelAndView bad4(String url) { - ModelAndView modelAndView = new ModelAndView("forward:" + url); // $ hasUnsafeUrlForward + ModelAndView modelAndView = new ModelAndView("forward:" + url); // $ hasUrlForward return modelAndView; } @GetMapping("/bad5") public void bad5(String url, HttpServletRequest request, HttpServletResponse response) { try { - request.getRequestDispatcher(url).include(request, response); // $ hasUnsafeUrlForward + request.getRequestDispatcher(url).include(request, response); // $ hasUrlForward } catch (ServletException e) { e.printStackTrace(); } catch (IOException e) { @@ -46,7 +46,7 @@ public class UnsafeUrlForward { @GetMapping("/bad6") public void bad6(String url, HttpServletRequest request, HttpServletResponse response) { try { - request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").include(request, response); // $ hasUnsafeUrlForward + request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").include(request, response); // $ hasUrlForward } catch (ServletException e) { e.printStackTrace(); } catch (IOException e) { @@ -57,7 +57,7 @@ public class UnsafeUrlForward { @GetMapping("/bad7") public void bad7(String url, HttpServletRequest request, HttpServletResponse response) { try { - request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").forward(request, response); // $ hasUnsafeUrlForward + request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").forward(request, response); // $ hasUrlForward } catch (ServletException e) { e.printStackTrace(); } catch (IOException e) { diff --git a/java/ql/test/query-tests/security/CWE-552/UrlForwardTest.ql b/java/ql/test/query-tests/security/CWE-552/UrlForwardTest.ql new file mode 100644 index 00000000000..4e62a35752b --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-552/UrlForwardTest.ql @@ -0,0 +1,18 @@ +import java +import TestUtilities.InlineExpectationsTest +import semmle.code.java.security.UrlForwardQuery + +module UrlForwardTest implements TestSig { + string getARelevantTag() { result = "hasUrlForward" } + + predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "hasUrlForward" and + exists(UrlForwardFlow::PathNode sink | UrlForwardFlow::flowPath(_, sink) | + location = sink.getNode().getLocation() and + element = sink.getNode().toString() and + value = "" + ) + } +} + +import MakeTest