Skip to content

Commit b230681

Browse files
geoffw0geoffw0
authored
Merge pull request github#7650 from geoffw0/clrtxt3
C++: Improve cpp/cleartext-transmission
2 parents c09b669 + 8bdbaf4 commit b230681

File tree

6 files changed

+56
-17
lines changed

6 files changed

+56
-17
lines changed

‎cpp/ql/lib/semmle/code/cpp/models/implementations/Gets.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,4 +65,6 @@ private class GetsFunction extends DataFlowFunction, TaintFunction, ArrayFunctio
6565
}
6666

6767
override predicate hasArrayOutput(int bufParam) { bufParam = 0 }
68+
69+
override predicate hasSocketInput(FunctionInput input) { input.isParameter(2) }
6870
}

‎cpp/ql/lib/semmle/code/cpp/models/interfaces/FlowSource.qll

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,9 @@ abstract class RemoteFlowSourceFunction extends Function {
2020
abstract predicate hasRemoteFlowSource(FunctionOutput output, string description);
2121

2222
/**
23-
* Holds if remote data from this source comes from a socket described by
24-
* `input`. There is no result if a socket is not specified.
23+
* Holds if remote data from this source comes from a socket or stream
24+
* described by `input`. There is no result if none is specified by a
25+
* parameter.
2526
*/
2627
predicate hasSocketInput(FunctionInput input) { none() }
2728
}
@@ -59,8 +60,9 @@ abstract class RemoteFlowSinkFunction extends Function {
5960
abstract predicate hasRemoteFlowSink(FunctionInput input, string description);
6061

6162
/**
62-
* Holds if data put into this sink is transmitted through a socket described
63-
* by `input`. There is no result if a socket is not specified.
63+
* Holds if data put into this sink is transmitted through a socket or stream
64+
* described by `input`. There is no result if none is specified by a
65+
* parameter.
6466
*/
6567
predicate hasSocketInput(FunctionInput input) { none() }
6668
}

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

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,8 @@ class Recv extends SendRecv instanceof RemoteFlowSourceFunction {
105105
* practice it usually isn't very important which query reports a result as
106106
* long as its reported exactly once.
107107
*
108-
* We do exclude function calls that specify a constant socket, which is
109-
* likely to mean standard input, standard output or a similar channel.
108+
* We do exclude function calls that specify an apparently constant socket,
109+
* which is likely to mean standard input, standard output or a similar channel.
110110
*/
111111
abstract class NetworkSendRecv extends FunctionCall {
112112
SendRecv target;
@@ -125,6 +125,13 @@ abstract class NetworkSendRecv extends FunctionCall {
125125
v.getInitializer().getExpr() instanceof Literal and
126126
g = globalValueNumber(v.getAnAccess())
127127
)
128+
or
129+
// result of a function call with literal inputs (likely constant)
130+
exists(FunctionCall fc |
131+
forex(Expr arg | arg = fc.getAnArgument() | arg instanceof Literal) and
132+
g = globalValueNumber(fc)
133+
)
134+
// (this is far from exhaustive)
128135
)
129136
)
130137
}
@@ -147,13 +154,18 @@ class NetworkRecv extends NetworkSendRecv {
147154
}
148155

