Skip to content

Commit e90803a

Browse files
MathiasVPMathiasVP
committed
C++: Rewrite 'cpp/unbounded-write' away from DefaultTaintTracking.
1 parent 2787f0a commit e90803a

File tree

2 files changed

+66
-66
lines changed

2 files changed

+66
-66
lines changed

‎cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql

Lines changed: 51 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,10 @@
1515
*/
1616

1717
import semmle.code.cpp.security.BufferWrite
18-
import semmle.code.cpp.security.Security
19-
import semmle.code.cpp.ir.dataflow.internal.DefaultTaintTrackingImpl
20-
import TaintedWithPath
18+
import semmle.code.cpp.security.FlowSources as FS
19+
import semmle.code.cpp.dataflow.new.TaintTracking
20+
import semmle.code.cpp.controlflow.IRGuards
21+
import Flow::PathGraph
2122

2223
/*
2324
* --- Summary of CWE-120 alerts ---
@@ -47,15 +48,6 @@ predicate isUnboundedWrite(BufferWrite bw) {
4748
not exists(bw.getMaxData(_)) // and we can't deduce an upper bound to the amount copied
4849
}
4950

50-
/*
51-
* predicate isMaybeUnboundedWrite(BufferWrite bw)
52-
* {
53-
* not bw.hasExplicitLimit() // has no explicit size limit
54-
* and exists(bw.getMaxData()) // and we can deduce an upper bound to the amount copied
55-
* and (not exists(getBufferSize(bw.getDest(), _))) // but we can't work out the size of the destination to be sure
56-
* }
57-
*/
58-
5951
/**
6052
* Holds if `e` is a source buffer going into an unbounded write `bw` or a
6153
* qualifier of (a qualifier of ...) such a source.
@@ -66,19 +58,43 @@ predicate unboundedWriteSource(Expr e, BufferWrite bw) {
6658
exists(FieldAccess fa | unboundedWriteSource(fa, bw) and e = fa.getQualifier())
6759
}
6860

69-
/*
70-
* --- user input reach ---
71-
*/
61+
predicate isSource(FS::FlowSource source, string sourceType) { source.getSourceType() = sourceType }
62+
63+
predicate isSink(DataFlow::Node sink, BufferWrite bw) {
64+
unboundedWriteSource(sink.asIndirectExpr(), bw)
65+
or
66+
// `gets` and `scanf` reads from stdin so there's no real input.
67+
// The `BufferWrite` library models this as the call itself being
68+
// the source. In this case we mark the output argument as being
69+
// the sink so that we report a path where source = sink (because
70+
// the same output argument is also included in `isSource`).
71+
bw.getASource() = bw and
72+
unboundedWriteSource(sink.asDefiningArgument(), bw)
73+
}
74+
75+
predicate lessThanOrEqual(IRGuardCondition g, Expr e, boolean branch) {
76+
exists(Operand left |
77+
g.comparesLt(left, _, _, true, branch) or
78+
g.comparesEq(left, _, _, true, branch)
79+
|
80+
left.getDef().getUnconvertedResultExpression() = e
81+
)
82+
}
83+
84+
module Config implements DataFlow::ConfigSig {
85+
predicate isSource(DataFlow::Node source) { isSource(source, _) }
7286

73-
class Configuration extends TaintTrackingConfiguration {
74-
override predicate isSink(Element tainted) { unboundedWriteSource(tainted, _) }
87+
predicate isSink(DataFlow::Node sink) { isSink(sink, _) }
7588

76-
override predicate taintThroughGlobals() { any() }
89+
predicate isBarrierOut(DataFlow::Node node) { isSink(node) }
90+
91+
predicate isBarrier(DataFlow::Node node) {
92+
// Block flow is the node is guarded by any <, <= or = operations.
93+
node = DataFlow::BarrierGuard<lessThanOrEqual/3>::getABarrierNode()
94+
}
7795
}
7896

79-
/*
80-
* --- put it together ---
81-
*/
97+
module Flow = TaintTracking::Global<Config>;
8298

8399
/*
84100
* An unbounded write is, for example `strcpy(..., tainted)`. We're looking
@@ -87,17 +103,20 @@ class Configuration extends TaintTrackingConfiguration {
87103
*
88104
* In the case of `gets` and `scanf`, where the source buffer is implicit, the
89105
* `BufferWrite` library reports the source buffer to be the same as the
90-
* destination buffer. Since those destination-buffer arguments are also
91-
* modeled in the taint-tracking library as being _sources_ of taint, they are
92-
* in practice reported as being tainted because the `security.TaintTracking`
93-
* library does not distinguish between taint going into an argument and out of
94-
* an argument. Thus, we get the desired alerts.
106+
* destination buffer. So to report an alert on a pattern like:
107+
* ```
108+
* char s[32];
109+
* gets(s);
110+
* ```
111+
* we define the sink as the node corresponding to the output argument of `gets`.
112+
* This gives us a path where the source is equal to the sink.
95113
*/
96114

97-
from BufferWrite bw, Expr inputSource, Expr tainted, PathNode sourceNode, PathNode sinkNode
115+
from BufferWrite bw, Flow::PathNode source, Flow::PathNode sink, string sourceType
98116
where
99-
taintedWithPath(inputSource, tainted, sourceNode, sinkNode) and
100-
unboundedWriteSource(tainted, bw)
101-
select bw, sourceNode, sinkNode,
102-
"This '" + bw.getBWDesc() + "' with input from $@ may overflow the destination.", inputSource,
103-
inputSource.toString()
117+
Flow::flowPath(source, sink) and
118+
isSource(source.getNode(), sourceType) and
119+
isSink(sink.getNode(), bw)
120+
select bw, source, sink,
121+
"This '" + bw.getBWDesc() + "' with input from $@ may overflow the destination.",
122+
source.getNode(), sourceType

‎cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/UnboundedWrite.expected

Lines changed: 15 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,37 +1,18 @@
11
edges
2-
| tests.c:28:22:28:25 | argv | tests.c:28:22:28:28 | access to array |
3-
| tests.c:28:22:28:25 | argv | tests.c:28:22:28:28 | access to array |
4-
| tests.c:28:22:28:25 | argv | tests.c:28:22:28:28 | access to array |
5-
| tests.c:28:22:28:25 | argv | tests.c:28:22:28:28 | access to array |
6-
| tests.c:29:28:29:31 | argv | tests.c:29:28:29:34 | access to array |
7-
| tests.c:29:28:29:31 | argv | tests.c:29:28:29:34 | access to array |
8-
| tests.c:29:28:29:31 | argv | tests.c:29:28:29:34 | access to array |
9-
| tests.c:29:28:29:31 | argv | tests.c:29:28:29:34 | access to array |
10-
| tests.c:34:10:34:13 | argv | tests.c:34:10:34:16 | access to array |
11-
| tests.c:34:10:34:13 | argv | tests.c:34:10:34:16 | access to array |
12-
| tests.c:34:10:34:13 | argv | tests.c:34:10:34:16 | access to array |
13-
| tests.c:34:10:34:13 | argv | tests.c:34:10:34:16 | access to array |
14-
subpaths
2+
| tests.c:16:26:16:29 | argv indirection | tests.c:28:22:28:28 | access to array indirection |
3+
| tests.c:16:26:16:29 | argv indirection | tests.c:29:28:29:34 | access to array indirection |
4+
| tests.c:16:26:16:29 | argv indirection | tests.c:34:10:34:16 | access to array indirection |
155
nodes
16-
| tests.c:28:22:28:25 | argv | semmle.label | argv |
17-
| tests.c:28:22:28:25 | argv | semmle.label | argv |
18-
| tests.c:28:22:28:28 | access to array | semmle.label | access to array |
19-
| tests.c:28:22:28:28 | access to array | semmle.label | access to array |
20-
| tests.c:29:28:29:31 | argv | semmle.label | argv |
21-
| tests.c:29:28:29:31 | argv | semmle.label | argv |
22-
| tests.c:29:28:29:34 | access to array | semmle.label | access to array |
23-
| tests.c:29:28:29:34 | access to array | semmle.label | access to array |
24-
| tests.c:31:15:31:23 | buffer100 | semmle.label | buffer100 |
25-
| tests.c:31:15:31:23 | buffer100 | semmle.label | buffer100 |
26-
| tests.c:33:21:33:29 | buffer100 | semmle.label | buffer100 |
27-
| tests.c:33:21:33:29 | buffer100 | semmle.label | buffer100 |
28-
| tests.c:34:10:34:13 | argv | semmle.label | argv |
29-
| tests.c:34:10:34:13 | argv | semmle.label | argv |
30-
| tests.c:34:10:34:16 | access to array | semmle.label | access to array |
31-
| tests.c:34:10:34:16 | access to array | semmle.label | access to array |
6+
| tests.c:16:26:16:29 | argv indirection | semmle.label | argv indirection |
7+
| tests.c:28:22:28:28 | access to array indirection | semmle.label | access to array indirection |
8+
| tests.c:29:28:29:34 | access to array indirection | semmle.label | access to array indirection |
9+
| tests.c:31:15:31:23 | scanf output argument | semmle.label | scanf output argument |
10+
| tests.c:33:21:33:29 | scanf output argument | semmle.label | scanf output argument |
11+
| tests.c:34:10:34:16 | access to array indirection | semmle.label | access to array indirection |
12+
subpaths
3213
#select
33-
| tests.c:28:3:28:9 | call to sprintf | tests.c:28:22:28:25 | argv | tests.c:28:22:28:28 | access to array | This 'call to sprintf' with input from $@ may overflow the destination. | tests.c:28:22:28:25 | argv | argv |
34-
| tests.c:29:3:29:9 | call to sprintf | tests.c:29:28:29:31 | argv | tests.c:29:28:29:34 | access to array | This 'call to sprintf' with input from $@ may overflow the destination. | tests.c:29:28:29:31 | argv | argv |
35-
| tests.c:31:15:31:23 | buffer100 | tests.c:31:15:31:23 | buffer100 | tests.c:31:15:31:23 | buffer100 | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:31:15:31:23 | buffer100 | buffer100 |
36-
| tests.c:33:21:33:29 | buffer100 | tests.c:33:21:33:29 | buffer100 | tests.c:33:21:33:29 | buffer100 | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:33:21:33:29 | buffer100 | buffer100 |
37-
| tests.c:34:25:34:33 | buffer100 | tests.c:34:10:34:13 | argv | tests.c:34:10:34:16 | access to array | This 'sscanf string argument' with input from $@ may overflow the destination. | tests.c:34:10:34:13 | argv | argv |
14+
| tests.c:28:3:28:9 | call to sprintf | tests.c:16:26:16:29 | argv indirection | tests.c:28:22:28:28 | access to array indirection | This 'call to sprintf' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument |
15+
| tests.c:29:3:29:9 | call to sprintf | tests.c:16:26:16:29 | argv indirection | tests.c:29:28:29:34 | access to array indirection | This 'call to sprintf' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument |
16+
| tests.c:31:15:31:23 | buffer100 | tests.c:31:15:31:23 | scanf output argument | tests.c:31:15:31:23 | scanf output argument | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:31:15:31:23 | scanf output argument | value read by scanf |
17+
| tests.c:33:21:33:29 | buffer100 | tests.c:33:21:33:29 | scanf output argument | tests.c:33:21:33:29 | scanf output argument | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:33:21:33:29 | scanf output argument | value read by scanf |
18+
| tests.c:34:25:34:33 | buffer100 | tests.c:16:26:16:29 | argv indirection | tests.c:34:10:34:16 | access to array indirection | This 'sscanf string argument' with input from $@ may overflow the destination. | tests.c:16:26:16:29 | argv indirection | a command-line argument |

0 commit comments

Comments
 (0)