Skip to content

Commit 9642e59

Browse files
MathiasVPMathiasVP
authored
Merge pull request #8382 from MathiasVP/use-taint-configuration-in-three-more-queries
C++: Use a `TaintTracking::Configuration` in three more queries
2 parents 6d54142 + 7e0e7d5 commit 9642e59

File tree

9 files changed

+321
-179
lines changed

9 files changed

+321
-179
lines changed

‎cpp/ql/src/Critical/OverflowDestination.ql

Lines changed: 51 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
* @name Copy function using source size
33
* @description Calling a copy operation with a size derived from the source
44
* buffer instead of the destination buffer may result in a buffer overflow.
5-
* @kind problem
5+
* @kind path-problem
66
* @id cpp/overflow-destination
77
* @problem.severity warning
88
* @security-severity 9.3
@@ -14,7 +14,10 @@
1414
*/
1515

1616
import cpp
17-
import semmle.code.cpp.security.TaintTracking
17+
import semmle.code.cpp.ir.dataflow.TaintTracking
18+
import semmle.code.cpp.controlflow.IRGuards
19+
import semmle.code.cpp.security.FlowSources
20+
import DataFlow::PathGraph
1821

1922
/**
2023
* Holds if `fc` is a call to a copy operation where the size argument contains
@@ -27,9 +30,9 @@ predicate sourceSized(FunctionCall fc, Expr src) {
2730
fc.getTarget().hasGlobalOrStdName(["strncpy", "strncat", "memcpy", "memmove"]) and
2831
exists(Expr dest, Expr size, Variable v |
2932
fc.getArgument(0) = dest and
30-
fc.getArgument(1) = src and
33+
fc.getArgument(1).getFullyConverted() = src and
3134
fc.getArgument(2) = size and
32-
src = v.getAnAccess() and
35+
src = v.getAnAccess().getFullyConverted() and
3336
size.getAChild+() = v.getAnAccess() and
3437
// exception: `dest` is also referenced in the size argument
3538
not exists(Variable other |
@@ -45,9 +48,49 @@ predicate sourceSized(FunctionCall fc, Expr src) {
4548
)
4649
}
4750

48-
from FunctionCall fc, Expr vuln, Expr taintSource
51+
predicate readsVariable(LoadInstruction load, Variable var) {
52+
load.getSourceAddress().(VariableAddressInstruction).getAstVariable() = var
53+
}
54+
55+
predicate hasUpperBoundsCheck(Variable var) {
56+
exists(RelationalOperation oper, VariableAccess access |
57+
oper.getAnOperand() = access and
58+
access.getTarget() = var and
59+
// Comparing to 0 is not an upper bound check
60+
not oper.getAnOperand().getValue() = "0"
61+
)
62+
}
63+
64+
predicate nodeIsBarrierEqualityCandidate(DataFlow::Node node, Operand access, Variable checkedVar) {
65+
readsVariable(node.asInstruction(), checkedVar) and
66+
any(IRGuardCondition guard).ensuresEq(access, _, _, node.asInstruction().getBlock(), true)
67+
}
68+
69+
class OverflowDestinationConfig extends TaintTracking::Configuration {
70+
OverflowDestinationConfig() { this = "OverflowDestinationConfig" }
71+
72+
override predicate isSource(DataFlow::Node source) { source instanceof FlowSource }
73+
74+
override predicate isSink(DataFlow::Node sink) { sourceSized(_, sink.asConvertedExpr()) }
75+
76+
override predicate isSanitizer(DataFlow::Node node) {
77+
exists(Variable checkedVar |
78+
readsVariable(node.asInstruction(), checkedVar) and
79+
hasUpperBoundsCheck(checkedVar)
80+
)
81+
or
82+
exists(Variable checkedVar, Operand access |
83+
readsVariable(access.getDef(), checkedVar) and
84+
nodeIsBarrierEqualityCandidate(node, access, checkedVar)
85+
)
86+
}
87+
}
88+
89+
from
90+
FunctionCall fc, OverflowDestinationConfig conf, DataFlow::PathNode source,
91+
DataFlow::PathNode sink
4992
where
50-
sourceSized(fc, vuln) and
51-
tainted(taintSource, vuln)
52-
select fc,
93+
conf.hasFlowPath(source, sink) and
94+
sourceSized(fc, sink.getNode().asConvertedExpr())
95+
select fc, source, sink,
5396
"To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size."

‎cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql

Lines changed: 95 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,18 +3,22 @@
33
* @description Accessing an array without first checking
44
* that the index is within the bounds of the array can
55
* cause undefined behavior and can also be a security risk.
6-
* @kind problem
6+
* @kind path-problem
77
* @id cpp/unclear-array-index-validation
88
* @problem.severity warning
99
* @security-severity 8.8
10+
* @precision low
1011
* @tags security
1112
* external/cwe/cwe-129
1213
*/
1314

