Merge pull request #15565 from atorralba/atorralba/java/open-redirect-sanitizer

Java: Add extension point and default sanitizer to Open Redirect query

Author: atorralba

java/ql/lib/change-notes/2024-02-09-url-redirect-sanitizer.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* An extension point for sanitizers of the query `java/unvalidated-url-redirection` has been added.

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

@@ -6,10 +6,14 @@ private import semmle.code.java.dataflow.ExternalFlow
 import semmle.code.java.frameworks.Servlets
 import semmle.code.java.frameworks.ApacheHttp
 private import semmle.code.java.frameworks.JaxWS
+private import semmle.code.java.security.RequestForgery
 
 /** A URL redirection sink. */
 abstract class UrlRedirectSink extends DataFlow::Node { }
 
+/** A URL redirection sanitizer. */
+abstract class UrlRedirectSanitizer extends DataFlow::Node { }
+
 /** A default sink represeting methods susceptible to URL redirection attacks. */
 private class DefaultUrlRedirectSink extends UrlRedirectSink {
   DefaultUrlRedirectSink() { sinkNode(this, "url-redirection") }
@@ -42,3 +46,6 @@ private class ApacheUrlRedirectSink extends UrlRedirectSink {
     )
   }
 }
+
+private class DefaultUrlRedirectSanitizer extends UrlRedirectSanitizer instanceof RequestForgerySanitizer
+{ }

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

@@ -11,6 +11,8 @@ module UrlRedirectConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) { source instanceof ThreatModelFlowSource }
 
   predicate isSink(DataFlow::Node sink) { sink instanceof UrlRedirectSink }
+
+  predicate isBarrier(DataFlow::Node node) { node instanceof UrlRedirectSanitizer }
 }
 
 /**

java/ql/src/change-notes/2024-02-09-url-redirect-sanitizer.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The query `java/unvalidated-url-redirection` now sanitizes results following the same logic as the query `java/ssrf`. URLs the destination of which cannot be externally controlled will not be reported anymore.

java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect.expected

@@ -1,7 +1,6 @@
 edges
 | UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:32:25:32:67 | weakCleanup(...) | provenance |  |
 | UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:45:28:45:39 | input : String | provenance |  |
-| UrlRedirect.java:36:58:36:89 | getParameter(...) : String | UrlRedirect.java:36:25:36:89 | ... + ... | provenance |  |
 | UrlRedirect.java:45:28:45:39 | input : String | UrlRedirect.java:46:10:46:14 | input : String | provenance |  |
 | UrlRedirect.java:46:10:46:14 | input : String | UrlRedirect.java:46:10:46:40 | replaceAll(...) : String | provenance |  |
 | mad/Test.java:9:16:9:41 | getParameter(...) : String | mad/Test.java:14:31:14:38 | source(...) : String | provenance |  |
@@ -10,8 +9,6 @@ nodes
 | UrlRedirect.java:23:25:23:54 | getParameter(...) | semmle.label | getParameter(...) |
 | UrlRedirect.java:32:25:32:67 | weakCleanup(...) | semmle.label | weakCleanup(...) |
 | UrlRedirect.java:32:37:32:66 | getParameter(...) : String | semmle.label | getParameter(...) : String |
-| UrlRedirect.java:36:25:36:89 | ... + ... | semmle.label | ... + ... |
-| UrlRedirect.java:36:58:36:89 | getParameter(...) : String | semmle.label | getParameter(...) : String |
 | UrlRedirect.java:39:34:39:63 | getParameter(...) | semmle.label | getParameter(...) |
 | UrlRedirect.java:42:43:42:72 | getParameter(...) | semmle.label | getParameter(...) |
 | UrlRedirect.java:45:28:45:39 | input : String | semmle.label | input : String |
@@ -25,7 +22,6 @@ subpaths
 #select
 | UrlRedirect.java:23:25:23:54 | getParameter(...) | UrlRedirect.java:23:25:23:54 | getParameter(...) | UrlRedirect.java:23:25:23:54 | getParameter(...) | Untrusted URL redirection depends on a $@. | UrlRedirect.java:23:25:23:54 | getParameter(...) | user-provided value |
 | UrlRedirect.java:32:25:32:67 | weakCleanup(...) | UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:32:25:32:67 | weakCleanup(...) | Untrusted URL redirection depends on a $@. | UrlRedirect.java:32:37:32:66 | getParameter(...) | user-provided value |
-| UrlRedirect.java:36:25:36:89 | ... + ... | UrlRedirect.java:36:58:36:89 | getParameter(...) : String | UrlRedirect.java:36:25:36:89 | ... + ... | Untrusted URL redirection depends on a $@. | UrlRedirect.java:36:58:36:89 | getParameter(...) | user-provided value |
 | UrlRedirect.java:39:34:39:63 | getParameter(...) | UrlRedirect.java:39:34:39:63 | getParameter(...) | UrlRedirect.java:39:34:39:63 | getParameter(...) | Untrusted URL redirection depends on a $@. | UrlRedirect.java:39:34:39:63 | getParameter(...) | user-provided value |
 | UrlRedirect.java:42:43:42:72 | getParameter(...) | UrlRedirect.java:42:43:42:72 | getParameter(...) | UrlRedirect.java:42:43:42:72 | getParameter(...) | Untrusted URL redirection depends on a $@. | UrlRedirect.java:42:43:42:72 | getParameter(...) | user-provided value |
 | mad/Test.java:14:22:14:38 | (...)... | mad/Test.java:9:16:9:41 | getParameter(...) : String | mad/Test.java:14:22:14:38 | (...)... | Untrusted URL redirection depends on a $@. | mad/Test.java:9:16:9:41 | getParameter(...) | user-provided value |

java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect.java

@@ -31,7 +31,7 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response)
 		// if the argument is "hthttp://tp://malicious.com"
 		response.sendRedirect(weakCleanup(request.getParameter("target")));
 
-		// FALSE POSITIVE: the user input is not used in a position that allows it to dictate
+		// GOOD: the user input is not used in a position that allows it to dictate
 		// the target of the redirect
 		response.sendRedirect("http://example.com?username=" + request.getParameter("username"));