Skip to content

Commit 511bee7

Browse files
geoffw0geoffw0
committed
C++: Fix results that flow to/from encryption routines.
1 parent b82425a commit 511bee7

File tree

3 files changed

+135
-31
lines changed

3 files changed

+135
-31
lines changed

‎cpp/ql/src/Security/CWE/CWE-311/CleartextTransmission.ql

Lines changed: 63 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,14 @@
1414
import cpp
1515
import semmle.code.cpp.security.SensitiveExprs
1616
import semmle.code.cpp.dataflow.TaintTracking
17+
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
1718
import semmle.code.cpp.models.interfaces.FlowSource
1819
import DataFlow::PathGraph
1920

2021
/**
2122
* A function call that sends or receives data over a network.
23+
*
24+
* note: functions such as `write` may be writing to a network source or a file. We could attempt to determine which, and sort results into `cpp/cleartext-transmission` and perhaps `cpp/cleartext-storage-file`. In practice it usually isn't very important which query reports a result as long as its reported exactly once. See `checkSocket` to narrow this down somewhat.
2225
*/
2326
abstract class NetworkSendRecv extends FunctionCall {
2427
/**
@@ -31,12 +34,21 @@ abstract class NetworkSendRecv extends FunctionCall {
3134
* Gets the expression for the buffer to be sent from / received into.
3235
*/
3336
abstract Expr getDataExpr();
37+
38+
/**
39+
* Holds if any socket used by this call could be a true network socket.
40+
* A zero socket descriptor is standard input, which is not a network
41+
* operation.
42+
*/
43+
predicate checkSocket() {
44+
not exists(Zero zero |
45+
DataFlow::localFlow(DataFlow::exprNode(zero), DataFlow::exprNode(getSocketExpr()))
46+
)
47+
}
3448
}
3549

3650
/**
3751
* A function call that sends data over a network.
38-
*
39-
* note: functions such as `write` may be writing to a network source or a file. We could attempt to determine which, and sort results into `cpp/cleartext-transmission` and perhaps `cpp/cleartext-storage-file`. In practice it usually isn't very important which query reports a result as long as its reported exactly once.
4052
*/
4153
class NetworkSend extends NetworkSendRecv {
4254
RemoteFlowSinkFunction target;
@@ -86,33 +98,65 @@ class NetworkRecv extends NetworkSendRecv {
8698
}
8799

88100
/**
89-
* Taint flow from a sensitive expression to a network operation with data
90-
* tainted by that expression.
101+
* An expression that is an argument or return value from an encryption or
102+
* decryption call.
103+
*/
104+
class Encrypted extends Expr {
105+
Encrypted() {
106+
exists(FunctionCall fc |
107+
fc.getTarget().getName().toLowerCase().regexpMatch(".*(crypt|encode|decode).*") and
108+
(
109+
this = fc or
110+
this = fc.getAnArgument()
111+
)
112+
)
113+
}
114+
}
115+
116+
/**
117+
* Taint flow from a sensitive expression.
91118
*/
92-
class SensitiveSendRecvConfiguration extends TaintTracking::Configuration {
93-
SensitiveSendRecvConfiguration() { this = "SensitiveSendRecvConfiguration" }
119+
class FromSensitiveConfiguration extends TaintTracking::Configuration {
120+
FromSensitiveConfiguration() { this = "FromSensitiveConfiguration" }
94121

95122
override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof SensitiveExpr }
96123

97124
override predicate isSink(DataFlow::Node sink) {
98-
exists(NetworkSendRecv transmission |
99-
sink.asExpr() = transmission.getDataExpr() and
100-
// a zero socket descriptor is standard input, which is not interesting for this query.
101-
not exists(Zero zero |
102-
DataFlow::localFlow(DataFlow::exprNode(zero),
103-
DataFlow::exprNode(transmission.getSocketExpr()))
104-
)
105-
)
125+
sink.asExpr() = any(NetworkSendRecv nsr | nsr.checkSocket()).getDataExpr()
126+
or
127+
sink.asExpr() instanceof Encrypted
128+
}
129+
130+
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
131+
// flow from pre-update to post-update of the source
132+
isSource(node1) and
133+
node2.(DataFlow::PostUpdateNode).getPreUpdateNode() = node1
134+
or
135+
// flow from pre-update to post-update of the sink (in case we can reach other sinks)
136+
isSink(node1) and
137+
node2.(DataFlow::PostUpdateNode).getPreUpdateNode() = node1
138+
or
139+
// flow through encryption functions to the return value (in case we can reach other sinks)
140+
node2.asExpr().(Encrypted).(FunctionCall).getAnArgument() = node1.asExpr()
106141
}
107142
}
108143

