-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Rust: Weak hashing query #18471
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
Merged
Merged
Rust: Weak hashing query #18471
Changes from 15 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
509c6ff
Rust: Add tests for weak hashing.
geoffw0 8f4a520
Rust: Add query framework.
geoffw0 d72b978
Rust: Add sensitive data sources.
geoffw0 ae0f4f1
Rust: Add hash function sinks.
geoffw0 babfa75
Rust: Add models for an alternative md5 library.
geoffw0 5938659
Rust: Add .qhelp.
geoffw0 9b8f561
Rust: Add another reference.
geoffw0 ae26cd6
Rust: Update test for changes on main.
geoffw0 c115169
Rust: Move ModelledHashOperation to a more logical location.
geoffw0 bb4322c
Rust: Make a type more accurate.
geoffw0 39a38c4
Rust: Tweak .qhelp layout.
geoffw0 ad26822
Rust: Address QL-for-QL comments.
geoffw0 19d3e9d
Rust: Correct the qhelp.
geoffw0 1b6c289
Rust: Unrelated MaD test impact. :(
geoffw0 edd1f25
Rust: Attempt to fix the test on CI.
geoffw0 722b7bb
Apply suggestions from code review
geoffw0 676141b
Rust: More suggestions from review.
geoffw0 e61d6ae
Rust: Autoformat.
geoffw0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
10 changes: 10 additions & 0 deletions
10
rust/ql/lib/codeql/rust/frameworks/rustcrypto/rustcrypto.model.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| extensions: | ||
| - addsTo: | ||
| pack: codeql/rust-all | ||
| extensible: sinkModel | ||
| data: | ||
| - ["repo:https://github.com/RustCrypto/traits:digest", "<_ as crate::digest::Digest>::new_with_prefix", "Argument[0]", "hasher-input", "manual"] | ||
| - ["repo:https://github.com/RustCrypto/traits:digest", "<_ as crate::digest::Digest>::update", "Argument[0]", "hasher-input", "manual"] | ||
| - ["repo:https://github.com/RustCrypto/traits:digest", "<_ as crate::digest::Digest>::chain_update", "Argument[0]", "hasher-input", "manual"] | ||
| - ["repo:https://github.com/RustCrypto/traits:digest", "<_ as crate::digest::Digest>::digest", "Argument[0]", "hasher-input", "manual"] | ||
| - ["repo:https://github.com/stainless-steel/md5:md5", "crate::compute", "Argument[0]", "hasher-input", "manual"] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
200 changes: 200 additions & 0 deletions
200
rust/ql/lib/codeql/rust/security/WeakSensitiveDataHashingExtensions.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,200 @@ | ||
| /** | ||
| * Provides default sources, sinks and sanitizers for detecting "use of a | ||
| * broken or weak cryptographic hashing algorithm on sensitive data" | ||
| * vulnerabilities, as well as extension points for adding your own. This is | ||
| * divided into two general cases: | ||
| * - hashing sensitive data | ||
| * - hashing passwords (which requires the hashing algorithm to be | ||
| * sufficiently computationally expensive in addition to other requirements) | ||
| */ | ||
|
|
||
| import rust | ||
| private import codeql.rust.Concepts | ||
| private import codeql.rust.security.SensitiveData | ||
| private import codeql.rust.dataflow.DataFlow | ||
| private import codeql.rust.dataflow.FlowSource | ||
| private import codeql.rust.dataflow.FlowSink | ||
| private import codeql.rust.dataflow.internal.DataFlowImpl | ||
|
|
||
| /** | ||
| * Provides default sources, sinks and sanitizers for detecting "use of a broken or weak | ||
| * cryptographic hashing algorithm on sensitive data" vulnerabilities on sensitive data that does | ||
| * NOT require computationally expensive hashing, as well as extension points for adding your own. | ||
| * | ||
| * Also see the `ComputationallyExpensiveHashFunction` module. | ||
| */ | ||
| module NormalHashFunction { | ||
| /** | ||
| * A data flow source for "use of a broken or weak cryptographic hashing algorithm on sensitive | ||
| * data" vulnerabilities that does not require computationally expensive hashing. That is, a | ||
| * piece of sensitive data that is not a password. | ||
| */ | ||
| abstract class Source extends DataFlow::Node { | ||
| Source() { not this instanceof ComputationallyExpensiveHashFunction::Source } | ||
|
|
||
| /** | ||
| * Gets the classification of the sensitive data. | ||
| */ | ||
| abstract string getClassification(); | ||
| } | ||
|
|
||
| /** | ||
| * A data flow sink for "use of a broken or weak cryptographic hashing algorithm on sensitive | ||
| * data" vulnerabilities that applies to data that does not require computationally expensive | ||
| * hashing. That is, a broken or weak hashing algorithm. | ||
| */ | ||
| abstract class Sink extends DataFlow::Node { | ||
| /** | ||
| * Gets the name of the weak hashing algorithm. | ||
| */ | ||
| abstract string getAlgorithmName(); | ||
| } | ||
|
|
||
| /** | ||
| * A barrier for "use of a broken or weak cryptographic hashing algorithm on sensitive data" | ||
| * vulnerabilities that applies to data that does not require computationally expensive hashing. | ||
| */ | ||
| abstract class Barrier extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * A flow source modeled by the `SensitiveData` library. | ||
| */ | ||
| class SensitiveDataAsSource extends Source instanceof SensitiveData { | ||
| SensitiveDataAsSource() { | ||
| not this.(SensitiveData).getClassification() = SensitiveDataClassification::password() and // (covered in ComputationallyExpensiveHashFunction) | ||
| not this.(SensitiveData).getClassification() = SensitiveDataClassification::id() // (not accurate enough) | ||
geoffw0 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| override SensitiveDataClassification getClassification() { | ||
| result = this.(SensitiveData).getClassification() | ||
geoffw0 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A flow sink modeled by the `Cryptography` module. | ||
| */ | ||
| class WeakHashingOperationInputAsSink extends Sink { | ||
| Cryptography::HashingAlgorithm algorithm; | ||
|
|
||
| WeakHashingOperationInputAsSink() { | ||
| exists(Cryptography::CryptographicOperation operation | | ||
| algorithm.isWeak() and | ||
| algorithm = operation.getAlgorithm() and | ||
| this = operation.getAnInput() | ||
| ) | ||
| } | ||
|
|
||
| override string getAlgorithmName() { result = algorithm.getName() } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Provides default sources, sinks and sanitizers for detecting "use of a broken or weak | ||
| * cryptographic hashing algorithm on sensitive data" vulnerabilities on sensitive data that DOES | ||
| * require computationally expensive hashing, as well as extension points for adding your own. | ||
| * | ||
| * Also see the `NormalHashFunction` module. | ||
| */ | ||
| module ComputationallyExpensiveHashFunction { | ||
| /** | ||
| * A data flow source for "use of a broken or weak cryptographic hashing algorithm on sensitive | ||
| * data" vulnerabilities that does require computationally expensive hashing. That is, a | ||
| * password. | ||
| */ | ||
| abstract class Source extends DataFlow::Node { | ||
| /** | ||
| * Gets the classification of the sensitive data. | ||
| */ | ||
| abstract string getClassification(); | ||
| } | ||
|
|
||
| /** | ||
| * A data flow sink for "use of a broken or weak cryptographic hashing algorithm on sensitive | ||
| * data" vulnerabilities that applies to data that does require computationally expensive | ||
| * hashing. That is, a broken or weak hashing algorithm or one that is not computationally | ||
| * expensive enough for password hashing. | ||
| */ | ||
| abstract class Sink extends DataFlow::Node { | ||
| /** | ||
| * Gets the name of the weak hashing algorithm. | ||
| */ | ||
| abstract string getAlgorithmName(); | ||
|
|
||
| /** | ||
| * Holds if this sink is for a computationally expensive hash function (meaning that hash | ||
| * function is just weak in some other regard. | ||
| */ | ||
| abstract predicate isComputationallyExpensive(); | ||
| } | ||
|
|
||
| /** | ||
| * A barrier for "use of a broken or weak cryptographic hashing algorithm on sensitive data" | ||
| * vulnerabilities that applies to data that does require computationally expensive hashing. | ||
| */ | ||
| abstract class Barrier extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * A flow source modeled by the `SensitiveData` library. | ||
| */ | ||
| class PasswordAsSource extends Source instanceof SensitiveData { | ||
| PasswordAsSource() { | ||
| this.(SensitiveData).getClassification() = SensitiveDataClassification::password() | ||
| } | ||
|
|
||
| override SensitiveDataClassification getClassification() { | ||
| result = this.(SensitiveData).getClassification() | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A flow sink modeled by the `Cryptography` module. | ||
| */ | ||
| class WeakPasswordHashingOperationInputSink extends Sink { | ||
| Cryptography::CryptographicAlgorithm algorithm; | ||
|
|
||
| WeakPasswordHashingOperationInputSink() { | ||
| exists(Cryptography::CryptographicOperation operation | | ||
| ( | ||
| algorithm instanceof Cryptography::PasswordHashingAlgorithm and | ||
| algorithm.isWeak() | ||
| or | ||
| algorithm instanceof Cryptography::HashingAlgorithm // Note that HashingAlgorithm and PasswordHashingAlgorithm are disjoint | ||
| ) and | ||
| algorithm = operation.getAlgorithm() and | ||
| this = operation.getAnInput() | ||
| ) | ||
| } | ||
|
|
||
| override string getAlgorithmName() { result = algorithm.getName() } | ||
|
|
||
| override predicate isComputationallyExpensive() { | ||
| algorithm instanceof Cryptography::PasswordHashingAlgorithm | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * An externally modeled operation that hashes data, for example a call to `md5::Md5::digest(data)`. | ||
| */ | ||
| class ModeledHashOperation extends Cryptography::CryptographicOperation::Range { | ||
| DataFlow::Node input; | ||
| string algorithmName; | ||
|
|
||
| ModeledHashOperation() { | ||
| exists(CallExpr call | | ||
| sinkNode(input, "hasher-input") and | ||
| call = input.(Node::FlowSummaryNode).getSinkElement().getCall() and | ||
| call = this.asExpr().getExpr() and | ||
| algorithmName = | ||
| call.getFunction().(PathExpr).getPath().getQualifier().getPart().getNameRef().getText() | ||
| ) | ||
| } | ||
|
|
||
| override DataFlow::Node getInitialization() { result = this } | ||
|
|
||
| override Cryptography::HashingAlgorithm getAlgorithm() { result.matchesName(algorithmName) } | ||
|
|
||
| override DataFlow::Node getAnInput() { result = input } | ||
|
|
||
| override Cryptography::BlockMode getBlockMode() { none() } // (does not apply for hashing) | ||
| } | ||
117 changes: 117 additions & 0 deletions
117
rust/ql/src/queries/security/CWE-328/WeakSensitiveDataHashing.qhelp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
| <overview> | ||
| <p> | ||
| A broken or weak cryptographic hash function can leave data | ||
| vulnerable, and should not be used in security related code. | ||
geoffw0 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| </p> | ||
|
|
||
| <p> | ||
| A strong cryptographic hash function should be resistant to: | ||
| </p> | ||
| <ul> | ||
| <li> | ||
| <b>Pre-image attacks</b>. If you know a hash value <code>h(x)</code>, | ||
| you should not be able to easily find the input <code>x</code>. | ||
| </li> | ||
| <li> | ||
| <b>Collision attacks</b>. If you know a hash value <code>h(x)</code>, | ||
| you should not be able to easily find a different input | ||
| <code>y</code> | ||
| with the same hash value <code>h(x) = h(y)</code>. | ||
| </li> | ||
| <li> | ||
| <b>Brute force</b>. For passwords and other data with limited | ||
| input space, if you know a hash value <code>h(x)</code> | ||
geoffw0 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| you should not be able to find the input <code>x</code> even using | ||
| a brute force attack (without significant computational effort). | ||
| </li> | ||
| </ul> | ||
|
|
||
| <p> | ||
| As an example, both MD5 and SHA-1 are known to be vulnerable to collision attacks. | ||
| </p> | ||
|
|
||
| <p> | ||
| All of MD5, SHA-1, SHA-2 and SHA-3 are weak against offline brute forcing, so | ||
| they are not suitable for hashing passwords. This includes SHA-224, SHA-256, | ||
| SHA-384 and SHA-512, which are in the SHA-2 family. | ||
geoffw0 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| </p> | ||
|
|
||
| <p> | ||
| Since it's OK to use a weak cryptographic hash function in a non-security | ||
| context, this query only alerts when these are used to hash sensitive | ||
| data (such as passwords, certificates, usernames). | ||
| </p> | ||
|
|
||
| </overview> | ||
| <recommendation> | ||
|
|
||
| <p> | ||
| Ensure that you use a strong, modern cryptographic hash function, such as: | ||
| </p> | ||
|
|
||
| <ul> | ||
| <li> | ||
| Argon2, scrypt, bcrypt, or PBKDF2 for passwords and other data with limited input space where | ||
| a dictionary-like attack is feasible. | ||
| </li> | ||
| <li> | ||
| SHA-2, or SHA-3 in other cases. | ||
| </li> | ||
| </ul> | ||
|
|
||
| <p> | ||
| Note that special purpose algorithms, which are used to ensure that a message comes from a | ||
| particular sender, exist for message authentication. These algorithms should be used when | ||
| appropriate, as they address common vulnerabilities of simple hashing schemes in this context. | ||
| </p> | ||
|
|
||
| </recommendation> | ||
| <example> | ||
|
|
||
| <p> | ||
| The following examples show hashing sensitive data using the MD5 hashing algorithm that is known to be | ||
| vulnerable to collision attacks, and hashing passwords using the SHA-3 algorithm that is weak to brute | ||
| force attacks: | ||
| </p> | ||
| <sample src="WeakSensitiveDataHashingBad.rs"/> | ||
|
|
||
| <p> | ||
| To make these secure, we can use the SHA-3 algorithm for sensitive data and Argon2 for passwords: | ||
| </p> | ||
| <sample src="WeakSensitiveDataHashingGood.rs"/> | ||
|
|
||
| </example> | ||
| <references> | ||
| <li> | ||
| OWASP: | ||
| <a href="https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html"> | ||
| Password Storage Cheat Sheet | ||
| </a> | ||
| and | ||
| <a href="https://cheatsheetseries.owasp.org/cheatsheets/Transport_Layer_Security_Cheat_Sheet.html"> | ||
| Transport Layer Security Cheat Sheet | ||
| </a> | ||
geoffw0 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| </li> | ||
| <li> | ||
| GitHub: | ||
| <a href="https://github.com/RustCrypto/hashes?tab=readme-ov-file#rustcrypto-hashes"> | ||
| RustCrypto: Hashes | ||
| </a> | ||
| and | ||
| <a href="https://github.com/RustCrypto/password-hashes?tab=readme-ov-file#rustcrypto-password-hashes"> | ||
| RustCrypto: Password Hashes | ||
| </a> | ||
geoffw0 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| </li> | ||
| <li> | ||
| The RustCrypto Book: | ||
| <a href="https://rustcrypto.org/key-derivation/hashing-password.html"> | ||
| Password Hashing | ||
| </a> | ||
geoffw0 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| </li> | ||
| </references> | ||
|
|
||
| </qhelp> | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.