Skip to content

Commit 6074f22

Browse files
MathiasVPMathiasVP
authored
Merge pull request #10335 from github/redsun82/swift-weak-hashing-phase-1
Swift: first version of query targeting weak hashing
2 parents 74eb6b2 + c3320a3 commit 6074f22

13 files changed

+410
-54
lines changed

‎swift/ql/lib/codeql/swift/elements/AstNode.qll

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,30 @@
11
private import codeql.swift.generated.AstNode
22
private import codeql.swift.elements.decl.AbstractFunctionDecl
3+
private import codeql.swift.elements.decl.Decl
34
private import codeql.swift.generated.ParentChild
45

5-
private Element getEnclosingFunctionStep(Element e) {
6-
not e instanceof AbstractFunctionDecl and
7-
result = getImmediateParent(e)
8-
}
6+
private module Cached {
7+
private Element getEnclosingDeclStep(Element e) {
8+
not e instanceof Decl and
9+
result = getImmediateParent(e)
10+
}
11+
12+
cached
13+
Decl getEnclosingDecl(AstNode ast) { result = getEnclosingDeclStep*(getImmediateParent(ast)) }
914

10-
cached
11-
private AbstractFunctionDecl getEnclosingFunctionCached(AstNode ast) {
12-
result = getEnclosingFunctionStep*(getImmediateParent(ast))
15+
private Element getEnclosingFunctionStep(Element e) {
16+
not e instanceof AbstractFunctionDecl and
17+
result = getEnclosingDecl(e)
18+
}
19+
20+
cached
21+
AbstractFunctionDecl getEnclosingFunction(AstNode ast) {
22+
result = getEnclosingFunctionStep*(getEnclosingDecl(ast))
23+
}
1324
}
1425

1526
class AstNode extends AstNodeBase {
16-
final AbstractFunctionDecl getEnclosingFunction() { result = getEnclosingFunctionCached(this) }
27+
final AbstractFunctionDecl getEnclosingFunction() { result = Cached::getEnclosingFunction(this) }
28+
29+
final Decl getEnclosingDecl() { result = Cached::getEnclosingDecl(this) }
1730
}

‎swift/ql/lib/codeql/swift/security/SensitiveExprs.qll

Lines changed: 18 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -121,20 +121,24 @@ class SensitiveExpr extends Expr {
121121
SensitiveDataType sensitiveType;
122122

123123
SensitiveExpr() {
124-
// variable reference
125-
this.(DeclRefExpr).getDecl().(SensitiveVarDecl).hasInfo(label, sensitiveType)
126-
or
127-
// member variable reference
128-
this.(MemberRefExpr).getMember().(SensitiveVarDecl).hasInfo(label, sensitiveType)
129-
or
130-
// function call
131-
this.(ApplyExpr).getStaticTarget().(SensitiveFunctionDecl).hasInfo(label, sensitiveType)
132-
or
133-
// sensitive argument
134-
exists(SensitiveArgument a |
135-
a.hasInfo(label, sensitiveType) and
136-
a.getExpr() = this
137-
)
124+
(
125+
// variable reference
126+
this.(DeclRefExpr).getDecl().(SensitiveVarDecl).hasInfo(label, sensitiveType)
127+
or
128+
// member variable reference
129+
this.(MemberRefExpr).getMember().(SensitiveVarDecl).hasInfo(label, sensitiveType)
130+
or
131+
// function call
132+
this.(ApplyExpr).getStaticTarget().(SensitiveFunctionDecl).hasInfo(label, sensitiveType)
133+
or
134+
// sensitive argument
135+
exists(SensitiveArgument a |
136+
a.hasInfo(label, sensitiveType) and
137+
a.getExpr() = this
138+
)
139+
) and
140+
// do not mark as sensitive it if it is probably safe
141+
not label.toLowerCase().regexpMatch(regexpProbablySafe())
138142
}
139143

140144
/**
@@ -146,13 +150,6 @@ class SensitiveExpr extends Expr {
146150
* Gets the type of sensitive expression this is.
147151
*/
148152
SensitiveDataType getSensitiveType() { result = sensitiveType }
149-
150-
/**
151-
* Holds if this sensitive expression might be safe because it contains
152-
* hashed or encrypted data, or is only a reference to data that is stored
153-
* elsewhere.
154-
*/
155-
predicate isProbablySafe() { label.toLowerCase().regexpMatch(regexpProbablySafe()) }
156153
}
157154

158155
/**

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

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -70,12 +70,7 @@ class RealmStore extends Stored {
7070
class CleartextStorageConfig extends TaintTracking::Configuration {
7171
CleartextStorageConfig() { this = "CleartextStorageConfig" }
7272

73-
override predicate isSource(DataFlow::Node node) {
74-
exists(SensitiveExpr e |
75-
node.asExpr() = e and
76-
not e.isProbablySafe()
77-
)
78-
}
73+
override predicate isSource(DataFlow::Node node) { node.asExpr() instanceof SensitiveExpr }
7974

8075
override predicate isSink(DataFlow::Node node) { node.asExpr() instanceof Stored }
8176

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

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -63,12 +63,7 @@ class URL extends Transmitted {
6363
class CleartextTransmissionConfig extends TaintTracking::Configuration {
6464
CleartextTransmissionConfig() { this = "CleartextTransmissionConfig" }
6565

66-
override predicate isSource(DataFlow::Node node) {
67-
exists(SensitiveExpr e |
68-
node.asExpr() = e and
69-
not e.isProbablySafe()
70-
)
71-
}
66+
override predicate isSource(DataFlow::Node node) { node.asExpr() instanceof SensitiveExpr }
7267

7368
override predicate isSink(DataFlow::Node node) { node.asExpr() instanceof Transmitted }
7469

‎swift/ql/src/queries/Security/CWE-328/WeakSensitiveDataHashing.qhelp

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
Using a broken or weak cryptographic hash function can leave data
8+
vulnerable, and should not be used in security related code.
9+
</p>
10+
11+
<p>
12+
A strong cryptographic hash function should be resistant to:
13+
</p>
14+
<ul>
15+
<li>
16+
<b>Pre-image attacks</b>. If you know a hash value <code>h(x)</code>,
17+
you should not be able to easily find the input <code>x</code>.
18+
</li>
19+
<li>
20+
<b>Collision attacks</b>. If you know a hash value <code>h(x)</code>,
21+
you should not be able to easily find a different input
22+
<code>y</code>
23+
with the same hash value <code>h(x) = h(y)</code>.
24+
</li>
25+
</ul>
26+
27+
<p>
28+
As an example, both MD5 and SHA-1 are known to be vulnerable to collision attacks.
29+
</p>
30+
31+
<p>
32+
Since it's OK to use a weak cryptographic hash function in a non-security
33+
context, this query only alerts when these are used to hash sensitive
34+
data (such as passwords, certificates, usernames).
35+
</p>
36+
37+
</overview>
38+
<recommendation>
39+
40+
<p>
41+
Ensure that you use a strong, modern cryptographic hash function, such as:
42+
</p>
43+
44+
<ul>
45+
<li>
46+
Argon2, scrypt, bcrypt, or PBKDF2 for passwords and other data with limited input space where
47+
a dictionary-like attack is feasible.
48+
</li>
49+
<li>
50+
SHA-2, or SHA-3 in other cases.
51+
</li>
52+
</ul>
53+
54+
</recommendation>
55+
<example>
56+
57+
<p>
58+
The following examples show a function for checking whether the hash
59+
of a certificate matches a known value -- to prevent tampering.
60+
61+
In the first case the MD5 hashing algorithm is used that is known to be vulnerable to collision attacks.
62+
</p>
63+
<sample src="WeakSensitiveDataHashingBad.swift"/>
64+
<p>
65+
66+
Here is the same function using SHA-512, which is a strong cryptographic hashing function.
67+
</p>
68+
<sample src="WeakSensitiveDataHashingGood.swift"/>
69+
70+
</example>
71+
<references>
72+
<li>
73+
OWASP:
74+
<a href="https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html">Password Storage
75+
Cheat Sheet
76+
</a>
77+
and
78+
<a href="https://cheatsheetseries.owasp.org/cheatsheets/Transport_Layer_Protection_Cheat_Sheet.html#use-strong-cryptographic-hashing-algorithms">
79+
Transport Layer Protection Cheat Sheet
80+
</a>
81+
</li>
82+
</references>
83+
84+
</qhelp>

‎swift/ql/src/queries/Security/CWE-328/WeakSensitiveDataHashing.ql

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
/**
2+
* @name Use of a broken or weak cryptographic hashing algorithm on sensitive data
3+
* @description Using broken or weak cryptographic hashing algorithms can compromise security.
4+
* @kind path-problem
5+
* @problem.severity warning
6+
* @security-severity 7.5
7+
* @precision high
8+
* @id swift/weak-sensitive-data-hashing
9+
* @tags security
10+
* external/cwe/cwe-327
11+
* external/cwe/cwe-328
12+
*/
13+
14+
import swift
15+
import codeql.swift.security.SensitiveExprs
16+
import codeql.swift.dataflow.DataFlow
17+
import codeql.swift.dataflow.TaintTracking
18+
import DataFlow::PathGraph
19+
20+
class WeakHashingConfig extends TaintTracking::Configuration {
21+
WeakHashingConfig() { this = "WeakHashingConfig" }
22+
23+
override predicate isSource(DataFlow::Node node) { node instanceof WeakHashingConfig::Source }
24+
25+
override predicate isSink(DataFlow::Node node) { node instanceof WeakHashingConfig::Sink }
26+
}
27+
28+
module WeakHashingConfig {
29+
class Source extends DataFlow::Node {
30+
Source() { this.asExpr() instanceof SensitiveExpr }
31+
}
32+
33+
abstract class Sink extends DataFlow::Node {
34+
abstract string getAlgorithm();
35+
}
36+
37+
class CryptoHash extends Sink {
38+
string algorithm;
39+
40+
CryptoHash() {
41+
exists(ApplyExpr call, FuncDecl func |
42+
call.getAnArgument().getExpr() = this.asExpr() and
43+
call.getStaticTarget() = func and
44+
func.getName().matches(["hash(%", "update(%"]) and
45+
algorithm = func.getEnclosingDecl().(StructDecl).getName() and
46+
algorithm = ["MD5", "SHA1"]
47+
)
48+
}
49+
50+
override string getAlgorithm() { result = algorithm }
51+
}
52+
}
53+
54+
from
55+
WeakHashingConfig config, DataFlow::PathNode source, DataFlow::PathNode sink, string algorithm,
56+
SensitiveExpr expr
57+
where
58+
config.hasFlowPath(source, sink) and
59+
algorithm = sink.getNode().(WeakHashingConfig::Sink).getAlgorithm() and
60+
expr = source.getNode().asExpr()
61+
select sink.getNode(), source, sink,
62+
"Insecure hashing algorithm (" + algorithm + ") depends on $@.", source.getNode(),
63+
"sensitive data (" + expr.getSensitiveType() + " " + expr.getLabel() + ")"

‎swift/ql/src/queries/Security/CWE-328/WeakSensitiveDataHashingBad.swift

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
typealias Hasher = Crypto.Insecure.MD5
2+
3+
func checkCertificate(cert: Array[UInt8], hash: Array[UInt8]) -> Bool
4+
return Hasher.hash(data: cert) == hash // BAD
5+
}