109144
from
110-
SensitiveSendRecvConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink,
111-
NetworkSendRecv transmission, string msg
145+
FromSensitiveConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink,
146+
NetworkSendRecv networkSendRecv, string msg
112147
where
148+
// flow from sensitive -> network data
113149
config.hasFlowPath(source, sink) and
114-
sink.getNode().asExpr() = transmission.getDataExpr() and
115-
if transmission instanceof NetworkSend
150+
sink.getNode().asExpr() = networkSendRecv.getDataExpr() and
151+
networkSendRecv.checkSocket() and
152+
// no flow from sensitive -> evidence of encryption
153+
not exists(DataFlow::Node anySource, DataFlow::Node encrypted |
154+
config.hasFlow(anySource, sink.getNode()) and
155+
config.hasFlow(anySource, encrypted) and
156+
encrypted.asExpr() instanceof Encrypted
157+
) and
158+
// construct result
159+
if networkSendRecv instanceof NetworkSend
116160
then
117161
msg =
118162
"This operation transmits '" + sink.toString() +
@@ -121,4 +165,4 @@ where
121165
msg =
122166
"This operation receives into '" + sink.toString() +
123167
"', which may put unencrypted sensitive data into $@"
124-
select transmission, source, sink, msg, source, source.getNode().asExpr().toString()
168+
select networkSendRecv, source, sink, msg, source, source.getNode().asExpr().toString()

‎cpp/ql/test/query-tests/Security/CWE/CWE-311/semmle/tests/CleartextTransmission.expected

