diff --git a/java/ql/lib/semmle/code/java/security/UnsafeUrlForward.qll b/java/ql/lib/semmle/code/java/security/UnsafeUrlForward.qll index cd65a6f6345..4a529896f86 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeUrlForward.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeUrlForward.qll @@ -1,20 +1,40 @@ +/** Provides classes related to unsafe URL forwarding in Java. */ + 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.frameworks.javaee.ejb.EJBRestrictions /** 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 { } - /** A default sink representing methods susceptible to unsafe URL forwarding. */ private class DefaultUnsafeUrlForwardSink extends UnsafeUrlForwardSink { DefaultUnsafeUrlForwardSink() { sinkNode(this, "url-forward") } } +/** + * An expression appended (perhaps indirectly) to `"forward:"`, and which + * is reachable from a Spring entry point. + */ +private class SpringUrlForwardSink extends UnsafeUrlForwardSink { + SpringUrlForwardSink() { + // TODO: check if can use MaD "Annotated" for `SpringRequestMappingMethod` or if too complicated for MaD (probably too complicated). + any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable()) and + this.asExpr() = any(ForwardPrefix fp).getAnAppendedExpression() + } +} + +// TODO: should this potentially be "include:" as well? Or does that not work similarly? +private class ForwardPrefix extends InterestingPrefix { + ForwardPrefix() { this.getStringValue() = "forward:" } + + override int getOffset() { result = 0 } +} + +/** A sanitizer for unsafe URL forward vulnerabilities. */ +abstract class UnsafeUrlForwardSanitizer extends DataFlow::Node { } + private class PrimitiveSanitizer extends UnsafeUrlForwardSanitizer { PrimitiveSanitizer() { this.getType() instanceof PrimitiveType or @@ -23,6 +43,11 @@ private class PrimitiveSanitizer extends UnsafeUrlForwardSanitizer { } } +// TODO: double-check this sanitizer (and should I switch all "sanitizer" naming to "barrier" instead?) +private class FollowsSanitizingPrefix extends UnsafeUrlForwardSanitizer { + FollowsSanitizingPrefix() { this.asExpr() = any(SanitizingPrefix fp).getAnAppendedExpression() } +} + private class SanitizingPrefix extends InterestingPrefix { SanitizingPrefix() { not this.getStringValue().matches("/WEB-INF/%") and @@ -31,24 +56,3 @@ private class SanitizingPrefix extends InterestingPrefix { 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/lib/semmle/code/java/security/UnsafeUrlForwardQuery.qll b/java/ql/lib/semmle/code/java/security/UnsafeUrlForwardQuery.qll index 4231af1a90a..49670234582 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeUrlForwardQuery.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeUrlForwardQuery.qll @@ -12,7 +12,7 @@ import semmle.code.java.security.PathSanitizer module UnsafeUrlForwardFlowConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source instanceof ThreatModelFlowSource and - // TODO: move below logic to class in UnsafeUrlForward.qll? + // TODO: move below logic to class in UnsafeUrlForward.qll? And check exactly why these were excluded. not exists(MethodCall ma, Method m | ma.getMethod() = m | ( m instanceof HttpServletRequestGetRequestUriMethod or