New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Swift: detect the use of constant salts #10993
base: main
Are you sure you want to change the base?
Swift: detect the use of constant salts #10993
Conversation
|
QHelp previews: swift/ql/src/queries/Security/CWE-760/ConstantSalt.qhelpConstant saltConstant salts should not be used for password hashing. Data hashed using constant salts are vulnerable to dictionary attacks, enabling attackers to recover the original input. RecommendationUse randomly generated salts to securely hash input data. ExampleThe following example shows a few cases of hashing input data. In the 'BAD' cases, the salt is constant, making the generated hashes vulnerable to dictionary attakcs. In the 'GOOD' cases, the salt is randomly generated, which protects the hashed data against recovery. References
|
| exists(ClassOrStructDecl c, AbstractFunctionDecl f, CallExpr call, int arg | | ||
| c.getFullName() = ["HKDF", "PBKDF1", "PBKDF2", "Scrypt"] and | ||
| c.getAMember() = f and | ||
| f.getName().matches("%init(%salt:%") and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a great start, my only concern is how often salts are specified in the wild using the functions modelled above. I did a quick MRVA query for parameters called "salt" (or similar) and the most common call that appears to be relevant is to a thing called CCKeyDerivationPBKDF(_:_:_:_:_:_:_:_:_:) from CommonCrypto. Do you think we should perhaps create a follow-up issue to add a model of that to this query as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add further support for CommonCrytpo, not just for this query, but perhaps other crypto queries too. This might be a good first step in supporting more crypto APIs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Constant salts should not be used for password hashing. Data hashed using constant salts are vulnerable to dictionary attacks, enabling attackers to recover the original input.
The rule currently supports all ciphers that the CryptoSwift API provides, but we can always extend it further if more APIs are added.
I'd appreciate a review of the query itself, the accompanying tests, and the associated documentation.