diff --git a/java/ql/lib/semmle/code/java/security/UrlForward.qll b/java/ql/lib/semmle/code/java/security/UrlForward.qll index aa21ed48aba..7e68f07987b 100644 --- a/java/ql/lib/semmle/code/java/security/UrlForward.qll +++ b/java/ql/lib/semmle/code/java/security/UrlForward.qll @@ -8,48 +8,49 @@ private import semmle.code.java.dataflow.StringPrefixes /** A URL forward sink. */ abstract class UrlForwardSink extends DataFlow::Node { } -/** A default sink representing methods susceptible to URL forwarding attacks. */ +/** + * A default sink representing methods susceptible to URL + * forwarding attacks. + */ private class DefaultUrlForwardSink extends UrlForwardSink { DefaultUrlForwardSink() { sinkNode(this, "url-forward") } } /** - * An expression appended (perhaps indirectly) to `"forward:"`, and which - * is reachable from a Spring entry point. + * An expression appended (perhaps indirectly) to `"forward:"` + * and reachable from a Spring entry point. */ private class SpringUrlForwardSink extends UrlForwardSink { SpringUrlForwardSink() { - // TODO: check if can use MaD "Annotated" for `SpringRequestMappingMethod` or if too complicated for MaD (probably too complicated). - any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable()) and + any(SpringRequestMappingMethod srmm).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 URL forward sanitizer. */ -abstract class UrlForwardSanitizer extends DataFlow::Node { } +/** A URL forward barrier. */ +abstract class UrlForwardBarrier extends DataFlow::Node { } -private class PrimitiveSanitizer extends UrlForwardSanitizer { - PrimitiveSanitizer() { +private class PrimitiveBarrier extends UrlForwardBarrier { + PrimitiveBarrier() { this.getType() instanceof PrimitiveType or this.getType() instanceof BoxedType or this.getType() instanceof NumberType } } -// TODO: double-check this sanitizer (and should I switch all "sanitizer" naming to "barrier" instead?) -private class FollowsSanitizingPrefix extends UrlForwardSanitizer { - FollowsSanitizingPrefix() { this.asExpr() = any(SanitizingPrefix fp).getAnAppendedExpression() } +private class FollowsBarrierPrefix extends UrlForwardBarrier { + FollowsBarrierPrefix() { this.asExpr() = any(BarrierPrefix fp).getAnAppendedExpression() } } -private class SanitizingPrefix extends InterestingPrefix { - SanitizingPrefix() { +private class BarrierPrefix extends InterestingPrefix { + BarrierPrefix() { + // TODO: why not META-INF here as well? (and are `/` correct?) not this.getStringValue().matches("/WEB-INF/%") and not this.getStringValue() = "forward:" } diff --git a/java/ql/lib/semmle/code/java/security/UrlForwardQuery.qll b/java/ql/lib/semmle/code/java/security/UrlForwardQuery.qll index dadf4be612f..c92467490f3 100644 --- a/java/ql/lib/semmle/code/java/security/UrlForwardQuery.qll +++ b/java/ql/lib/semmle/code/java/security/UrlForwardQuery.qll @@ -12,25 +12,24 @@ import semmle.code.java.security.PathSanitizer module UrlForwardFlowConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source instanceof ThreatModelFlowSource and - // TODO: move below logic to class in UrlForward.qll? And check exactly why these were excluded. - not exists(MethodCall ma, Method m | ma.getMethod() = m | + // excluded due to FPs + not exists(MethodCall mc, Method m | mc.getMethod() = m | ( m instanceof HttpServletRequestGetRequestUriMethod or m instanceof HttpServletRequestGetRequestUrlMethod or m instanceof HttpServletRequestGetPathMethod ) and - ma = source.asExpr() + mc = source.asExpr() ) } predicate isSink(DataFlow::Node sink) { sink instanceof UrlForwardSink } predicate isBarrier(DataFlow::Node node) { - node instanceof UrlForwardSanitizer or + node instanceof UrlForwardBarrier or node instanceof PathInjectionSanitizer } - // TODO: check if the below is still needed after removing path-injection related sinks. DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } }