Merge pull request #20183 from aschackmull/java/barrierguard-wrappers

Java: Enable BarrierGuard wrappers

java/ql/lib/semmle/code/java/ControlFlowGraph.qll

@@ -347,12 +347,28 @@ private module ControlFlowGraphImpl {
     )
   }
 
+  private predicate methodMayThrow(Method m, ThrowableType t) {
+    exists(AstNode n |
+      t = n.(ThrowStmt).getThrownExceptionType() and
+      not n.(ThrowStmt).getParent() = any(Method m0).getBody()
+      or
+      uncheckedExceptionFromMethod(n, t)
+    |
+      n.getEnclosingStmt().getEnclosingCallable() = m and
+      not exists(TryStmt try |
+        exists(try.getACatchClause()) and try.getBlock() = n.getEnclosingStmt().getEnclosingStmt*()
+      )
+    )
+  }
+
   /**
-   * Bind `t` to an unchecked exception that may occur in a precondition check.
+   * Bind `t` to an unchecked exception that may occur in a precondition check or guard wrapper.
    */
   private predicate uncheckedExceptionFromMethod(MethodCall ma, ThrowableType t) {
     conditionCheckArgument(ma, _, _) and
     (t instanceof TypeError or t instanceof TypeRuntimeException)
+    or
+    methodMayThrow(ma.getMethod(), t)
   }
 
   /**

java/ql/lib/semmle/code/java/Member.qll

@@ -256,7 +256,15 @@ class Callable extends StmtParent, Member, @callable {
   Exception getAnException() { exceptions(result, _, this) }
 
   /** Gets an exception type that occurs in the `throws` clause of this callable. */
-  RefType getAThrownExceptionType() { result = this.getAnException().getType() }
+  RefType getAThrownExceptionType() {
+    result = this.getAnException().getType()
+    or
+    exists(Annotation a |
+      this.getAnAnnotation() = a and
+      a.getType().hasQualifiedName("kotlin.jvm", "Throws") and
+      a.getATypeArrayValue(_) = result
+    )
+  }
 
   /** Gets a call site that references this callable. */
   Call getAReference() { result.getCallee() = this }

java/ql/lib/semmle/code/java/controlflow/Guards.qll

@@ -490,12 +490,6 @@ module Guards_v2 = GuardsImpl::Logic<LogicInput_v2>;
 /** INTERNAL: Don't use. */
 module Guards_v3 = GuardsImpl::Logic<LogicInput_v3>;
 
