diff --git a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll index 612bca35a600..c1c19fa44508 100644 --- a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll +++ b/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) } /** diff --git a/java/ql/lib/semmle/code/java/Member.qll b/java/ql/lib/semmle/code/java/Member.qll index 1e814117e9ee..7d84dbd379d0 100644 --- a/java/ql/lib/semmle/code/java/Member.qll +++ b/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 } diff --git a/java/ql/lib/semmle/code/java/controlflow/Guards.qll b/java/ql/lib/semmle/code/java/controlflow/Guards.qll index c33658cb67b6..778ebe6e8789 100644 --- a/java/ql/lib/semmle/code/java/controlflow/Guards.qll +++ b/java/ql/lib/semmle/code/java/controlflow/Guards.qll @@ -490,12 +490,6 @@ module Guards_v2 = GuardsImpl::Logic; /** INTERNAL: Don't use. */ module Guards_v3 = GuardsImpl::Logic; -/** 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 diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll b/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll index 5a6c5ff6339b..51da69e9d64a 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll @@ -562,14 +562,20 @@ private module Cached { cached // nothing is actually cached module BarrierGuard { - 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::guardChecksDef(g, def, val) and + exists(state) } private Node getABarrierNodeImpl() { - result = DataFlowIntegrationImpl::BarrierGuard::getABarrierNode() + result = + DataFlowIntegrationImpl::BarrierGuardDefWithState::getABarrierNode(_) } predicate getABarrierNode = getABarrierNodeImpl/0; diff --git a/java/ql/lib/semmle/code/java/security/ArithmeticCommon.qll b/java/ql/lib/semmle/code/java/security/ArithmeticCommon.qll index e0d6ff305c30..4f4c20a5263b 100644 --- a/java/ql/lib/semmle/code/java/security/ArithmeticCommon.qll +++ b/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::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::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 diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index e789d3c47785..da6f242bde52 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/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 { - /** 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::getABarrierNode() - or - this = ValidationMethod::getAValidatedNode() } } @@ -120,8 +91,7 @@ private predicate allowedPrefixGuard(Guard g, Expr e, boolean branch) { private class AllowedPrefixSanitizer extends PathInjectionSanitizer { AllowedPrefixSanitizer() { - this = DataFlow::BarrierGuard::getABarrierNode() or - this = ValidationMethod::getAValidatedNode() + this = DataFlow::BarrierGuard::getABarrierNode() } } @@ -139,10 +109,7 @@ private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) { } private class DotDotCheckSanitizer extends PathInjectionSanitizer { - DotDotCheckSanitizer() { - this = DataFlow::BarrierGuard::getABarrierNode() or - this = ValidationMethod::getAValidatedNode() - } + DotDotCheckSanitizer() { this = DataFlow::BarrierGuard::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::getABarrierNode() or - this = ValidationMethod::getAValidatedNode() - } + BlockListSanitizer() { this = DataFlow::BarrierGuard::getABarrierNode() } } private class ConstantOrRegex extends Expr { @@ -368,7 +332,6 @@ private class FileConstructorChildArgumentStep extends AdditionalTaintStep { n2.asExpr() = constrCall | not n1 = DataFlow::BarrierGuard::getABarrierNode() and - not n1 = ValidationMethod::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::getABarrierNode() or - this = ValidationMethod::getAValidatedNode() + this = DataFlow::BarrierGuard::getABarrierNode() } } diff --git a/shared/controlflow/codeql/controlflow/Guards.qll b/shared/controlflow/codeql/controlflow/Guards.qll index 34f42036292b..ab500456058d 100644 --- a/shared/controlflow/codeql/controlflow/Guards.qll +++ b/shared/controlflow/codeql/controlflow/Guards.qll @@ -935,18 +935,6 @@ module Make Input> { } } - private predicate booleanGuard(Guard guard, GuardValue val) { - exists(guard) and exists(val.asBooleanValue()) - } - - private module BooleanImplies = ImpliesTC; - - /** 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 Input> { private module StatefulWrapper = ValidationWrapperWithState; /** - * 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 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 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 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 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) + ) + } } /**