Lines changed: 66 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,10 @@
11
edges
2+
| test2.cpp:43:34:43:34 | s [post update] [password] | test2.cpp:63:22:63:22 | s [password] |
3+
| test2.cpp:43:36:43:43 | password | test2.cpp:43:36:43:43 | ref arg password |
4+
| test2.cpp:43:36:43:43 | ref arg password | test2.cpp:43:34:43:34 | s [post update] [password] |
5+
| test2.cpp:63:22:63:22 | s [password] | test2.cpp:63:24:63:31 | password |
6+
| test2.cpp:63:22:63:22 | s [password] | test2.cpp:63:24:63:31 | password |
7+
| test2.cpp:63:24:63:31 | password | test2.cpp:63:16:63:20 | call to crypt |
28
| test3.cpp:74:21:74:29 | password1 | test3.cpp:76:15:76:17 | ptr |
39
| test3.cpp:81:15:81:22 | password | test3.cpp:83:15:83:17 | ptr |
410
| test3.cpp:112:20:112:25 | buffer | test3.cpp:114:14:114:19 | buffer |
@@ -10,7 +16,37 @@ edges
1016
| test3.cpp:138:24:138:32 | password1 | test3.cpp:138:21:138:22 | call to id |
1117
| test3.cpp:144:16:144:29 | call to get_global_str | test3.cpp:146:15:146:18 | data |
1218
| test3.cpp:157:19:157:26 | password | test3.cpp:159:15:159:20 | buffer |
19+
| test3.cpp:173:15:173:22 | password | test3.cpp:175:3:175:17 | call to decrypt_inplace |
20+
| test3.cpp:173:15:173:22 | password | test3.cpp:175:19:175:26 | password |
21+
| test3.cpp:173:15:173:22 | password | test3.cpp:175:19:175:26 | password |
22+
| test3.cpp:175:19:175:26 | password | test3.cpp:175:3:175:17 | call to decrypt_inplace |
23+
| test3.cpp:181:15:181:22 | password | test3.cpp:184:3:184:17 | call to decrypt_inplace |
24+
| test3.cpp:181:15:181:22 | password | test3.cpp:184:19:184:26 | password |
25+
| test3.cpp:181:15:181:22 | password | test3.cpp:184:19:184:26 | password |
26+
| test3.cpp:184:19:184:26 | password | test3.cpp:184:3:184:17 | call to decrypt_inplace |
27+
| test3.cpp:191:15:191:22 | password | test3.cpp:193:18:193:28 | call to rtn_decrypt |
28+
| test3.cpp:191:15:191:22 | password | test3.cpp:193:30:193:37 | password |
29+
| test3.cpp:191:15:191:22 | password | test3.cpp:193:30:193:37 | password |
30+
| test3.cpp:193:30:193:37 | password | test3.cpp:193:18:193:28 | call to rtn_decrypt |
31+
| test3.cpp:199:19:199:26 | password | test3.cpp:199:3:199:17 | call to encrypt_inplace |
32+
| test3.cpp:199:19:199:26 | password | test3.cpp:201:15:201:22 | password |
33+
| test3.cpp:207:19:207:26 | password | test3.cpp:207:3:207:17 | call to encrypt_inplace |
34+
| test3.cpp:207:19:207:26 | password | test3.cpp:210:15:210:22 | password |
35+
| test3.cpp:217:18:217:28 | call to rtn_encrypt | test3.cpp:219:15:219:26 | password_ptr |
36+
| test3.cpp:217:30:217:37 | password | test3.cpp:217:18:217:28 | call to rtn_encrypt |
37+
| test3.cpp:217:30:217:37 | password | test3.cpp:217:18:217:28 | call to rtn_encrypt |
38+
| test3.cpp:217:30:217:37 | password | test3.cpp:219:15:219:26 | password_ptr |
39+
| test3.cpp:241:8:241:15 | password | test3.cpp:242:8:242:15 | password |
40+
| test.cpp:48:29:48:39 | thePassword | test.cpp:48:21:48:27 | call to encrypt |
41+
| test.cpp:76:29:76:39 | thePassword | test.cpp:76:21:76:27 | call to encrypt |
1342
nodes
43+
| test2.cpp:43:34:43:34 | s [post update] [password] | semmle.label | s [post update] [password] |
44+
| test2.cpp:43:36:43:43 | password | semmle.label | password |
45+
| test2.cpp:43:36:43:43 | ref arg password | semmle.label | ref arg password |
46+
| test2.cpp:63:16:63:20 | call to crypt | semmle.label | call to crypt |
47+
| test2.cpp:63:22:63:22 | s [password] | semmle.label | s [password] |
48+
| test2.cpp:63:24:63:31 | password | semmle.label | password |
49+
| test2.cpp:63:24:63:31 | password | semmle.label | password |
1450
| test3.cpp:22:15:22:23 | password1 | semmle.label | password1 |
1551
| test3.cpp:26:15:26:23 | password2 | semmle.label | password2 |
1652
| test3.cpp:38:23:38:31 | password2 | semmle.label | password2 |
@@ -35,15 +71,44 @@ nodes
3571
| test3.cpp:157:19:157:26 | password | semmle.label | password |
3672
| test3.cpp:159:15:159:20 | buffer | semmle.label | buffer |
3773
| test3.cpp:173:15:173:22 | password | semmle.label | password |
74+
| test3.cpp:173:15:173:22 | password | semmle.label | password |
75+
| test3.cpp:175:3:175:17 | call to decrypt_inplace | semmle.label | call to decrypt_inplace |
76+
| test3.cpp:175:19:175:26 | password | semmle.label | password |
77+
| test3.cpp:175:19:175:26 | password | semmle.label | password |
78+
| test3.cpp:181:15:181:22 | password | semmle.label | password |
3879
| test3.cpp:181:15:181:22 | password | semmle.label | password |
80+
| test3.cpp:184:3:184:17 | call to decrypt_inplace | semmle.label | call to decrypt_inplace |
81+
| test3.cpp:184:19:184:26 | password | semmle.label | password |
82+
| test3.cpp:184:19:184:26 | password | semmle.label | password |
3983
| test3.cpp:191:15:191:22 | password | semmle.label | password |
84+
| test3.cpp:191:15:191:22 | password | semmle.label | password |
85+
| test3.cpp:193:18:193:28 | call to rtn_decrypt | semmle.label | call to rtn_decrypt |
86+
| test3.cpp:193:30:193:37 | password | semmle.label | password |
87+
| test3.cpp:193:30:193:37 | password | semmle.label | password |
88+
| test3.cpp:199:3:199:17 | call to encrypt_inplace | semmle.label | call to encrypt_inplace |
89+
| test3.cpp:199:19:199:26 | password | semmle.label | password |
90+
| test3.cpp:199:19:199:26 | password | semmle.label | password |
4091
| test3.cpp:201:15:201:22 | password | semmle.label | password |
92+
| test3.cpp:207:3:207:17 | call to encrypt_inplace | semmle.label | call to encrypt_inplace |
93+
| test3.cpp:207:19:207:26 | password | semmle.label | password |
94+
| test3.cpp:207:19:207:26 | password | semmle.label | password |
4195
| test3.cpp:210:15:210:22 | password | semmle.label | password |
96+
| test3.cpp:217:18:217:28 | call to rtn_encrypt | semmle.label | call to rtn_encrypt |
97+
| test3.cpp:217:18:217:28 | call to rtn_encrypt | semmle.label | call to rtn_encrypt |
98+
| test3.cpp:217:30:217:37 | password | semmle.label | password |
99+
| test3.cpp:217:30:217:37 | password | semmle.label | password |
42100
| test3.cpp:219:15:219:26 | password_ptr | semmle.label | password_ptr |
43101
| test3.cpp:227:22:227:29 | password | semmle.label | password |
44102
| test3.cpp:228:26:228:33 | password | semmle.label | password |
45103
| test3.cpp:241:8:241:15 | password | semmle.label | password |
104+
| test3.cpp:241:8:241:15 | password | semmle.label | password |
46105
| test3.cpp:242:8:242:15 | password | semmle.label | password |
106+
| test.cpp:48:21:48:27 | call to encrypt | semmle.label | call to encrypt |
107+
| test.cpp:48:29:48:39 | thePassword | semmle.label | thePassword |
108+
| test.cpp:48:29:48:39 | thePassword | semmle.label | thePassword |
109+
| test.cpp:76:21:76:27 | call to encrypt | semmle.label | call to encrypt |
110+
| test.cpp:76:29:76:39 | thePassword | semmle.label | thePassword |
111+
| test.cpp:76:29:76:39 | thePassword | semmle.label | thePassword |
47112
subpaths
48113
| test3.cpp:138:24:138:32 | password1 | test3.cpp:117:28:117:33 | buffer | test3.cpp:119:9:119:14 | buffer | test3.cpp:138:21:138:22 | call to id |
49114
#select
@@ -59,13 +124,8 @@ subpaths
59124
| test3.cpp:140:3:140:6 | call to send | test3.cpp:138:24:138:32 | password1 | test3.cpp:140:15:140:17 | ptr | This operation transmits 'ptr', which may contain unencrypted sensitive data from $@ | test3.cpp:138:24:138:32 | password1 | password1 |
60125
| test3.cpp:146:3:146:6 | call to send | test3.cpp:126:9:126:23 | global_password | test3.cpp:146:15:146:18 | data | This operation transmits 'data', which may contain unencrypted sensitive data from $@ | test3.cpp:126:9:126:23 | global_password | global_password |
61126
| test3.cpp:159:3:159:6 | call to send | test3.cpp:157:19:157:26 | password | test3.cpp:159:15:159:20 | buffer | This operation transmits 'buffer', which may contain unencrypted sensitive data from $@ | test3.cpp:157:19:157:26 | password | password |
62-
| test3.cpp:173:3:173:6 | call to recv | test3.cpp:173:15:173:22 | password | test3.cpp:173:15:173:22 | password | This operation receives into 'password', which may put unencrypted sensitive data into $@ | test3.cpp:173:15:173:22 | password | password |
63-
| test3.cpp:181:3:181:6 | call to recv | test3.cpp:181:15:181:22 | password | test3.cpp:181:15:181:22 | password | This operation receives into 'password', which may put unencrypted sensitive data into $@ | test3.cpp:181:15:181:22 | password | password |
64-
| test3.cpp:191:3:191:6 | call to recv | test3.cpp:191:15:191:22 | password | test3.cpp:191:15:191:22 | password | This operation receives into 'password', which may put unencrypted sensitive data into $@ | test3.cpp:191:15:191:22 | password | password |
65-
| test3.cpp:201:3:201:6 | call to send | test3.cpp:201:15:201:22 | password | test3.cpp:201:15:201:22 | password | This operation transmits 'password', which may contain unencrypted sensitive data from $@ | test3.cpp:201:15:201:22 | password | password |
66-
| test3.cpp:210:3:210:6 | call to send | test3.cpp:210:15:210:22 | password | test3.cpp:210:15:210:22 | password | This operation transmits 'password', which may contain unencrypted sensitive data from $@ | test3.cpp:210:15:210:22 | password | password |
67-
| test3.cpp:219:3:219:6 | call to send | test3.cpp:219:15:219:26 | password_ptr | test3.cpp:219:15:219:26 | password_ptr | This operation transmits 'password_ptr', which may contain unencrypted sensitive data from $@ | test3.cpp:219:15:219:26 | password_ptr | password_ptr |
68127
| test3.cpp:227:2:227:5 | call to send | test3.cpp:227:22:227:29 | password | test3.cpp:227:22:227:29 | password | This operation transmits 'password', which may contain unencrypted sensitive data from $@ | test3.cpp:227:22:227:29 | password | password |
69128
| test3.cpp:228:2:228:5 | call to send | test3.cpp:228:26:228:33 | password | test3.cpp:228:26:228:33 | password | This operation transmits 'password', which may contain unencrypted sensitive data from $@ | test3.cpp:228:26:228:33 | password | password |
70129
| test3.cpp:241:2:241:6 | call to fgets | test3.cpp:241:8:241:15 | password | test3.cpp:241:8:241:15 | password | This operation receives into 'password', which may put unencrypted sensitive data into $@ | test3.cpp:241:8:241:15 | password | password |
130+
| test3.cpp:242:2:242:6 | call to fgets | test3.cpp:241:8:241:15 | password | test3.cpp:242:8:242:15 | password | This operation receives into 'password', which may put unencrypted sensitive data into $@ | test3.cpp:241:8:241:15 | password | password |
71131
| test3.cpp:242:2:242:6 | call to fgets | test3.cpp:242:8:242:15 | password | test3.cpp:242:8:242:15 | password | This operation receives into 'password', which may put unencrypted sensitive data into $@ | test3.cpp:242:8:242:15 | password | password |

