hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2025-12-16 16:53:25 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #17129 from geoffw0/swiftconstsalt

Browse Source 
Swift: Fixes for swift/constant-salt
This commit is contained in:
Geoffrey White
2024-08-02 13:57:05 +01:00
committed by GitHub
parent 49335e5b63 aae19ab9f5
commit b87d832fcb
6 changed files with 89 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
swift/ql/lib/codeql/swift/security/ConstantSaltExtensions.qll
View File

@@ -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()
)
)
}
}

5
swift/ql/src/change-notes/2024-08-01-constant-salt.md Normal file
View File

@@ -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.

13
swift/ql/src/queries/Security/CWE-760/ConstantSalt.ql
View File

@@ -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()

20
swift/ql/src/queries/Security/CWE-760/ConstantSalt.swift
View File

@@ -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)
// ...
}

36
swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.expected
View File

@@ -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 |

24
swift/ql/test/query-tests/Security/CWE-760/rncryptor.swift
View File

@@ -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
}