Skip to content

Commit 07502a6

Browse files
MathiasVPMathiasVP
authored
Merge pull request #23019 from MathiasVP/replace-ql-models-with-mad-for-assoc-cleaned-up-step-2
C++: Fix a FN in `cpp/cleartext-storage-file`
2 parents 5780a0d + c1402e6 commit 07502a6

7 files changed

Lines changed: 131 additions & 66 deletions

File tree

‎cpp/ql/src/Security/CWE/CWE-311/ClearText.qll‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
private import cpp
2+
private import semmle.code.cpp.dataflow.new.DataFlow
3+
4+
/**
5+
* Holds if `node` should be a barrier for the CWE-311 queries
6+
* because it is of a non-char integral type.
7+
*/
8+
predicate isBarrier(DataFlow::Node node) {
9+
exists(IntegralType type |
10+
type = node.asCertainDefinition().getUnspecifiedType() and
11+
not type instanceof CharType and
12+
not type instanceof WideCharType and
13+
not type instanceof Char8Type and
14+
not type instanceof Char16Type and
15+
not type instanceof Char32Type
16+
)
17+
}
18+
19+
private Expr asExprOrIndirectExpr(DataFlow::Node node, int indirectionIndex) {
20+
indirectionIndex = 0 and
21+
result = node.asExpr()
22+
or
23+
result = node.asIndirectExpr(indirectionIndex)
24+
}
25+
26+
/**
27+
* Holds if `source` is the relevant `DataFlow::Node` for
28+
* expression `e`.
29+
*/
30+
predicate isSource(DataFlow::Node source, Expr e) {
31+
exists(int indirectionIndex |
32+
e = asExprOrIndirectExpr(source, indirectionIndex) and
33+
if e.getUnspecifiedType() instanceof ReferenceType
34+
then indirectionIndex = 1
35+
else indirectionIndex = 0
36+
)
37+
}

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import semmle.code.cpp.ir.dataflow.DataFlow
1919
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
2020
import semmle.code.cpp.ir.dataflow.TaintTracking
2121
import FromSensitiveFlow::PathGraph
22+
import ClearText as ClearText
2223

