Skip to content

Commit b87d832

Browse files
geoffw0geoffw0
authored
Merge pull request #17129 from geoffw0/swiftconstsalt
Swift: Fixes for swift/constant-salt
2 parents 49335e5 + aae19ab commit b87d832

File tree

6 files changed

+89
-33
lines changed

6 files changed

+89
-33
lines changed

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,3 +69,27 @@ private class RnCryptorSaltSink extends ConstantSaltSink {
6969
private class DefaultSaltSink extends ConstantSaltSink {
7070
DefaultSaltSink() { sinkNode(this, "encryption-salt") }
7171
}
72+
73+
/**
74+
* A barrier for appending, since appending strings to a constant string
75+
* may (and often does) result in a non-constant string.
76+
*
77+
* Ideally, these would not be a barrier when there is flow to *all*
78+
* inputs.
79+
*/
80+
private class AppendConstantSaltBarrier extends ConstantSaltBarrier {
81+
AppendConstantSaltBarrier() {
82+
this.asExpr() = any(AddExpr ae).getAnOperand()
83+
or
84+
this.asExpr() = any(AssignAddExpr aae).getAnOperand()
85+
or
86+
exists(CallExpr ce |
87+
ce.getStaticTarget().getName() =
88+
["append(_:)", "appending(_:)", "appendLiteral(_:)", "appendInterpolation(_:)"] and
89+
(
90+
this.asExpr() = ce.getAnArgument().getExpr() or
91+
this.asExpr() = ce.getQualifier()
92+
)
93+
)
94+
}
95+
}

‎swift/ql/src/change-notes/2024-08-01-constant-salt.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The `swift/constant-salt` ("Use of constant salts") query now considers string concatenation and interpolation as a barrier. As a result, there will be fewer false positive results from this query involving constructed strings.
5+
* The `swift/constant-salt` ("Use of constant salts") query message now contains a link to the source node.

‎swift/ql/src/queries/Security/CWE-760/ConstantSalt.ql

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,11 @@ import swift
1414
import codeql.swift.security.ConstantSaltQuery
1515
import ConstantSaltFlow::PathGraph
1616

17-
from ConstantSaltFlow::PathNode sourceNode, ConstantSaltFlow::PathNode sinkNode
18-
where ConstantSaltFlow::flowPath(sourceNode, sinkNode)
19-
select sinkNode.getNode(), sourceNode, sinkNode,
20-
"The value '" + sourceNode.getNode().toString() +
21-
"' is used as a constant salt, which is insecure for hashing passwords."
17+
from
18+
ConstantSaltFlow::PathNode sourcePathNode, ConstantSaltFlow::PathNode sinkPathNode,
19+
DataFlow::Node sourceNode
20+
where
21+
ConstantSaltFlow::flowPath(sourcePathNode, sinkPathNode) and sourceNode = sourcePathNode.getNode()
22+
select sinkPathNode.getNode(), sourcePathNode, sinkPathNode,
23+
"The value $@ is used as a constant, which is insecure for hashing passwords.", sourceNode,
24+
sourceNode.toString()

‎swift/ql/src/queries/Security/CWE-760/ConstantSalt.swift

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,20 +3,20 @@ func encrypt(padding : Padding) {
33
// ...
44

55
// BAD: Using constant salts for hashing
6-
let salt: Array<UInt8> = [0x2a, 0x3a, 0x80, 0x05]
6+
let badSalt: Array<UInt8> = [0x2a, 0x3a, 0x80, 0x05]
77
let randomArray = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) })
8-
_ = try HKDF(password: randomArray, salt: salt, info: randomArray, keyLength: 0, variant: Variant.sha2)
9-
_ = try PKCS5.PBKDF1(password: randomArray, salt: salt, iterations: 120120, keyLength: 0)
10-
_ = try PKCS5.PBKDF2(password: randomArray, salt: salt, iterations: 120120, keyLength: 0)
11-
_ = try Scrypt(password: randomArray, salt: salt, dkLen: 64, N: 16384, r: 8, p: 1)
8+
_ = try HKDF(password: randomArray, salt: badSalt, info: randomArray, keyLength: 0, variant: Variant.sha2)
9+
_ = try PKCS5.PBKDF1(password: randomArray, salt: badSalt, iterations: 120120, keyLength: 0)
10+
_ = try PKCS5.PBKDF2(password: randomArray, salt: badSalt, iterations: 120120, keyLength: 0)
11+
_ = try Scrypt(password: randomArray, salt: badSalt, dkLen: 64, N: 16384, r: 8, p: 1)
1212

