Skip to content

Commit 25d3af9

Browse files
RasmusWLRasmusWL
committed
Merge branch 'main' into clean-tests
2 parents 69453aa + 1f3f1b5 commit 25d3af9

File tree

115 files changed

+1999
-1059
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

115 files changed

+1999
-1059
lines changed

‎cpp/ql/src/change-notes/2023-10-31-odbc-models.md renamed to ‎cpp/ql/lib/change-notes/2023-10-31-odbc-models.md

File renamed without changes.

‎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/src/Security/CWE/CWE-190/ArithmeticTainted.ql

Lines changed: 86 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,13 @@
1414

1515
import cpp
1616
import semmle.code.cpp.security.Overflow
17-
import semmle.code.cpp.security.Security
18-
import semmle.code.cpp.ir.dataflow.internal.DefaultTaintTrackingImpl
19-
import TaintedWithPath
17+
import semmle.code.cpp.dataflow.new.TaintTracking
18+
import semmle.code.cpp.dataflow.new.DataFlow
19+
import semmle.code.cpp.ir.IR
20+
import semmle.code.cpp.controlflow.IRGuards as IRGuards
21+
import semmle.code.cpp.security.FlowSources as FS
2022
import Bounded
23+
import Flow::PathGraph
2124

2225
bindingset[op]
2326
predicate missingGuard(Operation op, Expr e, string effect) {
@@ -28,28 +31,90 @@ predicate missingGuard(Operation op, Expr e, string effect) {
2831
not e instanceof VariableAccess and effect = "overflow"
2932
}
3033

31-
class Configuration extends TaintTrackingConfiguration {
32-
override predicate isSink(Element e) {
33-
exists(Operation op |
34-
missingGuard(op, e, _) and
35-
op.getAnOperand() = e
36-
|
37-
op instanceof UnaryArithmeticOperation or
38-
op instanceof BinaryArithmeticOperation or
39-
op instanceof AssignArithmeticOperation
40-
)
41-
}
34+
predicate isSource(FS::FlowSource source, string sourceType) { sourceType = source.getSourceType() }
35+
36+
predicate isSink(DataFlow::Node sink, Operation op, Expr e) {
37+
e = sink.asExpr() and
38+
missingGuard(op, e, _) and
39+
op.getAnOperand() = e and
40+
(
41+
op instanceof UnaryArithmeticOperation or
42+
op instanceof BinaryArithmeticOperation or
43+
op instanceof AssignArithmeticOperation
44+
)
45+
}
46+
47+
predicate hasUpperBoundsCheck(Variable var) {
48+
exists(RelationalOperation oper, VariableAccess access |
49+
oper.getAnOperand() = access and
50+
access.getTarget() = var and
51+
// Comparing to 0 is not an upper bound check
52+
not oper.getAnOperand().getValue() = "0"
53+
)
54+
}
55+
56+
predicate constantInstruction(Instruction instr) {
57+
instr instanceof ConstantInstruction or
58+
constantInstruction(instr.(UnaryInstruction).getUnary())
59+
}
60+
61+
predicate readsVariable(LoadInstruction load, Variable var) {
62+
load.getSourceAddress().(VariableAddressInstruction).getAstVariable() = var
63+
}
4264

43-
override predicate isBarrier(Expr e) {
44-
super.isBarrier(e) or bounded(e) or e.getUnspecifiedType().(IntegralType).getSize() <= 1
65+
predicate nodeIsBarrierEqualityCandidate(DataFlow::Node node, Operand access, Variable checkedVar) {
66+
exists(Instruction instr | instr = node.asInstruction() |
67+
readsVariable(instr, checkedVar) and
68+
any(IRGuards::IRGuardCondition guard).ensuresEq(access, _, _, instr.getBlock(), true)
69+
)
70+
}
71+
72+
module Config implements DataFlow::ConfigSig {
73+
predicate isSource(DataFlow::Node source) { isSource(source, _) }
74+
75+
predicate isSink(DataFlow::Node sink) { isSink(sink, _, _) }
76+
77+
predicate isBarrier(DataFlow::Node node) {
78+
exists(StoreInstruction store | store = node.asInstruction() |
79+
// Block flow to "likely small expressions"
80+
bounded(store.getSourceValue().getUnconvertedResultExpression())
81+
or
82+
// Block flow to "small types"
83+
store.getResultType().getUnspecifiedType().(IntegralType).getSize() <= 1
84+
)
85+
or
86+
// Block flow if there's an upper bound check of the variable anywhere in the program
87+
exists(Variable checkedVar, Instruction instr | instr = node.asInstruction() |
88+
readsVariable(instr, checkedVar) and
89+
hasUpperBoundsCheck(checkedVar)
90+
)
91+
or
92+
// Block flow if the node is guarded by an equality check
93+
exists(Variable checkedVar, Operand access |
94+
nodeIsBarrierEqualityCandidate(node, access, checkedVar) and
95+
readsVariable(access.getDef(), checkedVar)
96+
)
97+
or
98+
// Block flow to any binary instruction whose operands are both non-constants.
99+
exists(BinaryInstruction iTo |
100+
iTo = node.asInstruction() and
101+
not constantInstruction(iTo.getLeft()) and
102+
not constantInstruction(iTo.getRight()) and
103+
// propagate taint from either the pointer or the offset, regardless of constantness
104+
not iTo instanceof PointerArithmeticInstruction
105+
)
45106
}
46107
}
47108

48-
from Expr origin, Expr e, string effect, PathNode sourceNode, PathNode sinkNode, Operation op
109+
module Flow = TaintTracking::Global<Config>;
110+
111+
from
112+
Expr e, string effect, Flow::PathNode source, Flow::PathNode sink, Operation op, string sourceType
49113
where
50-
taintedWithPath(origin, e, sourceNode, sinkNode) and
51-
op.getAnOperand() = e and
114+
Flow::flowPath(source, sink) and
115+
isSource(source.getNode(), sourceType) and
116+
isSink(sink.getNode(), op, e) and
52117
missingGuard(op, e, effect)
53-
select e, sourceNode, sinkNode,
118+
select e, source, sink,
54119
"$@ flows to an operand of an arithmetic expression, potentially causing an " + effect + ".",
55-
origin, "User-provided value"
120+
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)