2324
/**
2425
* A taint flow configuration for flow from a sensitive expression to a `FileWrite` sink.
@@ -28,9 +29,7 @@ module FromSensitiveConfig implements DataFlow::ConfigSig {
2829

2930
predicate isSink(DataFlow::Node sink) { isSinkImpl(sink, _, _) }
3031

31-
predicate isBarrier(DataFlow::Node node) {
32-
node.asExpr().getUnspecifiedType() instanceof IntegralType
33-
}
32+
predicate isBarrier = ClearText::isBarrier/1;
3433

3534
predicate observeDiffInformedIncrementalMode() { any() }
3635

@@ -61,7 +60,7 @@ predicate isSinkImpl(DataFlow::Node sink, FileWrite w, Expr dest) {
6160

6261
predicate isSourceImpl(DataFlow::Node source, SensitiveExpr sensitive) {
6362
not isFileName(globalValueNumber(sensitive)) and // file names are not passwords
64-
source.asExpr() = sensitive
63+
ClearText::isSource(source, sensitive)
6564
}
6665

6766
/**

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

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import semmle.code.cpp.ir.dataflow.TaintTracking
1919
import semmle.code.cpp.models.interfaces.FlowSource
2020
import semmle.code.cpp.commons.File
2121
import FromSensitiveFlow::PathGraph
22+
import ClearText as ClearText
2223

2324
class SourceVariable extends Variable {
2425
SourceVariable() {
@@ -218,12 +219,12 @@ predicate isSinkEncrypt(DataFlow::Node sink, Encrypted enc) { sink.asExpr() = en
218219
* function returning sensitive data.
219220
*/
220221
predicate isSourceImpl(DataFlow::Node source) {
221-
exists(VariableAccess e |
222-
e = source.asExpr() and
223-
e.getTarget() instanceof SourceVariable
222+
exists(Expr e |
223+
e.(VariableAccess).getTarget() instanceof SourceVariable or
224+
e.(FunctionCall).getTarget() instanceof SourceFunction
225+
|
226+
ClearText::isSource(source, e)
224227
)
225-
or
226-
source.asExpr().(FunctionCall).getTarget() instanceof SourceFunction
227228
}
228229

229230
/**
@@ -235,9 +236,7 @@ module FromSensitiveConfig implements DataFlow::ConfigSig {
235236

236237
predicate isSink(DataFlow::Node sink) { isSinkSendRecv(sink, _) }
237238

238-
predicate isBarrier(DataFlow::Node node) {
239-
node.asExpr().getUnspecifiedType() instanceof IntegralType
240-
}
239+
predicate isBarrier = ClearText::isBarrier/1;
241240

242241
predicate isBarrierIn(DataFlow::Node node) {
243242
// As any use of a sensitive variable is a potential source, we need to block flow into
@@ -266,9 +265,7 @@ module ToEncryptionConfig implements DataFlow::ConfigSig {
266265

267266
predicate isSink(DataFlow::Node sink) { isSinkEncrypt(sink, _) }
268267

269-
predicate isBarrier(DataFlow::Node node) {
270-
node.asExpr().getUnspecifiedType() instanceof IntegralType
271-
}
268+
predicate isBarrier = ClearText::isBarrier/1;
272269

273270
predicate isBarrierIn(DataFlow::Node node) {
274271
// As any use of a sensitive variable is a potential source, we need to block flow into
@@ -291,9 +288,7 @@ module FromEncryptionConfig implements DataFlow::ConfigSig {
291288

292289
predicate isSink(DataFlow::Node sink) { FromSensitiveFlow::flowTo(sink) }
293290

294-
predicate isBarrier(DataFlow::Node node) {
295-
node.asExpr().getUnspecifiedType() instanceof IntegralType
296-
}
291+
predicate isBarrier = ClearText::isBarrier/1;
297292

298293
predicate observeDiffInformedIncrementalMode() {
299294
none() // only used negatively

‎cpp/ql/src/change-notes/2026-11-04-char-types-cwe-311.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/cleartext-transmission` and `cpp/cleartext-storage-file` queries now track flow through character types. As a result, these queries may find additional results.

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

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,24 +10,43 @@
1010
| test2.cpp:73:3:73:9 | call to fprintf | test2.cpp:72:15:72:24 | password | test2.cpp:73:30:73:32 | *buf | This write into file 'log' may contain unencrypted data from $@. | test2.cpp:72:17:72:24 | password | this source. |
1111
| test2.cpp:99:3:99:9 | call to fprintf | test2.cpp:98:45:98:52 | password | test2.cpp:99:27:99:32 | *buffer | This write into file 'log' may contain unencrypted data from $@. | test2.cpp:98:45:98:52 | password | this source. |
1212
| test2.cpp:154:2:154:8 | call to fprintf | test2.cpp:153:31:153:38 | password | test2.cpp:154:21:154:22 | *pw | This write into file 'log' may contain unencrypted data from $@. | test2.cpp:153:31:153:38 | password | this source. |
13-
| test2.cpp:174:2:174:8 | call to fprintf | test2.cpp:173:28:173:35 | password | test2.cpp:174:26:174:30 | *call to c_str | This write into file 'log' may contain unencrypted data from $@. | test2.cpp:173:28:173:35 | password | this source. |
13+
| test2.cpp:168:2:168:8 | call to fprintf | test2.cpp:167:41:167:60 | password | test2.cpp:168:30:168:34 | *call to c_str | This write into file 'log' may contain unencrypted data from $@. | test2.cpp:167:53:167:60 | password | this source. |
14+
| test2.cpp:174:2:174:8 | call to fprintf | test2.cpp:173:28:173:35 | *password | test2.cpp:174:26:174:30 | *call to c_str | This write into file 'log' may contain unencrypted data from $@. | test2.cpp:173:28:173:35 | password | this source. |
1415
| test.cpp:45:3:45:7 | call to fputs | test.cpp:45:9:45:19 | thePassword | test.cpp:45:9:45:19 | thePassword | This write into file 'file' may contain unencrypted data from $@. | test.cpp:45:9:45:19 | thePassword | this source. |
1516
| test.cpp:70:35:70:35 | call to operator<< | test.cpp:70:38:70:48 | thePassword | test.cpp:70:38:70:48 | thePassword | This write into file 'mystream' may contain unencrypted data from $@. | test.cpp:70:38:70:48 | thePassword | this source. |
1617
| test.cpp:73:37:73:41 | call to write | test.cpp:70:38:70:48 | thePassword | test.cpp:73:43:73:53 | thePassword | This write into file 'mystream' may contain unencrypted data from $@. | test.cpp:70:38:70:48 | thePassword | this source. |
1718
| test.cpp:73:37:73:41 | call to write | test.cpp:73:43:73:53 | thePassword | test.cpp:73:43:73:53 | thePassword | This write into file 'mystream' may contain unencrypted data from $@. | test.cpp:73:43:73:53 | thePassword | this source. |
1819
| test.cpp:73:37:73:41 | call to write | test.cpp:73:63:73:73 | thePassword | test.cpp:73:43:73:53 | thePassword | This write into file 'mystream' may contain unencrypted data from $@. | test.cpp:73:63:73:73 | thePassword | this source. |
1920
edges
21+
| file://:0:0:0:0 | * ... | test2.cpp:162:9:162:14 | result | provenance | TaintFunction |
22+
| file://:0:0:0:0 | *++ ... | file://:0:0:0:0 | * ... | provenance | |
23+
| file://:0:0:0:0 | ++ ... | file://:0:0:0:0 | * ... | provenance | |
24+
| file://:0:0:0:0 | ++ ... | file://:0:0:0:0 | *++ ... | provenance | |
2025
| test2.cpp:62:18:62:25 | password | test2.cpp:62:18:62:25 | password | provenance | |
2126
| test2.cpp:62:18:62:25 | password | test2.cpp:65:31:65:34 | cpy1 | provenance | |
2227
| test2.cpp:72:15:72:24 | password | test2.cpp:73:30:73:32 | *buf | provenance | DataFlowFunction |
2328
| test2.cpp:98:45:98:52 | password | test2.cpp:99:27:99:32 | *buffer | provenance | TaintFunction |
2429
| test2.cpp:153:31:153:38 | password | test2.cpp:153:40:153:44 | *call to c_str | provenance | TaintFunction |
2530
| test2.cpp:153:40:153:44 | *call to c_str | test2.cpp:154:21:154:22 | *pw | provenance | |
26-
| test2.cpp:173:28:173:35 | password | test2.cpp:173:28:173:35 | password | provenance | |
27-
| test2.cpp:173:28:173:35 | password | test2.cpp:174:26:174:30 | *call to c_str | provenance | TaintFunction |
31+
| test2.cpp:157:41:157:41 | s | test2.cpp:160:17:160:17 | *s | provenance | |
32+
| test2.cpp:160:17:160:17 | *s | test2.cpp:160:17:160:17 | call to begin | provenance | TaintFunction |
33+
| test2.cpp:160:17:160:17 | call to begin | file://:0:0:0:0 | * ... | provenance | |
34+
| test2.cpp:160:17:160:17 | call to begin | file://:0:0:0:0 | *++ ... | provenance | |
35+
| test2.cpp:160:17:160:17 | call to begin | file://:0:0:0:0 | ++ ... | provenance | |
36+
| test2.cpp:162:9:162:14 | result | test2.cpp:157:13:157:27 | *copy_characters | provenance | |
37+
| test2.cpp:167:25:167:39 | call to copy_characters | test2.cpp:167:25:167:39 | call to copy_characters | provenance | |
38+
| test2.cpp:167:25:167:39 | call to copy_characters | test2.cpp:168:30:168:34 | *call to c_str | provenance | TaintFunction |
39+
| test2.cpp:167:41:167:60 | password | test2.cpp:157:41:157:41 | s | provenance | |
40+
| test2.cpp:167:41:167:60 | password | test2.cpp:167:25:167:39 | call to copy_characters | provenance | TaintFunction |
41+
| test2.cpp:167:41:167:60 | password | test2.cpp:167:41:167:60 | password | provenance | |
42+
| test2.cpp:173:28:173:35 | *password | test2.cpp:173:28:173:35 | *password | provenance | |
43+
| test2.cpp:173:28:173:35 | *password | test2.cpp:174:26:174:30 | *call to c_str | provenance | TaintFunction |
2844
| test.cpp:70:38:70:48 | thePassword | test.cpp:73:43:73:53 | thePassword | provenance | |
2945
| test.cpp:73:63:73:73 | thePassword | test.cpp:73:43:73:53 | thePassword | provenance | |
3046
nodes
47+
| file://:0:0:0:0 | * ... | semmle.label | * ... |
48+
| file://:0:0:0:0 | *++ ... | semmle.label | *++ ... |
49+
| file://:0:0:0:0 | ++ ... | semmle.label | ++ ... |
3150
| test2.cpp:43:36:43:43 | password | semmle.label | password |
3251
| test2.cpp:44:37:44:45 | thepasswd | semmle.label | thepasswd |
3352
| test2.cpp:45:38:45:47 | accountkey | semmle.label | accountkey |
@@ -45,12 +64,23 @@ nodes
4564
| test2.cpp:153:31:153:38 | password | semmle.label | password |
4665
| test2.cpp:153:40:153:44 | *call to c_str | semmle.label | *call to c_str |
4766
| test2.cpp:154:21:154:22 | *pw | semmle.label | *pw |
48-
| test2.cpp:173:28:173:35 | password | semmle.label | password |
49-
| test2.cpp:173:28:173:35 | password | semmle.label | password |
67+
| test2.cpp:157:13:157:27 | *copy_characters | semmle.label | *copy_characters |
68+
| test2.cpp:157:41:157:41 | s | semmle.label | s |
69+
| test2.cpp:160:17:160:17 | *s | semmle.label | *s |
70+
| test2.cpp:160:17:160:17 | call to begin | semmle.label | call to begin |
71+
| test2.cpp:162:9:162:14 | result | semmle.label | result |
72+
| test2.cpp:167:25:167:39 | call to copy_characters | semmle.label | call to copy_characters |
73+
| test2.cpp:167:25:167:39 | call to copy_characters | semmle.label | call to copy_characters |
74+
| test2.cpp:167:41:167:60 | password | semmle.label | password |
75+
| test2.cpp:167:41:167:60 | password | semmle.label | password |
76+
| test2.cpp:168:30:168:34 | *call to c_str | semmle.label | *call to c_str |
77+
| test2.cpp:173:28:173:35 | *password | semmle.label | *password |
78+
| test2.cpp:173:28:173:35 | *password | semmle.label | *password |
5079
| test2.cpp:174:26:174:30 | *call to c_str | semmle.label | *call to c_str |
5180
| test.cpp:45:9:45:19 | thePassword | semmle.label | thePassword |
5281
| test.cpp:70:38:70:48 | thePassword | semmle.label | thePassword |
5382
| test.cpp:70:38:70:48 | thePassword | semmle.label | thePassword |
5483
| test.cpp:73:43:73:53 | thePassword | semmle.label | thePassword |
5584
| test.cpp:73:63:73:73 | thePassword | semmle.label | thePassword |
5685
subpaths
86+
| test2.cpp:167:41:167:60 | password | test2.cpp:157:41:157:41 | s | test2.cpp:157:13:157:27 | *copy_characters | test2.cpp:167:25:167:39 | call to copy_characters |

0 commit comments

Comments
 (0)