149156
/**
150-
* An expression that is an argument or return value from an encryption or
151-
* decryption call.
157+
* An expression that is an argument or return value from an encryption /
158+
* decryption call. This is quite inclusive to minimize false positives, for
159+
* example `SecureZeroMemory` is not an encryption routine but a clue that
160+
* encryption may be present.
152161
*/
153162
class Encrypted extends Expr {
154163
Encrypted() {
155164
exists(FunctionCall fc |
156-
fc.getTarget().getName().toLowerCase().regexpMatch(".*(crypt|encode|decode).*") and
165+
fc.getTarget()
166+
.getName()
167+
.toLowerCase()
168+
.regexpMatch(".*(crypt|encode|decode|hash|securezero).*") and
157169
(
158170
this = fc or
159171
this = fc.getAnArgument()

‎cpp/ql/src/change-notes/2022-01-19-cleartext-transmission.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 "Cleartext transmission of sensitive information" (`cpp/cleartext-transmission`) query has been improved in several ways to reduce false positive results.

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

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,8 @@ edges
3737
| test3.cpp:214:8:214:15 | password | test3.cpp:217:30:217:37 | password |
3838
| test3.cpp:214:8:214:15 | password | test3.cpp:219:15:219:26 | password_ptr |
3939
| test3.cpp:217:18:217:28 | call to rtn_encrypt | test3.cpp:219:15:219:26 | password_ptr |
40-
| test3.cpp:225:34:225:41 | password | test3.cpp:227:22:227:29 | password |
4140
| test3.cpp:225:34:225:41 | password | test3.cpp:228:26:228:33 | password |
4241
| test3.cpp:239:7:239:14 | password | test3.cpp:241:8:241:15 | password |
43-
| test3.cpp:239:7:239:14 | password | test3.cpp:242:8:242:15 | password |
4442
| test3.cpp:252:8:252:16 | password1 | test3.cpp:254:15:254:23 | password1 |
4543
| test3.cpp:252:8:252:16 | password1 | test3.cpp:256:3:256:19 | call to decrypt_to_buffer |
4644
| test3.cpp:252:8:252:16 | password1 | test3.cpp:256:21:256:29 | password1 |
@@ -84,6 +82,9 @@ edges
8482
| test3.cpp:350:9:350:16 | password | test3.cpp:352:16:352:23 | password |
8583
| test3.cpp:350:9:350:16 | password | test3.cpp:353:4:353:18 | call to decrypt_inplace |
8684
| test3.cpp:350:9:350:16 | password | test3.cpp:353:20:353:27 | password |
85+
| test3.cpp:366:8:366:15 | password | test3.cpp:368:15:368:22 | password |
86+
| test3.cpp:366:8:366:15 | password | test3.cpp:374:3:374:18 | call to SecureZeroBuffer |
87+
| test3.cpp:366:8:366:15 | password | test3.cpp:374:20:374:27 | password |
8788
| test.cpp:41:23:41:43 | cleartext password! | test.cpp:48:21:48:27 | call to encrypt |
8889
| test.cpp:41:23:41:43 | cleartext password! | test.cpp:48:29:48:39 | thePassword |
8990
| test.cpp:66:23:66:43 | cleartext password! | test.cpp:76:21:76:27 | call to encrypt |
@@ -144,11 +145,9 @@ nodes
144145
| test3.cpp:217:30:217:37 | password | semmle.label | password |
145146
| test3.cpp:219:15:219:26 | password_ptr | semmle.label | password_ptr |
146147
| test3.cpp:225:34:225:41 | password | semmle.label | password |
147-
| test3.cpp:227:22:227:29 | password | semmle.label | password |
148148
| test3.cpp:228:26:228:33 | password | semmle.label | password |
149149
| test3.cpp:239:7:239:14 | password | semmle.label | password |
150150
| test3.cpp:241:8:241:15 | password | semmle.label | password |
151-
| test3.cpp:242:8:242:15 | password | semmle.label | password |
152151
| test3.cpp:252:8:252:16 | password1 | semmle.label | password1 |
153152
| test3.cpp:252:24:252:32 | password2 | semmle.label | password2 |
154153
| test3.cpp:254:15:254:23 | password1 | semmle.label | password1 |
@@ -195,6 +194,10 @@ nodes
195194
| test3.cpp:352:16:352:23 | password | semmle.label | password |
196195
| test3.cpp:353:4:353:18 | call to decrypt_inplace | semmle.label | call to decrypt_inplace |
197196
| test3.cpp:353:20:353:27 | password | semmle.label | password |
197+
| test3.cpp:366:8:366:15 | password | semmle.label | password |
198+
| test3.cpp:368:15:368:22 | password | semmle.label | password |
199+
| test3.cpp:374:3:374:18 | call to SecureZeroBuffer | semmle.label | call to SecureZeroBuffer |
200+
| test3.cpp:374:20:374:27 | password | semmle.label | password |
198201
| test.cpp:41:23:41:43 | cleartext password! | semmle.label | cleartext password! |
199202
| test.cpp:48:21:48:27 | call to encrypt | semmle.label | call to encrypt |
200203
| test.cpp:48:29:48:39 | thePassword | semmle.label | thePassword |
@@ -218,10 +221,8 @@ subpaths
218221
| test3.cpp:140:3:140:6 | call to send | test3.cpp:129:39:129:47 | password1 | test3.cpp:140:15:140:17 | ptr | This operation transmits 'ptr', which may contain unencrypted sensitive data from $@ | test3.cpp:129:39:129:47 | password1 | password1 |
219222
| 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 |
220223
| test3.cpp:159:3:159:6 | call to send | test3.cpp:152:29:152:36 | password | test3.cpp:159:15:159:20 | buffer | This operation transmits 'buffer', which may contain unencrypted sensitive data from $@ | test3.cpp:152:29:152:36 | password | password |
221-
| test3.cpp:227:2:227:5 | call to send | test3.cpp:225:34:225:41 | password | test3.cpp:227:22:227:29 | password | This operation transmits 'password', which may contain unencrypted sensitive data from $@ | test3.cpp:225:34:225:41 | password | password |
222224
| test3.cpp:228:2:228:5 | call to send | test3.cpp:225:34:225:41 | password | test3.cpp:228:26:228:33 | password | This operation transmits 'password', which may contain unencrypted sensitive data from $@ | test3.cpp:225:34:225:41 | password | password |
223225
| test3.cpp:241:2:241:6 | call to fgets | test3.cpp:239:7:239:14 | password | test3.cpp:241:8:241:15 | password | This operation receives into 'password', which may put unencrypted sensitive data into $@ | test3.cpp:239:7:239:14 | password | password |
224-
| test3.cpp:242:2:242:6 | call to fgets | test3.cpp:239:7:239:14 | password | test3.cpp:242:8:242:15 | password | This operation receives into 'password', which may put unencrypted sensitive data into $@ | test3.cpp:239:7:239:14 | password | password |
225226
| test3.cpp:272:3:272:6 | call to send | test3.cpp:268:19:268:26 | password | test3.cpp:272:15:272:18 | data | This operation transmits 'data', which may contain unencrypted sensitive data from $@ | test3.cpp:268:19:268:26 | password | password |
226227
| test3.cpp:295:2:295:5 | call to send | test3.cpp:308:58:308:66 | password2 | test3.cpp:295:14:295:17 | data | This operation transmits 'data', which may contain unencrypted sensitive data from $@ | test3.cpp:308:58:308:66 | password2 | password2 |
227228
| test3.cpp:300:2:300:5 | call to send | test3.cpp:308:58:308:66 | password2 | test3.cpp:300:14:300:17 | data | This operation transmits 'data', which may contain unencrypted sensitive data from $@ | test3.cpp:308:58:308:66 | password2 | password2 |

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

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ int get_socket(int from);
224224

225225
void test_more_stdio(const char *password)
226226
{
227-
send(get_socket(1), password, 128, val()); // GOOD: `getsocket(1)` is probably standard output [FALSE POSITIVE]
227+
send(get_socket(1), password, 128, val()); // GOOD: `getsocket(1)` is probably standard output
228228
send(get_socket(val()), password, 128, val()); // BAD
229229
}
230230

@@ -239,7 +239,7 @@ void test_fgets(FILE *stream)
239239
char password[128];
240240

241241
fgets(password, 128, stream); // BAD
242-
fgets(password, 128, STDIN_STREAM); // GOOD: `STDIN_STREAM` is probably standard input [FALSE POSITIVE]
242+
fgets(password, 128, STDIN_STREAM); // GOOD: `STDIN_STREAM` is probably standard input
243243
}
244244

245245
void encrypt_to_buffer(const char *input, char* output);
@@ -356,3 +356,21 @@ void test_loops()
356356
}
357357
}
358358
}
359+
360+
void DoDisguisedOperation(char *buffer, size_t size);
361+
void SecureZeroBuffer(char *buffer, size_t size);
362+
363+
void test_securezero()
364+
{
365+
{
366+
char password[256];
367+
368+
recv(val(), password, 256, val()); // GOOD: password is (probably) encrypted
369+
370+
DoDisguisedOperation(password, 256); // decryption (disguised)
371+
372+
// ...
373+
374+
SecureZeroBuffer(password, 256); // evidence we may have been doing decryption
375+
}
376+
}

0 commit comments

Comments
 (0)