diff --git a/java/ql/lib/semmle/code/java/Concepts.qll b/java/ql/lib/semmle/code/java/Concepts.qll index 7ed61223ea8..025ba89d382 100644 --- a/java/ql/lib/semmle/code/java/Concepts.qll +++ b/java/ql/lib/semmle/code/java/Concepts.qll @@ -25,6 +25,12 @@ private module Frameworks { * * These are either method calls, which return `true` when there is a match, or * annotations, which are considered to match if they are present. + * + * To use a `RegexMatch` as a barrier or sanitizer, instantiate + * `DataFlow::BarrierGuard`. Method-call matches act as ordinary control-flow + * barrier guards, while annotation matches are treated as direct barriers, + * since an annotation does not dominate the expression it constrains; + * `DataFlow::BarrierGuard` handles both, so callers need not distinguish the two. */ class RegexMatch extends Expr instanceof RegexMatch::Range { /** Gets the expression for the regex being executed by this node. */ diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll index d18190ffa1b..e1df870da7e 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll @@ -418,7 +418,17 @@ module BarrierGuard { } /** Gets a node that is safely guarded by the given guard check. */ - Node getABarrierNode() { result = BarrierGuardValue::getABarrierNode() } + Node getABarrierNode() { + result = BarrierGuardValue::getABarrierNode() + or + // An annotation doesn't dominate the expression it constrains, so the + // barrier-guard machinery above never yields a node for it; treat it as a + // direct barrier instead. Annotations are always present, so we only + // consider the `true` branch. + exists(Guard g, Expr e | + g instanceof Annotation and guardChecks(g, e, true) and result.asExpr() = e + ) + } } bindingset[this] diff --git a/java/ql/lib/semmle/code/java/security/LogInjection.qll b/java/ql/lib/semmle/code/java/security/LogInjection.qll index b585c249d1e..0a670c41ae4 100644 --- a/java/ql/lib/semmle/code/java/security/LogInjection.qll +++ b/java/ql/lib/semmle/code/java/security/LogInjection.qll @@ -90,13 +90,6 @@ private predicate logInjectionSanitizer(Expr e) { target.getStringValue() = ["\n", "\r", "\\n", "\\r", "\\R"] ) ) - or - exists(RegexMatch rm, CompileTimeConstantExpr target | - rm instanceof Annotation and - e = rm.getASanitizedExpr() and - target = rm.getRegex() and - regexPreventsLogInjection(target.getStringValue(), true) - ) } /** @@ -113,7 +106,6 @@ private predicate logInjectionGuard(Guard g, Expr e, boolean branch) { or exists(RegexMatch rm, CompileTimeConstantExpr target | rm = g and - not rm instanceof Annotation and target = rm.getRegex() and e = rm.getASanitizedExpr() | diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index 1b833b7d916..42304bb72dc 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -10,6 +10,7 @@ private import semmle.code.java.dataflow.SSA private import semmle.code.java.frameworks.kotlin.IO private import semmle.code.java.frameworks.kotlin.Text private import semmle.code.java.dataflow.Nullness +private import semmle.code.java.security.Sanitizers /** A sanitizer that protects against path injection vulnerabilities. */ abstract class PathInjectionSanitizer extends DataFlow::Node { } @@ -470,12 +471,7 @@ private class DirectoryCharactersGuard extends PathGuard { Expr checkedExpr; boolean branch; - DirectoryCharactersGuard() { - // Annotations are handled directly as barriers in `DirectoryCharactersSanitizer`, - // since they don't dominate the sanitized expression and so can't act as barrier guards. - not this instanceof Annotation and - isMatchesCall(this, checkedExpr, branch) - } + DirectoryCharactersGuard() { isMatchesCall(this, checkedExpr, branch) } override Expr getCheckedExpr() { result = checkedExpr } @@ -501,12 +497,5 @@ private class DirectoryCharactersSanitizer extends PathInjectionSanitizer { this.asExpr() instanceof ReplaceDirectoryCharactersSanitizer or this = DataFlow::BarrierGuard::getABarrierNode() - or - // Annotations don't fit into the model of barrier guards because the - // annotation doesn't dominate the sanitized expression, so we instead - // treat them as barriers directly. - exists(RegexMatch rm | rm instanceof Annotation and isMatchesCall(rm, _, true) | - this.asExpr() = rm.getString() - ) } } diff --git a/java/ql/lib/semmle/code/java/security/Sanitizers.qll b/java/ql/lib/semmle/code/java/security/Sanitizers.qll index 0c5f9b98070..072f2234efe 100644 --- a/java/ql/lib/semmle/code/java/security/Sanitizers.qll +++ b/java/ql/lib/semmle/code/java/security/Sanitizers.qll @@ -37,11 +37,14 @@ class SimpleTypeSanitizer extends DataFlow::Node { * * This is overapproximate: we do not attempt to reason about the correctness of the regexp. * - * Use this if you want to define a derived `DataFlow::BarrierGuard` without - * make the type recursive. Otherwise use `RegexpCheckBarrier`. + * This holds for both method-call and annotation regular-expression matches. + * Method-call matches yield barrier nodes via ordinary control-flow dominance, + * while annotation matches are treated as direct barriers by + * `DataFlow::BarrierGuard`, since an annotation does not dominate the + * expression it constrains. */ predicate regexpMatchGuardChecks(Guard guard, Expr e, boolean branch) { - exists(RegexMatch rm | not rm instanceof Annotation | + exists(RegexMatch rm | guard = rm and e = rm.getASanitizedExpr() and branch = true @@ -56,11 +59,6 @@ predicate regexpMatchGuardChecks(Guard guard, Expr e, boolean branch) { class RegexpCheckBarrier extends DataFlow::Node { RegexpCheckBarrier() { this = DataFlow::BarrierGuard::getABarrierNode() - or - // Annotations don't fit into the model of barrier guards because the - // annotation doesn't dominate the sanitized expression, so we instead - // treat them as barriers directly. - exists(RegexMatch rm | rm instanceof Annotation | this.asExpr() = rm.getString()) } }