1313
// GOOD: Using randomly generated salts for hashing
14-
let salt = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) })
14+
let goodSalt = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) })
1515
let randomArray = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) })
16-
_ = try HKDF(password: randomArray, salt: salt, info: randomArray, keyLength: 0, variant: Variant.sha2)
17-
_ = try PKCS5.PBKDF1(password: randomArray, salt: salt, iterations: 120120, keyLength: 0)
18-
_ = try PKCS5.PBKDF2(password: randomArray, salt: salt, iterations: 120120, keyLength: 0)
19-
_ = try Scrypt(password: randomArray, salt: salt, dkLen: 64, N: 16384, r: 8, p: 1)
16+
_ = try HKDF(password: randomArray, salt: goodSalt, info: randomArray, keyLength: 0, variant: Variant.sha2)
17+
_ = try PKCS5.PBKDF1(password: randomArray, salt: goodSalt, iterations: 120120, keyLength: 0)
18+
_ = try PKCS5.PBKDF2(password: randomArray, salt: goodSalt, iterations: 120120, keyLength: 0)
19+
_ = try Scrypt(password: randomArray, salt: goodSalt, dkLen: 64, N: 16384, r: 8, p: 1)
2020

2121
// ...
2222
}

‎swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.expected

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -54,21 +54,21 @@ nodes
5454
| test.swift:68:53:68:53 | constantStringSalt | semmle.label | constantStringSalt |
5555
subpaths
5656
#select
57-
| rncryptor.swift:63:57:63:57 | myConstantSalt1 | rncryptor.swift:59:29:59:29 | abcdef123456 | rncryptor.swift:63:57:63:57 | myConstantSalt1 | The value 'abcdef123456' is used as a constant salt, which is insecure for hashing passwords. |
58-
| rncryptor.swift:65:55:65:55 | myConstantSalt2 | rncryptor.swift:60:29:60:29 | 0 | rncryptor.swift:65:55:65:55 | myConstantSalt2 | The value '0' is used as a constant salt, which is insecure for hashing passwords. |
59-
| rncryptor.swift:68:106:68:106 | myConstantSalt1 | rncryptor.swift:59:29:59:29 | abcdef123456 | rncryptor.swift:68:106:68:106 | myConstantSalt1 | The value 'abcdef123456' is used as a constant salt, which is insecure for hashing passwords. |
60-
| rncryptor.swift:69:131:69:131 | myConstantSalt2 | rncryptor.swift:60:29:60:29 | 0 | rncryptor.swift:69:131:69:131 | myConstantSalt2 | The value '0' is used as a constant salt, which is insecure for hashing passwords. |
61-
| rncryptor.swift:71:106:71:106 | myConstantSalt1 | rncryptor.swift:59:29:59:29 | abcdef123456 | rncryptor.swift:71:106:71:106 | myConstantSalt1 | The value 'abcdef123456' is used as a constant salt, which is insecure for hashing passwords. |
62-
| rncryptor.swift:72:131:72:131 | myConstantSalt2 | rncryptor.swift:60:29:60:29 | 0 | rncryptor.swift:72:131:72:131 | myConstantSalt2 | The value '0' is used as a constant salt, which is insecure for hashing passwords. |
63-
| rncryptor.swift:75:127:75:127 | myConstantSalt1 | rncryptor.swift:59:29:59:29 | abcdef123456 | rncryptor.swift:75:127:75:127 | myConstantSalt1 | The value 'abcdef123456' is used as a constant salt, which is insecure for hashing passwords. |
64-
| rncryptor.swift:76:152:76:152 | myConstantSalt2 | rncryptor.swift:60:29:60:29 | 0 | rncryptor.swift:76:152:76:152 | myConstantSalt2 | The value '0' is used as a constant salt, which is insecure for hashing passwords. |
65-
| rncryptor.swift:78:135:78:135 | myConstantSalt1 | rncryptor.swift:59:29:59:29 | abcdef123456 | rncryptor.swift:78:135:78:135 | myConstantSalt1 | The value 'abcdef123456' is used as a constant salt, which is insecure for hashing passwords. |
66-
| rncryptor.swift:79:160:79:160 | myConstantSalt2 | rncryptor.swift:60:29:60:29 | 0 | rncryptor.swift:79:160:79:160 | myConstantSalt2 | The value '0' is used as a constant salt, which is insecure for hashing passwords. |
67-
| test.swift:51:49:51:49 | constantSalt | test.swift:43:35:43:130 | [...] | test.swift:51:49:51:49 | constantSalt | The value '[...]' is used as a constant salt, which is insecure for hashing passwords. |
68-
| test.swift:52:49:52:49 | constantStringSalt | test.swift:29:3:29:3 | this string is constant | test.swift:52:49:52:49 | constantStringSalt | The value 'this string is constant' is used as a constant salt, which is insecure for hashing passwords. |
69-
| test.swift:56:59:56:59 | constantSalt | test.swift:43:35:43:130 | [...] | test.swift:56:59:56:59 | constantSalt | The value '[...]' is used as a constant salt, which is insecure for hashing passwords. |
70-
| test.swift:57:59:57:59 | constantStringSalt | test.swift:29:3:29:3 | this string is constant | test.swift:57:59:57:59 | constantStringSalt | The value 'this string is constant' is used as a constant salt, which is insecure for hashing passwords. |
71-
| test.swift:62:59:62:59 | constantSalt | test.swift:43:35:43:130 | [...] | test.swift:62:59:62:59 | constantSalt | The value '[...]' is used as a constant salt, which is insecure for hashing passwords. |
72-
| test.swift:63:59:63:59 | constantStringSalt | test.swift:29:3:29:3 | this string is constant | test.swift:63:59:63:59 | constantStringSalt | The value 'this string is constant' is used as a constant salt, which is insecure for hashing passwords. |
73-
| test.swift:67:53:67:53 | constantSalt | test.swift:43:35:43:130 | [...] | test.swift:67:53:67:53 | constantSalt | The value '[...]' is used as a constant salt, which is insecure for hashing passwords. |
74-
| test.swift:68:53:68:53 | constantStringSalt | test.swift:29:3:29:3 | this string is constant | test.swift:68:53:68:53 | constantStringSalt | The value 'this string is constant' is used as a constant salt, which is insecure for hashing passwords. |
57+
| rncryptor.swift:63:57:63:57 | myConstantSalt1 | rncryptor.swift:59:29:59:29 | abcdef123456 | rncryptor.swift:63:57:63:57 | myConstantSalt1 | The value $@ is used as a constant, which is insecure for hashing passwords. | rncryptor.swift:59:29:59:29 | abcdef123456 | abcdef123456 |
58+
| rncryptor.swift:65:55:65:55 | myConstantSalt2 | rncryptor.swift:60:29:60:29 | 0 | rncryptor.swift:65:55:65:55 | myConstantSalt2 | The value $@ is used as a constant, which is insecure for hashing passwords. | rncryptor.swift:60:29:60:29 | 0 | 0 |
59+
| rncryptor.swift:68:106:68:106 | myConstantSalt1 | rncryptor.swift:59:29:59:29 | abcdef123456 | rncryptor.swift:68:106:68:106 | myConstantSalt1 | The value $@ is used as a constant, which is insecure for hashing passwords. | rncryptor.swift:59:29:59:29 | abcdef123456 | abcdef123456 |
60+
| rncryptor.swift:69:131:69:131 | myConstantSalt2 | rncryptor.swift:60:29:60:29 | 0 | rncryptor.swift:69:131:69:131 | myConstantSalt2 | The value $@ is used as a constant, which is insecure for hashing passwords. | rncryptor.swift:60:29:60:29 | 0 | 0 |
61+
| rncryptor.swift:71:106:71:106 | myConstantSalt1 | rncryptor.swift:59:29:59:29 | abcdef123456 | rncryptor.swift:71:106:71:106 | myConstantSalt1 | The value $@ is used as a constant, which is insecure for hashing passwords. | rncryptor.swift:59:29:59:29 | abcdef123456 | abcdef123456 |
62+
| rncryptor.swift:72:131:72:131 | myConstantSalt2 | rncryptor.swift:60:29:60:29 | 0 | rncryptor.swift:72:131:72:131 | myConstantSalt2 | The value $@ is used as a constant, which is insecure for hashing passwords. | rncryptor.swift:60:29:60:29 | 0 | 0 |
63+
| rncryptor.swift:75:127:75:127 | myConstantSalt1 | rncryptor.swift:59:29:59:29 | abcdef123456 | rncryptor.swift:75:127:75:127 | myConstantSalt1 | The value $@ is used as a constant, which is insecure for hashing passwords. | rncryptor.swift:59:29:59:29 | abcdef123456 | abcdef123456 |
64+
| rncryptor.swift:76:152:76:152 | myConstantSalt2 | rncryptor.swift:60:29:60:29 | 0 | rncryptor.swift:76:152:76:152 | myConstantSalt2 | The value $@ is used as a constant, which is insecure for hashing passwords. | rncryptor.swift:60:29:60:29 | 0 | 0 |
65+
| rncryptor.swift:78:135:78:135 | myConstantSalt1 | rncryptor.swift:59:29:59:29 | abcdef123456 | rncryptor.swift:78:135:78:135 | myConstantSalt1 | The value $@ is used as a constant, which is insecure for hashing passwords. | rncryptor.swift:59:29:59:29 | abcdef123456 | abcdef123456 |
66+
| rncryptor.swift:79:160:79:160 | myConstantSalt2 | rncryptor.swift:60:29:60:29 | 0 | rncryptor.swift:79:160:79:160 | myConstantSalt2 | The value $@ is used as a constant, which is insecure for hashing passwords. | rncryptor.swift:60:29:60:29 | 0 | 0 |
67+
| test.swift:51:49:51:49 | constantSalt | test.swift:43:35:43:130 | [...] | test.swift:51:49:51:49 | constantSalt | The value $@ is used as a constant, which is insecure for hashing passwords. | test.swift:43:35:43:130 | [...] | [...] |
68+
| test.swift:52:49:52:49 | constantStringSalt | test.swift:29:3:29:3 | this string is constant | test.swift:52:49:52:49 | constantStringSalt | The value $@ is used as a constant, which is insecure for hashing passwords. | test.swift:29:3:29:3 | this string is constant | this string is constant |
69+
| test.swift:56:59:56:59 | constantSalt | test.swift:43:35:43:130 | [...] | test.swift:56:59:56:59 | constantSalt | The value $@ is used as a constant, which is insecure for hashing passwords. | test.swift:43:35:43:130 | [...] | [...] |
70+
| test.swift:57:59:57:59 | constantStringSalt | test.swift:29:3:29:3 | this string is constant | test.swift:57:59:57:59 | constantStringSalt | The value $@ is used as a constant, which is insecure for hashing passwords. | test.swift:29:3:29:3 | this string is constant | this string is constant |
71+
| test.swift:62:59:62:59 | constantSalt | test.swift:43:35:43:130 | [...] | test.swift:62:59:62:59 | constantSalt | The value $@ is used as a constant, which is insecure for hashing passwords. | test.swift:43:35:43:130 | [...] | [...] |
72+
| test.swift:63:59:63:59 | constantStringSalt | test.swift:29:3:29:3 | this string is constant | test.swift:63:59:63:59 | constantStringSalt | The value $@ is used as a constant, which is insecure for hashing passwords. | test.swift:29:3:29:3 | this string is constant | this string is constant |
73+
| test.swift:67:53:67:53 | constantSalt | test.swift:43:35:43:130 | [...] | test.swift:67:53:67:53 | constantSalt | The value $@ is used as a constant, which is insecure for hashing passwords. | test.swift:43:35:43:130 | [...] | [...] |
74+
| test.swift:68:53:68:53 | constantStringSalt | test.swift:29:3:29:3 | this string is constant | test.swift:68:53:68:53 | constantStringSalt | The value $@ is used as a constant, which is insecure for hashing passwords. | test.swift:29:3:29:3 | this string is constant | this string is constant |