‎swift/ql/src/queries/Security/CWE-328/WeakSensitiveDataHashingGood.swift

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
typealias Hasher = Crypto.SHA512
2+
3+
func checkCertificate(cert: Array[UInt8], hash: Array[UInt8]) -> Bool
4+
return Hasher.hash(data: cert) == hash // GOOD

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

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,8 @@
11
| testCoreData.swift:48:15:48:15 | password | label:password, type:credential |
2-
| testCoreData.swift:49:15:49:15 | password_hash | isProbablySafe, label:password_hash, type:credential |
32
| testCoreData.swift:51:24:51:24 | password | label:password, type:credential |
4-
| testCoreData.swift:52:24:52:24 | password_hash | isProbablySafe, label:password_hash, type:credential |
53
| testCoreData.swift:58:15:58:15 | password | label:password, type:credential |
6-
| testCoreData.swift:59:15:59:15 | password_file | isProbablySafe, label:password_file, type:credential |
74
| testCoreData.swift:61:25:61:25 | password | label:password, type:credential |
8-
| testCoreData.swift:62:25:62:25 | password_file | isProbablySafe, label:password_file, type:credential |
95
| testCoreData.swift:64:16:64:16 | password | label:password, type:credential |
10-
| testCoreData.swift:65:16:65:16 | password_file | isProbablySafe, label:password_file, type:credential |
116
| testCoreData.swift:77:2:77:25 | call to doSomething(password:) | label:doSomething(password:), type:credential |
127
| testCoreData.swift:77:24:77:24 | x | label:password, type:credential |
138
| testCoreData.swift:80:10:80:22 | call to getPassword() | label:getPassword(), type:credential |
@@ -16,22 +11,16 @@
1611
| testCoreData.swift:92:10:92:10 | passwd | label:passwd, type:credential |
1712
| testCoreData.swift:93:10:93:10 | passwd | label:passwd, type:credential |
1813
| testRealm.swift:34:11:34:11 | myPassword | label:myPassword, type:credential |
19-
| testRealm.swift:38:11:38:11 | myHashedPassword | isProbablySafe, label:myHashedPassword, type:credential |
2014
| testRealm.swift:42:11:42:11 | myPassword | label:myPassword, type:credential |
21-
| testRealm.swift:46:11:46:11 | myHashedPassword | isProbablySafe, label:myHashedPassword, type:credential |
2215
| testRealm.swift:52:12:52:12 | myPassword | label:myPassword, type:credential |
23-
| testRealm.swift:55:12:55:12 | myHashedPassword | isProbablySafe, label:myHashedPassword, type:credential |
2416
| testSend.swift:29:19:29:19 | passwordPlain | label:passwordPlain, type:credential |
25-
| testSend.swift:30:19:30:19 | passwordHash | isProbablySafe, label:passwordHash, type:credential |
2617
| testSend.swift:33:19:33:19 | passwordPlain | label:passwordPlain, type:credential |
27-
| testSend.swift:34:19:34:19 | passwordHash | isProbablySafe, label:passwordHash, type:credential |
2818
| testSend.swift:45:13:45:13 | password | label:password, type:credential |
2919
| testSend.swift:46:13:46:13 | password | label:password, type:credential |
3020
| testSend.swift:47:17:47:17 | password | label:password, type:credential |
3121
| testSend.swift:48:23:48:23 | password | label:password, type:credential |
3222
| testSend.swift:49:27:49:27 | password | label:password, type:credential |
3323
| testSend.swift:50:27:50:27 | password | label:password, type:credential |
3424
| testURL.swift:13:54:13:54 | passwd | label:passwd, type:credential |
35-
| testURL.swift:14:54:14:54 | encrypted_passwd | isProbablySafe, label:encrypted_passwd, type:credential |
3625
| testURL.swift:16:55:16:55 | credit_card_no | label:credit_card_no, type:private information |
3726
| testURL.swift:20:22:20:22 | passwd | label:passwd, type:credential |

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,6 @@ string describe(SensitiveExpr e) {
55
result = "label:" + e.getLabel()
66
or
77
result = "type:" + e.getSensitiveType().toString()
8-
or
9-
e.isProbablySafe() and result = "isProbablySafe"
108
}
119

1210
from SensitiveExpr e

0 commit comments

Comments
 (0)