Skip to content

Commit 91da0d5

Browse files
jbjjbj
authored
Merge pull request github#3592 from geoffw0/strlen
CPP: Don't taint the return value of strlen
2 parents f291749 + 9ee75aa commit 91da0d5

File tree

9 files changed

+101
-16
lines changed

9 files changed

+101
-16
lines changed

‎change-notes/1.25/analysis-cpp.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,4 +41,4 @@ The following changes in version 1.25 affect C/C++ analysis in all applications.
4141
};
4242
```
4343
* The security pack taint tracking library (`semmle.code.cpp.security.TaintTracking`) now considers that equality checks may block the flow of taint. This results in fewer false positive results from queries that use this library.
44-
44+
* The length of a tainted string (such as the return value of a call to `strlen` or `strftime` with tainted parameters) is no longer itself considered tainted by the `models` library. This leads to fewer false positive results in queries that use any of our taint libraries.

‎cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,7 @@ private predicate predictableInstruction(Instruction instr) {
3333
* Note that the list itself is not very principled; it consists of all the
3434
* functions listed in the old security library's [default] `isPureFunction`
3535
* that have more than one argument, but are not in the old taint tracking
36-
* library's `returnArgument` predicate. In addition, `strlen` is included
37-
* because it's also a special case in flow to return values.
36+
* library's `returnArgument` predicate.
3837
*/
3938
predicate predictableOnlyFlow(string name) {
4039
name = "strcasestr" or
@@ -43,7 +42,6 @@ predicate predictableOnlyFlow(string name) {
4342
name = "strchrnul" or
4443
name = "strcmp" or
4544
name = "strcspn" or
46-
name = "strlen" or // special case
4745
name = "strncmp" or
4846
name = "strndup" or
4947
name = "strnlen" or

‎cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll

Lines changed: 47 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,26 +20,15 @@ class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunction, SideE
2020
name = "strpbrk" or
2121
name = "strcmp" or
2222
name = "strcspn" or
23-
name = "strlen" or
2423
name = "strncmp" or
25-
name = "strnlen" or
2624
name = "strrchr" or
2725
name = "strspn" or
2826
name = "strtod" or
2927
name = "strtof" or
3028
name = "strtol" or
3129
name = "strtoll" or
3230
name = "strtoq" or
33-
name = "strtoul" or
34-
name = "wcslen"
35-
)
36-
or
37-
hasGlobalName(name) and
38-
(
39-
name = "_mbslen" or
40-
name = "_mbslen_l" or
41-
name = "_mbstrlen" or
42-
name = "_mbstrlen_l"
31+
name = "strtoul"
4332
)
4433
)
4534
}
@@ -90,6 +79,52 @@ class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunction, SideE
9079
}
9180
}
9281

82+
class StrLenFunction extends AliasFunction, ArrayFunction, SideEffectFunction {
83+
StrLenFunction() {
84+
exists(string name |
85+
hasGlobalOrStdName(name) and
86+
(
87+
name = "strlen" or
88+
name = "strnlen" or
89+
name = "wcslen"
90+
)
91+
or
92+
hasGlobalName(name) and
93+
(
94+
name = "_mbslen" or
95+
name = "_mbslen_l" or
96+
name = "_mbstrlen" or
97+
name = "_mbstrlen_l"
98+
)
99+
)
100+
}
101+
102+
override predicate hasArrayInput(int bufParam) {
103+
getParameter(bufParam).getUnspecifiedType() instanceof PointerType
104+
}
105+
106+
override predicate hasArrayWithNullTerminator(int bufParam) {
107+
getParameter(bufParam).getUnspecifiedType() instanceof PointerType
108+
}
109+
110+
override predicate parameterNeverEscapes(int i) {
111+
getParameter(i).getUnspecifiedType() instanceof PointerType
112+
}
113+
114+
override predicate parameterEscapesOnlyViaReturn(int i) { none() }
115+
116+
override predicate parameterIsAlwaysReturned(int i) { none() }
117+
118+
override predicate hasOnlySpecificReadSideEffects() { any() }
119+
120+
override predicate hasOnlySpecificWriteSideEffects() { any() }
121+
122+
override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) {
123+
getParameter(i).getUnspecifiedType() instanceof PointerType and
124+
buffer = true
125+
}
126+
}
127+
93128
class PureFunction extends TaintFunction, SideEffectFunction {
94129
PureFunction() {
95130
exists(string name |

‎cpp/ql/src/semmle/code/cpp/security/TaintTracking.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
/*
22
* Support for tracking tainted data through the program.
3+
*
4+
* Prefer to use `semmle.code.cpp.dataflow.TaintTracking` when designing new queries.
35
*/
46

57
import semmle.code.cpp.ir.dataflow.DefaultTaintTracking

‎cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
11
/**
2+
* DEPRECATED: we now use `semmle.code.cpp.ir.dataflow.DefaultTaintTracking`,
3+
* which is based on the IR but designed to behave similarly to this old
4+
* libarary.
5+
*
26
* Provides the implementation of `semmle.code.cpp.security.TaintTracking`. Do
37
* not import this file directly.
48
*/

‎cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,3 +136,24 @@ void test1()
136136
sink(buffer); // tainted [NOT DETECTED]
137137
}
138138
}
139+
140+
// ----------
141+
142+
size_t strlen(const char *s);
143+
size_t wcslen(const wchar_t *s);
144+
145+
void test2()
146+
{
147+
char *s = string::source();
148+
wchar_t *ws = wstring::source();
149+
int i;
150+
151+
sink(strlen(s));
152+
sink(wcslen(ws));
153+
154+
i = strlen(s) + 1;
155+
sink(i);
156+
157+
sink(s[strlen(s) - 1]); // tainted
158+
sink(ws + (wcslen(ws) / 2)); // tainted
159+
}

‎cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,26 @@
111111
| format.cpp:135:39:135:45 | ref arg & ... | format.cpp:135:40:135:45 | buffer [inner post update] | |
112112
| format.cpp:135:39:135:45 | ref arg & ... | format.cpp:136:8:136:13 | buffer | |
113113
| format.cpp:135:40:135:45 | buffer | format.cpp:135:39:135:45 | & ... | |
114+
| format.cpp:147:12:147:25 | call to source | format.cpp:151:14:151:14 | s | |
115+
| format.cpp:147:12:147:25 | call to source | format.cpp:154:13:154:13 | s | |
116+
| format.cpp:147:12:147:25 | call to source | format.cpp:157:7:157:7 | s | |
117+
| format.cpp:147:12:147:25 | call to source | format.cpp:157:16:157:16 | s | |
118+
| format.cpp:148:16:148:30 | call to source | format.cpp:152:14:152:15 | ws | |
119+
| format.cpp:148:16:148:30 | call to source | format.cpp:158:7:158:8 | ws | |
120+
| format.cpp:148:16:148:30 | call to source | format.cpp:158:20:158:21 | ws | |
121+
| format.cpp:154:6:154:11 | call to strlen | format.cpp:154:6:154:18 | ... + ... | TAINT |
122+
| format.cpp:154:6:154:18 | ... + ... | format.cpp:154:2:154:18 | ... = ... | |
123+
| format.cpp:154:6:154:18 | ... + ... | format.cpp:155:7:155:7 | i | |
124+
| format.cpp:154:18:154:18 | 1 | format.cpp:154:6:154:18 | ... + ... | TAINT |
125+
| format.cpp:157:7:157:7 | s | format.cpp:157:7:157:22 | access to array | TAINT |
126+
| format.cpp:157:9:157:14 | call to strlen | format.cpp:157:9:157:21 | ... - ... | TAINT |
127+
| format.cpp:157:9:157:21 | ... - ... | format.cpp:157:7:157:22 | access to array | TAINT |
128+
| format.cpp:157:21:157:21 | 1 | format.cpp:157:9:157:21 | ... - ... | TAINT |
129+
| format.cpp:158:7:158:8 | ws | format.cpp:158:7:158:27 | ... + ... | TAINT |
130+
| format.cpp:158:7:158:27 | ref arg ... + ... | format.cpp:158:7:158:8 | ws [inner post update] | |
131+
| format.cpp:158:13:158:18 | call to wcslen | format.cpp:158:13:158:26 | ... / ... | TAINT |
132+
| format.cpp:158:13:158:26 | ... / ... | format.cpp:158:7:158:27 | ... + ... | TAINT |
133+
| format.cpp:158:26:158:26 | 2 | format.cpp:158:13:158:26 | ... / ... | TAINT |
114134
| stl.cpp:67:12:67:17 | call to source | stl.cpp:71:7:71:7 | a | |
115135
| stl.cpp:68:16:68:20 | 123 | stl.cpp:68:16:68:21 | call to basic_string | TAINT |
116136
| stl.cpp:68:16:68:21 | call to basic_string | stl.cpp:72:7:72:7 | b | |

‎cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88
| format.cpp:100:8:100:13 | buffer | format.cpp:99:30:99:43 | call to source |
99
| format.cpp:105:8:105:13 | buffer | format.cpp:104:31:104:45 | call to source |
1010
| format.cpp:110:8:110:14 | wbuffer | format.cpp:109:38:109:52 | call to source |
11+
| format.cpp:157:7:157:22 | access to array | format.cpp:147:12:147:25 | call to source |
12+
| format.cpp:158:7:158:27 | ... + ... | format.cpp:148:16:148:30 | call to source |
1113
| stl.cpp:71:7:71:7 | a | stl.cpp:67:12:67:17 | call to source |
1214
| stl.cpp:73:7:73:7 | c | stl.cpp:69:16:69:21 | call to source |
1315
| stl.cpp:75:9:75:13 | call to c_str | stl.cpp:69:16:69:21 | call to source |

‎cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
| format.cpp:157:7:157:22 | (int)... | format.cpp:147:12:147:25 | call to source |
2+
| format.cpp:157:7:157:22 | access to array | format.cpp:147:12:147:25 | call to source |
3+
| format.cpp:158:7:158:27 | ... + ... | format.cpp:148:16:148:30 | call to source |
14
| stl.cpp:71:7:71:7 | (const char *)... | stl.cpp:67:12:67:17 | call to source |
25
| stl.cpp:71:7:71:7 | a | stl.cpp:67:12:67:17 | call to source |
36
| taint.cpp:8:8:8:13 | clean1 | taint.cpp:4:27:4:33 | source1 |

0 commit comments

Comments
 (0)