‎swift/ql/test/query-tests/Security/CWE-760/rncryptor.swift

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,4 +77,28 @@ func test(myPassword: String) {
7777
let _ = try? myEncryptor.encryptData(myData, withSettings: kRNCryptorAES256Settings, password: myPassword, IV: myIV, encryptionSalt: myRandomSalt1, HMACSalt: myRandomSalt2) // GOOD
7878
let _ = try? myEncryptor.encryptData(myData, withSettings: kRNCryptorAES256Settings, password: myPassword, IV: myIV, encryptionSalt: myConstantSalt1, HMACSalt: myRandomSalt2) // BAD
7979
let _ = try? myEncryptor.encryptData(myData, withSettings: kRNCryptorAES256Settings, password: myPassword, IV: myIV, encryptionSalt: myRandomSalt1, HMACSalt: myConstantSalt2) // BAD
80+
81+
// appending constants
82+
let _ = myEncryptor.key(forPassword: myPassword, salt: Data(getARandomString() + getARandomString()), settings: myKeyDerivationSettings) // GOOD
83+
let _ = myEncryptor.key(forPassword: myPassword, salt: Data("123" + getARandomString()), settings: myKeyDerivationSettings) // GOOD
84+
let _ = myEncryptor.key(forPassword: myPassword, salt: Data(getARandomString() + "abc"), settings: myKeyDerivationSettings) // GOOD
85+
let _ = myEncryptor.key(forPassword: myPassword, salt: Data("123" + "abc"), settings: myKeyDerivationSettings) // BAD (constant salt) [NOT DETECTED]
86+
let _ = myEncryptor.key(forPassword: myPassword, salt: Data("123\(getARandomString())abc"), settings: myKeyDerivationSettings) // GOOD
87+
let _ = myEncryptor.key(forPassword: myPassword, salt: Data("123\("const"))abc"), settings: myKeyDerivationSettings) // BAD (constant salt) [NOT DETECTED]
88+
89+
var myMutableString1 = "123"
90+
myMutableString1.append(getARandomString())
91+
let _ = myEncryptor.key(forPassword: myPassword, salt: Data(myMutableString1), settings: myKeyDerivationSettings) // GOOD
92+
93+
var myMutableString2 = getARandomString()
94+
myMutableString2.append("abc")
95+
let _ = myEncryptor.key(forPassword: myPassword, salt: Data(myMutableString2), settings: myKeyDerivationSettings) // GOOD
96+
97+
var myMutableString3 = "123"
98+
myMutableString3 += getARandomString()
99+
let _ = myEncryptor.key(forPassword: myPassword, salt: Data(myMutableString3), settings: myKeyDerivationSettings) // GOOD
100+
101+
var myMutableString4 = getARandomString()
102+
myMutableString4 += "abc"
103+
let _ = myEncryptor.key(forPassword: myPassword, salt: Data(myMutableString4), settings: myKeyDerivationSettings) // GOOD
80104
}

0 commit comments

Comments
 (0)