1415
import cpp
15-
import semmle.code.cpp.controlflow.Guards
16-
private import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
17-
import semmle.code.cpp.security.TaintTracking
16+
import semmle.code.cpp.controlflow.IRGuards
17+
import semmle.code.cpp.security.FlowSources
18+
import semmle.code.cpp.ir.dataflow.TaintTracking
19+
import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
20+
import DataFlow::PathGraph
21+
import semmle.code.cpp.security.Security
1822

1923
predicate hasUpperBound(VariableAccess offsetExpr) {
2024
exists(BasicBlock controlled, StackVariable offsetVar, SsaDefinition def |
@@ -32,11 +36,92 @@ predicate linearBoundControls(BasicBlock controlled, SsaDefinition def, StackVar
3236
)
3337
}
3438

35-
from Expr origin, ArrayExpr arrayExpr, VariableAccess offsetExpr
39+
predicate readsVariable(LoadInstruction load, Variable var) {
40+
load.getSourceAddress().(VariableAddressInstruction).getAstVariable() = var
41+
}
42+
43+
predicate hasUpperBoundsCheck(Variable var) {
44+
exists(RelationalOperation oper, VariableAccess access |
45+
oper.getAnOperand() = access and
46+
access.getTarget() = var and
47+
// Comparing to 0 is not an upper bound check
48+
not oper.getAnOperand().getValue() = "0"
49+
)
50+
}
51+
52+
predicate nodeIsBarrierEqualityCandidate(DataFlow::Node node, Operand access, Variable checkedVar) {
53+
readsVariable(node.asInstruction(), checkedVar) and
54+
any(IRGuardCondition guard).ensuresEq(access, _, _, node.asInstruction().getBlock(), true)
55+
}
56+
57+
predicate isFlowSource(FlowSource source, string sourceType) { sourceType = source.getSourceType() }
58+
59+
predicate predictableInstruction(Instruction instr) {
60+
instr instanceof ConstantInstruction
61+
or
62+
instr instanceof StringConstantInstruction
63+
or
64+
// This could be a conversion on a string literal
65+
predictableInstruction(instr.(UnaryInstruction).getUnary())
66+
}
67+
68+
class ImproperArrayIndexValidationConfig extends TaintTracking::Configuration {
69+
ImproperArrayIndexValidationConfig() { this = "ImproperArrayIndexValidationConfig" }
70+
71+
override predicate isSource(DataFlow::Node source) { isFlowSource(source, _) }
72+
73+
override predicate isSanitizer(DataFlow::Node node) {
74+
hasUpperBound(node.asExpr())
75+
or
76+
// These barriers are ported from `DefaultTaintTracking` because this query is quite noisy
77+
// otherwise.
78+
exists(Variable checkedVar |
79+
readsVariable(node.asInstruction(), checkedVar) and
80+
hasUpperBoundsCheck(checkedVar)
81+
)
82+
or
83+
exists(Variable checkedVar, Operand access |
84+
readsVariable(access.getDef(), checkedVar) and
85+
nodeIsBarrierEqualityCandidate(node, access, checkedVar)
86+
)
87+
or
88+
// Don't use dataflow into binary instructions if both operands are unpredictable
89+
exists(BinaryInstruction iTo |
90+
iTo = node.asInstruction() and
91+
not predictableInstruction(iTo.getLeft()) and
92+
not predictableInstruction(iTo.getRight()) and
93+
// propagate taint from either the pointer or the offset, regardless of predictability
94+
not iTo instanceof PointerArithmeticInstruction
95+
)
96+
or
97+
// don't use dataflow through calls to pure functions if two or more operands
98+
// are unpredictable
99+
exists(Instruction iFrom1, Instruction iFrom2, CallInstruction iTo |
100+
iTo = node.asInstruction() and
101+
isPureFunction(iTo.getStaticCallTarget().getName()) and
102+
iFrom1 = iTo.getAnArgument() and
103+
iFrom2 = iTo.getAnArgument() and
104+
not predictableInstruction(iFrom1) and
105+
not predictableInstruction(iFrom2) and
106+
iFrom1 != iFrom2
107+
)
108+
}
109+
110+
override predicate isSink(DataFlow::Node sink) {
111+
exists(ArrayExpr arrayExpr, VariableAccess offsetExpr |
112+
offsetExpr = arrayExpr.getArrayOffset() and
113+
sink.asExpr() = offsetExpr and
114+
not hasUpperBound(offsetExpr)
115+
)
116+
}
117+
}
118+
119+
from
120+
ImproperArrayIndexValidationConfig conf, DataFlow::PathNode source, DataFlow::PathNode sink,
121+
string sourceType
36122
where
37-
tainted(origin, offsetExpr) and
38-
offsetExpr = arrayExpr.getArrayOffset() and
39-
not hasUpperBound(offsetExpr)
40-
select offsetExpr,
123+
conf.hasFlowPath(source, sink) and
124+
isFlowSource(source.getNode(), sourceType)
125+
select sink.getNode(), source, sink,
41126
"$@ flows to here and is used in an array indexing expression, potentially causing an invalid access.",
42-
origin, "User-provided value"
127+
source.getNode(), sourceType

‎cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql

Lines changed: 73 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -15,55 +15,89 @@
1515

1616
import cpp
1717
import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
18-
import semmle.code.cpp.security.TaintTracking
19-
import TaintedWithPath
18+
import semmle.code.cpp.ir.dataflow.TaintTracking
19+
import semmle.code.cpp.ir.IR
20+
import semmle.code.cpp.controlflow.IRGuards
21+
import semmle.code.cpp.security.FlowSources
22+
import DataFlow::PathGraph
2023

2124
/**
2225
* Holds if `alloc` is an allocation, and `tainted` is a child of it that is a
2326
* taint sink.
2427
*/
25-
predicate allocSink(Expr alloc, Expr tainted) {
26-
isAllocationExpr(alloc) and
27-
tainted = alloc.getAChild() and
28-
tainted.getUnspecifiedType() instanceof IntegralType
28+
predicate allocSink(Expr alloc, DataFlow::Node sink) {
29+
exists(Expr e | e = sink.asConvertedExpr() |
30+
isAllocationExpr(alloc) and
31+
e = alloc.getAChild() and
32+
e.getUnspecifiedType() instanceof IntegralType
33+
)
34+
}
35+
36+
predicate readsVariable(LoadInstruction load, Variable var) {
37+
load.getSourceAddress().(VariableAddressInstruction).getAstVariable() = var
38+
}
39+
40+
predicate hasUpperBoundsCheck(Variable var) {
41+
exists(RelationalOperation oper, VariableAccess access |
42+
oper.getAnOperand() = access and
43+
access.getTarget() = var and
44+
// Comparing to 0 is not an upper bound check
45+
not oper.getAnOperand().getValue() = "0"
46+
)
47+
}
48+
49+
predicate nodeIsBarrierEqualityCandidate(DataFlow::Node node, Operand access, Variable checkedVar) {
50+
readsVariable(node.asInstruction(), checkedVar) and
51+
any(IRGuardCondition guard).ensuresEq(access, _, _, node.asInstruction().getBlock(), true)
2952
}
3053

31-
class TaintedAllocationSizeConfiguration extends TaintTrackingConfiguration {
32-
override predicate isSink(Element tainted) { allocSink(_, tainted) }
54+
predicate isFlowSource(FlowSource source, string sourceType) { sourceType = source.getSourceType() }
55+
56+
class TaintedAllocationSizeConfiguration extends TaintTracking::Configuration {
57+
TaintedAllocationSizeConfiguration() { this = "TaintedAllocationSizeConfiguration" }
58+
59+
override predicate isSource(DataFlow::Node source) { isFlowSource(source, _) }
60+
61+
override predicate isSink(DataFlow::Node sink) { allocSink(_, sink) }
3362

34-
override predicate isBarrier(Expr e) {
35-
super.isBarrier(e)
63+
override predicate isSanitizer(DataFlow::Node node) {
64+
exists(Expr e | e = node.asExpr() |
65+
// There can be two separate reasons for `convertedExprMightOverflow` not holding:
66+
// 1. `e` really cannot overflow.
67+
// 2. `e` isn't analyzable.
68+
// If we didn't rule out case 2 we would place barriers on anything that isn't analyzable.
69+
(
70+
e instanceof UnaryArithmeticOperation or
71+
e instanceof BinaryArithmeticOperation or
72+
e instanceof AssignArithmeticOperation
73+
) and
74+
not convertedExprMightOverflow(e)
75+
or
76+
// Subtracting two pointers is either well-defined (and the result will likely be small), or
77+
// terribly undefined and dangerous. Here, we assume that the programmer has ensured that the
78+
// result is well-defined (i.e., the two pointers point to the same object), and thus the result
79+
// will likely be small.
80+
e = any(PointerDiffExpr diff).getAnOperand()
81+
)
3682
or
37-
// There can be two separate reasons for `convertedExprMightOverflow` not holding:
38-
// 1. `e` really cannot overflow.
39-
// 2. `e` isn't analyzable.
40-
// If we didn't rule out case 2 we would place barriers on anything that isn't analyzable.
41-
(
42-
e instanceof UnaryArithmeticOperation or
43-
e instanceof BinaryArithmeticOperation or
44-
e instanceof AssignArithmeticOperation
45-
) and
46-
not convertedExprMightOverflow(e)
83+
exists(Variable checkedVar |
84+
readsVariable(node.asInstruction(), checkedVar) and
85+
hasUpperBoundsCheck(checkedVar)
86+
)
4787
or
48-
// Subtracting two pointers is either well-defined (and the result will likely be small), or
49-
// terribly undefined and dangerous. Here, we assume that the programmer has ensured that the
50-
// result is well-defined (i.e., the two pointers point to the same object), and thus the result
51-
// will likely be small.
52-
e = any(PointerDiffExpr diff).getAnOperand()
88+
exists(Variable checkedVar, Operand access |
89+
readsVariable(access.getDef(), checkedVar) and
90+
nodeIsBarrierEqualityCandidate(node, access, checkedVar)
91+
)
5392
}
5493
}
5594

56-
predicate taintedAllocSize(
57-
Expr source, Expr alloc, PathNode sourceNode, PathNode sinkNode, string taintCause
58-
) {
59-
isUserInput(source, taintCause) and
60-
exists(Expr tainted |
61-
allocSink(alloc, tainted) and
62-
taintedWithPath(source, tainted, sourceNode, sinkNode)
63-
)
64-
}
65-
66-
from Expr source, Expr alloc, PathNode sourceNode, PathNode sinkNode, string taintCause
67-
where taintedAllocSize(source, alloc, sourceNode, sinkNode, taintCause)
68-
select alloc, sourceNode, sinkNode, "This allocation size is derived from $@ and might overflow",
69-
source, "user input (" + taintCause + ")"
95+
from
96+
Expr alloc, DataFlow::PathNode source, DataFlow::PathNode sink, string taintCause,
97+
TaintedAllocationSizeConfiguration conf
98+
where
99+
isFlowSource(source.getNode(), taintCause) and
100+
conf.hasFlowPath(source, sink) and
101+
allocSink(alloc, sink.getNode())
102+
select alloc, source, sink, "This allocation size is derived from $@ and might overflow",
103+
source.getNode(), "user input (" + taintCause + ")"

‎cpp/ql/src/change-notes/2022-03-10-port-three-queries-to-taint-tracking.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 `cpp/overflow-destination`, `cpp/unclear-array-index-validation`, and `cpp/uncontrolled-allocation-size` queries have been modernized and converted to `path-problem` queries and provide more true positive results.

‎cpp/ql/test/query-tests/Security/CWE/CWE-119/SAMATE/OverflowDestination.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
edges
2+
nodes
3+
subpaths
4+
#select

‎cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowDestination.expected

Lines changed: 43 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,43 @@
1-
| overflowdestination.cpp:30:2:30:8 | call to strncpy | To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size. |
2-
| overflowdestination.cpp:46:2:46:7 | call to memcpy | To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size. |
3-
| overflowdestination.cpp:53:2:53:7 | call to memcpy | To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size. |
4-
| overflowdestination.cpp:64:2:64:7 | call to memcpy | To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size. |
1+
edges
2+
| overflowdestination.cpp:27:9:27:12 | argv | overflowdestination.cpp:30:17:30:20 | (const char *)... |
3+
| overflowdestination.cpp:43:8:43:10 | fgets output argument | overflowdestination.cpp:46:15:46:17 | (const void *)... |
4+
| overflowdestination.cpp:50:52:50:54 | *src | overflowdestination.cpp:50:52:50:54 | ReturnIndirection |
5+
| overflowdestination.cpp:50:52:50:54 | src | overflowdestination.cpp:53:15:53:17 | (const void *)... |
6+
| overflowdestination.cpp:57:52:57:54 | *src | overflowdestination.cpp:64:16:64:19 | (const void *)... |
7+
| overflowdestination.cpp:57:52:57:54 | src | overflowdestination.cpp:64:16:64:19 | (const void *)... |
8+
| overflowdestination.cpp:73:8:73:10 | fgets output argument | overflowdestination.cpp:75:30:75:32 | src |
9+
| overflowdestination.cpp:73:8:73:10 | fgets output argument | overflowdestination.cpp:75:30:75:32 | src indirection |
10+
| overflowdestination.cpp:73:8:73:10 | fgets output argument | overflowdestination.cpp:76:30:76:32 | src |
11+
| overflowdestination.cpp:73:8:73:10 | fgets output argument | overflowdestination.cpp:76:30:76:32 | src indirection |
12+
| overflowdestination.cpp:75:30:75:32 | overflowdest_test2 output argument | overflowdestination.cpp:76:30:76:32 | src |
13+
| overflowdestination.cpp:75:30:75:32 | overflowdest_test2 output argument | overflowdestination.cpp:76:30:76:32 | src indirection |
14+
| overflowdestination.cpp:75:30:75:32 | src | overflowdestination.cpp:50:52:50:54 | src |
15+
| overflowdestination.cpp:75:30:75:32 | src indirection | overflowdestination.cpp:50:52:50:54 | *src |
16+
| overflowdestination.cpp:75:30:75:32 | src indirection | overflowdestination.cpp:75:30:75:32 | overflowdest_test2 output argument |
17+
| overflowdestination.cpp:76:30:76:32 | src | overflowdestination.cpp:57:52:57:54 | src |
18+
| overflowdestination.cpp:76:30:76:32 | src indirection | overflowdestination.cpp:57:52:57:54 | *src |
19+
nodes
20+
| overflowdestination.cpp:27:9:27:12 | argv | semmle.label | argv |
21+
| overflowdestination.cpp:30:17:30:20 | (const char *)... | semmle.label | (const char *)... |
22+
| overflowdestination.cpp:43:8:43:10 | fgets output argument | semmle.label | fgets output argument |
23+
| overflowdestination.cpp:46:15:46:17 | (const void *)... | semmle.label | (const void *)... |
24+
| overflowdestination.cpp:50:52:50:54 | *src | semmle.label | *src |
25+
| overflowdestination.cpp:50:52:50:54 | ReturnIndirection | semmle.label | ReturnIndirection |
26+
| overflowdestination.cpp:50:52:50:54 | src | semmle.label | src |
27+
| overflowdestination.cpp:53:15:53:17 | (const void *)... | semmle.label | (const void *)... |
28+
| overflowdestination.cpp:57:52:57:54 | *src | semmle.label | *src |
29+
| overflowdestination.cpp:57:52:57:54 | src | semmle.label | src |
30+
| overflowdestination.cpp:64:16:64:19 | (const void *)... | semmle.label | (const void *)... |
31+
| overflowdestination.cpp:73:8:73:10 | fgets output argument | semmle.label | fgets output argument |
32+
| overflowdestination.cpp:75:30:75:32 | overflowdest_test2 output argument | semmle.label | overflowdest_test2 output argument |
33+
| overflowdestination.cpp:75:30:75:32 | src | semmle.label | src |
34+
| overflowdestination.cpp:75:30:75:32 | src indirection | semmle.label | src indirection |
35+
| overflowdestination.cpp:76:30:76:32 | src | semmle.label | src |
36+
| overflowdestination.cpp:76:30:76:32 | src indirection | semmle.label | src indirection |
37+
subpaths
38+
| overflowdestination.cpp:75:30:75:32 | src indirection | overflowdestination.cpp:50:52:50:54 | *src | overflowdestination.cpp:50:52:50:54 | ReturnIndirection | overflowdestination.cpp:75:30:75:32 | overflowdest_test2 output argument |
39+
#select
40+
| overflowdestination.cpp:30:2:30:8 | call to strncpy | overflowdestination.cpp:27:9:27:12 | argv | overflowdestination.cpp:30:17:30:20 | (const char *)... | To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size. |
41+
| overflowdestination.cpp:46:2:46:7 | call to memcpy | overflowdestination.cpp:43:8:43:10 | fgets output argument | overflowdestination.cpp:46:15:46:17 | (const void *)... | To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size. |
42+
| overflowdestination.cpp:53:2:53:7 | call to memcpy | overflowdestination.cpp:73:8:73:10 | fgets output argument | overflowdestination.cpp:53:15:53:17 | (const void *)... | To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size. |
43+
| overflowdestination.cpp:64:2:64:7 | call to memcpy | overflowdestination.cpp:73:8:73:10 | fgets output argument | overflowdestination.cpp:64:16:64:19 | (const void *)... | To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size. |

0 commit comments

Comments
 (0)