Skip to content

Commit 90f24d3

Browse files
MathiasVPMathiasVP
authored
Merge pull request #10430 from geoffw0/cleartextmissing
Swift: Fix missing results in swift/cleartext-storage-database
2 parents 30b54b2 + 3573dd6 commit 90f24d3

File tree

4 files changed

+62
-36
lines changed

4 files changed

+62
-36
lines changed

‎swift/ql/src/queries/Security/CWE-311/CleartextStorageDatabase.ql

Lines changed: 29 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,12 @@ import codeql.swift.dataflow.TaintTracking
1818
import DataFlow::PathGraph
1919

2020
/**
21-
* An `Expr` that is stored in a local database.
21+
* A `DataFlow::Node` that is something stored in a local database.
2222
*/
23-
abstract class Stored extends Expr { }
23+
abstract class Stored extends DataFlow::Node { }
2424

2525
/**
26-
* An `Expr` that is stored with the Core Data library.
26+
* A `DataFlow::Node` that is an expression stored with the Core Data library.
2727
*/
2828
class CoreDataStore extends Stored {
2929
CoreDataStore() {
@@ -33,32 +33,25 @@ class CoreDataStore extends Stored {
3333
c.getAMember() = f and
3434
f.getName() = ["setValue(_:forKey:)", "setPrimitiveValue(_:forKey:)"] and
3535
call.getStaticTarget() = f and
36-
call.getArgument(0).getExpr() = this
36+
call.getArgument(0).getExpr() = this.asExpr()
3737
)
3838
}
3939
}
4040

