From a0ef7955b1e5ac4129e61365ef25abeb199b4f04 Mon Sep 17 00:00:00 2001 From: Benjamin Rodes Date: Tue, 16 Jan 2024 13:15:36 -0500 Subject: [PATCH 01/12] Updating FlowAfterFree to not enforce dominance of source/sink. DoubleFree and UseAfterFree queries now enforce dominance. --- cpp/ql/src/Critical/DoubleFree.ql | 24 +++++++++++++++---- cpp/ql/src/Critical/FlowAfterFree.qll | 34 ++++++++++++++------------- cpp/ql/src/Critical/UseAfterFree.ql | 21 ++++++++++++++--- 3 files changed, 56 insertions(+), 23 deletions(-) diff --git a/cpp/ql/src/Critical/DoubleFree.ql b/cpp/ql/src/Critical/DoubleFree.ql index 92cdb0807e4d..cf7334f2d696 100644 --- a/cpp/ql/src/Critical/DoubleFree.ql +++ b/cpp/ql/src/Critical/DoubleFree.ql @@ -13,6 +13,7 @@ import cpp import semmle.code.cpp.dataflow.new.DataFlow +import semmle.code.cpp.ir.IR import FlowAfterFree import DoubleFree::PathGraph @@ -43,11 +44,26 @@ predicate isExcludeFreePair(DeallocationExpr dealloc1, Expr e) { module DoubleFree = FlowFromFree; -from DoubleFree::PathNode source, DoubleFree::PathNode sink, DeallocationExpr dealloc, Expr e2 +/* + * 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. + */ + +from + DoubleFree::PathNode source, DoubleFree::PathNode sink, DeallocationExpr dealloc, Expr e2, + DataFlow::Node srcNode, DataFlow::Node sinkNode where DoubleFree::flowPath(source, sink) and - isFree(source.getNode(), _, _, dealloc) and - isFree(sink.getNode(), e2) -select sink.getNode(), source, sink, + 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, "Memory pointed to by '" + e2.toString() + "' may already have been freed by $@.", dealloc, dealloc.toString() diff --git a/cpp/ql/src/Critical/FlowAfterFree.qll b/cpp/ql/src/Critical/FlowAfterFree.qll index caca108f4b26..1dc5ad854b12 100644 --- a/cpp/ql/src/Critical/FlowAfterFree.qll +++ b/cpp/ql/src/Critical/FlowAfterFree.qll @@ -38,14 +38,25 @@ 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) + ) +} + +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) + ) +} + /** * 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 FlowFromFreeConfig implements DataFlow::StateConfigSig { @@ -59,20 +70,11 @@ module FlowFromFree { pragma[inline] predicate isSink(DataFlow::Node sink, FlowState state) { - exists( - Expr e, DataFlow::Node source, IRBlock b1, int i1, IRBlock b2, int i2, - DeallocationExpr dealloc - | + exists(Expr e, DeallocationExpr dealloc | isASink(sink, e) and - isFree(source, _, state, dealloc) and + isFree(_, _, state, dealloc) and e != state and - source.hasIndexInBlock(b1, i1) and - sink.hasIndexInBlock(b2, i2) and not isExcluded(dealloc, e) - | - strictlyDominates(b1, i1, b2, i2) - or - strictlyPostDominates(b2, i2, b1, i1) ) } diff --git a/cpp/ql/src/Critical/UseAfterFree.ql b/cpp/ql/src/Critical/UseAfterFree.ql index 4b27369074c5..06198ddfc38a 100644 --- a/cpp/ql/src/Critical/UseAfterFree.ql +++ b/cpp/ql/src/Critical/UseAfterFree.ql @@ -175,9 +175,24 @@ predicate isExcludeFreeUsePair(DeallocationExpr dealloc1, Expr e) { module UseAfterFree = FlowFromFree; -from UseAfterFree::PathNode source, UseAfterFree::PathNode sink, DeallocationExpr dealloc +/* + * 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. + */ + +from + UseAfterFree::PathNode source, UseAfterFree::PathNode sink, DeallocationExpr dealloc, + DataFlow::Node srcNode, DataFlow::Node sinkNode where UseAfterFree::flowPath(source, sink) and - isFree(source.getNode(), _, _, dealloc) -select sink.getNode(), source, sink, "Memory may have been previously freed by $@.", dealloc, + 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, dealloc.toString() From 9a0e2e57baea1d4ea7d5ac1b8f8a86cbfe73e91f Mon Sep 17 00:00:00 2001 From: Benjamin Rodes Date: Tue, 16 Jan 2024 13:18:25 -0500 Subject: [PATCH 02/12] Updating .expected --- .../test/query-tests/Critical/MemoryFreed/DoubleFree.expected | 2 ++ .../test/query-tests/Critical/MemoryFreed/UseAfterFree.expected | 2 ++ 2 files changed, 4 insertions(+) diff --git a/cpp/ql/test/query-tests/Critical/MemoryFreed/DoubleFree.expected b/cpp/ql/test/query-tests/Critical/MemoryFreed/DoubleFree.expected index f30092f06265..bfd869b74109 100644 --- a/cpp/ql/test/query-tests/Critical/MemoryFreed/DoubleFree.expected +++ b/cpp/ql/test/query-tests/Critical/MemoryFreed/DoubleFree.expected @@ -2,6 +2,7 @@ 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 | @@ -19,6 +20,7 @@ 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 bf2ba1ad092b..707faf8d22d7 100644 --- a/cpp/ql/test/query-tests/Critical/MemoryFreed/UseAfterFree.expected +++ b/cpp/ql/test/query-tests/Critical/MemoryFreed/UseAfterFree.expected @@ -1,6 +1,7 @@ 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 | @@ -27,6 +28,7 @@ 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 | From 39dafd6f6ac06e2fd78d1863e15e9e62a36a8a0f Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Wed, 17 Jan 2024 16:02:46 +0000 Subject: [PATCH 03/12] C++: Suggestions to #15343 (#39) * C++: Change the interface of 'FlowAfterFree' so that the module it takes a single module as a parameter. * C++: Add another predicate to the module signature. * C++: Convert the use-after-free and double-free libraries to use new interface. * C++: Accept test changes. --- cpp/ql/src/Critical/DoubleFree.ql | 32 +++----- cpp/ql/src/Critical/FlowAfterFree.qll | 78 +++++++++++-------- cpp/ql/src/Critical/UseAfterFree.ql | 29 +++---- .../Critical/MemoryFreed/DoubleFree.expected | 2 - .../MemoryFreed/UseAfterFree.expected | 2 - 5 files changed, 69 insertions(+), 74 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/FlowAfterFree.qll b/cpp/ql/src/Critical/FlowAfterFree.qll index 1dc5ad854b12..bd3358f3c313 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,38 @@ 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); + + /** + * 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() } } /** * 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 +67,12 @@ 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 | + P::isSink(sink, e) and + isFree(source, _, state, dealloc) and e != state and - not isExcluded(dealloc, e) + not P::isExcluded(dealloc, e) and + 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) + ) +} 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() 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 | From 7e70b307727da40cf087622f8e0c6c071b42efec Mon Sep 17 00:00:00 2001 From: Benjamin Rodes Date: Thu, 18 Jan 2024 09:59:28 -0500 Subject: [PATCH 04/12] Adding missing windows library free functions to deallocation set --- .../models/implementations/Deallocation.qll | 168 +++++++++--------- 1 file changed, 86 insertions(+), 82 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Deallocation.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Deallocation.qll index de1c3389be00..b16f3aee0e85 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Deallocation.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Deallocation.qll @@ -4,86 +4,90 @@ * for usage information. */ -import semmle.code.cpp.models.interfaces.Deallocation + import semmle.code.cpp.models.interfaces.Deallocation -/** - * A deallocation function such as `free`. - */ -private class StandardDeallocationFunction extends DeallocationFunction { - int freedArg; - - StandardDeallocationFunction() { - this.hasGlobalOrStdOrBslName([ - // --- C library allocation - "free", "realloc" - ]) and - freedArg = 0 - or - this.hasGlobalName([ - // --- OpenSSL memory allocation - "CRYPTO_free", "CRYPTO_secure_free" - ]) and - freedArg = 0 - or - this.hasGlobalOrStdName([ - // --- Windows Memory Management for Windows Drivers - "ExFreePoolWithTag", "ExDeleteTimer", "IoFreeMdl", "IoFreeWorkItem", "IoFreeErrorLogEntry", - "MmFreeContiguousMemory", "MmFreeContiguousMemorySpecifyCache", "MmFreeNonCachedMemory", - "MmFreeMappingAddress", "MmFreePagesFromMdl", "MmUnmapReservedMapping", - "MmUnmapLockedPages", - // --- Windows Global / Local legacy allocation - "LocalFree", "GlobalFree", "LocalReAlloc", "GlobalReAlloc", - // --- Windows System Services allocation - "VirtualFree", - // --- Windows COM allocation - "CoTaskMemFree", "CoTaskMemRealloc", - // --- Windows Automation - "SysFreeString", - // --- Solaris/BSD kernel memory allocator - "kmem_free" - ]) and - freedArg = 0 - or - this.hasGlobalOrStdName([ - // --- Windows Memory Management for Windows Drivers - "ExFreeToLookasideListEx", "ExFreeToPagedLookasideList", "ExFreeToNPagedLookasideList", - // --- NetBSD pool manager - "pool_put", "pool_cache_put" - ]) and - freedArg = 1 - or - this.hasGlobalOrStdName(["HeapFree", "HeapReAlloc"]) and - freedArg = 2 - } - - override int getFreedArg() { result = freedArg } -} - -/** - * An deallocation expression that is a function call, such as call to `free`. - */ -private class CallDeallocationExpr extends DeallocationExpr, FunctionCall { - DeallocationFunction target; - - CallDeallocationExpr() { target = this.getTarget() } - - override Expr getFreedExpr() { result = this.getArgument(target.getFreedArg()) } -} - -/** - * An deallocation expression that is a `delete` expression. - */ -private class DeleteDeallocationExpr extends DeallocationExpr, DeleteExpr { - DeleteDeallocationExpr() { this instanceof DeleteExpr } - - override Expr getFreedExpr() { result = this.getExpr() } -} - -/** - * An deallocation expression that is a `delete []` expression. - */ -private class DeleteArrayDeallocationExpr extends DeallocationExpr, DeleteArrayExpr { - DeleteArrayDeallocationExpr() { this instanceof DeleteArrayExpr } - - override Expr getFreedExpr() { result = this.getExpr() } -} + /** + * A deallocation function such as `free`. + */ + private class StandardDeallocationFunction extends DeallocationFunction { + int freedArg; + + StandardDeallocationFunction() { + this.hasGlobalOrStdOrBslName([ + // --- C library allocation + "free", "realloc" + ]) and + freedArg = 0 + or + this.hasGlobalName([ + // --- OpenSSL memory allocation + "CRYPTO_free", "CRYPTO_secure_free" + ]) and + freedArg = 0 + or + this.hasGlobalOrStdName([ + // --- Windows Memory Management for Windows Drivers + "ExFreePool", "ExFreePoolWithTag", "ExDeleteTimer", "IoFreeIrp", "IoFreeMdl", + "IoFreeErrorLogEntry", "IoFreeWorkItem", "MmFreeContiguousMemory", + "MmFreeContiguousMemorySpecifyCache", "MmFreeNonCachedMemory", "MmFreeMappingAddress", + "MmFreePagesFromMdl", "MmUnmapReservedMapping", "MmUnmapLockedPages", + "NdisFreeGenericObject", "NdisFreeMemory", "NdisFreeMemoryWithTag", "NdisFreeMdl", + "NdisFreeNetBufferListPool", "NdisFreeNetBufferPool", + // --- Windows Global / Local legacy allocation + "LocalFree", "GlobalFree", "LocalReAlloc", "GlobalReAlloc", + // --- Windows System Services allocation + "VirtualFree", + // --- Windows COM allocation + "CoTaskMemFree", "CoTaskMemRealloc", + // --- Windows Automation + "SysFreeString", + // --- Solaris/BSD kernel memory allocator + "kmem_free" + ]) and + freedArg = 0 + or + this.hasGlobalOrStdName([ + // --- Windows Memory Management for Windows Drivers + "ExFreeToLookasideListEx", "ExFreeToPagedLookasideList", "ExFreeToNPagedLookasideList", + "NdisFreeMemoryWithTagPriority", "StorPortFreeMdl", "StorPortFreePool", + // --- NetBSD pool manager + "pool_put", "pool_cache_put" + ]) and + freedArg = 1 + or + this.hasGlobalOrStdName(["HeapFree", "HeapReAlloc"]) and + freedArg = 2 + } + + override int getFreedArg() { result = freedArg } + } + + /** + * An deallocation expression that is a function call, such as call to `free`. + */ + private class CallDeallocationExpr extends DeallocationExpr, FunctionCall { + DeallocationFunction target; + + CallDeallocationExpr() { target = this.getTarget() } + + override Expr getFreedExpr() { result = this.getArgument(target.getFreedArg()) } + } + + /** + * An deallocation expression that is a `delete` expression. + */ + private class DeleteDeallocationExpr extends DeallocationExpr, DeleteExpr { + DeleteDeallocationExpr() { this instanceof DeleteExpr } + + override Expr getFreedExpr() { result = this.getExpr() } + } + + /** + * An deallocation expression that is a `delete []` expression. + */ + private class DeleteArrayDeallocationExpr extends DeallocationExpr, DeleteArrayExpr { + DeleteArrayDeallocationExpr() { this instanceof DeleteArrayExpr } + + override Expr getFreedExpr() { result = this.getExpr() } + } + \ No newline at end of file From 8bd682b3f29c3167b96a6047dfa990a53375d953 Mon Sep 17 00:00:00 2001 From: Benjamin Rodes Date: Thu, 18 Jan 2024 10:49:23 -0500 Subject: [PATCH 05/12] Deallocation.qll formatting. --- .../models/implementations/Deallocation.qll | 171 +++++++++--------- 1 file changed, 85 insertions(+), 86 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Deallocation.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Deallocation.qll index b16f3aee0e85..1162c09b0b64 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Deallocation.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Deallocation.qll @@ -4,90 +4,89 @@ * for usage information. */ - import semmle.code.cpp.models.interfaces.Deallocation +import semmle.code.cpp.models.interfaces.Deallocation - /** - * A deallocation function such as `free`. - */ - private class StandardDeallocationFunction extends DeallocationFunction { - int freedArg; - - StandardDeallocationFunction() { - this.hasGlobalOrStdOrBslName([ - // --- C library allocation - "free", "realloc" - ]) and - freedArg = 0 - or - this.hasGlobalName([ - // --- OpenSSL memory allocation - "CRYPTO_free", "CRYPTO_secure_free" - ]) and - freedArg = 0 - or - this.hasGlobalOrStdName([ - // --- Windows Memory Management for Windows Drivers - "ExFreePool", "ExFreePoolWithTag", "ExDeleteTimer", "IoFreeIrp", "IoFreeMdl", - "IoFreeErrorLogEntry", "IoFreeWorkItem", "MmFreeContiguousMemory", - "MmFreeContiguousMemorySpecifyCache", "MmFreeNonCachedMemory", "MmFreeMappingAddress", - "MmFreePagesFromMdl", "MmUnmapReservedMapping", "MmUnmapLockedPages", - "NdisFreeGenericObject", "NdisFreeMemory", "NdisFreeMemoryWithTag", "NdisFreeMdl", - "NdisFreeNetBufferListPool", "NdisFreeNetBufferPool", - // --- Windows Global / Local legacy allocation - "LocalFree", "GlobalFree", "LocalReAlloc", "GlobalReAlloc", - // --- Windows System Services allocation - "VirtualFree", - // --- Windows COM allocation - "CoTaskMemFree", "CoTaskMemRealloc", - // --- Windows Automation - "SysFreeString", - // --- Solaris/BSD kernel memory allocator - "kmem_free" - ]) and - freedArg = 0 - or - this.hasGlobalOrStdName([ - // --- Windows Memory Management for Windows Drivers - "ExFreeToLookasideListEx", "ExFreeToPagedLookasideList", "ExFreeToNPagedLookasideList", - "NdisFreeMemoryWithTagPriority", "StorPortFreeMdl", "StorPortFreePool", - // --- NetBSD pool manager - "pool_put", "pool_cache_put" - ]) and - freedArg = 1 - or - this.hasGlobalOrStdName(["HeapFree", "HeapReAlloc"]) and - freedArg = 2 - } - - override int getFreedArg() { result = freedArg } - } - - /** - * An deallocation expression that is a function call, such as call to `free`. - */ - private class CallDeallocationExpr extends DeallocationExpr, FunctionCall { - DeallocationFunction target; - - CallDeallocationExpr() { target = this.getTarget() } - - override Expr getFreedExpr() { result = this.getArgument(target.getFreedArg()) } - } - - /** - * An deallocation expression that is a `delete` expression. - */ - private class DeleteDeallocationExpr extends DeallocationExpr, DeleteExpr { - DeleteDeallocationExpr() { this instanceof DeleteExpr } - - override Expr getFreedExpr() { result = this.getExpr() } - } - - /** - * An deallocation expression that is a `delete []` expression. - */ - private class DeleteArrayDeallocationExpr extends DeallocationExpr, DeleteArrayExpr { - DeleteArrayDeallocationExpr() { this instanceof DeleteArrayExpr } - - override Expr getFreedExpr() { result = this.getExpr() } - } - \ No newline at end of file +/** + * A deallocation function such as `free`. + */ +private class StandardDeallocationFunction extends DeallocationFunction { + int freedArg; + + StandardDeallocationFunction() { + this.hasGlobalOrStdOrBslName([ + // --- C library allocation + "free", "realloc" + ]) and + freedArg = 0 + or + this.hasGlobalName([ + // --- OpenSSL memory allocation + "CRYPTO_free", "CRYPTO_secure_free" + ]) and + freedArg = 0 + or + this.hasGlobalOrStdName([ + // --- Windows Memory Management for Windows Drivers + "ExFreePool", "ExFreePoolWithTag", "ExDeleteTimer", "IoFreeIrp", "IoFreeMdl", + "IoFreeErrorLogEntry", "IoFreeWorkItem", "MmFreeContiguousMemory", + "MmFreeContiguousMemorySpecifyCache", "MmFreeNonCachedMemory", "MmFreeMappingAddress", + "MmFreePagesFromMdl", "MmUnmapReservedMapping", "MmUnmapLockedPages", + "NdisFreeGenericObject", "NdisFreeMemory", "NdisFreeMemoryWithTag", "NdisFreeMdl", + "NdisFreeNetBufferListPool", "NdisFreeNetBufferPool", + // --- Windows Global / Local legacy allocation + "LocalFree", "GlobalFree", "LocalReAlloc", "GlobalReAlloc", + // --- Windows System Services allocation + "VirtualFree", + // --- Windows COM allocation + "CoTaskMemFree", "CoTaskMemRealloc", + // --- Windows Automation + "SysFreeString", + // --- Solaris/BSD kernel memory allocator + "kmem_free" + ]) and + freedArg = 0 + or + this.hasGlobalOrStdName([ + // --- Windows Memory Management for Windows Drivers + "ExFreeToLookasideListEx", "ExFreeToPagedLookasideList", "ExFreeToNPagedLookasideList", + "NdisFreeMemoryWithTagPriority", "StorPortFreeMdl", "StorPortFreePool", + // --- NetBSD pool manager + "pool_put", "pool_cache_put" + ]) and + freedArg = 1 + or + this.hasGlobalOrStdName(["HeapFree", "HeapReAlloc"]) and + freedArg = 2 + } + + override int getFreedArg() { result = freedArg } +} + +/** + * An deallocation expression that is a function call, such as call to `free`. + */ +private class CallDeallocationExpr extends DeallocationExpr, FunctionCall { + DeallocationFunction target; + + CallDeallocationExpr() { target = this.getTarget() } + + override Expr getFreedExpr() { result = this.getArgument(target.getFreedArg()) } +} + +/** + * An deallocation expression that is a `delete` expression. + */ +private class DeleteDeallocationExpr extends DeallocationExpr, DeleteExpr { + DeleteDeallocationExpr() { this instanceof DeleteExpr } + + override Expr getFreedExpr() { result = this.getExpr() } +} + +/** + * An deallocation expression that is a `delete []` expression. + */ +private class DeleteArrayDeallocationExpr extends DeallocationExpr, DeleteArrayExpr { + DeleteArrayDeallocationExpr() { this instanceof DeleteArrayExpr } + + override Expr getFreedExpr() { result = this.getExpr() } +} From 967526b2852a3b0d07a3c1dba5d9c0206d11ad92 Mon Sep 17 00:00:00 2001 From: Benjamin Rodes Date: Thu, 18 Jan 2024 10:59:17 -0500 Subject: [PATCH 06/12] Separating out use after free logic into a library and a ql so the query can be expanded easily. --- cpp/ql/src/Critical/UseAfterFree.ql | 173 +-------------------------- cpp/ql/src/Critical/UseAfterFree.qll | 169 ++++++++++++++++++++++++++ 2 files changed, 174 insertions(+), 168 deletions(-) create mode 100644 cpp/ql/src/Critical/UseAfterFree.qll diff --git a/cpp/ql/src/Critical/UseAfterFree.ql b/cpp/ql/src/Critical/UseAfterFree.ql index 40f4bbf3eac9..90d62dd02891 100644 --- a/cpp/ql/src/Critical/UseAfterFree.ql +++ b/cpp/ql/src/Critical/UseAfterFree.ql @@ -15,177 +15,14 @@ import cpp import semmle.code.cpp.dataflow.new.DataFlow import semmle.code.cpp.ir.IR import FlowAfterFree -import UseAfterFree::PathGraph +import UseAfterFree +import UseAfterFreeTrace::PathGraph -/** - * Holds if `call` is a call to a function that obviously - * doesn't dereference its `i`'th argument. - */ -private predicate externalCallNeverDereferences(FormattingFunctionCall call, int arg) { - exists(int formatArg | - pragma[only_bind_out](call.getFormatArgument(formatArg)) = - pragma[only_bind_out](call.getArgument(arg)) and - call.getFormat().(FormatLiteral).getConvSpec(formatArg) != "%s" - ) -} - -predicate isUse0(Expr e) { - not isFree(_, _, e, _) and - ( - e = any(PointerDereferenceExpr pde).getOperand() - or - e = any(PointerFieldAccess pfa).getQualifier() - or - e = any(ArrayExpr ae).getArrayBase() - or - e = any(Call call).getQualifier() - or - // Assume any function without a body will dereference the pointer - exists(int i, Call call, Function f | - e = call.getArgument(i) and - f = call.getTarget() and - not f.hasEntryPoint() and - // Exclude known functions we know won't dereference the pointer. - // For example, a call such as `printf("%p", myPointer)`. - not externalCallNeverDereferences(call, i) - ) - ) -} - -module ParameterSinks { - import semmle.code.cpp.ir.ValueNumbering - - predicate flowsToUse(DataFlow::Node n) { - isUse0(n.asExpr()) - or - exists(DataFlow::Node succ | - flowsToUse(succ) and - DataFlow::localFlowStep(n, succ) - ) - } - - private predicate flowsFromParam(DataFlow::Node n) { - flowsToUse(n) and - ( - n.asParameter().getUnspecifiedType() instanceof PointerType - or - exists(DataFlow::Node prev | - flowsFromParam(prev) and - DataFlow::localFlowStep(prev, n) - ) - ) - } - - private predicate step(DataFlow::Node n1, DataFlow::Node n2) { - flowsFromParam(n1) and - flowsFromParam(n2) and - DataFlow::localFlowStep(n1, n2) - } - - private predicate paramToUse(DataFlow::Node n1, DataFlow::Node n2) = fastTC(step/2)(n1, n2) - - private predicate hasFlow( - DataFlow::Node source, InitializeParameterInstruction init, DataFlow::Node sink - ) { - pragma[only_bind_out](source.asParameter()) = pragma[only_bind_out](init.getParameter()) and - paramToUse(source, sink) and - isUse0(sink.asExpr()) - } - - private InitializeParameterInstruction getAnAlwaysDereferencedParameter0() { - exists(DataFlow::Node source, DataFlow::Node sink, IRBlock b1, int i1, IRBlock b2, int i2 | - hasFlow(pragma[only_bind_into](source), result, pragma[only_bind_into](sink)) and - source.hasIndexInBlock(b1, pragma[only_bind_into](i1)) and - sink.hasIndexInBlock(b2, pragma[only_bind_into](i2)) and - strictlyPostDominates(b2, i2, b1, i1) - ) - } - - private CallInstruction getAnAlwaysReachedCallInstruction() { - exists(IRFunction f | result.getBlock().postDominates(f.getEntryBlock())) - } - - pragma[nomagic] - private predicate callHasTargetAndArgument(Function f, int i, Instruction argument) { - exists(CallInstruction call | - call.getStaticCallTarget() = f and - call.getArgument(i) = argument and - call = getAnAlwaysReachedCallInstruction() - ) - } - - pragma[nomagic] - private predicate initializeParameterInFunction(Function f, int i) { - exists(InitializeParameterInstruction init | - pragma[only_bind_out](init.getEnclosingFunction()) = f and - init.hasIndex(i) and - init = getAnAlwaysDereferencedParameter() - ) - } - - pragma[nomagic] - private predicate alwaysDereferencedArgumentHasValueNumber(ValueNumber vn) { - exists(int i, Function f, Instruction argument | - callHasTargetAndArgument(f, i, argument) and - initializeParameterInFunction(pragma[only_bind_into](f), pragma[only_bind_into](i)) and - vn.getAnInstruction() = argument - ) - } - - InitializeParameterInstruction getAnAlwaysDereferencedParameter() { - result = getAnAlwaysDereferencedParameter0() - or - exists(ValueNumber vn | - alwaysDereferencedArgumentHasValueNumber(vn) and - vn.getAnInstruction() = result - ) - } -} - -module IsUse { - private import semmle.code.cpp.ir.dataflow.internal.DataFlowImplCommon - - predicate isUse(DataFlow::Node n, Expr e) { - isUse0(e) and n.asExpr() = e - or - exists(CallInstruction call, InitializeParameterInstruction init | - n.asOperand().getDef().getUnconvertedResultExpression() = e and - pragma[only_bind_into](init) = ParameterSinks::getAnAlwaysDereferencedParameter() and - viableParamArg(call, DataFlow::instructionNode(init), n) and - pragma[only_bind_out](init.getEnclosingFunction()) = - pragma[only_bind_out](call.getStaticCallTarget()) - ) - } -} - -import IsUse - -/** - * `dealloc1` is a deallocation expression, `e` is an expression that dereferences a - * pointer, and the `(dealloc1, e)` pair should be excluded by the `FlowFromFree` library. - */ -bindingset[dealloc1, e] -predicate isExcludeFreeUsePair(DeallocationExpr dealloc1, Expr e) { - // From https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-mmfreepagesfrommdl: - // "After calling MmFreePagesFromMdl, the caller must also call ExFreePool - // to release the memory that was allocated for the MDL structure." - dealloc1.(FunctionCall).getTarget().hasGlobalName("MmFreePagesFromMdl") and - isExFreePoolCall(_, e) -} - -module UseAfterFreeParam implements FlowFromFreeParamSig { - predicate isSink = isUse/2; - - predicate isExcluded = isExcludeFreeUsePair/2; - - predicate sourceSinkIsRelated = defaultSourceSinkIsRelated/2; -} - -module UseAfterFree = FlowFromFree; +module UseAfterFreeTrace = FlowFromFree; -from UseAfterFree::PathNode source, UseAfterFree::PathNode sink, DeallocationExpr dealloc +from UseAfterFreeTrace::PathNode source, UseAfterFreeTrace::PathNode sink, DeallocationExpr dealloc where - UseAfterFree::flowPath(source, sink) and + UseAfterFreeTrace::flowPath(source, sink) and isFree(source.getNode(), _, _, dealloc) select sink.getNode(), source, sink, "Memory may have been previously freed by $@.", dealloc, dealloc.toString() diff --git a/cpp/ql/src/Critical/UseAfterFree.qll b/cpp/ql/src/Critical/UseAfterFree.qll new file mode 100644 index 000000000000..72351b627b59 --- /dev/null +++ b/cpp/ql/src/Critical/UseAfterFree.qll @@ -0,0 +1,169 @@ +import cpp +private import FlowAfterFree +private import semmle.code.cpp.ir.IR + +/** + * Holds if `call` is a call to a function that obviously + * doesn't dereference its `i`'th argument. + */ +private predicate externalCallNeverDereferences(FormattingFunctionCall call, int arg) { + exists(int formatArg | + pragma[only_bind_out](call.getFormatArgument(formatArg)) = + pragma[only_bind_out](call.getArgument(arg)) and + call.getFormat().(FormatLiteral).getConvSpec(formatArg) != "%s" + ) +} + +predicate isUse0(Expr e) { + not isFree(_, _, e, _) and + ( + e = any(PointerDereferenceExpr pde).getOperand() + or + e = any(PointerFieldAccess pfa).getQualifier() + or + e = any(ArrayExpr ae).getArrayBase() + or + e = any(Call call).getQualifier() + or + // Assume any function without a body will dereference the pointer + exists(int i, Call call, Function f | + e = call.getArgument(i) and + f = call.getTarget() and + not f.hasEntryPoint() and + // Exclude known functions we know won't dereference the pointer. + // For example, a call such as `printf("%p", myPointer)`. + not externalCallNeverDereferences(call, i) + ) + ) +} + +module ParameterSinks { + import semmle.code.cpp.ir.ValueNumbering + + predicate flowsToUse(DataFlow::Node n) { + isUse0(n.asExpr()) + or + exists(DataFlow::Node succ | + flowsToUse(succ) and + DataFlow::localFlowStep(n, succ) + ) + } + + private predicate flowsFromParam(DataFlow::Node n) { + flowsToUse(n) and + ( + n.asParameter().getUnspecifiedType() instanceof PointerType + or + exists(DataFlow::Node prev | + flowsFromParam(prev) and + DataFlow::localFlowStep(prev, n) + ) + ) + } + + private predicate step(DataFlow::Node n1, DataFlow::Node n2) { + flowsFromParam(n1) and + flowsFromParam(n2) and + DataFlow::localFlowStep(n1, n2) + } + + private predicate paramToUse(DataFlow::Node n1, DataFlow::Node n2) = fastTC(step/2)(n1, n2) + + private predicate hasFlow( + DataFlow::Node source, InitializeParameterInstruction init, DataFlow::Node sink + ) { + pragma[only_bind_out](source.asParameter()) = pragma[only_bind_out](init.getParameter()) and + paramToUse(source, sink) and + isUse0(sink.asExpr()) + } + + private InitializeParameterInstruction getAnAlwaysDereferencedParameter0() { + exists(DataFlow::Node source, DataFlow::Node sink, IRBlock b1, int i1, IRBlock b2, int i2 | + hasFlow(pragma[only_bind_into](source), result, pragma[only_bind_into](sink)) and + source.hasIndexInBlock(b1, pragma[only_bind_into](i1)) and + sink.hasIndexInBlock(b2, pragma[only_bind_into](i2)) and + strictlyPostDominates(b2, i2, b1, i1) + ) + } + + private CallInstruction getAnAlwaysReachedCallInstruction() { + exists(IRFunction f | result.getBlock().postDominates(f.getEntryBlock())) + } + + pragma[nomagic] + private predicate callHasTargetAndArgument(Function f, int i, Instruction argument) { + exists(CallInstruction call | + call.getStaticCallTarget() = f and + call.getArgument(i) = argument and + call = getAnAlwaysReachedCallInstruction() + ) + } + + pragma[nomagic] + private predicate initializeParameterInFunction(Function f, int i) { + exists(InitializeParameterInstruction init | + pragma[only_bind_out](init.getEnclosingFunction()) = f and + init.hasIndex(i) and + init = getAnAlwaysDereferencedParameter() + ) + } + + pragma[nomagic] + private predicate alwaysDereferencedArgumentHasValueNumber(ValueNumber vn) { + exists(int i, Function f, Instruction argument | + callHasTargetAndArgument(f, i, argument) and + initializeParameterInFunction(pragma[only_bind_into](f), pragma[only_bind_into](i)) and + vn.getAnInstruction() = argument + ) + } + + InitializeParameterInstruction getAnAlwaysDereferencedParameter() { + result = getAnAlwaysDereferencedParameter0() + or + exists(ValueNumber vn | + alwaysDereferencedArgumentHasValueNumber(vn) and + vn.getAnInstruction() = result + ) + } +} + +module IsUse { + private import semmle.code.cpp.ir.dataflow.internal.DataFlowImplCommon + + predicate isUse(DataFlow::Node n, Expr e) { + isUse0(e) and n.asExpr() = e + or + exists(CallInstruction call, InitializeParameterInstruction init | + n.asOperand().getDef().getUnconvertedResultExpression() = e and + pragma[only_bind_into](init) = ParameterSinks::getAnAlwaysDereferencedParameter() and + viableParamArg(call, DataFlow::instructionNode(init), n) and + pragma[only_bind_out](init.getEnclosingFunction()) = + pragma[only_bind_out](call.getStaticCallTarget()) + ) + } +} + +import IsUse + +/** + * `dealloc1` is a deallocation expression, `e` is an expression that dereferences a + * pointer, and the `(dealloc1, e)` pair should be excluded by the `FlowFromFree` library. + */ +bindingset[dealloc1, e] +predicate isExcludeFreeUsePair(DeallocationExpr dealloc1, Expr e) { + // From https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-mmfreepagesfrommdl: + // "After calling MmFreePagesFromMdl, the caller must also call ExFreePool + // to release the memory that was allocated for the MDL structure." + dealloc1.(FunctionCall).getTarget().hasGlobalName("MmFreePagesFromMdl") and + isExFreePoolCall(_, e) +} + +module UseAfterFreeParam implements FlowFromFreeParamSig { + predicate isSink = isUse/2; + + predicate isExcluded = isExcludeFreeUsePair/2; + + predicate sourceSinkIsRelated = defaultSourceSinkIsRelated/2; +} + +import UseAfterFreeParam From 833ef9d6d6dc2e4439fbb4e18f9243ee4ae29195 Mon Sep 17 00:00:00 2001 From: Benjamin Rodes Date: Thu, 18 Jan 2024 11:17:24 -0500 Subject: [PATCH 07/12] Further reorg of libraries and predicates to allow for more reusable and consistent libraries. --- cpp/ql/src/Critical/DoubleFree.ql | 21 +-------------------- cpp/ql/src/Critical/FlowAfterFree.qll | 19 +++++++++++++++++++ cpp/ql/src/Critical/UseAfterFree.ql | 10 ++++++++++ cpp/ql/src/Critical/UseAfterFree.qll | 23 ----------------------- 4 files changed, 30 insertions(+), 43 deletions(-) diff --git a/cpp/ql/src/Critical/DoubleFree.ql b/cpp/ql/src/Critical/DoubleFree.ql index 1a17bc702bda..1cc1333c5017 100644 --- a/cpp/ql/src/Critical/DoubleFree.ql +++ b/cpp/ql/src/Critical/DoubleFree.ql @@ -22,29 +22,10 @@ import DoubleFree::PathGraph */ predicate isFree(DataFlow::Node n, Expr e) { isFree(_, n, e, _) } -/** - * `dealloc1` is a deallocation expression and `e` is an expression such - * that is deallocated by a deallocation expression, and the `(dealloc1, e)` pair - * should be excluded by the `FlowFromFree` library. - * - * Note that `e` is not necessarily the expression deallocated by `dealloc1`. It will - * be bound to the second deallocation as identified by the `FlowFromFree` library. - */ -bindingset[dealloc1, e] -predicate isExcludeFreePair(DeallocationExpr dealloc1, Expr e) { - exists(DeallocationExpr dealloc2 | isFree(_, _, e, dealloc2) | - dealloc1.(FunctionCall).getTarget().hasGlobalName("MmFreePagesFromMdl") and - // From https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-mmfreepagesfrommdl: - // "After calling MmFreePagesFromMdl, the caller must also call ExFreePool - // to release the memory that was allocated for the MDL structure." - isExFreePoolCall(dealloc2, _) - ) -} - module DoubleFreeParam implements FlowFromFreeParamSig { predicate isSink = isFree/2; - predicate isExcluded = isExcludeFreePair/2; + predicate isExcluded = isExcludedMmFreePageFromMdl/2; predicate sourceSinkIsRelated = defaultSourceSinkIsRelated/2; } diff --git a/cpp/ql/src/Critical/FlowAfterFree.qll b/cpp/ql/src/Critical/FlowAfterFree.qll index bd3358f3c313..a1c82b576769 100644 --- a/cpp/ql/src/Critical/FlowAfterFree.qll +++ b/cpp/ql/src/Critical/FlowAfterFree.qll @@ -143,3 +143,22 @@ predicate defaultSourceSinkIsRelated(DataFlow::Node source, DataFlow::Node sink) strictlyPostDominates(b2, i2, b1, i1) ) } + +/** + * `dealloc1` is a deallocation expression, `e` is an expression that dereferences a + * pointer, and the `(dealloc1, e)` pair should be excluded by the `FlowFromFree` library. + * + * Note that `e` is not necessarily the expression deallocated by `dealloc1`. It will + * be bound to the second deallocation as identified by the `FlowFromFree` library. + * + * From https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-mmfreepagesfrommdl: + * "After calling MmFreePagesFromMdl, the caller must also call ExFreePool + * to release the memory that was allocated for the MDL structure." + */ +bindingset[dealloc1, e] +predicate isExcludedMmFreePageFromMdl(DeallocationExpr dealloc1, Expr e) { + exists(DeallocationExpr dealloc2 | isFree(_, _, e, dealloc2) | + dealloc1.(FunctionCall).getTarget().hasGlobalName("MmFreePagesFromMdl") and + isExFreePoolCall(dealloc2, _) + ) +} diff --git a/cpp/ql/src/Critical/UseAfterFree.ql b/cpp/ql/src/Critical/UseAfterFree.ql index 90d62dd02891..a4a5e586160d 100644 --- a/cpp/ql/src/Critical/UseAfterFree.ql +++ b/cpp/ql/src/Critical/UseAfterFree.ql @@ -18,6 +18,16 @@ import FlowAfterFree import UseAfterFree import UseAfterFreeTrace::PathGraph +module UseAfterFreeParam implements FlowFromFreeParamSig { + predicate isSink = isUse/2; + + predicate isExcluded = isExcludedMmFreePageFromMdl/2; + + predicate sourceSinkIsRelated = defaultSourceSinkIsRelated/2; +} + +import UseAfterFreeParam + module UseAfterFreeTrace = FlowFromFree; from UseAfterFreeTrace::PathNode source, UseAfterFreeTrace::PathNode sink, DeallocationExpr dealloc diff --git a/cpp/ql/src/Critical/UseAfterFree.qll b/cpp/ql/src/Critical/UseAfterFree.qll index 72351b627b59..62952030cd45 100644 --- a/cpp/ql/src/Critical/UseAfterFree.qll +++ b/cpp/ql/src/Critical/UseAfterFree.qll @@ -144,26 +144,3 @@ module IsUse { } import IsUse - -/** - * `dealloc1` is a deallocation expression, `e` is an expression that dereferences a - * pointer, and the `(dealloc1, e)` pair should be excluded by the `FlowFromFree` library. - */ -bindingset[dealloc1, e] -predicate isExcludeFreeUsePair(DeallocationExpr dealloc1, Expr e) { - // From https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-mmfreepagesfrommdl: - // "After calling MmFreePagesFromMdl, the caller must also call ExFreePool - // to release the memory that was allocated for the MDL structure." - dealloc1.(FunctionCall).getTarget().hasGlobalName("MmFreePagesFromMdl") and - isExFreePoolCall(_, e) -} - -module UseAfterFreeParam implements FlowFromFreeParamSig { - predicate isSink = isUse/2; - - predicate isExcluded = isExcludeFreeUsePair/2; - - predicate sourceSinkIsRelated = defaultSourceSinkIsRelated/2; -} - -import UseAfterFreeParam From 2181fcf284e3e539668b6cb31d95a38c6dc576df Mon Sep 17 00:00:00 2001 From: Benjamin Rodes Date: Mon, 22 Jan 2024 10:36:24 -0500 Subject: [PATCH 08/12] Updating .expected to account for new free/deallocation sources. --- .../test/query-tests/Critical/MemoryFreed/MemoryFreed.expected | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp/ql/test/query-tests/Critical/MemoryFreed/MemoryFreed.expected b/cpp/ql/test/query-tests/Critical/MemoryFreed/MemoryFreed.expected index f5a3553ce542..14b04cc5c2dd 100644 --- a/cpp/ql/test/query-tests/Critical/MemoryFreed/MemoryFreed.expected +++ b/cpp/ql/test/query-tests/Critical/MemoryFreed/MemoryFreed.expected @@ -89,6 +89,7 @@ | test_free.cpp:216:10:216:10 | a | | test_free.cpp:220:10:220:10 | a | | test_free.cpp:227:24:227:45 | memory_descriptor_list | +| test_free.cpp:228:16:228:37 | memory_descriptor_list | | test_free.cpp:233:14:233:15 | * ... | | test_free.cpp:239:14:239:15 | * ... | | test_free.cpp:245:10:245:11 | * ... | From da10e6ca5bc2bd872cfd7285e044a07ec4a3dd6e Mon Sep 17 00:00:00 2001 From: Benjamin Rodes Date: Mon, 22 Jan 2024 11:18:03 -0500 Subject: [PATCH 09/12] Moving FlowAfterFree and UseAfterFree.qll as a general purpose lib. --- .../code/cpp/security/flowafterfree}/FlowAfterFree.qll | 4 ++++ .../code/cpp/security/flowafterfree}/UseAfterFree.qll | 6 +++++- cpp/ql/src/Critical/DoubleFree.ql | 2 +- cpp/ql/src/Critical/UseAfterFree.ql | 4 ++-- 4 files changed, 12 insertions(+), 4 deletions(-) rename cpp/ql/{src/Critical => lib/semmle/code/cpp/security/flowafterfree}/FlowAfterFree.qll (98%) rename cpp/ql/{src/Critical => lib/semmle/code/cpp/security/flowafterfree}/UseAfterFree.qll (97%) diff --git a/cpp/ql/src/Critical/FlowAfterFree.qll b/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/FlowAfterFree.qll similarity index 98% rename from cpp/ql/src/Critical/FlowAfterFree.qll rename to cpp/ql/lib/semmle/code/cpp/security/flowafterfree/FlowAfterFree.qll index a1c82b576769..3890861742ef 100644 --- a/cpp/ql/src/Critical/FlowAfterFree.qll +++ b/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/FlowAfterFree.qll @@ -1,3 +1,7 @@ +/** + * General library for finding flow from a pointer being freed to a user-specified sink + */ + import cpp import semmle.code.cpp.dataflow.new.DataFlow private import semmle.code.cpp.ir.IR diff --git a/cpp/ql/src/Critical/UseAfterFree.qll b/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll similarity index 97% rename from cpp/ql/src/Critical/UseAfterFree.qll rename to cpp/ql/lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll index 62952030cd45..b98f64c3f9fd 100644 --- a/cpp/ql/src/Critical/UseAfterFree.qll +++ b/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll @@ -1,5 +1,9 @@ +/** + * General library for tracing Use After Free vulnerabilities. + */ + import cpp -private import FlowAfterFree +private import semmle.code.cpp.security.flowafterfree.FlowAfterFree private import semmle.code.cpp.ir.IR /** diff --git a/cpp/ql/src/Critical/DoubleFree.ql b/cpp/ql/src/Critical/DoubleFree.ql index 1cc1333c5017..802c81fd050b 100644 --- a/cpp/ql/src/Critical/DoubleFree.ql +++ b/cpp/ql/src/Critical/DoubleFree.ql @@ -13,7 +13,7 @@ import cpp import semmle.code.cpp.dataflow.new.DataFlow -import FlowAfterFree +import semmle.code.cpp.security.flowafterfree.FlowAfterFree import DoubleFree::PathGraph /** diff --git a/cpp/ql/src/Critical/UseAfterFree.ql b/cpp/ql/src/Critical/UseAfterFree.ql index a4a5e586160d..435a0993d537 100644 --- a/cpp/ql/src/Critical/UseAfterFree.ql +++ b/cpp/ql/src/Critical/UseAfterFree.ql @@ -14,8 +14,8 @@ import cpp import semmle.code.cpp.dataflow.new.DataFlow import semmle.code.cpp.ir.IR -import FlowAfterFree -import UseAfterFree +import semmle.code.cpp.security.flowafterfree.FlowAfterFree +import semmle.code.cpp.security.flowafterfree.UseAfterFree import UseAfterFreeTrace::PathGraph module UseAfterFreeParam implements FlowFromFreeParamSig { From 42fd3fc8368e80542df1c9be42d47bd2e9b9d4ad Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 23 Jan 2024 15:27:01 +0000 Subject: [PATCH 10/12] C++: Make more things 'private' and add QLDoc to public things. (#40) --- .../cpp/security/flowafterfree/FlowAfterFree.qll | 9 ++++++--- .../code/cpp/security/flowafterfree/UseAfterFree.qll | 12 +++++++++--- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/FlowAfterFree.qll b/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/FlowAfterFree.qll index 3890861742ef..3172b6e35622 100644 --- a/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/FlowAfterFree.qll +++ b/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/FlowAfterFree.qll @@ -28,10 +28,13 @@ predicate strictlyDominates(IRBlock b1, int i1, IRBlock b2, int i2) { b1.strictlyDominates(b2) } +/** + * The signature for a module that is used to specify the inputs to the `FlowFromFree` module. + */ signature module FlowFromFreeParamSig { /** - * Signature for a predicate that holds if `n.asExpr() = e` and `n` is a sink in - * the `FlowFromFreeConfig` module. + * Holds if `n.asExpr() = e` and `n` is a sink in the `FlowFromFreeConfig` + * module. */ predicate isSink(DataFlow::Node n, Expr e); @@ -60,7 +63,7 @@ signature module FlowFromFreeParamSig { * 2. The sink post-dominates the source. */ module FlowFromFree { - module FlowFromFreeConfig implements DataFlow::StateConfigSig { + private module FlowFromFreeConfig implements DataFlow::StateConfigSig { class FlowState instanceof Expr { FlowState() { isFree(_, _, this, _) } diff --git a/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll b/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll index b98f64c3f9fd..15872bd11f59 100644 --- a/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll +++ b/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll @@ -41,10 +41,10 @@ predicate isUse0(Expr e) { ) } -module ParameterSinks { +private module ParameterSinks { import semmle.code.cpp.ir.ValueNumbering - predicate flowsToUse(DataFlow::Node n) { + private predicate flowsToUse(DataFlow::Node n) { isUse0(n.asExpr()) or exists(DataFlow::Node succ | @@ -131,9 +131,15 @@ module ParameterSinks { } } -module IsUse { +private module IsUse { private import semmle.code.cpp.ir.dataflow.internal.DataFlowImplCommon + /** + * Holds if `n` represents the expression `e`, and `e` is a pointer that is + * guarenteed to be dereferenced (either because it's an operand of a + * dereference operation, or because it's an argument to a function that + * always dereferences the parameter). + */ predicate isUse(DataFlow::Node n, Expr e) { isUse0(e) and n.asExpr() = e or From dfb3aec002c88498502618c2ef7e007c18437ca9 Mon Sep 17 00:00:00 2001 From: Benjamin Rodes Date: Tue, 23 Jan 2024 10:47:38 -0500 Subject: [PATCH 11/12] Removing unnecessary private modules and adding comments. --- .../security/flowafterfree/UseAfterFree.qll | 47 ++++++++++--------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll b/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll index 15872bd11f59..5df773aa19b0 100644 --- a/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll +++ b/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll @@ -18,9 +18,16 @@ private predicate externalCallNeverDereferences(FormattingFunctionCall call, int ) } +/** + * Holds if `e` is a use. A use is a pointer dereference or a + * parameter to a call with no function definition. + * Uses in deallocation expressions (e.g., free) are excluded. + * Default isUse definition for an expression. + */ predicate isUse0(Expr e) { not isFree(_, _, e, _) and ( + // TODO: use DirectDefereferencedByOperation in Dereferenced.qll e = any(PointerDereferenceExpr pde).getOperand() or e = any(PointerFieldAccess pfa).getQualifier() @@ -131,26 +138,22 @@ private module ParameterSinks { } } -private module IsUse { - private import semmle.code.cpp.ir.dataflow.internal.DataFlowImplCommon - - /** - * Holds if `n` represents the expression `e`, and `e` is a pointer that is - * guarenteed to be dereferenced (either because it's an operand of a - * dereference operation, or because it's an argument to a function that - * always dereferences the parameter). - */ - predicate isUse(DataFlow::Node n, Expr e) { - isUse0(e) and n.asExpr() = e - or - exists(CallInstruction call, InitializeParameterInstruction init | - n.asOperand().getDef().getUnconvertedResultExpression() = e and - pragma[only_bind_into](init) = ParameterSinks::getAnAlwaysDereferencedParameter() and - viableParamArg(call, DataFlow::instructionNode(init), n) and - pragma[only_bind_out](init.getEnclosingFunction()) = - pragma[only_bind_out](call.getStaticCallTarget()) - ) - } -} +private import semmle.code.cpp.ir.dataflow.internal.DataFlowImplCommon -import IsUse +/** + * Holds if `n` represents the expression `e`, and `e` is a pointer that is + * guarenteed to be dereferenced (either because it's an operand of a + * dereference operation, or because it's an argument to a function that + * always dereferences the parameter). + */ +predicate isUse(DataFlow::Node n, Expr e) { + isUse0(e) and n.asExpr() = e + or + exists(CallInstruction call, InitializeParameterInstruction init | + n.asOperand().getDef().getUnconvertedResultExpression() = e and + pragma[only_bind_into](init) = ParameterSinks::getAnAlwaysDereferencedParameter() and + viableParamArg(call, DataFlow::instructionNode(init), n) and + pragma[only_bind_out](init.getEnclosingFunction()) = + pragma[only_bind_out](call.getStaticCallTarget()) + ) +} From 55fe8d376c97505c9bd34ae42e91f56da86ac9a5 Mon Sep 17 00:00:00 2001 From: Ben Rodes Date: Tue, 23 Jan 2024 10:49:47 -0500 Subject: [PATCH 12/12] Update cpp/ql/lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll --- .../lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll b/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll index 5df773aa19b0..bea0b73b8748 100644 --- a/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll +++ b/cpp/ql/lib/semmle/code/cpp/security/flowafterfree/UseAfterFree.qll @@ -142,7 +142,7 @@ private import semmle.code.cpp.ir.dataflow.internal.DataFlowImplCommon /** * Holds if `n` represents the expression `e`, and `e` is a pointer that is - * guarenteed to be dereferenced (either because it's an operand of a + * guaranteed to be dereferenced (either because it's an operand of a * dereference operation, or because it's an argument to a function that * always dereferences the parameter). */