-/** INTERNAL: Don't use. */
-predicate implies_v3(Guard g1, boolean b1, Guard g2, boolean b2) {
-  Guards_v3::boolImplies(g1, any(GuardValue v | v.asBooleanValue() = b1), g2,
-    any(GuardValue v | v.asBooleanValue() = b2))
-}
-
 /**
  * A guard. This may be any expression whose value determines subsequent
  * control flow. It may also be a switch case, which as a guard is considered

java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll

@@ -562,14 +562,20 @@ private module Cached {
 
     cached // nothing is actually cached
     module BarrierGuard<guardChecksSig/3 guardChecks> {
-      private predicate guardChecksAdjTypes(
-        DataFlowIntegrationInput::Guard g, DataFlowIntegrationInput::Expr e, Guards::GuardValue val
+      private predicate guardChecksAdjTypes(Guards::Guards_v3::Guard g, Expr e, boolean branch) {
+        guardChecks(g, e, branch)
+      }
+
+      private predicate guardChecksWithWrappers(
+        DataFlowIntegrationInput::Guard g, Definition def, Guards::GuardValue val, Unit state
       ) {
-        guardChecks(g, e, val.asBooleanValue())
+        Guards::Guards_v3::ValidationWrapper<guardChecksAdjTypes/3>::guardChecksDef(g, def, val) and
+        exists(state)
       }
 
       private Node getABarrierNodeImpl() {
-        result = DataFlowIntegrationImpl::BarrierGuard<guardChecksAdjTypes/3>::getABarrierNode()
+        result =
+          DataFlowIntegrationImpl::BarrierGuardDefWithState<Unit, guardChecksWithWrappers/4>::getABarrierNode(_)
       }
 
       predicate getABarrierNode = getABarrierNodeImpl/0;

java/ql/lib/semmle/code/java/security/ArithmeticCommon.qll

@@ -20,16 +20,16 @@ predicate narrowerThanOrEqualTo(ArithExpr exp, NumType numType) {
   exists(CastingExpr cast | cast.getAChildExpr() = exp | numType.widerThanOrEqualTo(cast.getType()))
 }
 
-private Guard sizeGuard(SsaVariable v, boolean branch, boolean upper) {
+private Guard sizeGuard(Expr e, boolean branch, boolean upper) {
   exists(ComparisonExpr comp | comp = result |
-    comp.getLesserOperand() = ssaRead(v, 0) and
+    comp.getLesserOperand() = e and
     (
       branch = true and upper = true
       or
       branch = false and upper = false
     )
     or
-    comp.getGreaterOperand() = ssaRead(v, 0) and
+    comp.getGreaterOperand() = e and
     (
       branch = true and upper = false
       or
@@ -38,7 +38,7 @@ private Guard sizeGuard(SsaVariable v, boolean branch, boolean upper) {
     or
     exists(MethodCall ma |
       ma.getMethod() instanceof MethodAbs and
-      ma.getArgument(0) = ssaRead(v, 0) and
+      ma.getArgument(0) = e and
       (
         comp.getLesserOperand() = ma and branch = true
         or
@@ -49,7 +49,7 @@ private Guard sizeGuard(SsaVariable v, boolean branch, boolean upper) {
     or
     // overflow test
     exists(AddExpr add, VarRead use, Expr pos |
-      use = ssaRead(v, 0) and
+      use = e and
       add.hasOperands(use, pos) and
       positive(use) and
       positive(pos) and
@@ -65,70 +65,38 @@ private Guard sizeGuard(SsaVariable v, boolean branch, boolean upper) {
     )
   )
   or
-  result.isEquality(ssaRead(v, 0), _, branch) and
+  result.isEquality(e, _, branch) and
   (upper = true or upper = false)
-  or
-  exists(MethodCall call, Method m, int ix |
-    call = result and
-    call.getArgument(ix) = ssaRead(v, 0) and
-    call.getMethod().getSourceDeclaration() = m and
-    m = customSizeGuard(ix, branch, upper)
-  )
 }
 
-private Guard derivedSizeGuard(SsaVariable v, boolean branch, boolean upper) {
-  result = sizeGuard(v, branch, upper) or
-  exists(boolean branch0 | implies_v3(result, branch, derivedSizeGuard(v, branch0, upper), branch0))
+private predicate sizeGuardLessThan(Guard g, Expr e, boolean branch) {
+  g = sizeGuard(e, branch, true)
 }
 
-private Method customSizeGuard(int index, boolean retval, boolean upper) {
-  exists(Parameter p, SsaImplicitInit v |
-    result.getReturnType().(PrimitiveType).hasName("boolean") and
-    not result.isOverridable() and
-    p.getCallable() = result and
-    not p.isVarargs() and
-    p.getType() instanceof NumericOrCharType and
-    p.getPosition() = index and
-    v.isParameterDefinition(p) and
-    forex(ReturnStmt ret |
-      ret.getEnclosingCallable() = result and
-      exists(Expr res | res = ret.getResult() |
-        not res.(BooleanLiteral).getBooleanValue() = retval.booleanNot()
-      )
-    |
-      ret.getResult() = derivedSizeGuard(v, retval, upper)
-    )
-  )
+private predicate sizeGuardGreaterThan(Guard g, Expr e, boolean branch) {
+  g = sizeGuard(e, branch, false)
 }
 
 /**
- * Holds if `e` is bounded in a way that is likely to prevent overflow.
+ * Holds if `n` is bounded in a way that is likely to prevent overflow.
  */
