diff --git a/java/ql/src/Security/CWE/CWE-601/UrlRedirect.ql b/java/ql/src/Security/CWE/CWE-601/UrlRedirect.ql index 455f6add626..8e3741e436b 100644 --- a/java/ql/src/Security/CWE/CWE-601/UrlRedirect.ql +++ b/java/ql/src/Security/CWE/CWE-601/UrlRedirect.ql @@ -13,6 +13,7 @@ import java import semmle.code.java.dataflow.FlowSources import semmle.code.java.security.UrlRedirect +import semmle.code.java.dataflow.ExternalFlow import DataFlow::PathGraph class UrlRedirectConfig extends TaintTracking::Configuration { @@ -20,7 +21,11 @@ class UrlRedirectConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - override predicate isSink(DataFlow::Node sink) { sink instanceof UrlRedirectSink } + override predicate isSink(DataFlow::Node sink) { + sink instanceof UrlRedirectSink + or + sinkNode(sink, "url-redirect") + } } from DataFlow::PathNode source, DataFlow::PathNode sink, UrlRedirectConfig conf diff --git a/java/ql/src/semmle/code/java/frameworks/JaxWS.qll b/java/ql/src/semmle/code/java/frameworks/JaxWS.qll index cff79fc280d..fa031e557fd 100644 --- a/java/ql/src/semmle/code/java/frameworks/JaxWS.qll +++ b/java/ql/src/semmle/code/java/frameworks/JaxWS.qll @@ -308,6 +308,20 @@ class JaxRSConsumesAnnotation extends JaxRSAnnotation { JaxRSConsumesAnnotation() { this.getType().hasQualifiedName(getAJaxRsPackage(), "Consumes") } } +/** A URL redirection sink from JAX-RS */ +private class JaxRsUrlRedirectSink extends SinkModelCsv { + override predicate row(string row) { + row = + [ + //`namespace; type; subtypes; name; signature; ext; input; kind` + "javax.ws.rs.core;Response;true;seeOther;;;Argument[0];url-redirect", + "javax.ws.rs.core;Response;true;temporaryRedirect;;;Argument[0];url-redirect", + "jakarta.ws.rs.core;Response;true;seeOther;;;Argument[0];url-redirect", + "jakarta.ws.rs.core;Response;true;temporaryRedirect;;;Argument[0];url-redirect" + ] + } +} + /** * Model Response: * diff --git a/java/ql/src/semmle/code/java/security/UrlRedirect.qll b/java/ql/src/semmle/code/java/security/UrlRedirect.qll index ee3e9cb9b1c..49ba24c77a9 100644 --- a/java/ql/src/semmle/code/java/security/UrlRedirect.qll +++ b/java/ql/src/semmle/code/java/security/UrlRedirect.qll @@ -36,17 +36,3 @@ private class ApacheUrlRedirectSink extends UrlRedirectSink { ) } } - -/** A URL redirection sink from JAX-RS */ -private class JaxRsUrlRedirectSink extends UrlRedirectSink { - JaxRsUrlRedirectSink() { - exists(MethodAccess ma | - ma.getMethod() - .getDeclaringType() - .getAnAncestor() - .hasQualifiedName(getAJaxRsPackage("core"), "Response") and - ma.getMethod().getName() in ["seeOther", "temporaryRedirect"] and - this.asExpr() = ma.getArgument(0) - ) - } -} diff --git a/java/ql/test/library-tests/frameworks/JaxWs/UrlRedirect.expected b/java/ql/test/library-tests/frameworks/JaxWs/UrlRedirect.expected new file mode 100644 index 00000000000..9ad1a630516 --- /dev/null +++ b/java/ql/test/library-tests/frameworks/JaxWs/UrlRedirect.expected @@ -0,0 +1,11 @@ +edges +| UrlRedirect.java:10:32:10:61 | getParameter(...) : String | UrlRedirect.java:10:24:10:62 | new URI(...) | +| UrlRedirect.java:13:41:13:70 | getParameter(...) : String | UrlRedirect.java:13:33:13:71 | new URI(...) | +nodes +| UrlRedirect.java:10:24:10:62 | new URI(...) | semmle.label | new URI(...) | +| UrlRedirect.java:10:32:10:61 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| UrlRedirect.java:13:33:13:71 | new URI(...) | semmle.label | new URI(...) | +| UrlRedirect.java:13:41:13:70 | getParameter(...) : String | semmle.label | getParameter(...) : String | +#select +| UrlRedirect.java:10:24:10:62 | new URI(...) | UrlRedirect.java:10:32:10:61 | getParameter(...) : String | UrlRedirect.java:10:24:10:62 | new URI(...) | Potentially untrusted URL redirection due to $@. | UrlRedirect.java:10:32:10:61 | getParameter(...) | user-provided value | +| UrlRedirect.java:13:33:13:71 | new URI(...) | UrlRedirect.java:13:41:13:70 | getParameter(...) : String | UrlRedirect.java:13:33:13:71 | new URI(...) | Potentially untrusted URL redirection due to $@. | UrlRedirect.java:13:41:13:70 | getParameter(...) | user-provided value | diff --git a/java/ql/test/library-tests/frameworks/JaxWs/UrlRedirect.qlref b/java/ql/test/library-tests/frameworks/JaxWs/UrlRedirect.qlref new file mode 100644 index 00000000000..b4772fb438f --- /dev/null +++ b/java/ql/test/library-tests/frameworks/JaxWs/UrlRedirect.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-601/UrlRedirect.ql \ No newline at end of file diff --git a/java/ql/test/library-tests/frameworks/JaxWs/UrlRedirectJax.java b/java/ql/test/library-tests/frameworks/JaxWs/UrlRedirectJax.java new file mode 100644 index 00000000000..4ba3d1f1331 --- /dev/null +++ b/java/ql/test/library-tests/frameworks/JaxWs/UrlRedirectJax.java @@ -0,0 +1,15 @@ +import java.io.IOException; +import java.net.URI; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.core.Response; + +public class UrlRedirectJax extends HttpServlet { + protected void doGetJax(HttpServletRequest request, Response jaxResponse) throws Exception { + // BAD + jaxResponse.seeOther(new URI(request.getParameter("target"))); + + // BAD + jaxResponse.temporaryRedirect(new URI(request.getParameter("target"))); + } +} diff --git a/java/ql/test/library-tests/frameworks/JaxWs/options b/java/ql/test/library-tests/frameworks/JaxWs/options index 92727b95566..f84495b1c7e 100644 --- a/java/ql/test/library-tests/frameworks/JaxWs/options +++ b/java/ql/test/library-tests/frameworks/JaxWs/options @@ -1 +1 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/javax-ws-rs-api-2.1.1:${testdir}/../../../stubs/jsr311-api-1.1.1:${testdir}/../../../stubs/jsr181-api:${testdir}/../../../stubs/jaxws-api-2.0 +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/javax-ws-rs-api-2.1.1:${testdir}/../../../stubs/jsr311-api-1.1.1:${testdir}/../../../stubs/jsr181-api:${testdir}/../../../stubs/jaxws-api-2.0:${testdir}/../../../stubs/servlet-api-2.4