diff --git a/java/ql/consistency-queries/SsaConsistency.ql b/java/ql/consistency-queries/SsaConsistency.ql new file mode 100644 index 000000000000..b62db63ac5ba --- /dev/null +++ b/java/ql/consistency-queries/SsaConsistency.ql @@ -0,0 +1,3 @@ +import java +import semmle.code.java.dataflow.internal.SsaImpl +import Impl::Consistency diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/BaseSSA.qll b/java/ql/lib/semmle/code/java/dataflow/internal/BaseSSA.qll index 2c37efea1a46..1b912f919756 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/BaseSSA.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/BaseSSA.qll @@ -168,12 +168,15 @@ private module SsaInput implements SsaImplCommon::InputSig { * Holds if the `i`th of basic block `bb` reads source variable `v`. */ predicate variableRead(BasicBlock bb, int i, SourceVariable v, boolean certain) { - exists(VarRead use | - v.getAnAccess() = use and bb.getNode(i) = use.getControlFlowNode() and certain = true + hasDominanceInformation(bb) and + ( + exists(VarRead use | + v.getAnAccess() = use and bb.getNode(i) = use.getControlFlowNode() and certain = true + ) + or + variableCapture(v, _, bb, i) and + certain = false ) - or - variableCapture(v, _, bb, i) and - certain = false } } 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 d5343834ffe2..95ba126bcf71 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll @@ -204,12 +204,15 @@ private module SsaInput implements SsaImplCommon::InputSig { * This includes implicit reads via calls. */ predicate variableRead(BasicBlock bb, int i, SourceVariable v, boolean certain) { - exists(VarRead use | - v.getAnAccess() = use and bb.getNode(i) = use.getControlFlowNode() and certain = true + hasDominanceInformation(bb) and + ( + exists(VarRead use | + v.getAnAccess() = use and bb.getNode(i) = use.getControlFlowNode() and certain = true + ) + or + variableCapture(v, _, bb, i) and + certain = false ) - or - variableCapture(v, _, bb, i) and - certain = false } } diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected index 3e838551f81d..eabbc3ad4b8c 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected @@ -12,7 +12,6 @@ newStyleBarrierGuards | barrier-guards.rb:28:5:28:7 | foo | | barrier-guards.rb:37:21:38:19 | [input] SSA phi read(foo) | | barrier-guards.rb:38:5:38:7 | foo | -| barrier-guards.rb:43:16:46:5 | [input] SSA phi read(foo) | | barrier-guards.rb:45:9:45:11 | foo | | barrier-guards.rb:70:22:71:19 | [input] SSA phi read(foo) | | barrier-guards.rb:71:5:71:7 | foo | @@ -51,9 +50,7 @@ newStyleBarrierGuards | barrier-guards.rb:199:4:199:15 | [input] SSA phi read(foo) | | barrier-guards.rb:199:4:199:31 | [input] SSA phi read(foo) | | barrier-guards.rb:199:20:199:31 | [input] SSA phi read(foo) | -| barrier-guards.rb:203:4:203:15 | [input] SSA phi read(foo) | | barrier-guards.rb:203:36:203:47 | [input] SSA phi read(foo) | -| barrier-guards.rb:207:21:207:21 | [input] SSA phi read(foo) | | barrier-guards.rb:207:22:208:19 | [input] SSA phi read(foo) | | barrier-guards.rb:208:5:208:7 | foo | | barrier-guards.rb:211:22:212:19 | [input] SSA phi read(foo) | @@ -64,8 +61,6 @@ newStyleBarrierGuards | barrier-guards.rb:219:21:219:32 | [input] SSA phi read(foo) | | barrier-guards.rb:219:95:220:19 | [input] SSA phi read(foo) | | barrier-guards.rb:220:5:220:7 | foo | -| barrier-guards.rb:227:21:227:21 | [input] SSA phi read(foo) | -| barrier-guards.rb:227:22:228:7 | [input] SSA phi read(foo) | | barrier-guards.rb:232:18:233:19 | [input] SSA phi read(foo) | | barrier-guards.rb:233:5:233:7 | foo | | barrier-guards.rb:237:19:237:38 | [input] SSA phi read(foo) | diff --git a/rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/SsaConsistency.expected b/rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/SsaConsistency.expected new file mode 100644 index 000000000000..d5074d0d43a3 --- /dev/null +++ b/rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/SsaConsistency.expected @@ -0,0 +1,4 @@ +uselessPhiNode +| sqlx.rs:155:5:157:5 | phi | 1 | +phiWithoutTwoPriorRefs +| sqlx.rs:155:5:157:5 | phi | 1 | diff --git a/shared/ssa/codeql/ssa/Ssa.qll b/shared/ssa/codeql/ssa/Ssa.qll index 5bdc2b8a28ad..6b0c6b0bf4d6 100644 --- a/shared/ssa/codeql/ssa/Ssa.qll +++ b/shared/ssa/codeql/ssa/Ssa.qll @@ -307,6 +307,7 @@ module Make Input> { private predicate inReadDominanceFrontier(BasicBlock bb, SourceVariable v) { exists(BasicBlock readbb | inDominanceFrontier(readbb, bb) | ssaDefReachesRead(v, _, readbb, _) and + variableRead(readbb, _, v, true) and not variableWrite(readbb, _, v, _) or synthPhiRead(readbb, v) and @@ -1379,6 +1380,56 @@ module Make Input> { not def.definesAt(v, getImmediateBasicBlockDominator*(bb), _) ) } + + /** Holds if the end of a basic block can be reached by multiple definitions. */ + query predicate nonUniqueDefReachesEndOfBlock(Definition def, SourceVariable v, BasicBlock bb) { + ssaDefReachesEndOfBlock(bb, def, v) and + not exists(unique(Definition def0 | ssaDefReachesEndOfBlock(bb, def0, v))) + } + + /** Holds if a phi node has less than two inputs. */ + query predicate uselessPhiNode(PhiNode phi, int inputs) { + inputs = count(Definition inp | phiHasInputFromBlock(phi, inp, _)) and + inputs < 2 + } + + /** Holds if a certain read does not have a prior reference. */ + query predicate readWithoutPriorRef(SourceVariable v, BasicBlock bb, int i) { + variableRead(bb, i, v, true) and + not AdjacentSsaRefs::adjacentRefRead(_, _, bb, i, v) + } + + /** + * Holds if a certain read has multiple prior references. The introduction + * of phi reads should make the prior reference unique. + */ + query predicate readWithMultiplePriorRefs( + SourceVariable v, BasicBlock bb1, int i1, BasicBlock bb2, int i2 + ) { + AdjacentSsaRefs::adjacentRefRead(bb1, i1, bb2, i2, v) and + 2 <= + strictcount(BasicBlock bb0, int i0 | AdjacentSsaRefs::adjacentRefRead(bb0, i0, bb1, i1, v)) + } + + /** Holds if `phi` has less than 2 immediately prior references. */ + query predicate phiWithoutTwoPriorRefs(PhiNode phi, int inputRefs) { + exists(BasicBlock bbPhi, SourceVariable v | + phi.definesAt(v, bbPhi, _) and + inputRefs = + count(BasicBlock bb, int i | AdjacentSsaRefs::adjacentRefPhi(bb, i, _, bbPhi, v)) and + inputRefs < 2 + ) + } + + /** + * Holds if the phi read for `v` at `bb` has less than 2 immediately prior + * references. + */ + query predicate phiReadWithoutTwoPriorRefs(BasicBlock bbPhi, SourceVariable v, int inputRefs) { + synthPhiRead(bbPhi, v) and + inputRefs = count(BasicBlock bb, int i | AdjacentSsaRefs::adjacentRefPhi(bb, i, _, bbPhi, v)) and + inputRefs < 2 + } } /** Provides the input to `DataFlowIntegration`. */