4141
/**
42-
* An `Expr` that is stored with the Realm database library.
42+
* A `DataFlow::Node` that is an expression stored with the Realm database
43+
* library.
4344
*/
44-
class RealmStore extends Stored {
45+
class RealmStore extends Stored instanceof DataFlow::PostUpdateNode {
4546
RealmStore() {
46-
// `object` arg to `Realm.add` is a sink
47-
exists(ClassDecl c, AbstractFunctionDecl f, CallExpr call |
48-
c.getName() = "Realm" and
49-
c.getAMember() = f and
50-
f.getName() = "add(_:update:)" and
51-
call.getStaticTarget() = f and
52-
call.getArgument(0).getExpr() = this
53-
)
54-
or
55-
// `value` arg to `Realm.create` is a sink
56-
exists(ClassDecl c, AbstractFunctionDecl f, CallExpr call |
57-
c.getName() = "Realm" and
58-
c.getAMember() = f and
59-
f.getName() = "create(_:value:update:)" and
60-
call.getStaticTarget() = f and
61-
call.getArgument(1).getExpr() = this
47+
// any write into a class derived from `RealmSwiftObject` is a sink. For
48+
// example in `realmObj.data = sensitive` the post-update node corresponding
49+
// with `realmObj.data` is a sink.
50+
exists(ClassDecl cd, Expr e |
51+
cd.getABaseTypeDecl*().getName() = "RealmSwiftObject" and
52+
this.getPreUpdateNode().asExpr() = e and
53+
e.getFullyConverted().getType() = cd.getType() and
54+
not e.(DeclRefExpr).getDecl() instanceof SelfParamDecl
6255
)
6356
}
6457
}
@@ -72,7 +65,7 @@ class CleartextStorageConfig extends TaintTracking::Configuration {
7265

7366
override predicate isSource(DataFlow::Node node) { node.asExpr() instanceof SensitiveExpr }
7467

75-
override predicate isSink(DataFlow::Node node) { node.asExpr() instanceof Stored }
68+
override predicate isSink(DataFlow::Node node) { node instanceof Stored }
7669

7770
override predicate isSanitizerIn(DataFlow::Node node) {
7871
// make sources barriers so that we only report the closest instance
@@ -85,7 +78,8 @@ class CleartextStorageConfig extends TaintTracking::Configuration {
8578
}
8679

8780
override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
88-
// flow out from fields of a `RealmSwiftObject` at the sink, for example in `obj.var = tainted; sink(obj)`.
81+
// flow out from fields of a `RealmSwiftObject` at the sink, for example in
82+
// `realmObj.data = sensitive`.
8983
isSink(node) and
9084
exists(ClassDecl cd |
9185
c.getAReadContent().(DataFlow::Content::FieldContent).getField() = cd.getAMember() and
@@ -97,9 +91,19 @@ class CleartextStorageConfig extends TaintTracking::Configuration {
9791
}
9892
}
9993

94+
/**
95+
* Gets a prettier node to use in the results.
96+
*/
97+
DataFlow::Node cleanupNode(DataFlow::Node n) {
98+
result = n.(DataFlow::PostUpdateNode).getPreUpdateNode()
99+
or
100+
not n instanceof DataFlow::PostUpdateNode and
101+
result = n
102+
}
103+
100104
from CleartextStorageConfig config, DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode
101105
where config.hasFlowPath(sourceNode, sinkNode)
102-
select sinkNode.getNode(), sourceNode, sinkNode,
106+
select cleanupNode(sinkNode.getNode()), sourceNode, sinkNode,
103107
"This operation stores '" + sinkNode.getNode().toString() +
104108
"' in a database. It may contain unencrypted sensitive data from $@", sourceNode,
105109
sourceNode.getNode().toString()

‎swift/ql/test/query-tests/Security/CWE-311/CleartextStorageDatabase.expected

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,18 @@ edges
1010
| testCoreData.swift:92:10:92:10 | passwd : | testCoreData.swift:96:15:96:15 | y |
1111
| testCoreData.swift:93:10:93:10 | passwd : | testCoreData.swift:97:15:97:15 | z |
1212
| testRealm.swift:16:6:16:6 | value : | file://:0:0:0:0 | value : |
13-
| testRealm.swift:34:2:34:2 | [post] a [data] : | testRealm.swift:35:12:35:12 | a |
13+
| testRealm.swift:34:2:34:2 | [post] a [data] : | testRealm.swift:34:2:34:2 | [post] a |
1414
| testRealm.swift:34:11:34:11 | myPassword : | testRealm.swift:16:6:16:6 | value : |
1515
| testRealm.swift:34:11:34:11 | myPassword : | testRealm.swift:34:2:34:2 | [post] a [data] : |
16-
| testRealm.swift:42:2:42:2 | [post] c [data] : | testRealm.swift:43:47:43:47 | c |
16+
| testRealm.swift:42:2:42:2 | [post] c [data] : | testRealm.swift:42:2:42:2 | [post] c |
1717
| testRealm.swift:42:11:42:11 | myPassword : | testRealm.swift:16:6:16:6 | value : |
1818
| testRealm.swift:42:11:42:11 | myPassword : | testRealm.swift:42:2:42:2 | [post] c [data] : |
19+
| testRealm.swift:52:2:52:3 | [post] ...! [data] : | testRealm.swift:52:2:52:3 | [post] ...! |
20+
| testRealm.swift:52:12:52:12 | myPassword : | testRealm.swift:16:6:16:6 | value : |
21+
| testRealm.swift:52:12:52:12 | myPassword : | testRealm.swift:52:2:52:3 | [post] ...! [data] : |
22+
| testRealm.swift:59:2:59:2 | [post] g [data] : | testRealm.swift:59:2:59:2 | [post] g |
23+
| testRealm.swift:59:11:59:11 | myPassword : | testRealm.swift:16:6:16:6 | value : |
24+
| testRealm.swift:59:11:59:11 | myPassword : | testRealm.swift:59:2:59:2 | [post] g [data] : |
1925
nodes
2026
| file://:0:0:0:0 | [post] self [data] : | semmle.label | [post] self [data] : |
2127
| file://:0:0:0:0 | value : | semmle.label | value : |
@@ -40,15 +46,23 @@ nodes
4046
| testCoreData.swift:96:15:96:15 | y | semmle.label | y |
4147
| testCoreData.swift:97:15:97:15 | z | semmle.label | z |
4248
| testRealm.swift:16:6:16:6 | value : | semmle.label | value : |
49+
| testRealm.swift:34:2:34:2 | [post] a | semmle.label | [post] a |
4350
| testRealm.swift:34:2:34:2 | [post] a [data] : | semmle.label | [post] a [data] : |
4451
| testRealm.swift:34:11:34:11 | myPassword : | semmle.label | myPassword : |
45-
| testRealm.swift:35:12:35:12 | a | semmle.label | a |
52+
| testRealm.swift:42:2:42:2 | [post] c | semmle.label | [post] c |
4653
| testRealm.swift:42:2:42:2 | [post] c [data] : | semmle.label | [post] c [data] : |
4754
| testRealm.swift:42:11:42:11 | myPassword : | semmle.label | myPassword : |
48-
| testRealm.swift:43:47:43:47 | c | semmle.label | c |
55+
| testRealm.swift:52:2:52:3 | [post] ...! | semmle.label | [post] ...! |
56+
| testRealm.swift:52:2:52:3 | [post] ...! [data] : | semmle.label | [post] ...! [data] : |
57+
| testRealm.swift:52:12:52:12 | myPassword : | semmle.label | myPassword : |
58+
| testRealm.swift:59:2:59:2 | [post] g | semmle.label | [post] g |
59+
| testRealm.swift:59:2:59:2 | [post] g [data] : | semmle.label | [post] g [data] : |
60+
| testRealm.swift:59:11:59:11 | myPassword : | semmle.label | myPassword : |
4961
subpaths
5062
| testRealm.swift:34:11:34:11 | myPassword : | testRealm.swift:16:6:16:6 | value : | file://:0:0:0:0 | [post] self [data] : | testRealm.swift:34:2:34:2 | [post] a [data] : |
5163
| testRealm.swift:42:11:42:11 | myPassword : | testRealm.swift:16:6:16:6 | value : | file://:0:0:0:0 | [post] self [data] : | testRealm.swift:42:2:42:2 | [post] c [data] : |
64+
| testRealm.swift:52:12:52:12 | myPassword : | testRealm.swift:16:6:16:6 | value : | file://:0:0:0:0 | [post] self [data] : | testRealm.swift:52:2:52:3 | [post] ...! [data] : |
65+
| testRealm.swift:59:11:59:11 | myPassword : | testRealm.swift:16:6:16:6 | value : | file://:0:0:0:0 | [post] self [data] : | testRealm.swift:59:2:59:2 | [post] g [data] : |
5266
#select
5367
| testCoreData.swift:19:12:19:12 | value | testCoreData.swift:61:25:61:25 | password : | testCoreData.swift:19:12:19:12 | value | This operation stores 'value' in a database. It may contain unencrypted sensitive data from $@ | testCoreData.swift:61:25:61:25 | password : | password |
5468
| testCoreData.swift:32:13:32:13 | newValue | testCoreData.swift:64:16:64:16 | password : | testCoreData.swift:32:13:32:13 | newValue | This operation stores 'newValue' in a database. It may contain unencrypted sensitive data from $@ | testCoreData.swift:64:16:64:16 | password : | password |
@@ -61,5 +75,7 @@ subpaths
6175
| testCoreData.swift:95:15:95:15 | x | testCoreData.swift:91:10:91:10 | passwd : | testCoreData.swift:95:15:95:15 | x | This operation stores 'x' in a database. It may contain unencrypted sensitive data from $@ | testCoreData.swift:91:10:91:10 | passwd : | passwd |
6276
| testCoreData.swift:96:15:96:15 | y | testCoreData.swift:92:10:92:10 | passwd : | testCoreData.swift:96:15:96:15 | y | This operation stores 'y' in a database. It may contain unencrypted sensitive data from $@ | testCoreData.swift:92:10:92:10 | passwd : | passwd |
6377
| testCoreData.swift:97:15:97:15 | z | testCoreData.swift:93:10:93:10 | passwd : | testCoreData.swift:97:15:97:15 | z | This operation stores 'z' in a database. It may contain unencrypted sensitive data from $@ | testCoreData.swift:93:10:93:10 | passwd : | passwd |
64-
| testRealm.swift:35:12:35:12 | a | testRealm.swift:34:11:34:11 | myPassword : | testRealm.swift:35:12:35:12 | a | This operation stores 'a' in a database. It may contain unencrypted sensitive data from $@ | testRealm.swift:34:11:34:11 | myPassword : | myPassword |
65-
| testRealm.swift:43:47:43:47 | c | testRealm.swift:42:11:42:11 | myPassword : | testRealm.swift:43:47:43:47 | c | This operation stores 'c' in a database. It may contain unencrypted sensitive data from $@ | testRealm.swift:42:11:42:11 | myPassword : | myPassword |
78+
| testRealm.swift:34:2:34:2 | a | testRealm.swift:34:11:34:11 | myPassword : | testRealm.swift:34:2:34:2 | [post] a | This operation stores '[post] a' in a database. It may contain unencrypted sensitive data from $@ | testRealm.swift:34:11:34:11 | myPassword : | myPassword |
79+
| testRealm.swift:42:2:42:2 | c | testRealm.swift:42:11:42:11 | myPassword : | testRealm.swift:42:2:42:2 | [post] c | This operation stores '[post] c' in a database. It may contain unencrypted sensitive data from $@ | testRealm.swift:42:11:42:11 | myPassword : | myPassword |
80+
| testRealm.swift:52:2:52:3 | ...! | testRealm.swift:52:12:52:12 | myPassword : | testRealm.swift:52:2:52:3 | [post] ...! | This operation stores '[post] ...!' in a database. It may contain unencrypted sensitive data from $@ | testRealm.swift:52:12:52:12 | myPassword : | myPassword |
81+
| testRealm.swift:59:2:59:2 | g | testRealm.swift:59:11:59:11 | myPassword : | testRealm.swift:59:2:59:2 | [post] g | This operation stores '[post] g' in a database. It may contain unencrypted sensitive data from $@ | testRealm.swift:59:11:59:11 | myPassword : | myPassword |

‎swift/ql/test/query-tests/Security/CWE-311/SensitiveExprs.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
| testRealm.swift:34:11:34:11 | myPassword | label:myPassword, type:credential |
1414
| testRealm.swift:42:11:42:11 | myPassword | label:myPassword, type:credential |
1515
| testRealm.swift:52:12:52:12 | myPassword | label:myPassword, type:credential |
16+
| testRealm.swift:59:11:59:11 | myPassword | label:myPassword, type:credential |
1617
| testSend.swift:29:19:29:19 | passwordPlain | label:passwordPlain, type:credential |
1718
| testSend.swift:33:19:33:19 | passwordPlain | label:passwordPlain, type:credential |
1819
| testSend.swift:45:13:45:13 | password | label:password, type:credential |

‎swift/ql/test/query-tests/Security/CWE-311/testRealm.swift

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,16 +31,16 @@ func test1(realm : Realm, myPassword : String, myHashedPassword : String) {
3131
// add objects (within a transaction) ...
3232

3333
let a = MyRealmSwiftObject()
34-
a.data = myPassword
35-
realm.add(a) // BAD
34+
a.data = myPassword // BAD
35+
realm.add(a)
3636

3737
let b = MyRealmSwiftObject()
3838
b.data = myHashedPassword
3939
realm.add(b) // GOOD (not sensitive)
4040

4141
let c = MyRealmSwiftObject()
42-
c.data = myPassword
43-
realm.create(MyRealmSwiftObject.self, value: c) // BAD
42+
c.data = myPassword // BAD
43+
realm.create(MyRealmSwiftObject.self, value: c)
4444

4545
let d = MyRealmSwiftObject()
4646
d.data = myHashedPassword
@@ -49,10 +49,15 @@ func test1(realm : Realm, myPassword : String, myHashedPassword : String) {
4949
// retrieve objects ...
5050

5151
var e = realm.object(ofType: MyRealmSwiftObject.self, forPrimaryKey: "key")
52-
e!.data = myPassword // BAD [NOT DETECTED]
52+
e!.data = myPassword // BAD
5353

5454
var f = realm.object(ofType: MyRealmSwiftObject.self, forPrimaryKey: "key")
5555
f!.data = myHashedPassword // GOOD (not sensitive)
56+
57+
let g = MyRealmSwiftObject()
58+
g.data = "" // GOOD (not sensitive)
59+
g.data = myPassword // BAD
60+
g.data = "" // GOOD (not sensitive)
5661
}
5762

5863
// limitation: its possible to configure a Realm DB to be stored encrypted, if this is done correctly

0 commit comments

Comments
 (0)