From f662cceb0b56249cb07753ba9e5ba3ae14af43cd Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 15 Aug 2023 11:07:14 +0100 Subject: [PATCH 1/2] C++: Use value numbering to better detect whether a write is certain. --- .../code/cpp/ir/dataflow/internal/SsaInternalsCommon.qll | 3 ++- cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp | 2 +- cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternalsCommon.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternalsCommon.qll index 60cb843b61f..4410e2e9e69 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternalsCommon.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternalsCommon.qll @@ -6,6 +6,7 @@ private import DataFlowImplCommon as DataFlowImplCommon private import DataFlowUtil private import semmle.code.cpp.models.interfaces.PointerWrapper private import DataFlowPrivate +private import semmle.code.cpp.ir.ValueNumbering /** * Holds if `operand` is an operand that is not used by the dataflow library. @@ -864,7 +865,7 @@ private module Cached { * to a specific address. */ private predicate isCertainAddress(Operand operand) { - operand.getDef() instanceof VariableAddressInstruction + valueNumberOfOperand(operand).getAnInstruction() instanceof VariableAddressInstruction or operand.getType() instanceof Cpp::ReferenceType } diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp index c5004157af1..c49d9092cd7 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp @@ -732,7 +732,7 @@ void test_does_not_write_source_to_dereference() { int x; does_not_write_source_to_dereference(&x); - sink(x); // $ ast,ir=733:7 SPURIOUS: ast,ir=726:11 + sink(x); // $ ast=733:7 ir SPURIOUS: ast=726:11 } void sometimes_calls_sink_eq(int x, int n) { diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp index 9810418a95e..2df0fc85bb6 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp @@ -134,7 +134,7 @@ void pointer_test() { sink(*p3); // $ ast,ir *p3 = 0; - sink(*p3); // $ SPURIOUS: ast,ir + sink(*p3); // $ SPURIOUS: ast } // --- return values --- From 569f3c9b788b1f94381c267f9909993778348a5b Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 15 Aug 2023 11:08:01 +0100 Subject: [PATCH 2/2] C++: Don't do indirect (instruction -> operand) flow when there's a store to the address in between the instruction and the operand. --- .../cpp/ir/dataflow/internal/DataFlowUtil.qll | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll index f834259dc37..6470741d541 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll @@ -1520,6 +1520,25 @@ private module Cached { ) } + /** + * Holds if `operand.getDef() = instr`, but there exists a `StoreInstruction` that + * writes to an address that is equivalent to the value computed by `instr` in + * between `instr` and `operand`, and therefore there should not be flow from `*instr` + * to `*operand`. + */ + pragma[nomagic] + private predicate isStoredToBetween(Instruction instr, Operand operand) { + simpleOperandLocalFlowStep(pragma[only_bind_into](instr), pragma[only_bind_into](operand)) and + exists(StoreInstruction store, IRBlock block, int storeIndex, int instrIndex, int operandIndex | + store.getDestinationAddress() = instr and + block.getInstruction(storeIndex) = store and + block.getInstruction(instrIndex) = instr and + block.getInstruction(operandIndex) = operand.getUse() and + instrIndex < storeIndex and + storeIndex < operandIndex + ) + } + private predicate indirectionInstructionFlow( RawIndirectInstruction nodeFrom, IndirectOperand nodeTo ) { @@ -1529,7 +1548,8 @@ private module Cached { simpleOperandLocalFlowStep(pragma[only_bind_into](instr), pragma[only_bind_into](operand)) | hasOperandAndIndex(nodeTo, operand, pragma[only_bind_into](indirectionIndex)) and - hasInstructionAndIndex(nodeFrom, instr, pragma[only_bind_into](indirectionIndex)) + hasInstructionAndIndex(nodeFrom, instr, pragma[only_bind_into](indirectionIndex)) and + not isStoredToBetween(instr, operand) ) }