diff --git a/java/ql/lib/ext/experimental/io.undertow.server.handlers.resource.model.yml b/java/ql/lib/ext/experimental/io.undertow.server.handlers.resource.model.yml deleted file mode 100644 index 5c86c75522c9..000000000000 --- a/java/ql/lib/ext/experimental/io.undertow.server.handlers.resource.model.yml +++ /dev/null @@ -1,8 +0,0 @@ -extensions: - - addsTo: - pack: codeql/java-all - extensible: experimentalSummaryModel - data: - - ["io.undertow.server.handlers.resource", "Resource", True, "getFile", "", "", "Argument[this]", "ReturnValue", "taint", "manual", "unsafe-url-forward"] - - ["io.undertow.server.handlers.resource", "Resource", True, "getFilePath", "", "", "Argument[this]", "ReturnValue", "taint", "manual", "unsafe-url-forward"] - - ["io.undertow.server.handlers.resource", "Resource", True, "getPath", "", "", "Argument[this]", "ReturnValue", "taint", "manual", "unsafe-url-forward"] diff --git a/java/ql/lib/ext/experimental/java.nio.file.model.yml b/java/ql/lib/ext/experimental/java.nio.file.model.yml deleted file mode 100644 index 647d72329d0b..000000000000 --- a/java/ql/lib/ext/experimental/java.nio.file.model.yml +++ /dev/null @@ -1,10 +0,0 @@ -extensions: - - addsTo: - pack: codeql/java-all - extensible: experimentalSummaryModel - data: - - ["java.nio.file", "Path", True, "normalize", "", "", "Argument[this]", "ReturnValue", "taint", "manual", "unsafe-url-forward"] - - ["java.nio.file", "Path", True, "resolve", "", "", "Argument[this]", "ReturnValue", "taint", "manual", "unsafe-url-forward"] - - ["java.nio.file", "Path", True, "resolve", "", "", "Argument[0]", "ReturnValue", "taint", "manual", "unsafe-url-forward"] - - ["java.nio.file", "Path", True, "toString", "", "", "Argument[this]", "ReturnValue", "taint", "manual", "unsafe-url-forward"] - - ["java.nio.file", "Paths", True, "get", "", "", "Argument[0..1]", "ReturnValue", "taint", "manual", "unsafe-url-forward"] diff --git a/java/ql/lib/ext/experimental/java.util.concurrent.model.yml b/java/ql/lib/ext/experimental/java.util.concurrent.model.yml index 82ff0a00570a..9484a5f5eb96 100644 --- a/java/ql/lib/ext/experimental/java.util.concurrent.model.yml +++ b/java/ql/lib/ext/experimental/java.util.concurrent.model.yml @@ -4,4 +4,3 @@ extensions: extensible: experimentalSinkModel data: - ["java.util.concurrent", "TimeUnit", True, "sleep", "", "", "Argument[0]", "thread-pause", "manual", "thread-resource-abuse"] - - ["java.util.concurrent", "TimeUnit", True, "sleep", "", "", "Argument[0]", "thread-pause", "manual", "unsafe-url-forward"] diff --git a/java/ql/lib/ext/experimental/javax.servlet.http.model.yml b/java/ql/lib/ext/experimental/javax.servlet.http.model.yml index db140149a99f..04681b300cab 100644 --- a/java/ql/lib/ext/experimental/javax.servlet.http.model.yml +++ b/java/ql/lib/ext/experimental/javax.servlet.http.model.yml @@ -1,9 +1,4 @@ extensions: - - addsTo: - pack: codeql/java-all - extensible: experimentalSourceModel - data: - - ["javax.servlet.http", "HttpServletRequest", True, "getServletPath", "", "", "ReturnValue", "remote", "manual", "unsafe-url-forward"] - addsTo: pack: codeql/java-all extensible: experimentalSourceModel @@ -13,4 +8,3 @@ extensions: - ["javax.servlet.http", "HttpServletRequest", False, "getRequestURI", "()", "", "ReturnValue", "uri-path", "manual", "permissive-dot-regex-query"] - ["javax.servlet.http", "HttpServletRequest", False, "getRequestURL", "()", "", "ReturnValue", "uri-path", "manual", "permissive-dot-regex-query"] - ["javax.servlet.http", "HttpServletRequest", False, "getServletPath", "()", "", "ReturnValue", "uri-path", "manual", "permissive-dot-regex-query"] - diff --git a/java/ql/lib/ext/experimental/org.springframework.core.io.model.yml b/java/ql/lib/ext/experimental/org.springframework.core.io.model.yml deleted file mode 100644 index e929260f21bf..000000000000 --- a/java/ql/lib/ext/experimental/org.springframework.core.io.model.yml +++ /dev/null @@ -1,16 +0,0 @@ -extensions: - - addsTo: - pack: codeql/java-all - extensible: experimentalSinkModel - data: - - ["org.springframework.core.io", "ClassPathResource", True, "getFilename", "", "", "Argument[this]", "get-resource", "manual", "unsafe-url-forward"] - - ["org.springframework.core.io", "ClassPathResource", True, "getPath", "", "", "Argument[this]", "get-resource", "manual", "unsafe-url-forward"] - - ["org.springframework.core.io", "ClassPathResource", True, "getURL", "", "", "Argument[this]", "get-resource", "manual", "unsafe-url-forward"] - - ["org.springframework.core.io", "ClassPathResource", True, "resolveURL", "", "", "Argument[this]", "get-resource", "manual", "unsafe-url-forward"] - - addsTo: - pack: codeql/java-all - extensible: experimentalSummaryModel - data: - - ["org.springframework.core.io", "ClassPathResource", False, "ClassPathResource", "", "", "Argument[0]", "Argument[this]", "taint", "manual", "unsafe-url-forward"] - - ["org.springframework.core.io", "Resource", True, "createRelative", "", "", "Argument[0]", "ReturnValue", "taint", "manual", "unsafe-url-forward"] - - ["org.springframework.core.io", "ResourceLoader", True, "getResource", "", "", "Argument[0]", "ReturnValue", "taint", "manual", "unsafe-url-forward"] diff --git a/java/ql/lib/ext/experimental/jakarta.servlet.http.model.yml b/java/ql/lib/ext/jakarta.servlet.http.model.yml similarity index 50% rename from java/ql/lib/ext/experimental/jakarta.servlet.http.model.yml rename to java/ql/lib/ext/jakarta.servlet.http.model.yml index 9500beba15b6..5a83b1ac08d8 100644 --- a/java/ql/lib/ext/experimental/jakarta.servlet.http.model.yml +++ b/java/ql/lib/ext/jakarta.servlet.http.model.yml @@ -1,6 +1,6 @@ extensions: - addsTo: pack: codeql/java-all - extensible: experimentalSourceModel + extensible: sourceModel data: - - ["jakarta.servlet.http", "HttpServletRequest", True, "getServletPath", "", "", "ReturnValue", "remote", "manual", "unsafe-url-forward"] + - ["jakarta.servlet.http", "HttpServletRequest", True, "getServletPath", "", "", "ReturnValue", "remote", "manual"] diff --git a/java/ql/lib/ext/jakarta.servlet.model.yml b/java/ql/lib/ext/jakarta.servlet.model.yml new file mode 100644 index 000000000000..be2feeb3c375 --- /dev/null +++ b/java/ql/lib/ext/jakarta.servlet.model.yml @@ -0,0 +1,7 @@ +extensions: + - addsTo: + pack: codeql/java-all + extensible: sinkModel + data: + - ["jakarta.servlet", "ServletContext", True, "getRequestDispatcher", "(String)", "", "Argument[0]", "url-forward", "manual"] + - ["jakarta.servlet", "ServletRequest", True, "getRequestDispatcher", "(String)", "", "Argument[0]", "url-forward", "manual"] diff --git a/java/ql/lib/ext/javax.portlet.model.yml b/java/ql/lib/ext/javax.portlet.model.yml new file mode 100644 index 000000000000..15d108866247 --- /dev/null +++ b/java/ql/lib/ext/javax.portlet.model.yml @@ -0,0 +1,6 @@ +extensions: + - addsTo: + pack: codeql/java-all + extensible: sinkModel + data: + - ["javax.portlet", "PortletContext", True, "getRequestDispatcher", "(String)", "", "Argument[0]", "url-forward", "manual"] diff --git a/java/ql/lib/ext/javax.servlet.http.model.yml b/java/ql/lib/ext/javax.servlet.http.model.yml index afac25e4bc85..ec35445d199a 100644 --- a/java/ql/lib/ext/javax.servlet.http.model.yml +++ b/java/ql/lib/ext/javax.servlet.http.model.yml @@ -18,6 +18,8 @@ extensions: - ["javax.servlet.http", "HttpServletRequest", False, "getRemoteUser", "()", "", "ReturnValue", "remote", "manual"] - ["javax.servlet.http", "HttpServletRequest", False, "getRequestURI", "()", "", "ReturnValue", "remote", "manual"] - ["javax.servlet.http", "HttpServletRequest", False, "getRequestURL", "()", "", "ReturnValue", "remote", "manual"] + - ["javax.servlet.http", "HttpServletRequest", False, "getServletPath", "()", "", "ReturnValue", "remote", "manual"] + - addsTo: pack: codeql/java-all extensible: sinkModel diff --git a/java/ql/lib/ext/javax.servlet.model.yml b/java/ql/lib/ext/javax.servlet.model.yml index 581863c74f7e..d27011c6e127 100644 --- a/java/ql/lib/ext/javax.servlet.model.yml +++ b/java/ql/lib/ext/javax.servlet.model.yml @@ -14,6 +14,8 @@ extensions: extensible: sinkModel data: - ["javax.servlet", "ServletContext", True, "getResourceAsStream", "(String)", "", "Argument[0]", "path-injection", "ai-manual"] + - ["javax.servlet", "ServletContext", True, "getRequestDispatcher", "(String)", "", "Argument[0]", "url-forward", "manual"] + - ["javax.servlet", "ServletRequest", True, "getRequestDispatcher", "(String)", "", "Argument[0]", "url-forward", "manual"] - addsTo: pack: codeql/java-all extensible: summaryModel diff --git a/java/ql/lib/ext/org.kohsuke.stapler.model.yml b/java/ql/lib/ext/org.kohsuke.stapler.model.yml index 63bbdbfd52a6..ca9f08ba78c9 100644 --- a/java/ql/lib/ext/org.kohsuke.stapler.model.yml +++ b/java/ql/lib/ext/org.kohsuke.stapler.model.yml @@ -9,7 +9,7 @@ extensions: - ["org.kohsuke.stapler", "HttpResponses", True, "staticResource", "(URL,long)", "", "Argument[0]", "request-forgery", "manual"] - ["org.kohsuke.stapler", "HttpResponses", True, "html", "(String)", "", "Argument[0]", "html-injection", "manual"] - ["org.kohsuke.stapler", "HttpResponses", True, "literalHtml", "(String)", "", "Argument[0]", "html-injection", "manual"] - - ["org.kohsuke.stapler", "StaplerResponse", True, "forward", "(Object,String,StaplerRequest)", "", "Argument[1]", "request-forgery", "manual"] + - ["org.kohsuke.stapler", "StaplerResponse", True, "forward", "(Object,String,StaplerRequest)", "", "Argument[1]", "url-forward", "manual"] - ["org.kohsuke.stapler", "StaplerResponse", True, "sendRedirect2", "(String)", "", "Argument[0]", "url-redirection", "manual"] - ["org.kohsuke.stapler", "StaplerResponse", True, "sendRedirect", "(int,String)", "", "Argument[1]", "url-redirection", "manual"] - ["org.kohsuke.stapler", "StaplerResponse", True, "sendRedirect", "(String)", "", "Argument[0]", "url-redirection", "manual"] diff --git a/java/ql/lib/ext/org.springframework.web.portlet.model.yml b/java/ql/lib/ext/org.springframework.web.portlet.model.yml new file mode 100644 index 000000000000..ba90b531c33e --- /dev/null +++ b/java/ql/lib/ext/org.springframework.web.portlet.model.yml @@ -0,0 +1,7 @@ +extensions: + - addsTo: + pack: codeql/java-all + extensible: sinkModel + data: + - ["org.springframework.web.portlet", "ModelAndView", False, "ModelAndView", "", "", "Argument[0]", "url-forward", "manual"] + - ["org.springframework.web.portlet", "ModelAndView", False, "setViewName", "", "", "Argument[0]", "url-forward", "manual"] diff --git a/java/ql/lib/ext/org.springframework.web.servlet.model.yml b/java/ql/lib/ext/org.springframework.web.servlet.model.yml new file mode 100644 index 000000000000..acdda3d569f6 --- /dev/null +++ b/java/ql/lib/ext/org.springframework.web.servlet.model.yml @@ -0,0 +1,7 @@ +extensions: + - addsTo: + pack: codeql/java-all + extensible: sinkModel + data: + - ["org.springframework.web.servlet", "ModelAndView", False, "ModelAndView", "", "", "Argument[0]", "url-forward", "manual"] + - ["org.springframework.web.servlet", "ModelAndView", False, "setViewName", "", "", "Argument[0]", "url-forward", "manual"] diff --git a/java/ql/lib/semmle/code/java/JDK.qll b/java/ql/lib/semmle/code/java/JDK.qll index 7623cc87393c..55d420dbcaec 100644 --- a/java/ql/lib/semmle/code/java/JDK.qll +++ b/java/ql/lib/semmle/code/java/JDK.qll @@ -38,6 +38,13 @@ class StringLengthMethod extends Method { StringLengthMethod() { this.hasName("length") and this.getDeclaringType() instanceof TypeString } } +/** The `contains()` method of the class `java.lang.String`. */ +class StringContainsMethod extends Method { + StringContainsMethod() { + this.hasName("contains") and this.getDeclaringType() instanceof TypeString + } +} + /** * The methods on the class `java.lang.String` that are used to perform partial matches with a specified substring or char. */ diff --git a/java/ql/lib/semmle/code/java/frameworks/Networking.qll b/java/ql/lib/semmle/code/java/frameworks/Networking.qll index c473cc9fc09c..f86cecd5b4ee 100644 --- a/java/ql/lib/semmle/code/java/frameworks/Networking.qll +++ b/java/ql/lib/semmle/code/java/frameworks/Networking.qll @@ -24,6 +24,11 @@ class TypeUrl extends RefType { TypeUrl() { this.hasQualifiedName("java.net", "URL") } } +/** The type `java.net.URLDecoder`. */ +class TypeUrlDecoder extends RefType { + TypeUrlDecoder() { this.hasQualifiedName("java.net", "URLDecoder") } +} + /** The type `java.net.URI`. */ class TypeUri extends RefType { TypeUri() { this.hasQualifiedName("java.net", "URI") } @@ -157,6 +162,14 @@ class UrlOpenConnectionMethod extends Method { } } +/** The method `java.net.URLDecoder::decode`. */ +class UrlDecodeMethod extends Method { + UrlDecodeMethod() { + this.getDeclaringType() instanceof TypeUrlDecoder and + this.getName() = "decode" + } +} + /** The method `javax.net.SocketFactory::createSocket`. */ class CreateSocketMethod extends Method { CreateSocketMethod() { diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index 4ca08f5becc6..77803e3e27dc 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -64,7 +64,11 @@ private predicate exactPathMatchGuard(Guard g, Expr e, boolean branch) { ) } -private class ExactPathMatchSanitizer extends PathInjectionSanitizer { +/** + * A sanitizer that protects against path injection vulnerabilities + * by checking for a matching path. + */ +class ExactPathMatchSanitizer extends PathInjectionSanitizer { ExactPathMatchSanitizer() { this = DataFlow::BarrierGuard::getABarrierNode() or diff --git a/java/ql/lib/semmle/code/java/security/UrlForwardQuery.qll b/java/ql/lib/semmle/code/java/security/UrlForwardQuery.qll new file mode 100644 index 000000000000..2ca38d695512 --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/UrlForwardQuery.qll @@ -0,0 +1,203 @@ +/** Provides classes and a taint-tracking configuration to reason about unsafe URL forwarding. */ + +import java +private import semmle.code.java.dataflow.ExternalFlow +private import semmle.code.java.dataflow.FlowSources +private import semmle.code.java.dataflow.StringPrefixes +private import semmle.code.java.security.PathSanitizer +private import semmle.code.java.controlflow.Guards +private import semmle.code.java.security.Sanitizers + +/** A URL forward sink. */ +abstract class UrlForwardSink extends DataFlow::Node { } + +/** + * 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 reachable from a Spring entry point. + */ +private class SpringUrlForwardPrefixSink extends UrlForwardSink { + SpringUrlForwardPrefixSink() { + any(SpringRequestMappingMethod srmm).polyCalls*(this.getEnclosingCallable()) and + appendedToForwardPrefix(this) + } +} + +pragma[nomagic] +private predicate appendedToForwardPrefix(DataFlow::ExprNode exprNode) { + exists(ForwardPrefix fp | exprNode.asExpr() = fp.getAnAppendedExpression()) +} + +private class ForwardPrefix extends InterestingPrefix { + ForwardPrefix() { this.getStringValue() = "forward:" } + + override int getOffset() { result = 0 } +} + +/** A URL forward barrier. */ +abstract class UrlForwardBarrier extends DataFlow::Node { } + +private class PrimitiveBarrier extends UrlForwardBarrier instanceof SimpleTypeSanitizer { } + +/** + * A barrier for values appended to a "redirect:" prefix. + * These results are excluded because they should be handled + * by the `java/unvalidated-url-redirection` query instead. + */ +private class RedirectPrefixBarrier extends UrlForwardBarrier { + RedirectPrefixBarrier() { this.asExpr() = any(RedirectPrefix fp).getAnAppendedExpression() } +} + +private class RedirectPrefix extends InterestingPrefix { + RedirectPrefix() { this.getStringValue() = "redirect:" } + + override int getOffset() { result = 0 } +} + +/** + * A value that is the result of prepending a string that prevents + * any value from controlling the path of a URL. + */ +private class FollowsBarrierPrefix extends UrlForwardBarrier { + FollowsBarrierPrefix() { this.asExpr() = any(BarrierPrefix fp).getAnAppendedExpression() } +} + +private class BarrierPrefix extends InterestingPrefix { + int offset; + + BarrierPrefix() { + // Matches strings that look like when prepended to untrusted input, they will restrict + // the path of a URL: for example, anything containing `?` or `#`. + exists(this.getStringValue().regexpFind("[?#]", 0, offset)) + or + this.(CharacterLiteral).getValue() = ["?", "#"] and offset = 0 + } + + override int getOffset() { result = offset } +} + +/** + * A barrier that protects against path injection vulnerabilities + * while accounting for URL encoding. + */ +private class UrlPathBarrier extends UrlForwardBarrier instanceof PathInjectionSanitizer { + UrlPathBarrier() { + this instanceof ExactPathMatchSanitizer or + this instanceof NoUrlEncodingBarrier or + this instanceof FullyDecodesUrlBarrier + } +} + +/** A call to a method that decodes a URL. */ +abstract class UrlDecodeCall extends MethodCall { } + +private class DefaultUrlDecodeCall extends UrlDecodeCall { + DefaultUrlDecodeCall() { + this.getMethod() instanceof UrlDecodeMethod or + this.getMethod().hasQualifiedName("org.eclipse.jetty.util.URIUtil", "URIUtil", "decodePath") + } +} + +/** A repeated call to a method that decodes a URL. */ +abstract class RepeatedUrlDecodeCall extends MethodCall { } + +private class DefaultRepeatedUrlDecodeCall extends RepeatedUrlDecodeCall instanceof UrlDecodeCall { + DefaultRepeatedUrlDecodeCall() { this.getAnEnclosingStmt() instanceof LoopStmt } +} + +/** A method call that checks a string for URL encoding. */ +abstract class CheckUrlEncodingCall extends MethodCall { } + +private class DefaultCheckUrlEncodingCall extends CheckUrlEncodingCall { + DefaultCheckUrlEncodingCall() { + this.getMethod() instanceof StringContainsMethod and + this.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "%" + } +} + +/** A guard that looks for a method call that checks for URL encoding. */ +private class CheckUrlEncodingGuard extends Guard instanceof CheckUrlEncodingCall { + Expr getCheckedExpr() { result = this.(MethodCall).getQualifier() } +} + +/** Holds if `g` is guard for a URL that does not contain URL encoding. */ +private predicate noUrlEncodingGuard(Guard g, Expr e, boolean branch) { + e = g.(CheckUrlEncodingGuard).getCheckedExpr() and + branch = false + or + branch = false and + g.(Expr).getType() instanceof BooleanType and + ( + exists(CheckUrlEncodingCall call, AssignExpr ae | + ae.getSource() = call and + e = call.getQualifier() and + g = ae.getDest() + ) + or + exists(CheckUrlEncodingCall call, LocalVariableDeclExpr vde | + vde.getInitOrPatternSource() = call and + e = call.getQualifier() and + g = vde.getAnAccess() + ) + ) +} + +/** A barrier for URLs that do not contain URL encoding. */ +private class NoUrlEncodingBarrier extends DataFlow::Node { + NoUrlEncodingBarrier() { this = DataFlow::BarrierGuard::getABarrierNode() } +} + +/** Holds if `g` is guard for a URL that is fully decoded. */ +private predicate fullyDecodesUrlGuard(Expr e) { + exists(CheckUrlEncodingGuard g, RepeatedUrlDecodeCall decodeCall | + e = g.getCheckedExpr() and + g.controls(decodeCall.getBasicBlock(), true) + ) +} + +/** A barrier for URLs that are fully decoded. */ +private class FullyDecodesUrlBarrier extends DataFlow::Node { + FullyDecodesUrlBarrier() { + exists(Variable v, Expr e | this.asExpr() = v.getAnAccess() | + fullyDecodesUrlGuard(e) and + e = v.getAnAccess() and + e.getBasicBlock().bbDominates(this.asExpr().getBasicBlock()) + ) + } +} + +/** + * A taint-tracking configuration for reasoning about URL forwarding. + */ +module UrlForwardFlowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + source instanceof ThreatModelFlowSource and + // excluded due to FPs + not exists(MethodCall mc, Method m | + m instanceof HttpServletRequestGetRequestUriMethod or + m instanceof HttpServletRequestGetRequestUrlMethod or + m instanceof HttpServletRequestGetPathMethod + | + mc.getMethod() = m and + mc = source.asExpr() + ) + } + + predicate isSink(DataFlow::Node sink) { sink instanceof UrlForwardSink } + + predicate isBarrier(DataFlow::Node node) { node instanceof UrlForwardBarrier } + + DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } +} + +/** + * Taint-tracking flow for URL forwarding. + */ +module UrlForwardFlow = TaintTracking::Global; diff --git a/java/ql/src/Security/CWE/CWE-552/UrlForward.java b/java/ql/src/Security/CWE/CWE-552/UrlForward.java new file mode 100644 index 000000000000..db701fbcd9a8 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-552/UrlForward.java @@ -0,0 +1,17 @@ +public class UrlForward extends HttpServlet { + private static final String VALID_FORWARD = "https://cwe.mitre.org/data/definitions/552.html"; + + protected void doGet(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); + + // 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); + } + } +} diff --git a/java/ql/src/Security/CWE/CWE-552/UrlForward.qhelp b/java/ql/src/Security/CWE/CWE-552/UrlForward.qhelp new file mode 100644 index 000000000000..71316385335c --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-552/UrlForward.qhelp @@ -0,0 +1,36 @@ + + + + + +

Directly incorporating user input into a URL forward request without validating the input +can cause file information disclosure by allowing an attacker to access unauthorized URLs.

+ + + + +

To guard against untrusted URL forwarding, you should avoid putting user input +directly into a forwarded URL. Instead, you should maintain a list of authorized +URLs on the server, then choose from that list based on the user input provided.

+ + + + +

The following example shows an HTTP request parameter being used directly in a URL forward +without validating the input, which may cause file information disclosure. +It also shows how to remedy the problem by validating the user input against a known fixed string. +

+ + + + + + +
  • OWASP: + Unvalidated Redirects and Forwards Cheat Sheet. +
    • + +     +     diff --git a/java/ql/src/Security/CWE/CWE-552/UrlForward.ql b/java/ql/src/Security/CWE/CWE-552/UrlForward.ql new file mode 100644 index 000000000000..91e244a81522 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-552/UrlForward.ql @@ -0,0 +1,21 @@ +/** + * @name URL forward from a remote source + * @description URL forward based on unvalidated user input + * may cause file information disclosure. + * @kind path-problem + * @problem.severity error + * @security-severity 7.5 + * @precision high + * @id java/unvalidated-url-forward + * @tags security + * external/cwe/cwe-552 + */ + +import java +import semmle.code.java.security.UrlForwardQuery +import UrlForwardFlow::PathGraph + +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/src/change-notes/2024-03-06-url-forward-query.md b/java/ql/src/change-notes/2024-03-06-url-forward-query.md new file mode 100644 index 000000000000..46028bda4f21 --- /dev/null +++ b/java/ql/src/change-notes/2024-03-06-url-forward-query.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* The query `java/unsafe-url-forward-dispatch-load` has been promoted from experimental to the main query pack as `java/unvalidated-url-forward`. Its results will now appear by default. This query was originally submitted as an experimental query [by @haby0](https://github.com/github/codeql/pull/6240) and [by @luchua-bc](https://github.com/github/codeql/pull/7286). diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeLoadSpringResource.java b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeLoadSpringResource.java deleted file mode 100644 index ce462fe490ef..000000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeLoadSpringResource.java +++ /dev/null @@ -1,21 +0,0 @@ -//BAD: no path validation in Spring resource loading -@GetMapping("/file") -public String getFileContent(@RequestParam(name="fileName") String fileName) { - ClassPathResource clr = new ClassPathResource(fileName); - - File file = ResourceUtils.getFile(fileName); - - Resource resource = resourceLoader.getResource(fileName); -} - -//GOOD: check for a trusted prefix, ensuring path traversal is not used to erase that prefix in Spring resource loading: -@GetMapping("/file") -public String getFileContent(@RequestParam(name="fileName") String fileName) { - if (!fileName.contains("..") && fileName.hasPrefix("/public-content")) { - ClassPathResource clr = new ClassPathResource(fileName); - - File file = ResourceUtils.getFile(fileName); - - Resource resource = resourceLoader.getResource(fileName); - } -} diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeResourceGet.java b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeResourceGet.java deleted file mode 100644 index 8b3583bf59e2..000000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeResourceGet.java +++ /dev/null @@ -1,18 +0,0 @@ -// BAD: no URI validation -URL url = request.getServletContext().getResource(requestUrl); -url = getClass().getResource(requestUrl); -InputStream in = url.openStream(); - -InputStream in = request.getServletContext().getResourceAsStream(requestPath); -in = getClass().getClassLoader().getResourceAsStream(requestPath); - -// GOOD: check for a trusted prefix, ensuring path traversal is not used to erase that prefix: -// (alternatively use `Path.normalize` instead of checking for `..`) -if (!requestPath.contains("..") && requestPath.startsWith("/trusted")) { - InputStream in = request.getServletContext().getResourceAsStream(requestPath); -} - -Path path = Paths.get(requestUrl).normalize().toRealPath(); -if (path.startsWith("/trusted")) { - URL url = request.getServletContext().getResource(path.toString()); -} diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java deleted file mode 100644 index 88a794ab9c64..000000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java +++ /dev/null @@ -1,11 +0,0 @@ -// BAD: no URI validation -String returnURL = request.getParameter("returnURL"); -RequestDispatcher rd = sc.getRequestDispatcher(returnURL); -rd.forward(request, response); - -// GOOD: check for a trusted prefix, ensuring path traversal is not used to erase that prefix: -// (alternatively use `Path.normalize` instead of checking for `..`) -if (!returnURL.contains("..") && returnURL.hasPrefix("/pages")) { ... } -// Also GOOD: check for a forbidden prefix, ensuring URL-encoding is not used to evade the check: -// (alternatively use `URLDecoder.decode` before `hasPrefix`) -if (returnURL.hasPrefix("/internal") && !returnURL.contains("%")) { ... } \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.java b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.java deleted file mode 100644 index d159c4057362..000000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.java +++ /dev/null @@ -1,38 +0,0 @@ -import java.io.IOException; -import javax.servlet.ServletException; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; -import org.springframework.stereotype.Controller; -import org.springframework.web.bind.annotation.GetMapping; -import org.springframework.web.servlet.ModelAndView; - -@Controller -public class UnsafeUrlForward { - - @GetMapping("/bad1") - public ModelAndView bad1(String url) { - return new ModelAndView(url); - } - - @GetMapping("/bad2") - public void bad2(String url, HttpServletRequest request, HttpServletResponse response) { - try { - request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").include(request, response); - } catch (ServletException e) { - e.printStackTrace(); - } catch (IOException e) { - e.printStackTrace(); - } - } - - @GetMapping("/good1") - public void good1(String url, HttpServletRequest request, HttpServletResponse response) { - try { - request.getRequestDispatcher("/index.jsp?token=" + url).forward(request, response); - } catch (ServletException e) { - e.printStackTrace(); - } catch (IOException e) { - e.printStackTrace(); - } - } -} diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp deleted file mode 100644 index 2e425952edc3..000000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp +++ /dev/null @@ -1,70 +0,0 @@ - - - - - -

    Constructing a server-side redirect path with user input could allow an attacker to download application binaries -(including application classes or jar files) or view arbitrary files within protected directories.

    - -     - - -

    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 against allowed (e.g., must come within user_content/) or disallowed -(e.g. must not come within /internal) paths, ensuring that neither path traversal using ../ -or URL encoding are used to evade these checks. -

    - -     - - -

    The following examples show the bad case and the good case respectively. -The bad methods show an HTTP request parameter being used directly in a URL forward -without validating the input, which may cause file leakage. In the good1 method, -ordinary forwarding requests are shown, which will not cause file leakage. -

    - - - -

    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 -attacks. It also shows how to remedy the problem by validating the user input. -

    - - - -

    The following examples show an HTTP request parameter or request path being used directly to -retrieve a resource of a Java EE application without validating the input, which allows sensitive -file exposure attacks. It also shows how to remedy the problem by validating the user input. -

    - - - -

    The following examples show an HTTP request parameter being used directly to retrieve a resource - of a Java Spring application without validating the input, which allows sensitive file exposure - attacks. It also shows how to remedy the problem by validating the user input. -

    - - -     - -
  • File Disclosure: - Unsafe Url Forward. -
    • -
  • Jakarta Javadoc: - Security vulnerability with unsafe usage of RequestDispatcher. -
    • -
  • Micro Focus: - File Disclosure: J2EE -
    • -
  • CVE-2015-5174: - Apache Tomcat 6.0/7.0/8.0/9.0 Servletcontext getResource/getResourceAsStream/getResourcePaths Path Traversal -
    • -
  • CVE-2019-3799: - CVE-2019-3799 - Spring-Cloud-Config-Server Directory Traversal < 2.1.2, 2.0.4, 1.4.6 -
    • -     -     diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql deleted file mode 100644 index 15dd04a0a76f..000000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql +++ /dev/null @@ -1,64 +0,0 @@ -/** - * @name Unsafe URL forward, dispatch, or load from remote source - * @description URL forward, dispatch, or load based on unvalidated user-input - * may cause file information disclosure. - * @kind path-problem - * @problem.severity error - * @precision high - * @id java/unsafe-url-forward-dispatch-load - * @tags security - * experimental - * external/cwe/cwe-552 - */ - -import java -import UnsafeUrlForward -import semmle.code.java.dataflow.FlowSources -import semmle.code.java.dataflow.TaintTracking -import experimental.semmle.code.java.frameworks.Jsf -import semmle.code.java.security.PathSanitizer -import UnsafeUrlForwardFlow::PathGraph - -module UnsafeUrlForwardFlowConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node source) { - source instanceof ThreatModelFlowSource and - not exists(MethodCall ma, Method m | ma.getMethod() = m | - ( - m instanceof HttpServletRequestGetRequestUriMethod or - m instanceof HttpServletRequestGetRequestUrlMethod or - m instanceof HttpServletRequestGetPathMethod - ) and - ma = source.asExpr() - ) - } - - predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeUrlForwardSink } - - predicate isBarrier(DataFlow::Node node) { - node instanceof UnsafeUrlForwardSanitizer or - node instanceof PathInjectionSanitizer - } - - DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } - - predicate isAdditionalFlowStep(DataFlow::Node prev, DataFlow::Node succ) { - exists(MethodCall ma | - ( - ma.getMethod() instanceof GetServletResourceMethod or - ma.getMethod() instanceof GetFacesResourceMethod or - ma.getMethod() instanceof GetClassResourceMethod or - ma.getMethod() instanceof GetClassLoaderResourceMethod or - ma.getMethod() instanceof GetWildflyResourceMethod - ) and - ma.getArgument(0) = prev.asExpr() and - ma = succ.asExpr() - ) - } -} - -module UnsafeUrlForwardFlow = TaintTracking::Global; - -from UnsafeUrlForwardFlow::PathNode source, UnsafeUrlForwardFlow::PathNode sink -where UnsafeUrlForwardFlow::flowPath(source, sink) -select sink.getNode(), source, sink, "Potentially untrusted URL forward due to $@.", - source.getNode(), "user-provided value" diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll deleted file mode 100644 index 1baec2dd1fa5..000000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ /dev/null @@ -1,163 +0,0 @@ -import java -private import experimental.semmle.code.java.frameworks.Jsf -private import semmle.code.java.dataflow.ExternalFlow -private import semmle.code.java.dataflow.FlowSources -private import semmle.code.java.dataflow.StringPrefixes -private import semmle.code.java.frameworks.javaee.ejb.EJBRestrictions -private import experimental.semmle.code.java.frameworks.SpringResource -private import semmle.code.java.security.Sanitizers - -private class ActiveModels extends ActiveExperimentalModels { - ActiveModels() { this = "unsafe-url-forward" } -} - -/** A sink for unsafe URL forward vulnerabilities. */ -abstract class UnsafeUrlForwardSink extends DataFlow::Node { } - -/** A sanitizer for unsafe URL forward vulnerabilities. */ -abstract class UnsafeUrlForwardSanitizer extends DataFlow::Node { } - -/** An argument to `getRequestDispatcher`. */ -private class RequestDispatcherSink extends UnsafeUrlForwardSink { - RequestDispatcherSink() { - exists(MethodCall ma | - ma.getMethod() instanceof GetRequestDispatcherMethod and - ma.getArgument(0) = this.asExpr() - ) - } -} - -/** The `getResource` method of `Class`. */ -class GetClassResourceMethod extends Method { - GetClassResourceMethod() { - this.getDeclaringType() instanceof TypeClass and - this.hasName("getResource") - } -} - -/** The `getResourceAsStream` method of `Class`. */ -class GetClassResourceAsStreamMethod extends Method { - GetClassResourceAsStreamMethod() { - this.getDeclaringType() instanceof TypeClass and - this.hasName("getResourceAsStream") - } -} - -/** The `getResource` method of `ClassLoader`. */ -class GetClassLoaderResourceMethod extends Method { - GetClassLoaderResourceMethod() { - this.getDeclaringType() instanceof ClassLoaderClass and - this.hasName("getResource") - } -} - -/** The `getResourceAsStream` method of `ClassLoader`. */ -class GetClassLoaderResourceAsStreamMethod extends Method { - GetClassLoaderResourceAsStreamMethod() { - this.getDeclaringType() instanceof ClassLoaderClass and - this.hasName("getResourceAsStream") - } -} - -/** The JBoss class `FileResourceManager`. */ -class FileResourceManager extends RefType { - FileResourceManager() { - this.hasQualifiedName("io.undertow.server.handlers.resource", "FileResourceManager") - } -} - -/** The JBoss method `getResource` of `FileResourceManager`. */ -class GetWildflyResourceMethod extends Method { - GetWildflyResourceMethod() { - this.getDeclaringType().getASupertype*() instanceof FileResourceManager and - this.hasName("getResource") - } -} - -/** The JBoss class `VirtualFile`. */ -class VirtualFile extends RefType { - VirtualFile() { this.hasQualifiedName("org.jboss.vfs", "VirtualFile") } -} - -/** The JBoss method `getChild` of `FileResourceManager`. */ -class GetVirtualFileChildMethod extends Method { - GetVirtualFileChildMethod() { - this.getDeclaringType().getASupertype*() instanceof VirtualFile and - this.hasName("getChild") - } -} - -/** An argument to `getResource()` or `getResourceAsStream()`. */ -private class GetResourceSink extends UnsafeUrlForwardSink { - GetResourceSink() { - sinkNode(this, "request-forgery") - or - sinkNode(this, "get-resource") - or - exists(MethodCall ma | - ( - ma.getMethod() instanceof GetServletResourceAsStreamMethod or - ma.getMethod() instanceof GetFacesResourceAsStreamMethod or - ma.getMethod() instanceof GetClassResourceAsStreamMethod or - ma.getMethod() instanceof GetClassLoaderResourceAsStreamMethod or - ma.getMethod() instanceof GetVirtualFileChildMethod - ) and - ma.getArgument(0) = this.asExpr() - ) - } -} - -/** A sink for methods that load Spring resources. */ -private class SpringResourceSink extends UnsafeUrlForwardSink { - SpringResourceSink() { - exists(MethodCall ma | - ma.getMethod() instanceof GetResourceUtilsMethod and - ma.getArgument(0) = this.asExpr() - ) - } -} - -/** An argument to `new ModelAndView` or `ModelAndView.setViewName`. */ -private class SpringModelAndViewSink extends UnsafeUrlForwardSink { - SpringModelAndViewSink() { - exists(ClassInstanceExpr cie | - cie.getConstructedType() instanceof ModelAndView and - cie.getArgument(0) = this.asExpr() - ) - or - exists(SpringModelAndViewSetViewNameCall smavsvnc | smavsvnc.getArgument(0) = this.asExpr()) - } -} - -private class PrimitiveSanitizer extends UnsafeUrlForwardSanitizer instanceof SimpleTypeSanitizer { -} - -private class SanitizingPrefix extends InterestingPrefix { - SanitizingPrefix() { - not this.getStringValue().matches("/WEB-INF/%") and - not this.getStringValue() = "forward:" - } - - override int getOffset() { result = 0 } -} - -private class FollowsSanitizingPrefix extends UnsafeUrlForwardSanitizer { - FollowsSanitizingPrefix() { this.asExpr() = any(SanitizingPrefix fp).getAnAppendedExpression() } -} - -private class ForwardPrefix extends InterestingPrefix { - ForwardPrefix() { this.getStringValue() = "forward:" } - - override int getOffset() { result = 0 } -} - -/** - * An expression appended (perhaps indirectly) to `"forward:"`, and which - * is reachable from a Spring entry point. - */ -private class SpringUrlForwardSink extends UnsafeUrlForwardSink { - SpringUrlForwardSink() { - any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable()) and - this.asExpr() = any(ForwardPrefix fp).getAnAppendedExpression() - } -} diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeLoadSpringResource.java b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeLoadSpringResource.java deleted file mode 100644 index c7e114aede35..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeLoadSpringResource.java +++ /dev/null @@ -1,155 +0,0 @@ -package com.example; - -import java.io.File; -import java.io.FileReader; -import java.io.InputStreamReader; -import java.io.IOException; -import java.io.Reader; -import java.io.UnsupportedEncodingException; -import java.net.URLDecoder; -import java.nio.file.Files; - -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.core.io.ClassPathResource; -import org.springframework.core.io.Resource; -import org.springframework.core.io.ResourceLoader; -import org.springframework.util.ResourceUtils; -import org.springframework.util.StringUtils; -import org.springframework.web.bind.annotation.GetMapping; -import org.springframework.web.bind.annotation.RequestParam; -import org.springframework.web.bind.annotation.RestController; - -/** Sample class of Spring RestController */ -@RestController -public class UnsafeLoadSpringResource { - @GetMapping("/file1") - //BAD: Get resource from ClassPathResource without input validation - public String getFileContent1(@RequestParam(name="fileName") String fileName) { - // A request such as the following can disclose source code and application configuration - // fileName=/../../WEB-INF/views/page.jsp - // fileName=/com/example/package/SampleController.class - ClassPathResource clr = new ClassPathResource(fileName); - char[] buffer = new char[4096]; - StringBuilder out = new StringBuilder(); - try { - Reader in = new FileReader(clr.getFilename()); - for (int numRead; (numRead = in.read(buffer, 0, buffer.length)) > 0; ) { - out.append(buffer, 0, numRead); - } - } catch (IOException ie) { - ie.printStackTrace(); - } - return out.toString(); - } - - @GetMapping("/file1a") - //GOOD: Get resource from ClassPathResource with input path validation - public String getFileContent1a(@RequestParam(name="fileName") String fileName) { - String result = null; - if (fileName.startsWith("/safe_dir") && !fileName.contains("..")) { - ClassPathResource clr = new ClassPathResource(fileName); - char[] buffer = new char[4096]; - StringBuilder out = new StringBuilder(); - try { - Reader in = new InputStreamReader(clr.getInputStream(), "UTF-8"); - for (int numRead; (numRead = in.read(buffer, 0, buffer.length)) > 0; ) { - out.append(buffer, 0, numRead); - } - } catch (IOException ie) { - ie.printStackTrace(); - } - result = out.toString(); - } - return result; - } - - @GetMapping("/file2") - //BAD: Get resource from ResourceUtils without input validation - public String getFileContent2(@RequestParam(name="fileName") String fileName) { - String content = null; - - try { - // A request such as the following can disclose source code and system configuration - // fileName=/etc/hosts - // fileName=file:/etc/hosts - // fileName=/opt/appdir/WEB-INF/views/page.jsp - File file = ResourceUtils.getFile(fileName); - //Read File Content - content = new String(Files.readAllBytes(file.toPath())); - } catch (IOException ie) { - ie.printStackTrace(); - } - return content; - } - - @GetMapping("/file2a") - //GOOD: Get resource from ResourceUtils with input path validation - public String getFileContent2a(@RequestParam(name="fileName") String fileName) { - String content = null; - - if (fileName.startsWith("/safe_dir") && !fileName.contains("..")) { - try { - File file = ResourceUtils.getFile(fileName); - //Read File Content - content = new String(Files.readAllBytes(file.toPath())); - } catch (IOException ie) { - ie.printStackTrace(); - } - } - return content; - } - - @Autowired - ResourceLoader resourceLoader; - - @GetMapping("/file3") - //BAD: Get resource from ResourceLoader (same as application context) without input validation - // Note it is not detected without the generic `resource.getInputStream()` check - public String getFileContent3(@RequestParam(name="fileName") String fileName) { - String content = null; - - try { - // A request such as the following can disclose source code and system configuration - // fileName=/WEB-INF/views/page.jsp - // fileName=/WEB-INF/classes/com/example/package/SampleController.class - // fileName=file:/etc/hosts - Resource resource = resourceLoader.getResource(fileName); - - char[] buffer = new char[4096]; - StringBuilder out = new StringBuilder(); - - Reader in = new InputStreamReader(resource.getInputStream(), "UTF-8"); - for (int numRead; (numRead = in.read(buffer, 0, buffer.length)) > 0; ) { - out.append(buffer, 0, numRead); - } - content = out.toString(); - } catch (IOException ie) { - ie.printStackTrace(); - } - return content; - } - - @GetMapping("/file3a") - //GOOD: Get resource from ResourceLoader (same as application context) with input path validation - public String getFileContent3a(@RequestParam(name="fileName") String fileName) { - String content = null; - - if (fileName.startsWith("/safe_dir") && !fileName.contains("..")) { - try { - Resource resource = resourceLoader.getResource(fileName); - - char[] buffer = new char[4096]; - StringBuilder out = new StringBuilder(); - - Reader in = new InputStreamReader(resource.getInputStream(), "UTF-8"); - for (int numRead; (numRead = in.read(buffer, 0, buffer.length)) > 0; ) { - out.append(buffer, 0, numRead); - } - content = out.toString(); - } catch (IOException ie) { - ie.printStackTrace(); - } - } - return content; - } -} diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeRequestPath.java b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeRequestPath.java deleted file mode 100644 index 2de0cae0d3c5..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeRequestPath.java +++ /dev/null @@ -1,52 +0,0 @@ -import java.io.IOException; - -import javax.servlet.Filter; -import javax.servlet.FilterChain; -import javax.servlet.FilterConfig; -import javax.servlet.ServletException; -import javax.servlet.ServletRequest; -import javax.servlet.ServletResponse; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; - -// @WebFilter("/*") -public class UnsafeRequestPath implements Filter { - private static final String BASE_PATH = "/pages"; - - @Override - // BAD: Request dispatcher from servlet path without check - public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) - throws IOException, ServletException { - 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); - } else { - chain.doFilter(request, response); - } - } - - // GOOD: Request dispatcher from servlet path with check - public void doFilter2(ServletRequest request, ServletResponse response, FilterChain chain) - throws IOException, ServletException { - String path = ((HttpServletRequest) request).getServletPath(); - - if (path.startsWith(BASE_PATH) && !path.contains("..")) { - request.getRequestDispatcher(path).forward(request, response); - } else { - chain.doFilter(request, response); - } - } - - // GOOD: Request dispatcher from servlet path with whitelisted string comparison - public void doFilter3(ServletRequest request, ServletResponse response, FilterChain chain) - throws IOException, ServletException { - String path = ((HttpServletRequest) request).getServletPath(); - - if (path.equals("/comaction")) { - request.getRequestDispatcher(path).forward(request, response); - } else { - chain.doFilter(request, response); - } - } -} diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeResourceGet.java b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeResourceGet.java deleted file mode 100644 index 64c23334f187..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeResourceGet.java +++ /dev/null @@ -1,270 +0,0 @@ -package com.example; - -import java.io.InputStream; -import java.io.IOException; -import java.io.PrintWriter; -import java.nio.file.Path; -import java.nio.file.Paths; -import java.net.URI; -import java.net.URL; -import java.net.URISyntaxException; - -import javax.servlet.http.HttpServlet; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; -import javax.servlet.ServletOutputStream; -import javax.servlet.ServletException; -import javax.servlet.ServletConfig; -import javax.servlet.ServletContext; - -import io.undertow.server.handlers.resource.FileResourceManager; -import io.undertow.server.handlers.resource.Resource; -import org.jboss.vfs.VFS; -import org.jboss.vfs.VirtualFile; - -public class UnsafeResourceGet extends HttpServlet { - private static final String BASE_PATH = "/pages"; - - @Override - // BAD: getResource constructed from `ServletContext` without input validation - protected void doGet(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestUrl = request.getParameter("requestURL"); - ServletOutputStream out = response.getOutputStream(); - - ServletConfig cfg = getServletConfig(); - ServletContext sc = cfg.getServletContext(); - - // A sample request /fake.jsp/../WEB-INF/web.xml can load the web.xml file - URL url = sc.getResource(requestUrl); - - InputStream in = url.openStream(); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - - // GOOD: getResource constructed from `ServletContext` with input validation - protected void doGetGood(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestUrl = request.getParameter("requestURL"); - ServletOutputStream out = response.getOutputStream(); - - ServletConfig cfg = getServletConfig(); - ServletContext sc = cfg.getServletContext(); - - Path path = Paths.get(requestUrl).normalize().toRealPath(); - if (path.startsWith(BASE_PATH)) { - URL url = sc.getResource(path.toString()); - - InputStream in = url.openStream(); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - } - - // GOOD: getResource constructed from `ServletContext` with null check only - protected void doGetGood2(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestUrl = request.getParameter("requestURL"); - PrintWriter writer = response.getWriter(); - - ServletConfig cfg = getServletConfig(); - ServletContext sc = cfg.getServletContext(); - - // A sample request /fake.jsp/../WEB-INF/web.xml can load the web.xml file - URL url = sc.getResource(requestUrl); - if (url == null) { - writer.println("Requested source not found"); - } - } - - // GOOD: getResource constructed from `ServletContext` with `equals` check - protected void doGetGood3(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestUrl = request.getParameter("requestURL"); - ServletOutputStream out = response.getOutputStream(); - - ServletContext sc = request.getServletContext(); - - if (requestUrl.equals("/public/crossdomain.xml")) { - URL url = sc.getResource(requestUrl); - - InputStream in = url.openStream(); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - } - - @Override - // BAD: getResourceAsStream constructed from `ServletContext` without input validation - protected void doPost(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestPath = request.getParameter("requestPath"); - ServletOutputStream out = response.getOutputStream(); - - // A sample request /fake.jsp/../WEB-INF/web.xml can load the web.xml file - InputStream in = request.getServletContext().getResourceAsStream(requestPath); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - - // GOOD: getResourceAsStream constructed from `ServletContext` with input validation - protected void doPostGood(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestPath = request.getParameter("requestPath"); - ServletOutputStream out = response.getOutputStream(); - - if (!requestPath.contains("..") && requestPath.startsWith("/trusted")) { - InputStream in = request.getServletContext().getResourceAsStream(requestPath); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - } - - @Override - // BAD: getResource constructed from `Class` without input validation - protected void doHead(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestUrl = request.getParameter("requestURL"); - ServletOutputStream out = response.getOutputStream(); - - // A sample request /fake.jsp/../../../WEB-INF/web.xml can load the web.xml file - // Note the class is in two levels of subpackages and `Class.getResource` starts from its own directory - URL url = getClass().getResource(requestUrl); - - InputStream in = url.openStream(); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - - // GOOD: getResource constructed from `Class` with input validation - protected void doHeadGood(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestUrl = request.getParameter("requestURL"); - ServletOutputStream out = response.getOutputStream(); - - Path path = Paths.get(requestUrl).normalize().toRealPath(); - if (path.startsWith(BASE_PATH)) { - URL url = getClass().getResource(path.toString()); - - InputStream in = url.openStream(); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - } - - @Override - // BAD: getResourceAsStream constructed from `ClassLoader` without input validation - protected void doPut(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestPath = request.getParameter("requestPath"); - ServletOutputStream out = response.getOutputStream(); - - ServletConfig cfg = getServletConfig(); - ServletContext sc = cfg.getServletContext(); - - // A sample request /fake.jsp/../../../WEB-INF/web.xml can load the web.xml file - // Note the class is in two levels of subpackages and `ClassLoader.getResourceAsStream` starts from its own directory - InputStream in = getClass().getClassLoader().getResourceAsStream(requestPath); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - - // GOOD: getResourceAsStream constructed from `ClassLoader` with input validation - protected void doPutGood(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestPath = request.getParameter("requestPath"); - ServletOutputStream out = response.getOutputStream(); - - ServletConfig cfg = getServletConfig(); - ServletContext sc = cfg.getServletContext(); - - if (!requestPath.contains("..") && requestPath.startsWith("/trusted")) { - InputStream in = getClass().getClassLoader().getResourceAsStream(requestPath); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - } - - // BAD: getResource constructed from `ClassLoader` without input validation - protected void doPutBad(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestUrl = request.getParameter("requestURL"); - ServletOutputStream out = response.getOutputStream(); - - // A sample request /fake.jsp/../../../WEB-INF/web.xml can load the web.xml file - // Note the class is in two levels of subpackages and `ClassLoader.getResource` starts from its own directory - URL url = getClass().getClassLoader().getResource(requestUrl); - - InputStream in = url.openStream(); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - - // BAD: getResource constructed using Undertow IO without input validation - protected void doPutBad2(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestPath = request.getParameter("requestPath"); - - try { - FileResourceManager rm = new FileResourceManager(VFS.getChild(new URI("/usr/share")).getPhysicalFile()); - Resource rs = rm.getResource(requestPath); - - VirtualFile overlay = VFS.getChild(new URI("EAP_HOME/modules/")); - // Do file operations - overlay.getChild(rs.getPath()); - } catch (URISyntaxException ue) { - throw new IOException("Cannot parse the URI"); - } - } - - // GOOD: getResource constructed using Undertow IO with input validation - protected void doPutGood2(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestPath = request.getParameter("requestPath"); - - try { - FileResourceManager rm = new FileResourceManager(VFS.getChild(new URI("/usr/share")).getPhysicalFile()); - Resource rs = rm.getResource(requestPath); - - VirtualFile overlay = VFS.getChild(new URI("EAP_HOME/modules/")); - String path = rs.getPath(); - if (path.startsWith("/trusted_path") && !path.contains("..")) { - // Do file operations - overlay.getChild(path); - } - } catch (URISyntaxException ue) { - throw new IOException("Cannot parse the URI"); - } - } -} diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeResourceGet2.java b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeResourceGet2.java deleted file mode 100644 index b3d041d024cf..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeResourceGet2.java +++ /dev/null @@ -1,58 +0,0 @@ -package com.example; - -import javax.faces.context.FacesContext; -import java.io.BufferedReader; -import java.io.InputStream; -import java.io.InputStreamReader; -import java.io.IOException; -import java.net.URL; -import java.util.Map; - -/** Sample class of JSF managed bean */ -public class UnsafeResourceGet2 { - // BAD: getResourceAsStream constructed from `ExternalContext` without input validation - public String parameterActionBad1() throws IOException { - FacesContext fc = FacesContext.getCurrentInstance(); - Map params = fc.getExternalContext().getRequestParameterMap(); - String loadUrl = params.get("loadUrl"); - - InputStreamReader isr = new InputStreamReader(fc.getExternalContext().getResourceAsStream(loadUrl)); - BufferedReader br = new BufferedReader(isr); - if(br.ready()) { - //Do Stuff - return "result"; - } - - return "home"; - } - - // BAD: getResource constructed from `ExternalContext` without input validation - public String parameterActionBad2() throws IOException { - FacesContext fc = FacesContext.getCurrentInstance(); - Map params = fc.getExternalContext().getRequestParameterMap(); - String loadUrl = params.get("loadUrl"); - - URL url = fc.getExternalContext().getResource(loadUrl); - - InputStream in = url.openStream(); - //Do Stuff - return "result"; - } - - // GOOD: getResource constructed from `ExternalContext` with input validation - public String parameterActionGood1() throws IOException { - FacesContext fc = FacesContext.getCurrentInstance(); - Map params = fc.getExternalContext().getRequestParameterMap(); - String loadUrl = params.get("loadUrl"); - - if (loadUrl.equals("/public/crossdomain.xml")) { - URL url = fc.getExternalContext().getResource(loadUrl); - - InputStream in = url.openStream(); - //Do Stuff - return "result"; - } - - return "home"; - } -} diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java deleted file mode 100644 index ee63939b209e..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java +++ /dev/null @@ -1,131 +0,0 @@ -import java.io.IOException; -import java.net.URLDecoder; -import java.io.File; -import java.nio.file.Path; -import java.nio.file.Paths; - -import javax.servlet.http.HttpServlet; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; -import javax.servlet.RequestDispatcher; -import javax.servlet.ServletException; -import javax.servlet.ServletConfig; -import javax.servlet.ServletContext; - -public class UnsafeServletRequestDispatch extends HttpServlet { - private static final String BASE_PATH = "/pages"; - - @Override - // BAD: Request dispatcher constructed from `ServletContext` without input validation - protected void doGet(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String action = request.getParameter("action"); - String returnURL = request.getParameter("returnURL"); - - ServletConfig cfg = getServletConfig(); - if (action.equals("Login")) { - ServletContext sc = cfg.getServletContext(); - RequestDispatcher rd = sc.getRequestDispatcher("/Login.jsp"); - rd.forward(request, response); - } else { - ServletContext sc = cfg.getServletContext(); - RequestDispatcher rd = sc.getRequestDispatcher(returnURL); - rd.forward(request, response); - } - } - - @Override - // BAD: Request dispatcher constructed from `HttpServletRequest` without input validation - protected void doPost(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String action = request.getParameter("action"); - String returnURL = request.getParameter("returnURL"); - - if (action.equals("Login")) { - RequestDispatcher rd = request.getRequestDispatcher("/Login.jsp"); - rd.forward(request, response); - } else { - RequestDispatcher rd = request.getRequestDispatcher(returnURL); - rd.forward(request, response); - } - } - - @Override - // GOOD: Request dispatcher with a whitelisted URI - protected void doPut(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String action = request.getParameter("action"); - - if (action.equals("Login")) { - RequestDispatcher rd = request.getRequestDispatcher("/Login.jsp"); - rd.forward(request, response); - } else if (action.equals("Register")) { - RequestDispatcher rd = request.getRequestDispatcher("/Register.jsp"); - rd.forward(request, response); - } - } - - // BAD: Request dispatcher without path traversal check - protected void doHead2(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - 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); - } - } - - // GOOD: Request dispatcher with path traversal check - protected void doHead3(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - 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 and comparison - protected void doHead4(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - 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); - } - } - - // FN: Request dispatcher with negation check and path normalization, but without URL decoding - // When promoting this query, consider using FlowStates to make `getRequestDispatcher` a sink - // only if a URL-decoding step has NOT been crossed (i.e. make URLDecoder.decode change the - // state to a different value than the one required at the sink). - protected void doHead5(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String path = request.getParameter("path"); - 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); - } - } - - // GOOD: Request dispatcher with path traversal check and URL decoding - protected void doHead6(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String path = request.getParameter("path"); - 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); - } - } -} diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected deleted file mode 100644 index 545471868e76..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected +++ /dev/null @@ -1,129 +0,0 @@ -edges -| UnsafeLoadSpringResource.java:27:32:27:77 | fileName : String | UnsafeLoadSpringResource.java:31:49:31:56 | fileName : String | provenance | | -| UnsafeLoadSpringResource.java:31:27:31:57 | new ClassPathResource(...) : ClassPathResource | UnsafeLoadSpringResource.java:35:31:35:33 | clr | provenance | | -| UnsafeLoadSpringResource.java:31:49:31:56 | fileName : String | UnsafeLoadSpringResource.java:31:27:31:57 | new ClassPathResource(...) : ClassPathResource | provenance | | -| UnsafeLoadSpringResource.java:68:32:68:77 | fileName : String | UnsafeLoadSpringResource.java:76:38:76:45 | fileName | provenance | | -| UnsafeLoadSpringResource.java:108:32:108:77 | fileName : String | UnsafeLoadSpringResource.java:116:51:116:58 | fileName | provenance | | -| UnsafeRequestPath.java:20:17:20:63 | getServletPath(...) : String | UnsafeRequestPath.java:23:33:23:36 | path | provenance | | -| UnsafeResourceGet2.java:16:32:16:79 | getRequestParameterMap(...) : Map | UnsafeResourceGet2.java:17:20:17:25 | params : Map | provenance | | -| UnsafeResourceGet2.java:17:20:17:25 | params : Map | UnsafeResourceGet2.java:17:20:17:40 | get(...) : String | provenance | | -| UnsafeResourceGet2.java:17:20:17:40 | get(...) : String | UnsafeResourceGet2.java:19:93:19:99 | loadUrl | provenance | | -| UnsafeResourceGet2.java:32:32:32:79 | getRequestParameterMap(...) : Map | UnsafeResourceGet2.java:33:20:33:25 | params : Map | provenance | | -| UnsafeResourceGet2.java:33:20:33:25 | params : Map | UnsafeResourceGet2.java:33:20:33:40 | get(...) : String | provenance | | -| UnsafeResourceGet2.java:33:20:33:40 | get(...) : String | UnsafeResourceGet2.java:35:49:35:55 | loadUrl : String | provenance | | -| UnsafeResourceGet2.java:35:13:35:56 | getResource(...) : URL | UnsafeResourceGet2.java:37:20:37:22 | url | provenance | | -| UnsafeResourceGet2.java:35:49:35:55 | loadUrl : String | UnsafeResourceGet2.java:35:13:35:56 | getResource(...) : URL | provenance | | -| UnsafeResourceGet.java:32:23:32:56 | getParameter(...) : String | UnsafeResourceGet.java:39:28:39:37 | requestUrl : String | provenance | | -| UnsafeResourceGet.java:39:13:39:38 | getResource(...) : URL | UnsafeResourceGet.java:41:20:41:22 | url | provenance | | -| UnsafeResourceGet.java:39:28:39:37 | requestUrl : String | UnsafeResourceGet.java:39:13:39:38 | getResource(...) : URL | provenance | | -| UnsafeResourceGet.java:111:24:111:58 | getParameter(...) : String | UnsafeResourceGet.java:115:68:115:78 | requestPath | provenance | | -| UnsafeResourceGet.java:143:23:143:56 | getParameter(...) : String | UnsafeResourceGet.java:148:36:148:45 | requestUrl : String | provenance | | -| UnsafeResourceGet.java:148:13:148:46 | getResource(...) : URL | UnsafeResourceGet.java:150:20:150:22 | url | provenance | | -| UnsafeResourceGet.java:148:36:148:45 | requestUrl : String | UnsafeResourceGet.java:148:13:148:46 | getResource(...) : URL | provenance | | -| UnsafeResourceGet.java:181:24:181:58 | getParameter(...) : String | UnsafeResourceGet.java:189:68:189:78 | requestPath | provenance | | -| UnsafeResourceGet.java:219:23:219:56 | getParameter(...) : String | UnsafeResourceGet.java:224:53:224:62 | requestUrl : String | provenance | | -| UnsafeResourceGet.java:224:13:224:63 | getResource(...) : URL | UnsafeResourceGet.java:226:20:226:22 | url | provenance | | -| UnsafeResourceGet.java:224:53:224:62 | requestUrl : String | UnsafeResourceGet.java:224:13:224:63 | getResource(...) : URL | provenance | | -| UnsafeResourceGet.java:237:24:237:58 | getParameter(...) : String | UnsafeResourceGet.java:241:33:241:43 | requestPath : String | provenance | | -| UnsafeResourceGet.java:241:18:241:44 | getResource(...) : Resource | UnsafeResourceGet.java:245:21:245:22 | rs : Resource | provenance | | -| UnsafeResourceGet.java:241:33:241:43 | requestPath : String | UnsafeResourceGet.java:241:18:241:44 | getResource(...) : Resource | provenance | | -| UnsafeResourceGet.java:245:21:245:22 | rs : Resource | UnsafeResourceGet.java:245:21:245:32 | getPath(...) | provenance | | -| UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | provenance | | -| UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | provenance | | -| UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:76:53:76:56 | path | provenance | | -| UnsafeUrlForward.java:13:27:13:36 | url : String | UnsafeUrlForward.java:14:27:14:29 | url | provenance | | -| UnsafeUrlForward.java:18:27:18:36 | url : String | UnsafeUrlForward.java:20:28:20:30 | url | provenance | | -| UnsafeUrlForward.java:25:21:25:30 | url : String | UnsafeUrlForward.java:26:23:26:25 | url | provenance | | -| UnsafeUrlForward.java:30:27:30:36 | url : String | UnsafeUrlForward.java:31:48:31:63 | ... + ... | provenance | | -| UnsafeUrlForward.java:30:27:30:36 | url : String | UnsafeUrlForward.java:31:61:31:63 | url | provenance | | -| UnsafeUrlForward.java:36:19:36:28 | url : String | UnsafeUrlForward.java:38:33:38:35 | url | provenance | | -| UnsafeUrlForward.java:47:19:47:28 | url : String | UnsafeUrlForward.java:49:33:49:62 | ... + ... | provenance | | -| UnsafeUrlForward.java:58:19:58:28 | url : String | UnsafeUrlForward.java:60:33:60:62 | ... + ... | provenance | | -nodes -| UnsafeLoadSpringResource.java:27:32:27:77 | fileName : String | semmle.label | fileName : String | -| UnsafeLoadSpringResource.java:31:27:31:57 | new ClassPathResource(...) : ClassPathResource | semmle.label | new ClassPathResource(...) : ClassPathResource | -| UnsafeLoadSpringResource.java:31:49:31:56 | fileName : String | semmle.label | fileName : String | -| UnsafeLoadSpringResource.java:35:31:35:33 | clr | semmle.label | clr | -| UnsafeLoadSpringResource.java:68:32:68:77 | fileName : String | semmle.label | fileName : String | -| UnsafeLoadSpringResource.java:76:38:76:45 | fileName | semmle.label | fileName | -| UnsafeLoadSpringResource.java:108:32:108:77 | fileName : String | semmle.label | fileName : String | -| UnsafeLoadSpringResource.java:116:51:116:58 | fileName | semmle.label | fileName | -| UnsafeRequestPath.java:20:17:20:63 | getServletPath(...) : String | semmle.label | getServletPath(...) : String | -| UnsafeRequestPath.java:23:33:23:36 | path | semmle.label | path | -| UnsafeResourceGet2.java:16:32:16:79 | getRequestParameterMap(...) : Map | semmle.label | getRequestParameterMap(...) : Map | -| UnsafeResourceGet2.java:17:20:17:25 | params : Map | semmle.label | params : Map | -| UnsafeResourceGet2.java:17:20:17:40 | get(...) : String | semmle.label | get(...) : String | -| UnsafeResourceGet2.java:19:93:19:99 | loadUrl | semmle.label | loadUrl | -| UnsafeResourceGet2.java:32:32:32:79 | getRequestParameterMap(...) : Map | semmle.label | getRequestParameterMap(...) : Map | -| UnsafeResourceGet2.java:33:20:33:25 | params : Map | semmle.label | params : Map | -| UnsafeResourceGet2.java:33:20:33:40 | get(...) : String | semmle.label | get(...) : String | -| UnsafeResourceGet2.java:35:13:35:56 | getResource(...) : URL | semmle.label | getResource(...) : URL | -| UnsafeResourceGet2.java:35:49:35:55 | loadUrl : String | semmle.label | loadUrl : String | -| UnsafeResourceGet2.java:37:20:37:22 | url | semmle.label | url | -| UnsafeResourceGet.java:32:23:32:56 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| UnsafeResourceGet.java:39:13:39:38 | getResource(...) : URL | semmle.label | getResource(...) : URL | -| UnsafeResourceGet.java:39:28:39:37 | requestUrl : String | semmle.label | requestUrl : String | -| UnsafeResourceGet.java:41:20:41:22 | url | semmle.label | url | -| UnsafeResourceGet.java:111:24:111:58 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| UnsafeResourceGet.java:115:68:115:78 | requestPath | semmle.label | requestPath | -| UnsafeResourceGet.java:143:23:143:56 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| UnsafeResourceGet.java:148:13:148:46 | getResource(...) : URL | semmle.label | getResource(...) : URL | -| UnsafeResourceGet.java:148:36:148:45 | requestUrl : String | semmle.label | requestUrl : String | -| UnsafeResourceGet.java:150:20:150:22 | url | semmle.label | url | -| UnsafeResourceGet.java:181:24:181:58 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| UnsafeResourceGet.java:189:68:189:78 | requestPath | semmle.label | requestPath | -| UnsafeResourceGet.java:219:23:219:56 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| UnsafeResourceGet.java:224:13:224:63 | getResource(...) : URL | semmle.label | getResource(...) : URL | -| UnsafeResourceGet.java:224:53:224:62 | requestUrl : String | semmle.label | requestUrl : String | -| UnsafeResourceGet.java:226:20:226:22 | url | semmle.label | url | -| UnsafeResourceGet.java:237:24:237:58 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| UnsafeResourceGet.java:241:18:241:44 | getResource(...) : Resource | semmle.label | getResource(...) : Resource | -| UnsafeResourceGet.java:241:33:241:43 | requestPath : String | semmle.label | requestPath : String | -| UnsafeResourceGet.java:245:21:245:22 | rs : Resource | semmle.label | rs : Resource | -| UnsafeResourceGet.java:245:21:245:32 | getPath(...) | semmle.label | getPath(...) | -| UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | semmle.label | returnURL | -| UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | semmle.label | returnURL | -| UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| UnsafeServletRequestDispatch.java:76:53:76:56 | path | semmle.label | path | -| UnsafeUrlForward.java:13:27:13:36 | url : String | semmle.label | url : String | -| UnsafeUrlForward.java:14:27:14:29 | url | semmle.label | url | -| UnsafeUrlForward.java:18:27:18:36 | url : String | semmle.label | url : String | -| UnsafeUrlForward.java:20:28:20:30 | url | semmle.label | url | -| UnsafeUrlForward.java:25:21:25:30 | url : String | semmle.label | url : String | -| UnsafeUrlForward.java:26:23:26:25 | url | semmle.label | url | -| UnsafeUrlForward.java:30:27:30:36 | url : String | semmle.label | url : String | -| UnsafeUrlForward.java:31:48:31:63 | ... + ... | semmle.label | ... + ... | -| UnsafeUrlForward.java:31:61:31:63 | url | semmle.label | url | -| UnsafeUrlForward.java:36:19:36:28 | url : String | semmle.label | url : String | -| UnsafeUrlForward.java:38:33:38:35 | url | semmle.label | url | -| UnsafeUrlForward.java:47:19:47:28 | url : String | semmle.label | url : String | -| UnsafeUrlForward.java:49:33:49:62 | ... + ... | semmle.label | ... + ... | -| UnsafeUrlForward.java:58:19:58:28 | url : String | semmle.label | url : String | -| UnsafeUrlForward.java:60:33:60:62 | ... + ... | semmle.label | ... + ... | -subpaths -#select -| UnsafeLoadSpringResource.java:35:31:35:33 | clr | UnsafeLoadSpringResource.java:27:32:27:77 | fileName : String | UnsafeLoadSpringResource.java:35:31:35:33 | clr | Potentially untrusted URL forward due to $@. | UnsafeLoadSpringResource.java:27:32:27:77 | fileName | user-provided value | -| UnsafeLoadSpringResource.java:76:38:76:45 | fileName | UnsafeLoadSpringResource.java:68:32:68:77 | fileName : String | UnsafeLoadSpringResource.java:76:38:76:45 | fileName | Potentially untrusted URL forward due to $@. | UnsafeLoadSpringResource.java:68:32:68:77 | fileName | user-provided value | -| UnsafeLoadSpringResource.java:116:51:116:58 | fileName | UnsafeLoadSpringResource.java:108:32:108:77 | fileName : String | UnsafeLoadSpringResource.java:116:51:116:58 | fileName | Potentially untrusted URL forward due to $@. | UnsafeLoadSpringResource.java:108:32:108:77 | fileName | user-provided value | -| UnsafeRequestPath.java:23:33:23:36 | path | UnsafeRequestPath.java:20:17:20:63 | getServletPath(...) : String | UnsafeRequestPath.java:23:33:23:36 | path | Potentially untrusted URL forward due to $@. | UnsafeRequestPath.java:20:17:20:63 | getServletPath(...) | user-provided value | -| UnsafeResourceGet2.java:19:93:19:99 | loadUrl | UnsafeResourceGet2.java:16:32:16:79 | getRequestParameterMap(...) : Map | UnsafeResourceGet2.java:19:93:19:99 | loadUrl | Potentially untrusted URL forward due to $@. | UnsafeResourceGet2.java:16:32:16:79 | getRequestParameterMap(...) | user-provided value | -| UnsafeResourceGet2.java:37:20:37:22 | url | UnsafeResourceGet2.java:32:32:32:79 | getRequestParameterMap(...) : Map | UnsafeResourceGet2.java:37:20:37:22 | url | Potentially untrusted URL forward due to $@. | UnsafeResourceGet2.java:32:32:32:79 | getRequestParameterMap(...) | user-provided value | -| UnsafeResourceGet.java:41:20:41:22 | url | UnsafeResourceGet.java:32:23:32:56 | getParameter(...) : String | UnsafeResourceGet.java:41:20:41:22 | url | Potentially untrusted URL forward due to $@. | UnsafeResourceGet.java:32:23:32:56 | getParameter(...) | user-provided value | -| UnsafeResourceGet.java:115:68:115:78 | requestPath | UnsafeResourceGet.java:111:24:111:58 | getParameter(...) : String | UnsafeResourceGet.java:115:68:115:78 | requestPath | Potentially untrusted URL forward due to $@. | UnsafeResourceGet.java:111:24:111:58 | getParameter(...) | user-provided value | -| UnsafeResourceGet.java:150:20:150:22 | url | UnsafeResourceGet.java:143:23:143:56 | getParameter(...) : String | UnsafeResourceGet.java:150:20:150:22 | url | Potentially untrusted URL forward due to $@. | UnsafeResourceGet.java:143:23:143:56 | getParameter(...) | user-provided value | -| UnsafeResourceGet.java:189:68:189:78 | requestPath | UnsafeResourceGet.java:181:24:181:58 | getParameter(...) : String | UnsafeResourceGet.java:189:68:189:78 | requestPath | Potentially untrusted URL forward due to $@. | UnsafeResourceGet.java:181:24:181:58 | getParameter(...) | user-provided value | -| UnsafeResourceGet.java:226:20:226:22 | url | UnsafeResourceGet.java:219:23:219:56 | getParameter(...) : String | UnsafeResourceGet.java:226:20:226:22 | url | Potentially untrusted URL forward due to $@. | UnsafeResourceGet.java:219:23:219:56 | getParameter(...) | user-provided value | -| UnsafeResourceGet.java:245:21:245:32 | getPath(...) | UnsafeResourceGet.java:237:24:237:58 | getParameter(...) : String | UnsafeResourceGet.java:245:21:245:32 | getPath(...) | Potentially untrusted URL forward due to $@. | UnsafeResourceGet.java:237:24:237:58 | getParameter(...) | user-provided value | -| UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) | user-provided value | -| UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) | user-provided value | -| UnsafeServletRequestDispatch.java:76:53:76:56 | path | UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:76:53:76:56 | path | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) | user-provided value | -| UnsafeUrlForward.java:14:27:14:29 | url | UnsafeUrlForward.java:13:27:13:36 | url : String | UnsafeUrlForward.java:14:27:14:29 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:13:27:13:36 | url | user-provided value | -| UnsafeUrlForward.java:20:28:20:30 | url | UnsafeUrlForward.java:18:27:18:36 | url : String | UnsafeUrlForward.java:20:28:20:30 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:18:27:18:36 | url | user-provided value | -| UnsafeUrlForward.java:26:23:26:25 | url | UnsafeUrlForward.java:25:21:25:30 | url : String | UnsafeUrlForward.java:26:23:26:25 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:25:21:25:30 | url | user-provided value | -| UnsafeUrlForward.java:31:48:31:63 | ... + ... | UnsafeUrlForward.java:30:27:30:36 | url : String | UnsafeUrlForward.java:31:48:31:63 | ... + ... | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:30:27:30:36 | url | user-provided value | -| UnsafeUrlForward.java:31:61:31:63 | url | UnsafeUrlForward.java:30:27:30:36 | url : String | UnsafeUrlForward.java:31:61:31:63 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:30:27:30:36 | url | user-provided value | -| UnsafeUrlForward.java:38:33:38:35 | url | UnsafeUrlForward.java:36:19:36:28 | url : String | UnsafeUrlForward.java:38:33:38:35 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:36:19:36:28 | url | user-provided value | -| UnsafeUrlForward.java:49:33:49:62 | ... + ... | UnsafeUrlForward.java:47:19:47:28 | url : String | UnsafeUrlForward.java:49:33:49:62 | ... + ... | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:47:19:47:28 | url | user-provided value | -| UnsafeUrlForward.java:60:33:60:62 | ... + ... | UnsafeUrlForward.java:58:19:58:28 | url : String | UnsafeUrlForward.java:60:33:60:62 | ... + ... | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:58:19:58:28 | url | user-provided value | diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.java b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.java deleted file mode 100644 index 4018ed289481..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.java +++ /dev/null @@ -1,78 +0,0 @@ -import java.io.IOException; -import javax.servlet.ServletException; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; -import org.springframework.stereotype.Controller; -import org.springframework.web.bind.annotation.GetMapping; -import org.springframework.web.servlet.ModelAndView; - -@Controller -public class UnsafeUrlForward { - - @GetMapping("/bad1") - public ModelAndView bad1(String url) { - return new ModelAndView(url); - } - - @GetMapping("/bad2") - public ModelAndView bad2(String url) { - ModelAndView modelAndView = new ModelAndView(); - modelAndView.setViewName(url); - return modelAndView; - } - - @GetMapping("/bad3") - public String bad3(String url) { - return "forward:" + url + "/swagger-ui/index.html"; - } - - @GetMapping("/bad4") - public ModelAndView bad4(String url) { - ModelAndView modelAndView = new ModelAndView("forward:" + url); - return modelAndView; - } - - @GetMapping("/bad5") - public void bad5(String url, HttpServletRequest request, HttpServletResponse response) { - try { - request.getRequestDispatcher(url).include(request, response); - } catch (ServletException e) { - e.printStackTrace(); - } catch (IOException e) { - e.printStackTrace(); - } - } - - @GetMapping("/bad6") - public void bad6(String url, HttpServletRequest request, HttpServletResponse response) { - try { - request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").include(request, response); - } catch (ServletException e) { - e.printStackTrace(); - } catch (IOException e) { - e.printStackTrace(); - } - } - - @GetMapping("/bad7") - public void bad7(String url, HttpServletRequest request, HttpServletResponse response) { - try { - request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").forward(request, response); - } catch (ServletException e) { - e.printStackTrace(); - } catch (IOException e) { - e.printStackTrace(); - } - } - - @GetMapping("/good1") - public void good1(String url, HttpServletRequest request, HttpServletResponse response) { - try { - request.getRequestDispatcher("/index.jsp?token=" + url).forward(request, response); - } catch (ServletException e) { - e.printStackTrace(); - } catch (IOException e) { - e.printStackTrace(); - } - } -} diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.qlref b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.qlref deleted file mode 100644 index 2e4cb5e726a9..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/options b/java/ql/test/experimental/query-tests/security/CWE-552/options deleted file mode 100644 index 8fbf23e17dff..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-552/options +++ /dev/null @@ -1 +0,0 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.3.8/:${testdir}/../../../../stubs/javax-faces-2.3/:${testdir}/../../../../stubs/undertow-io-2.2/:${testdir}/../../../../stubs/jboss-vfs-3.2/:${testdir}/../../../../stubs/springframework-5.3.8/ diff --git a/java/ql/test/query-tests/security/CWE-552/UrlForwardTest.expected b/java/ql/test/query-tests/security/CWE-552/UrlForwardTest.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/java/ql/test/query-tests/security/CWE-552/UrlForwardTest.java b/java/ql/test/query-tests/security/CWE-552/UrlForwardTest.java new file mode 100644 index 000000000000..a1437a692a2f --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-552/UrlForwardTest.java @@ -0,0 +1,411 @@ +import java.io.IOException; +import java.net.URLDecoder; +import java.nio.file.Path; +import java.nio.file.Paths; + +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.RequestDispatcher; +import javax.servlet.ServletConfig; +import javax.servlet.ServletContext; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import org.springframework.stereotype.Controller; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.servlet.ModelAndView; +import org.kohsuke.stapler.StaplerRequest; +import org.kohsuke.stapler.StaplerResponse; + +@Controller +public class UrlForwardTest extends HttpServlet implements Filter { + + // Spring `ModelAndView` test cases + @GetMapping("/bad1") + public ModelAndView bad1(String url) { + return new ModelAndView(url); // $ hasTaintFlow + } + + @GetMapping("/bad2") + public ModelAndView bad2(String url) { + ModelAndView modelAndView = new ModelAndView(); + modelAndView.setViewName(url); // $ hasTaintFlow + return modelAndView; + } + + // Spring `"forward:"` prefix test cases + @GetMapping("/bad3") + public String bad3(String url) { + return "forward:" + url + "/swagger-ui/index.html"; // $ hasTaintFlow + } + + @GetMapping("/bad4") + public ModelAndView bad4(String url) { + ModelAndView modelAndView = new ModelAndView("forward:" + url); // $ hasTaintFlow + return modelAndView; + } + + // Not relevant for this query since redirecting instead of forwarding + // This result should be found by the `java/unvalidated-url-redirection` query instead. + @GetMapping("/redirect") + public ModelAndView redirect(String url) { + ModelAndView modelAndView = new ModelAndView("redirect:" + url); + return modelAndView; + } + + // `RequestDispatcher` test cases from a Spring `GetMapping` entry point + @GetMapping("/bad5") + public void bad5(String url, HttpServletRequest request, HttpServletResponse response) { + try { + request.getRequestDispatcher(url).include(request, response); // $ hasTaintFlow + } catch (ServletException e) { + e.printStackTrace(); + } catch (IOException e) { + e.printStackTrace(); + } + } + + @GetMapping("/bad6") + public void bad6(String url, HttpServletRequest request, HttpServletResponse response) { + try { + request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").include(request, response); // $ hasTaintFlow + } catch (ServletException e) { + e.printStackTrace(); + } catch (IOException e) { + e.printStackTrace(); + } + } + + @GetMapping("/bad7") + public void bad7(String url, HttpServletRequest request, HttpServletResponse response) { + try { + request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").forward(request, response); // $ hasTaintFlow + } catch (ServletException e) { + e.printStackTrace(); + } catch (IOException e) { + e.printStackTrace(); + } + } + + @GetMapping("/good1") + public void good1(String url, HttpServletRequest request, HttpServletResponse response) { + try { + request.getRequestDispatcher("/index.jsp?token=" + url).forward(request, response); + } catch (ServletException e) { + e.printStackTrace(); + } catch (IOException e) { + e.printStackTrace(); + } + } + + // BAD: appended to a prefix without path sanitization + @GetMapping("/bad8") + public void bad8(String urlPath, HttpServletRequest request, HttpServletResponse response) { + try { + String url = "/pages" + urlPath; + request.getRequestDispatcher(url).forward(request, response); // $ hasTaintFlow + } catch (ServletException e) { + e.printStackTrace(); + } catch (IOException e) { + e.printStackTrace(); + } + } + + // GOOD: appended to a prefix with path sanitization + @GetMapping("/good2") + public void good2(String urlPath, HttpServletRequest request, HttpServletResponse response) { + try { + while (urlPath.contains("%")) { + urlPath = URLDecoder.decode(urlPath, "UTF-8"); + } + + if (!urlPath.contains("..") && !urlPath.startsWith("/WEB-INF")) { + // Note: path injection sanitizer does not account for string concatenation instead of a `startswith` check + String url = "/pages" + urlPath; + request.getRequestDispatcher(url).forward(request, response); + } + + } catch (ServletException e) { + e.printStackTrace(); + } catch (IOException e) { + e.printStackTrace(); + } + } + + // `RequestDispatcher` test cases from non-Spring entry points + private static final String BASE_PATH = "/pages"; + + @Override + // BAD: Request dispatcher from servlet path without check + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + 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); // $ hasTaintFlow + } else { + chain.doFilter(request, response); + } + } + + // 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(); + + if (path.startsWith(BASE_PATH) && !path.contains("..")) { + request.getRequestDispatcher(path).forward(request, response); // $ hasTaintFlow + } else { + chain.doFilter(request, response); + } + } + + // GOOD: Request dispatcher from servlet path with whitelisted string comparison + public void doFilter3(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + String path = ((HttpServletRequest) request).getServletPath(); + + if (path.equals("/comaction")) { + request.getRequestDispatcher(path).forward(request, response); + } else { + chain.doFilter(request, response); + } + } + + @Override + // BAD: Request dispatcher constructed from `ServletContext` without input validation + protected void doGet(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String action = request.getParameter("action"); + String returnURL = request.getParameter("returnURL"); + + ServletConfig cfg = getServletConfig(); + if (action.equals("Login")) { + ServletContext sc = cfg.getServletContext(); + RequestDispatcher rd = sc.getRequestDispatcher("/Login.jsp"); + rd.forward(request, response); + } else { + ServletContext sc = cfg.getServletContext(); + RequestDispatcher rd = sc.getRequestDispatcher(returnURL); // $ hasTaintFlow + rd.forward(request, response); + } + } + + @Override + // BAD: Request dispatcher constructed from `HttpServletRequest` without input validation + protected void doPost(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String action = request.getParameter("action"); + String returnURL = request.getParameter("returnURL"); + + if (action.equals("Login")) { + RequestDispatcher rd = request.getRequestDispatcher("/Login.jsp"); + rd.forward(request, response); + } else { + RequestDispatcher rd = request.getRequestDispatcher(returnURL); // $ hasTaintFlow + rd.forward(request, response); + } + } + + @Override + // GOOD: Request dispatcher with a whitelisted URI + protected void doPut(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String action = request.getParameter("action"); + + if (action.equals("Login")) { + RequestDispatcher rd = request.getRequestDispatcher("/Login.jsp"); + rd.forward(request, response); + } else if (action.equals("Register")) { + RequestDispatcher rd = request.getRequestDispatcher("/Register.jsp"); + rd.forward(request, response); + } + } + + // BAD: Request dispatcher without path traversal check + protected void doHead1(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String path = request.getParameter("path"); + + // A sample payload "/pages/welcome.jsp/../WEB-INF/web.xml" can bypass the `startsWith` check + if (path.startsWith(BASE_PATH)) { + request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasTaintFlow + } + } + + // 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 doHead2(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String path = request.getParameter("path"); + + if (path.startsWith(BASE_PATH) && !path.contains("..")) { + request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasTaintFlow + } + } + + // BAD: Request dispatcher with path normalization and comparison, but + // does not decode before normalization. + protected void doHead3(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(); + + if (requestedPath.startsWith(BASE_PATH)) { + request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response); // $ hasTaintFlow + } + } + + // BAD: Request dispatcher with negation check and path normalization, but without URL decoding. + protected void doHead4(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); // $ hasTaintFlow + } + } + + // BAD: Request dispatcher with path traversal check and single URL decoding; may be vulnerable to double-encoding + protected void doHead5(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String path = request.getParameter("path"); + path = URLDecoder.decode(path, "UTF-8"); + + if (!path.startsWith("/WEB-INF/") && !path.contains("..")) { + request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasTaintFlow + } + } + + // 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"); + + 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); + } + } + + // GOOD: Request dispatcher with URL encoding check and path traversal check + protected void doHead7(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); + } + } + } + + // BAD: Request dispatcher without URL decoding before WEB-INF and path traversal checks + protected void doHead8(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String path = request.getParameter("path"); + if (path.contains("%")){ // incorrect check + if (!path.startsWith("/WEB-INF/") && !path.contains("..")) { + request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasTaintFlow + } + } + } + + // GOOD: Request dispatcher with WEB-INF, path traversal, and URL encoding checks + protected void doHead9(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String path = request.getParameter("path"); + + if (!path.startsWith("/WEB-INF/") && !path.contains("..")) { + if (!path.contains("%")){ // correct 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 doHead10(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String path = request.getParameter("path"); + while (path.contains("%")) { + path = URLDecoder.decode(path, "UTF-8"); + } + + if (!path.startsWith("/WEB-INF/") && !path.contains("..")) { + 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 doHead11(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String path = request.getParameter("path"); + // FP: we don't currently handle the scenario where the + // `path.contains("%")` check is stored in a variable. + 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); // $ SPURIOUS: hasTaintFlow + } + } + + // 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); // $ hasTaintFlow + } + + // 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); // $ hasTaintFlow + + // 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); + } + } + + // GOOD: char `?` appended before the user input + private static final String LOGIN_URL = "/UI/Login"; + + public void doPost2(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + StringBuilder forwardUrl = new StringBuilder(200); + forwardUrl.append(LOGIN_URL); + + String queryString = request.getQueryString(); + + forwardUrl.append('?').append(queryString); + + String fUrl = forwardUrl.toString(); + + ServletConfig config = getServletConfig(); + + RequestDispatcher dispatcher = config.getServletContext().getRequestDispatcher(fUrl); + dispatcher.forward(request, response); + } +} 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 000000000000..34841885bc34 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-552/UrlForwardTest.ql @@ -0,0 +1,4 @@ +import java +import TestUtilities.InlineFlowTest +import semmle.code.java.security.UrlForwardQuery +import TaintFlowTest diff --git a/java/ql/test/query-tests/security/CWE-552/options b/java/ql/test/query-tests/security/CWE-552/options new file mode 100644 index 000000000000..bda9516fb580 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-552/options @@ -0,0 +1 @@ +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/servlet-api-2.4:${testdir}/../../../stubs/springframework-5.3.8/:${testdir}/../../../stubs/javax-faces-2.3/:${testdir}/../../../stubs/undertow-io-2.2/:${testdir}/../../../stubs/jboss-vfs-3.2/:${testdir}/../../../stubs/stapler-1.263/:${testdir}/../../../stubs/apache-commons-fileupload-1.4/:${testdir}/../../../stubs/apache-commons-beanutils/:${testdir}/../../../stubs/saxon-xqj-9.x/:${testdir}/../../../stubs/apache-commons-lang/:${testdir}/../../../stubs/javax-servlet-2.5/ diff --git a/shared/mad/codeql/mad/ModelValidation.qll b/shared/mad/codeql/mad/ModelValidation.qll index bb3b8c174b97..20bcdd1908cd 100644 --- a/shared/mad/codeql/mad/ModelValidation.qll +++ b/shared/mad/codeql/mad/ModelValidation.qll @@ -33,8 +33,8 @@ module KindValidation { "bean-validation", "fragment-injection", "groovy-injection", "hostname-verification", "information-leak", "intent-redirection", "jexl-injection", "jndi-injection", "mvel-injection", "notification", "ognl-injection", "pending-intents", - "response-splitting", "trust-boundary-violation", "template-injection", "xpath-injection", - "xslt-injection", + "response-splitting", "trust-boundary-violation", "template-injection", "url-forward", + "xpath-injection", "xslt-injection", // JavaScript-only currently, but may be shared in the future "mongodb.sink", "nosql-injection", "unsafe-deserialization", // Swift-only currently, but may be shared in the future