-predicate guardedLessThanSomething(Expr e) {
-  exists(SsaVariable v, Guard guard, boolean branch |
-    e = v.getAUse() and
-    guard = sizeGuard(v.getAPhiInputOrPriorDef*(), branch, true) and
-    guard.controls(e.getBasicBlock(), branch)
-  )
+predicate guardedLessThanSomething(DataFlow::Node n) {
+  DataFlow::BarrierGuard<sizeGuardLessThan/3>::getABarrierNode() = n
   or
-  negative(e)
+  negative(n.asExpr())
   or
-  e.(MethodCall).getMethod() instanceof MethodMathMin
+  n.asExpr().(MethodCall).getMethod() instanceof MethodMathMin
 }
 
 /**
  * Holds if `e` is bounded in a way that is likely to prevent underflow.
  */
-predicate guardedGreaterThanSomething(Expr e) {
-  exists(SsaVariable v, Guard guard, boolean branch |
-    e = v.getAUse() and
-    guard = sizeGuard(v.getAPhiInputOrPriorDef*(), branch, false) and
-    guard.controls(e.getBasicBlock(), branch)
-  )
+predicate guardedGreaterThanSomething(DataFlow::Node n) {
+  DataFlow::BarrierGuard<sizeGuardGreaterThan/3>::getABarrierNode() = n
   or
-  positive(e)
+  positive(n.asExpr())
   or
-  e.(MethodCall).getMethod() instanceof MethodMathMax
+  n.asExpr().(MethodCall).getMethod() instanceof MethodMathMax
 }
 
 /** Holds if `e` occurs in a context where it will be upcast to a wider type. */
@@ -182,7 +150,7 @@ private predicate unlikelyNode(DataFlow::Node n) {
 /** Holds if `n` is likely guarded against overflow. */
 predicate overflowBarrier(DataFlow::Node n) {
   n.getType() instanceof BooleanType or
-  guardedLessThanSomething(n.asExpr()) or
+  guardedLessThanSomething(n) or
   unlikelyNode(n) or
   upcastToWiderType(n.asExpr()) or
   overflowIrrelevant(n.asExpr())
@@ -191,7 +159,7 @@ predicate overflowBarrier(DataFlow::Node n) {
 /** Holds if `n` is likely guarded against underflow. */
 predicate underflowBarrier(DataFlow::Node n) {
   n.getType() instanceof BooleanType or
-  guardedGreaterThanSomething(n.asExpr()) or
+  guardedGreaterThanSomething(n) or
   unlikelyNode(n) or
   upcastToWiderType(n.asExpr()) or
   overflowIrrelevant(n.asExpr())
@@ -210,7 +178,6 @@ predicate overflowSink(ArithExpr exp, VarAccess use) {
     exp instanceof PostIncExpr or
     exp instanceof MulExpr
   ) and
-  not guardedLessThanSomething(use) and
   // Exclude widening conversions of tainted values due to binary numeric promotion (JLS 5.6.2)
   // unless there is an enclosing cast down to a narrower type.
   narrowerThanOrEqualTo(exp, use.getType()) and
@@ -230,7 +197,6 @@ predicate underflowSink(ArithExpr exp, VarAccess use) {
     exp instanceof PostDecExpr or
     exp instanceof MulExpr
   ) and
-  not guardedGreaterThanSomething(use) and
   // Exclude widening conversions of tainted values due to binary numeric promotion (JLS 5.6.2)
   // unless there is an enclosing cast down to a narrower type.
   narrowerThanOrEqualTo(exp, use.getType()) and

java/ql/lib/semmle/code/java/security/PathSanitizer.qll

@@ -13,33 +13,6 @@ private import semmle.code.java.dataflow.Nullness
 /** A sanitizer that protects against path injection vulnerabilities. */
 abstract class PathInjectionSanitizer extends DataFlow::Node { }
 
-/**
- * Provides a set of nodes validated by a method that uses a validation guard.
- */
-private module ValidationMethod<DataFlow::guardChecksSig/3 validationGuard> {
-  /** Gets a node that is safely guarded by a method that uses the given guard check. */
-  DataFlow::Node getAValidatedNode() {
-    exists(MethodCall ma, int pos, VarRead rv |
-      validationMethod(ma.getMethod(), pos) and
-      ma.getArgument(pos) = rv and
-      adjacentUseUseSameVar(rv, result.asExpr()) and
-      ma.getBasicBlock().dominates(result.asExpr().getBasicBlock())
-    )
-  }
-
-  /**
-   * Holds if `m` validates its `arg`th parameter by using `validationGuard`.
-   */
-  private predicate validationMethod(Method m, int arg) {
-    exists(Guard g, SsaImplicitInit var, ControlFlow::NormalExitNode normexit, boolean branch |
-      validationGuard(g, var.getAUse(), branch) and
-      var.isParameterDefinition(m.getParameter(arg)) and
-      normexit.getEnclosingCallable() = m and
-      g.controls(normexit.getBasicBlock(), branch)
-    )
-  }
-}
-
 /**
  * Holds if `g` is guard that compares a path to a trusted value.
  */
@@ -68,8 +41,6 @@ private predicate exactPathMatchGuard(Guard g, Expr e, boolean branch) {
 class ExactPathMatchSanitizer extends PathInjectionSanitizer {
   ExactPathMatchSanitizer() {
     this = DataFlow::BarrierGuard<exactPathMatchGuard/3>::getABarrierNode()
-    or
-    this = ValidationMethod<exactPathMatchGuard/3>::getAValidatedNode()
   }
 }
 
@@ -120,8 +91,7 @@ private predicate allowedPrefixGuard(Guard g, Expr e, boolean branch) {
 
 private class AllowedPrefixSanitizer extends PathInjectionSanitizer {
   AllowedPrefixSanitizer() {
-    this = DataFlow::BarrierGuard<allowedPrefixGuard/3>::getABarrierNode() or
-    this = ValidationMethod<allowedPrefixGuard/3>::getAValidatedNode()
+    this = DataFlow::BarrierGuard<allowedPrefixGuard/3>::getABarrierNode()
   }
 }
 
@@ -139,10 +109,7 @@ private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) {
 }
 
 private class DotDotCheckSanitizer extends PathInjectionSanitizer {
-  DotDotCheckSanitizer() {
-    this = DataFlow::BarrierGuard<dotDotCheckGuard/3>::getABarrierNode() or
-    this = ValidationMethod<dotDotCheckGuard/3>::getAValidatedNode()
-  }
+  DotDotCheckSanitizer() { this = DataFlow::BarrierGuard<dotDotCheckGuard/3>::getABarrierNode() }
 }
 
 private class BlockListGuard extends PathGuard instanceof MethodCall {
@@ -179,10 +146,7 @@ private predicate blockListGuard(Guard g, Expr e, boolean branch) {
 }
 
 private class BlockListSanitizer extends PathInjectionSanitizer {
-  BlockListSanitizer() {
-    this = DataFlow::BarrierGuard<blockListGuard/3>::getABarrierNode() or
-    this = ValidationMethod<blockListGuard/3>::getAValidatedNode()
-  }
+  BlockListSanitizer() { this = DataFlow::BarrierGuard<blockListGuard/3>::getABarrierNode() }
 }
 
 private class ConstantOrRegex extends Expr {
@@ -368,7 +332,6 @@ private class FileConstructorChildArgumentStep extends AdditionalTaintStep {
       n2.asExpr() = constrCall
     |
       not n1 = DataFlow::BarrierGuard<pathTraversalGuard/3>::getABarrierNode() and
-      not n1 = ValidationMethod<pathTraversalGuard/3>::getAValidatedNode() and
       not TaintTracking::localExprTaint(any(PathNormalizeSanitizer p), n1.asExpr())
       or
       DataFlow::localExprFlow(nullExpr(), constrCall.getArgument(0))
@@ -546,7 +509,6 @@ private predicate directoryCharactersGuard(Guard g, Expr e, boolean branch) {
 private class DirectoryCharactersSanitizer extends PathInjectionSanitizer {
   DirectoryCharactersSanitizer() {
     this.asExpr() instanceof ReplaceDirectoryCharactersSanitizer or
-    this = DataFlow::BarrierGuard<directoryCharactersGuard/3>::getABarrierNode() or
-    this = ValidationMethod<directoryCharactersGuard/3>::getAValidatedNode()
+    this = DataFlow::BarrierGuard<directoryCharactersGuard/3>::getABarrierNode()
   }
 }

shared/controlflow/codeql/controlflow/Guards.qll

@@ -935,18 +935,6 @@ module Make<LocationSig Location, InputSig<Location> Input> {
       }
     }
 
-    private predicate booleanGuard(Guard guard, GuardValue val) {
-      exists(guard) and exists(val.asBooleanValue())
-    }
-
-    private module BooleanImplies = ImpliesTC<booleanGuard/2>;
-
-    /** INTERNAL: Don't use. */
-    predicate boolImplies(Guard g1, GuardValue v1, Guard g2, GuardValue v2) {
-      BooleanImplies::guardControls(g2, v2, g1, v1) and
-      g2 != g1
-    }
-
     /**
      * Holds if `guard` evaluating to `v` implies that `e` is guaranteed to be
      * null if `isNull` is true, and non-null if `isNull` is false.
@@ -1130,10 +1118,10 @@ module Make<LocationSig Location, InputSig<Location> Input> {
       private module StatefulWrapper = ValidationWrapperWithState<Unit, guardChecksWithState/4>;
 
       /**
-       * Holds if the guard `g` validates the expression `e` upon evaluating to `val`.
+       * Holds if the guard `g` validates the SSA definition `def` upon evaluating to `val`.
        */
-      predicate guardChecks(Guard g, Expr e, GuardValue val) {
-        StatefulWrapper::guardChecks(g, e, val, _)
+      predicate guardChecksDef(Guard g, SsaDefinition def, GuardValue val) {
+        StatefulWrapper::guardChecksDef(g, def, val, _)
       }
     }
 
@@ -1156,7 +1144,7 @@ module Make<LocationSig Location, InputSig<Location> Input> {
         exists(NonOverridableMethod m, SsaDefinition param, Guard guard, GuardValue val |
           m.getAReturnExpr() = ret and
           parameterDefinition(m.getParameter(ppos), param) and
-          guardChecks(guard, param.getARead(), val, state)
+          guardChecksDef(guard, param, val, state)
         |
           guard.valueControls(ret.getBasicBlock(), val) and
           relevantReturnValue(m, retval)
@@ -1185,7 +1173,7 @@ module Make<LocationSig Location, InputSig<Location> Input> {
         or
         exists(SsaDefinition param, BasicBlock bb, Guard guard, GuardValue val |
           parameterDefinition(result.getParameter(ppos), param) and
-          guardChecks(guard, param.getARead(), val, state) and
+          guardChecksDef(guard, param, val, state) and
           guard.valueControls(bb, val) and
           normalExitBlock(bb) and
           retval = TException(false)
@@ -1195,7 +1183,7 @@ module Make<LocationSig Location, InputSig<Location> Input> {
       /**
        * Holds if the guard `g` validates the expression `e` upon evaluating to `val`.
        */
-      predicate guardChecks(Guard g, Expr e, GuardValue val, State state) {
+      private predicate guardChecks(Guard g, Expr e, GuardValue val, State state) {
         guardChecks0(g, e, val.asBooleanValue(), state)
         or
         exists(NonOverridableMethodCall call, ParameterPosition ppos, ArgumentPosition apos |
@@ -1205,6 +1193,16 @@ module Make<LocationSig Location, InputSig<Location> Input> {
           parameterMatch(pragma[only_bind_out](ppos), pragma[only_bind_out](apos))
         )
       }
+
+      /**
+       * Holds if the guard `g` validates the SSA definition `def` upon evaluating to `val`.
+       */
+      predicate guardChecksDef(Guard g, SsaDefinition def, GuardValue val, State state) {
+        exists(Expr e |
+          guardChecks(g, e, val, state) and
+          guardReadsSsaVar(e, def)
+        )
+      }
     }
 
     /**