‎cpp/ql/test/query-tests/Security/CWE/CWE-311/semmle/tests/test3.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -170,15 +170,15 @@ void test_decrypt()
170170
{
171171
char password[256];
172172

173-
recv(val(), password, 256, val()); // GOOD: password is encrypted [FALSE POSITIVE]
173+
recv(val(), password, 256, val()); // GOOD: password is encrypted
174174

175175
decrypt_inplace(password); // proof that `password` was in fact encrypted
176176
}
177177

178178
{
179179
char password[256];
180180

181-
recv(val(), password, 256, val()); // GOOD: password is encrypted [FALSE POSITIVE]
181+
recv(val(), password, 256, val()); // GOOD: password is encrypted
182182
password[255] = 0;
183183

184184
decrypt_inplace(password); // proof that `password` was in fact encrypted
@@ -188,7 +188,7 @@ void test_decrypt()
188188
char password[256];
189189
char *password_ptr;
190190

191-
recv(val(), password, 256, val()); // GOOD: password is encrypted [FALSE POSITIVE]
191+
recv(val(), password, 256, val()); // GOOD: password is encrypted
192192

193193
password_ptr = rtn_decrypt(password); // proof that `password` was in fact encrypted
194194
}
@@ -198,7 +198,7 @@ void test_decrypt()
198198

199199
encrypt_inplace(password); // proof that `password` is in fact encrypted
200200

201-
send(val(), password, strlen(password), val()); // GOOD: password is encrypted [FALSE POSITIVE]
201+
send(val(), password, strlen(password), val()); // GOOD: password is encrypted
202202
}
203203

204204
{
@@ -207,7 +207,7 @@ void test_decrypt()
207207
encrypt_inplace(password); // proof that `password` is in fact encrypted
208208
password[255] = 0;
209209

210-
send(val(), password, strlen(password), val()); // GOOD: password is encrypted [FALSE POSITIVE]
210+
send(val(), password, strlen(password), val()); // GOOD: password is encrypted
211211
}
212212

213213
{
@@ -216,7 +216,7 @@ void test_decrypt()
216216

217217
password_ptr = rtn_encrypt(password); // proof that `password` is in fact encrypted
218218

219-
send(val(), password_ptr, strlen(password_ptr), val()); // GOOD: password is encrypted [FALSE POSITIVE]
219+
send(val(), password_ptr, strlen(password_ptr), val()); // GOOD: password is encrypted
220220
}
221221
}
222222

0 commit comments

Comments
 (0)