From fb9f848fba7e54519ae9e697f56503981916f84e Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 16 Jan 2024 22:15:50 +0000 Subject: [PATCH 1/4] C++: Change the interface of 'FlowAfterFree' so that the module it takes a single module as a parameter. --- cpp/ql/src/Critical/FlowAfterFree.qll | 64 +++++++++++++-------------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/cpp/ql/src/Critical/FlowAfterFree.qll b/cpp/ql/src/Critical/FlowAfterFree.qll index 1dc5ad854b12..9fbed63b4495 100644 --- a/cpp/ql/src/Critical/FlowAfterFree.qll +++ b/cpp/ql/src/Critical/FlowAfterFree.qll @@ -2,20 +2,6 @@ import cpp import semmle.code.cpp.dataflow.new.DataFlow private import semmle.code.cpp.ir.IR -/** - * Signature for a predicate that holds if `n.asExpr() = e` and `n` is a sink in - * the `FlowFromFreeConfig` module. - */ -private signature predicate isSinkSig(DataFlow::Node n, Expr e); - -/** - * Holds if `dealloc` is a deallocation expression and `e` is an expression such - * that `isFree(_, e)` holds for some `isFree` predicate satisfying `isSinkSig`, - * and this source-sink pair should be excluded from the analysis. - */ -bindingset[dealloc, e] -private signature predicate isExcludedSig(DeallocationExpr dealloc, Expr e); - /** * Holds if `(b1, i1)` strictly post-dominates `(b2, i2)` */ @@ -38,27 +24,32 @@ predicate strictlyDominates(IRBlock b1, int i1, IRBlock b2, int i2) { b1.strictlyDominates(b2) } -predicate sinkStrictlyPostDominatesSource(DataFlow::Node source, DataFlow::Node sink) { - exists(IRBlock b1, int i1, IRBlock b2, int i2 | - source.hasIndexInBlock(b1, i1) and - sink.hasIndexInBlock(b2, i2) and - strictlyPostDominates(b2, i2, b1, i1) - ) -} +signature module FlowFromFreeParamSig { + /** + * Signature for a predicate that holds if `n.asExpr() = e` and `n` is a sink in + * the `FlowFromFreeConfig` module. + */ + predicate isSink(DataFlow::Node n, Expr e); -predicate sourceStrictlyDominatesSink(DataFlow::Node source, DataFlow::Node sink) { - exists(IRBlock b1, int i1, IRBlock b2, int i2 | - source.hasIndexInBlock(b1, i1) and - sink.hasIndexInBlock(b2, i2) and - strictlyDominates(b1, i1, b2, i2) - ) + /** + * Holds if `dealloc` is a deallocation expression and `e` is an expression such + * that `isFree(_, e)` holds for some `isFree` predicate satisfying `isSinkSig`, + * and this source-sink pair should be excluded from the analysis. + */ + bindingset[dealloc, e] + predicate isExcluded(DeallocationExpr dealloc, Expr e); } /** * Constructs a `FlowFromFreeConfig` module that can be used to find flow between * a pointer being freed by some deallocation function, and a user-specified sink. + * + * In order to reduce false positives, the set of sinks is restricted to only those + * that satisfy at least one of the following two criteria: + * 1. The source dominates the sink, or + * 2. The sink post-dominates the source. */ -module FlowFromFree { +module FlowFromFree { module FlowFromFreeConfig implements DataFlow::StateConfigSig { class FlowState instanceof Expr { FlowState() { isFree(_, _, this, _) } @@ -70,11 +61,20 @@ module FlowFromFree { pragma[inline] predicate isSink(DataFlow::Node sink, FlowState state) { - exists(Expr e, DeallocationExpr dealloc | - isASink(sink, e) and - isFree(_, _, state, dealloc) and + exists( + Expr e, DataFlow::Node source, DeallocationExpr dealloc, IRBlock b1, int i1, IRBlock b2, + int i2 + | + P::isSink(sink, e) and + isFree(source, _, state, dealloc) and e != state and - not isExcluded(dealloc, e) + not P::isExcluded(dealloc, e) and + source.hasIndexInBlock(b1, i1) and + sink.hasIndexInBlock(b2, i2) + | + strictlyDominates(b1, i1, b2, i2) + or + strictlyPostDominates(b2, i2, b1, i1) ) } From 5a72d9171b436f0e06b4a1d5edbbf9e40edc75b2 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 16 Jan 2024 22:18:50 +0000 Subject: [PATCH 2/4] C++: Add another predicate to the module signature. --- cpp/ql/src/Critical/FlowAfterFree.qll | 34 +++++++++++++++++++-------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/cpp/ql/src/Critical/FlowAfterFree.qll b/cpp/ql/src/Critical/FlowAfterFree.qll index 9fbed63b4495..bd3358f3c313 100644 --- a/cpp/ql/src/Critical/FlowAfterFree.qll +++ b/cpp/ql/src/Critical/FlowAfterFree.qll @@ -38,6 +38,12 @@ signature module FlowFromFreeParamSig { */ bindingset[dealloc, e] predicate isExcluded(DeallocationExpr dealloc, Expr e); + + /** + * Holds if `sink` should be considered a `sink` when the source of flow is `source`. + */ + bindingset[source, sink] + default predicate sourceSinkIsRelated(DataFlow::Node source, DataFlow::Node sink) { any() } } /** @@ -61,20 +67,12 @@ module FlowFromFree { pragma[inline] predicate isSink(DataFlow::Node sink, FlowState state) { - exists( - Expr e, DataFlow::Node source, DeallocationExpr dealloc, IRBlock b1, int i1, IRBlock b2, - int i2 - | + exists(Expr e, DataFlow::Node source, DeallocationExpr dealloc | P::isSink(sink, e) and isFree(source, _, state, dealloc) and e != state and not P::isExcluded(dealloc, e) and - source.hasIndexInBlock(b1, i1) and - sink.hasIndexInBlock(b2, i2) - | - strictlyDominates(b1, i1, b2, i2) - or - strictlyPostDominates(b2, i2, b1, i1) + P::sourceSinkIsRelated(source, sink) ) } @@ -129,3 +127,19 @@ predicate isExFreePoolCall(FunctionCall fc, Expr e) { fc.getTarget().hasGlobalName("ExFreePool") ) } + +/** + * Holds if either `source` strictly dominates `sink`, or `sink` strictly + * post-dominates `source`. + */ +bindingset[source, sink] +predicate defaultSourceSinkIsRelated(DataFlow::Node source, DataFlow::Node sink) { + exists(IRBlock b1, int i1, IRBlock b2, int i2 | + source.hasIndexInBlock(b1, i1) and + sink.hasIndexInBlock(b2, i2) + | + strictlyDominates(b1, i1, b2, i2) + or + strictlyPostDominates(b2, i2, b1, i1) + ) +} From 32caf64253cdecbe55febf24345ff363025710bc Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 16 Jan 2024 22:15:59 +0000 Subject: [PATCH 3/4] C++: Convert the use-after-free and double-free libraries to use new interface. --- cpp/ql/src/Critical/DoubleFree.ql | 32 +++++++++++------------------ cpp/ql/src/Critical/UseAfterFree.ql | 29 ++++++++++---------------- 2 files changed, 23 insertions(+), 38 deletions(-) diff --git a/cpp/ql/src/Critical/DoubleFree.ql b/cpp/ql/src/Critical/DoubleFree.ql index cf7334f2d696..1a17bc702bda 100644 --- a/cpp/ql/src/Critical/DoubleFree.ql +++ b/cpp/ql/src/Critical/DoubleFree.ql @@ -13,7 +13,6 @@ import cpp import semmle.code.cpp.dataflow.new.DataFlow -import semmle.code.cpp.ir.IR import FlowAfterFree import DoubleFree::PathGraph @@ -42,28 +41,21 @@ predicate isExcludeFreePair(DeallocationExpr dealloc1, Expr e) { ) } -module DoubleFree = FlowFromFree; +module DoubleFreeParam implements FlowFromFreeParamSig { + predicate isSink = isFree/2; -/* - * In order to reduce false positives, the set of sinks is restricted to only those - * that satisfy at least one of the following two criteria: - * 1. The source dominates the sink, or - * 2. The sink post-dominates the source. - */ + predicate isExcluded = isExcludeFreePair/2; + + predicate sourceSinkIsRelated = defaultSourceSinkIsRelated/2; +} -from - DoubleFree::PathNode source, DoubleFree::PathNode sink, DeallocationExpr dealloc, Expr e2, - DataFlow::Node srcNode, DataFlow::Node sinkNode +module DoubleFree = FlowFromFree; + +from DoubleFree::PathNode source, DoubleFree::PathNode sink, DeallocationExpr dealloc, Expr e2 where DoubleFree::flowPath(source, sink) and - source.getNode() = srcNode and - sink.getNode() = sinkNode and - isFree(srcNode, _, _, dealloc) and - isFree(sinkNode, e2) and - ( - sinkStrictlyPostDominatesSource(srcNode, sinkNode) or - sourceStrictlyDominatesSink(srcNode, sinkNode) - ) -select sinkNode, source, sink, + isFree(source.getNode(), _, _, dealloc) and + isFree(sink.getNode(), e2) +select sink.getNode(), source, sink, "Memory pointed to by '" + e2.toString() + "' may already have been freed by $@.", dealloc, dealloc.toString() diff --git a/cpp/ql/src/Critical/UseAfterFree.ql b/cpp/ql/src/Critical/UseAfterFree.ql index 06198ddfc38a..40f4bbf3eac9 100644 --- a/cpp/ql/src/Critical/UseAfterFree.ql +++ b/cpp/ql/src/Critical/UseAfterFree.ql @@ -173,26 +173,19 @@ predicate isExcludeFreeUsePair(DeallocationExpr dealloc1, Expr e) { isExFreePoolCall(_, e) } -module UseAfterFree = FlowFromFree; +module UseAfterFreeParam implements FlowFromFreeParamSig { + predicate isSink = isUse/2; -/* - * In order to reduce false positives, the set of sinks is restricted to only those - * that satisfy at least one of the following two criteria: - * 1. The source dominates the sink, or - * 2. The sink post-dominates the source. - */ + predicate isExcluded = isExcludeFreeUsePair/2; + + predicate sourceSinkIsRelated = defaultSourceSinkIsRelated/2; +} -from - UseAfterFree::PathNode source, UseAfterFree::PathNode sink, DeallocationExpr dealloc, - DataFlow::Node srcNode, DataFlow::Node sinkNode +module UseAfterFree = FlowFromFree; + +from UseAfterFree::PathNode source, UseAfterFree::PathNode sink, DeallocationExpr dealloc where UseAfterFree::flowPath(source, sink) and - source.getNode() = srcNode and - sink.getNode() = sinkNode and - isFree(srcNode, _, _, dealloc) and - ( - sinkStrictlyPostDominatesSource(srcNode, sinkNode) or - sourceStrictlyDominatesSink(srcNode, sinkNode) - ) -select sinkNode, source, sink, "Memory may have been previously freed by $@.", dealloc, + isFree(source.getNode(), _, _, dealloc) +select sink.getNode(), source, sink, "Memory may have been previously freed by $@.", dealloc, dealloc.toString() From ddc87ac1d71937ee04a0e9dc0829ef3321dbcd4e Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 16 Jan 2024 22:16:05 +0000 Subject: [PATCH 4/4] C++: Accept test changes. --- .../test/query-tests/Critical/MemoryFreed/DoubleFree.expected | 2 -- .../test/query-tests/Critical/MemoryFreed/UseAfterFree.expected | 2 -- 2 files changed, 4 deletions(-) diff --git a/cpp/ql/test/query-tests/Critical/MemoryFreed/DoubleFree.expected b/cpp/ql/test/query-tests/Critical/MemoryFreed/DoubleFree.expected index bfd869b74109..f30092f06265 100644 --- a/cpp/ql/test/query-tests/Critical/MemoryFreed/DoubleFree.expected +++ b/cpp/ql/test/query-tests/Critical/MemoryFreed/DoubleFree.expected @@ -2,7 +2,6 @@ edges | test_free.cpp:11:10:11:10 | pointer to free output argument | test_free.cpp:14:10:14:10 | a | | test_free.cpp:30:10:30:10 | pointer to free output argument | test_free.cpp:31:27:31:27 | a | | test_free.cpp:35:10:35:10 | pointer to free output argument | test_free.cpp:37:27:37:27 | a | -| test_free.cpp:42:27:42:27 | pointer to free output argument | test_free.cpp:44:27:44:27 | a | | test_free.cpp:42:27:42:27 | pointer to free output argument | test_free.cpp:46:10:46:10 | a | | test_free.cpp:44:27:44:27 | pointer to free output argument | test_free.cpp:46:10:46:10 | a | | test_free.cpp:50:27:50:27 | pointer to free output argument | test_free.cpp:51:10:51:10 | a | @@ -20,7 +19,6 @@ nodes | test_free.cpp:35:10:35:10 | pointer to free output argument | semmle.label | pointer to free output argument | | test_free.cpp:37:27:37:27 | a | semmle.label | a | | test_free.cpp:42:27:42:27 | pointer to free output argument | semmle.label | pointer to free output argument | -| test_free.cpp:44:27:44:27 | a | semmle.label | a | | test_free.cpp:44:27:44:27 | pointer to free output argument | semmle.label | pointer to free output argument | | test_free.cpp:46:10:46:10 | a | semmle.label | a | | test_free.cpp:46:10:46:10 | a | semmle.label | a | diff --git a/cpp/ql/test/query-tests/Critical/MemoryFreed/UseAfterFree.expected b/cpp/ql/test/query-tests/Critical/MemoryFreed/UseAfterFree.expected index 707faf8d22d7..bf2ba1ad092b 100644 --- a/cpp/ql/test/query-tests/Critical/MemoryFreed/UseAfterFree.expected +++ b/cpp/ql/test/query-tests/Critical/MemoryFreed/UseAfterFree.expected @@ -1,7 +1,6 @@ edges | test_free.cpp:11:10:11:10 | pointer to free output argument | test_free.cpp:12:5:12:5 | a | | test_free.cpp:11:10:11:10 | pointer to free output argument | test_free.cpp:13:5:13:6 | * ... | -| test_free.cpp:42:27:42:27 | pointer to free output argument | test_free.cpp:43:22:43:22 | a | | test_free.cpp:42:27:42:27 | pointer to free output argument | test_free.cpp:45:5:45:5 | a | | test_free.cpp:44:27:44:27 | pointer to free output argument | test_free.cpp:45:5:45:5 | a | | test_free.cpp:69:10:69:10 | pointer to free output argument | test_free.cpp:71:9:71:9 | a | @@ -28,7 +27,6 @@ nodes | test_free.cpp:12:5:12:5 | a | semmle.label | a | | test_free.cpp:13:5:13:6 | * ... | semmle.label | * ... | | test_free.cpp:42:27:42:27 | pointer to free output argument | semmle.label | pointer to free output argument | -| test_free.cpp:43:22:43:22 | a | semmle.label | a | | test_free.cpp:44:27:44:27 | pointer to free output argument | semmle.label | pointer to free output argument | | test_free.cpp:45:5:45:5 | a | semmle.label | a | | test_free.cpp:45:5:45:5 | a | semmle.label | a |