github · geoffw0 · Aug 2, 2024 · Jul 31, 2024 · Jul 31, 2024 · Jul 31, 2024
@@ -69,3 +69,27 @@ private class RnCryptorSaltSink extends ConstantSaltSink {
 private class DefaultSaltSink extends ConstantSaltSink {
   DefaultSaltSink() { sinkNode(this, "encryption-salt") }
 }
+
+/**
+ * A barrier for appending, since appending strings to a constant string
+ * may (and often does) result in a non-constant string.
+ *
+ * Ideally, these would not be a barrier when there is flow to *all*
+ * inputs.
+ */
+private class AppendConstantSaltBarrier extends ConstantSaltBarrier {
+  AppendConstantSaltBarrier() {
+    this.asExpr() = any(AddExpr ae).getAnOperand()
+    or
+    this.asExpr() = any(AssignAddExpr aae).getAnOperand()
+    or
+    exists(CallExpr ce |
+      ce.getStaticTarget().getName() =
+        ["append(_:)", "appending(_:)", "appendLiteral(_:)", "appendInterpolation(_:)"] and
+      (
+        this.asExpr() = ce.getAnArgument().getExpr() or
+        this.asExpr() = ce.getQualifier()
+      )
+    )
+  }
+}
@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* 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.
+* The `swift/constant-salt` ("Use of constant salts") query message now contains a link to the source node.
@@ -14,8 +14,11 @@ import swift
 import codeql.swift.security.ConstantSaltQuery
 import ConstantSaltFlow::PathGraph
 
-from ConstantSaltFlow::PathNode sourceNode, ConstantSaltFlow::PathNode sinkNode
-where ConstantSaltFlow::flowPath(sourceNode, sinkNode)
-select sinkNode.getNode(), sourceNode, sinkNode,
-  "The value '" + sourceNode.getNode().toString() +
-    "' is used as a constant salt, which is insecure for hashing passwords."
+from
+  ConstantSaltFlow::PathNode sourcePathNode, ConstantSaltFlow::PathNode sinkPathNode,
+  DataFlow::Node sourceNode
+where
+  ConstantSaltFlow::flowPath(sourcePathNode, sinkPathNode) and sourceNode = sourcePathNode.getNode()
+select sinkPathNode.getNode(), sourcePathNode, sinkPathNode,
+  "The value $@ is used as a constant, which is insecure for hashing passwords.", sourceNode,
+  sourceNode.toString()
@@ -3,20 +3,20 @@ func encrypt(padding : Padding) {
 	// ...
 
 	// BAD: Using constant salts for hashing
-	let salt: Array<UInt8> = [0x2a, 0x3a, 0x80, 0x05]
+	let badSalt: Array<UInt8> = [0x2a, 0x3a, 0x80, 0x05]
 	let randomArray = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) })
-	_ = try HKDF(password: randomArray, salt: salt, info: randomArray, keyLength: 0, variant: Variant.sha2)
-	_ = try PKCS5.PBKDF1(password: randomArray, salt: salt, iterations: 120120, keyLength: 0)
-	_ = try PKCS5.PBKDF2(password: randomArray, salt: salt, iterations: 120120, keyLength: 0)
-	_ = try Scrypt(password: randomArray, salt: salt, dkLen: 64, N: 16384, r: 8, p: 1)
+	_ = try HKDF(password: randomArray, salt: badSalt, info: randomArray, keyLength: 0, variant: Variant.sha2)
+	_ = try PKCS5.PBKDF1(password: randomArray, salt: badSalt, iterations: 120120, keyLength: 0)
+	_ = try PKCS5.PBKDF2(password: randomArray, salt: badSalt, iterations: 120120, keyLength: 0)
+	_ = try Scrypt(password: randomArray, salt: badSalt, dkLen: 64, N: 16384, r: 8, p: 1)
 
 	// GOOD: Using randomly generated salts for hashing
-	let salt = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) })
+	let goodSalt = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) })
 	let randomArray = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) })
