Skip to content

Commit a46dc55

Browse files
RasmusWLRasmusWL
committed
Merge branch 'main' into python-3.12
2 parents e02c32f + 14268f3 commit a46dc55

22 files changed

Lines changed: 235 additions & 152 deletions

File tree

‎cpp/ql/lib/change-notes/2023-11-15-return-stack-allocated-memory.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The "Returning stack-allocated memory" (`cpp/return-stack-allocated-memory`) query now also detects returning stack-allocated memory allocated by calls to `alloca`, `strdupa`, and `strndupa`.

‎cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -27,16 +27,26 @@ class ReturnStackAllocatedMemoryConfig extends MustFlowConfiguration {
2727
ReturnStackAllocatedMemoryConfig() { this = "ReturnStackAllocatedMemoryConfig" }
2828

2929
override predicate isSource(Instruction source) {
30-
// Holds if `source` is a node that represents the use of a stack variable
31-
exists(VariableAddressInstruction var, Function func |
32-
var = source and
33-
func = source.getEnclosingFunction() and
34-
var.getAstVariable() instanceof StackVariable and
35-
// Pointer-to-member types aren't properly handled in the dbscheme.
36-
not var.getResultType() instanceof PointerToMemberType and
30+
exists(Function func |
3731
// Rule out FPs caused by extraction errors.
3832
not any(ErrorExpr e).getEnclosingFunction() = func and
39-
not intentionallyReturnsStackPointer(func)
33+
not intentionallyReturnsStackPointer(func) and
34+
func = source.getEnclosingFunction()
35+
|
36+
// `source` is an instruction that represents the use of a stack variable
37+
exists(VariableAddressInstruction var |
38+
var = source and
39+
var.getAstVariable() instanceof StackVariable and
40+
// Pointer-to-member types aren't properly handled in the dbscheme.
41+
not var.getResultType() instanceof PointerToMemberType
42+
)
43+
or
44+
// `source` is an instruction that represents the return value of a
45+
// function that is known to return stack-allocated memory.
46+
exists(Call call |
47+
call.getTarget().hasGlobalName(["alloca", "strdupa", "strndupa", "_alloca", "_malloca"]) and
48+
source.getUnconvertedResultExpression() = call
49+
)
4050
)
4151
}
4252

@@ -85,10 +95,10 @@ class ReturnStackAllocatedMemoryConfig extends MustFlowConfiguration {
8595
}
8696

8797
from
88-
MustFlowPathNode source, MustFlowPathNode sink, VariableAddressInstruction var,
98+
MustFlowPathNode source, MustFlowPathNode sink, Instruction instr,
8999
ReturnStackAllocatedMemoryConfig conf
90100
where
91101
conf.hasFlowPath(pragma[only_bind_into](source), pragma[only_bind_into](sink)) and
92-
source.getInstruction() = var
102+
source.getInstruction() = instr
93103
select sink.getInstruction(), source, sink, "May return stack-allocated memory from $@.",
94-
var.getAst(), var.getAst().toString()
104+
instr.getAst(), instr.getAst().toString()

‎cpp/ql/src/Security/CWE/CWE-114/UncontrolledProcessOperation.ql‎

Lines changed: 33 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,25 +14,47 @@
1414

1515
import cpp
1616
import semmle.code.cpp.security.Security
17-
import semmle.code.cpp.ir.dataflow.internal.DefaultTaintTrackingImpl
18-
import TaintedWithPath
17+
import semmle.code.cpp.security.FlowSources
18+
import semmle.code.cpp.ir.dataflow.TaintTracking
19+
import semmle.code.cpp.ir.IR
20+
import Flow::PathGraph
1921

20-
predicate isProcessOperationExplanation(Expr arg, string processOperation) {
22+
predicate isProcessOperationExplanation(DataFlow::Node arg, string processOperation) {
2123
exists(int processOperationArg, FunctionCall call |
2224
isProcessOperationArgument(processOperation, processOperationArg) and
2325
call.getTarget().getName() = processOperation and
24-
call.getArgument(processOperationArg) = arg
26+
call.getArgument(processOperationArg) = [arg.asExpr(), arg.asIndirectExpr()]
2527
)
2628
}
2729

28-
class Configuration extends TaintTrackingConfiguration {
29-
override predicate isSink(Element arg) { isProcessOperationExplanation(arg, _) }
30+
predicate isSource(FlowSource source, string sourceType) {
31+
not source instanceof DataFlow::ExprNode and
32+
sourceType = source.getSourceType()
3033
}
3134

32-
from string processOperation, Expr arg, Expr source, PathNode sourceNode, PathNode sinkNode
35+
module Config implements DataFlow::ConfigSig {
36+
predicate isSource(DataFlow::Node node) { isSource(node, _) }
37+
38+
predicate isSink(DataFlow::Node node) { isProcessOperationExplanation(node, _) }
39+
40+
predicate isBarrier(DataFlow::Node node) {
41+
isSink(node) and node.asExpr().getUnspecifiedType() instanceof ArithmeticType
42+
or
43+
node.asInstruction().(StoreInstruction).getResultType() instanceof ArithmeticType
44+
}
45+
}
46+
47+
module Flow = TaintTracking::Global<Config>;
48+
49+
from
50+
string processOperation, string sourceType, DataFlow::Node source, DataFlow::Node sink,
51+
Flow::PathNode sourceNode, Flow::PathNode sinkNode
3352
where
34-
isProcessOperationExplanation(arg, processOperation) and
35-
taintedWithPath(source, arg, sourceNode, sinkNode)
36-
select arg, sourceNode, sinkNode,
53+
source = sourceNode.getNode() and
54+
sink = sinkNode.getNode() and
55+
isSource(source, sourceType) and
56+
isProcessOperationExplanation(sink, processOperation) and
57+
Flow::flowPath(sourceNode, sinkNode)
58+
select sink, sourceNode, sinkNode,
3759
"The value of this argument may come from $@ and is being passed to " + processOperation + ".",
38-
source, source.toString()
60+
source, sourceType

‎cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,11 @@ edges
4343
| test.cpp:189:16:189:16 | p | test.cpp:189:16:189:16 | (reference to) |
4444
| test.cpp:190:10:190:13 | (reference dereference) | test.cpp:190:10:190:13 | (reference to) |
4545
| test.cpp:190:10:190:13 | pRef | test.cpp:190:10:190:13 | (reference dereference) |
46+
| test.cpp:237:12:237:17 | call to alloca | test.cpp:237:12:237:17 | call to alloca |
47+
| test.cpp:237:12:237:17 | call to alloca | test.cpp:238:9:238:9 | p |
48+
| test.cpp:249:13:249:20 | call to strndupa | test.cpp:249:13:249:20 | call to strndupa |
49+
| test.cpp:249:13:249:20 | call to strndupa | test.cpp:250:9:250:10 | s2 |
50+
| test.cpp:250:9:250:10 | s2 | test.cpp:250:9:250:10 | (void *)... |
4651
nodes
4752
| test.cpp:17:9:17:11 | & ... | semmle.label | & ... |
4853
| test.cpp:17:10:17:11 | mc | semmle.label | mc |
@@ -101,6 +106,14 @@ nodes
101106
| test.cpp:190:10:190:13 | (reference dereference) | semmle.label | (reference dereference) |
102107
| test.cpp:190:10:190:13 | (reference to) | semmle.label | (reference to) |
103108
| test.cpp:190:10:190:13 | pRef | semmle.label | pRef |
109+
| test.cpp:237:12:237:17 | call to alloca | semmle.label | call to alloca |
110+
| test.cpp:237:12:237:17 | call to alloca | semmle.label | call to alloca |
111+
| test.cpp:238:9:238:9 | p | semmle.label | p |
112+
| test.cpp:245:9:245:15 | call to strdupa | semmle.label | call to strdupa |
113+
| test.cpp:249:13:249:20 | call to strndupa | semmle.label | call to strndupa |
114+
| test.cpp:249:13:249:20 | call to strndupa | semmle.label | call to strndupa |
115+
| test.cpp:250:9:250:10 | (void *)... | semmle.label | (void *)... |
116+
| test.cpp:250:9:250:10 | s2 | semmle.label | s2 |
104117
#select
105118
| test.cpp:17:9:17:11 | CopyValue: & ... | test.cpp:17:10:17:11 | mc | test.cpp:17:9:17:11 | & ... | May return stack-allocated memory from $@. | test.cpp:17:10:17:11 | mc | mc |
106119
| test.cpp:25:9:25:11 | Load: ptr | test.cpp:23:18:23:19 | mc | test.cpp:25:9:25:11 | ptr | May return stack-allocated memory from $@. | test.cpp:23:18:23:19 | mc | mc |
@@ -115,3 +128,6 @@ nodes
115128
| test.cpp:177:10:177:23 | Convert: (void *)... | test.cpp:176:25:176:34 | localArray | test.cpp:177:10:177:23 | (void *)... | May return stack-allocated memory from $@. | test.cpp:176:25:176:34 | localArray | localArray |
116129
| test.cpp:183:10:183:19 | CopyValue: (reference to) | test.cpp:182:21:182:27 | myLocal | test.cpp:183:10:183:19 | (reference to) | May return stack-allocated memory from $@. | test.cpp:182:21:182:27 | myLocal | myLocal |
117130
| test.cpp:190:10:190:13 | CopyValue: (reference to) | test.cpp:189:16:189:16 | p | test.cpp:190:10:190:13 | (reference to) | May return stack-allocated memory from $@. | test.cpp:189:16:189:16 | p | p |
131+
| test.cpp:238:9:238:9 | Load: p | test.cpp:237:12:237:17 | call to alloca | test.cpp:238:9:238:9 | p | May return stack-allocated memory from $@. | test.cpp:237:12:237:17 | call to alloca | call to alloca |
132+
| test.cpp:245:9:245:15 | Call: call to strdupa | test.cpp:245:9:245:15 | call to strdupa | test.cpp:245:9:245:15 | call to strdupa | May return stack-allocated memory from $@. | test.cpp:245:9:245:15 | call to strdupa | call to strdupa |
133+
| test.cpp:250:9:250:10 | Convert: (void *)... | test.cpp:249:13:249:20 | call to strndupa | test.cpp:250:9:250:10 | (void *)... | May return stack-allocated memory from $@. | test.cpp:249:13:249:20 | call to strndupa | call to strndupa |

‎cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,4 +229,23 @@ int* id(int* px) {
229229
void f() {
230230
int x;
231231
int* px = id(&x); // GOOD
232+
}
233+
234+
void *alloca(size_t);
235+
236+
void* test_alloca() {
237+
void* p = alloca(10);
238+
return p; // BAD
239+
}
240+
241+
char *strdupa(const char *);
242+
char *strndupa(const char *, size_t);
243+
244+
char* test_strdupa(const char* s) {
245+
return strdupa(s); // BAD
246+
}
247+
248+
void* test_strndupa(const char* s, size_t size) {
249+
char* s2 = strndupa(s, size);
250+
return s2; // BAD
232251
}

‎cpp/ql/test/query-tests/Security/CWE/CWE-114/SAMATE/UncontrolledProcessOperation/UncontrolledProcessOperation.expected‎

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,12 @@
11
edges
2-
| test.cpp:37:73:37:76 | data | test.cpp:43:32:43:35 | data |
3-
| test.cpp:37:73:37:76 | data | test.cpp:43:32:43:35 | data |
4-
| test.cpp:37:73:37:76 | data indirection | test.cpp:43:32:43:35 | data |
5-
| test.cpp:37:73:37:76 | data indirection | test.cpp:43:32:43:35 | data |
6-
| test.cpp:64:30:64:35 | call to getenv | test.cpp:73:24:73:27 | data |
7-
| test.cpp:64:30:64:35 | call to getenv | test.cpp:73:24:73:27 | data |
8-
| test.cpp:64:30:64:35 | call to getenv | test.cpp:73:24:73:27 | data indirection |
9-
| test.cpp:64:30:64:35 | call to getenv | test.cpp:73:24:73:27 | data indirection |
10-
| test.cpp:73:24:73:27 | data | test.cpp:37:73:37:76 | data |
2+
| test.cpp:37:73:37:76 | data indirection | test.cpp:43:32:43:35 | data indirection |
3+
| test.cpp:64:30:64:35 | call to getenv indirection | test.cpp:73:24:73:27 | data indirection |
114
| test.cpp:73:24:73:27 | data indirection | test.cpp:37:73:37:76 | data indirection |
12-
subpaths
135
nodes
14-
| test.cpp:37:73:37:76 | data | semmle.label | data |
156
| test.cpp:37:73:37:76 | data indirection | semmle.label | data indirection |
16-
| test.cpp:43:32:43:35 | data | semmle.label | data |
17-
| test.cpp:43:32:43:35 | data | semmle.label | data |
18-
| test.cpp:64:30:64:35 | call to getenv | semmle.label | call to getenv |
19-
| test.cpp:64:30:64:35 | call to getenv | semmle.label | call to getenv |
20-
| test.cpp:73:24:73:27 | data | semmle.label | data |
7+
| test.cpp:43:32:43:35 | data indirection | semmle.label | data indirection |
8+
| test.cpp:64:30:64:35 | call to getenv indirection | semmle.label | call to getenv indirection |
219
| test.cpp:73:24:73:27 | data indirection | semmle.label | data indirection |
10+
subpaths
2211
#select
23-
| test.cpp:43:32:43:35 | data | test.cpp:64:30:64:35 | call to getenv | test.cpp:43:32:43:35 | data | The value of this argument may come from $@ and is being passed to LoadLibraryA. | test.cpp:64:30:64:35 | call to getenv | call to getenv |
12+
| test.cpp:43:32:43:35 | data indirection | test.cpp:64:30:64:35 | call to getenv indirection | test.cpp:43:32:43:35 | data indirection | The value of this argument may come from $@ and is being passed to LoadLibraryA. | test.cpp:64:30:64:35 | call to getenv indirection | an environment variable |

0 commit comments

Comments
 (0)