-	_ = try HKDF(password: randomArray, salt: salt, info: randomArray, keyLength: 0, variant: Variant.sha2)
-	_ = try PKCS5.PBKDF1(password: randomArray, salt: salt, iterations: 120120, keyLength: 0)
-	_ = try PKCS5.PBKDF2(password: randomArray, salt: salt, iterations: 120120, keyLength: 0)
-	_ = try Scrypt(password: randomArray, salt: salt, dkLen: 64, N: 16384, r: 8, p: 1)
+	_ = try HKDF(password: randomArray, salt: goodSalt, info: randomArray, keyLength: 0, variant: Variant.sha2)
+	_ = try PKCS5.PBKDF1(password: randomArray, salt: goodSalt, iterations: 120120, keyLength: 0)
+	_ = try PKCS5.PBKDF2(password: randomArray, salt: goodSalt, iterations: 120120, keyLength: 0)
+	_ = try Scrypt(password: randomArray, salt: goodSalt, dkLen: 64, N: 16384, r: 8, p: 1)
 
 	// ...
 }
@@ -54,21 +54,21 @@ nodes
 | test.swift:68:53:68:53 | constantStringSalt | semmle.label | constantStringSalt |
 subpaths
 #select
-| 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. |
-| 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. |
-| 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. |
-| 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. |
-| 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. |
-| 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. |
-| 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. |
-| 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. |
-| 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. |
-| 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. |
-| 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. |
-| 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. |
-| 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. |
-| 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. |
-| 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. |
-| 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. |
-| 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. |
-| 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. |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 | [...] | [...] |
+| 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 |
+| 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 | [...] | [...] |
+| 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 |
+| 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 | [...] | [...] |
+| 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 |
+| 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 | [...] | [...] |
+| 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 |
@@ -77,4 +77,28 @@ func test(myPassword: String) {
 	let _ = try? myEncryptor.encryptData(myData, withSettings: kRNCryptorAES256Settings, password: myPassword, IV: myIV, encryptionSalt: myRandomSalt1, HMACSalt: myRandomSalt2) // GOOD
 	let _ = try? myEncryptor.encryptData(myData, withSettings: kRNCryptorAES256Settings, password: myPassword, IV: myIV, encryptionSalt: myConstantSalt1, HMACSalt: myRandomSalt2) // BAD
 	let _ = try? myEncryptor.encryptData(myData, withSettings: kRNCryptorAES256Settings, password: myPassword, IV: myIV, encryptionSalt: myRandomSalt1, HMACSalt: myConstantSalt2) // BAD
+
+	// appending constants
+	let _ = myEncryptor.key(forPassword: myPassword, salt: Data(getARandomString() + getARandomString()), settings: myKeyDerivationSettings) // GOOD
+	let _ = myEncryptor.key(forPassword: myPassword, salt: Data("123" + getARandomString()), settings: myKeyDerivationSettings) // GOOD
+	let _ = myEncryptor.key(forPassword: myPassword, salt: Data(getARandomString() + "abc"), settings: myKeyDerivationSettings) // GOOD
+	let _ = myEncryptor.key(forPassword: myPassword, salt: Data("123" + "abc"), settings: myKeyDerivationSettings) // BAD (constant salt) [NOT DETECTED]
+	let _ = myEncryptor.key(forPassword: myPassword, salt: Data("123\(getARandomString())abc"), settings: myKeyDerivationSettings) // GOOD
+	let _ = myEncryptor.key(forPassword: myPassword, salt: Data("123\("const"))abc"), settings: myKeyDerivationSettings) // BAD (constant salt) [NOT DETECTED]
+
+	var myMutableString1 = "123"
+	myMutableString1.append(getARandomString())
+	let _ = myEncryptor.key(forPassword: myPassword, salt: Data(myMutableString1), settings: myKeyDerivationSettings) // GOOD
+
+	var myMutableString2 = getARandomString()
+	myMutableString2.append("abc")
+	let _ = myEncryptor.key(forPassword: myPassword, salt: Data(myMutableString2), settings: myKeyDerivationSettings) // GOOD
+
+	var myMutableString3 = "123"
+	myMutableString3 += getARandomString()
+	let _ = myEncryptor.key(forPassword: myPassword, salt: Data(myMutableString3), settings: myKeyDerivationSettings) // GOOD
+
+	var myMutableString4 = getARandomString()
+	myMutableString4 += "abc"
+	let _ = myEncryptor.key(forPassword: myPassword, salt: Data(myMutableString4), settings: myKeyDerivationSettings) // GOOD
 }