Rust: Weak hashing query #18471

geoffw0 · 2025-01-10T12:43:03Z

Add a weak hashing query for Rust (rust/weak-sensitive-data-hashing). We already had the necessary cryptography concepts, I recently added a sensitive data library, and @hvitved added the ability to express sources and sinks in MaD - which is everything needed for the query. Most of the new code is closely based on what we have in other languages, e.g. Ruby.

Naturally there are plans to add further models and data flow to catch more of the test cases in future, after the basic query has been merged.

Needs:

code review
docs review
DCA run

github-actions · 2025-01-10T12:43:59Z

QHelp previews:

rust/ql/src/queries/security/CWE-328/WeakSensitiveDataHashing.qhelp

Use of a broken or weak cryptographic hashing algorithm on sensitive data

A broken or weak cryptographic hash function can leave data vulnerable, and should not be used in security-related code.

A strong cryptographic hash function should be resistant to:

Pre-image attacks. If you know a hash value h(x), you should not be able to easily find the input x.
Collision attacks. If you know a hash value h(x), you should not be able to easily find a different input y with the same hash value h(x) = h(y).
Brute force. For passwords and other data with limited input space, if you know a hash value h(x), you should not be able to find the input x even using a brute force attack (without significant computational effort).
As an example, both MD5 and SHA-1 are known to be vulnerable to collision attacks.

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.

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

Recommendation

Ensure that you use a strong, modern cryptographic hash function, such as:

Argon2, scrypt, bcrypt, or PBKDF2 for passwords and other data with limited input space where a dictionary-like attack is feasible.
SHA-2, or SHA-3 in other cases.
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.

Example

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:

// MD5 is not appropriate for hashing sensitive data.
let mut md5_hasher = md5::Md5::new();
...
md5_hasher.update(emergency_contact); // BAD
md5_hasher.update(credit_card_no); // BAD
...
my_hash = md5_hasher.finalize();

// SHA3-256 is not appropriate for hashing passwords.
my_hash = sha3::Sha3_256::digest(password); // BAD

To make these secure, we can use the SHA-3 algorithm for sensitive data and Argon2 for passwords:

// SHA3-256 *is* appropriate for hashing sensitive data.
let mut sha3_256_hasher = sha3::Sha3_256::new();
...
sha3_256_hasher.update(emergency_contact); // GOOD
sha3_256_hasher.update(credit_card_no); // GOOD
...
my_hash = sha3_256_hasher.finalize();

// Argon2 is appropriate for hashing passwords.
let argon2_salt = argon2::password_hash::Salt::from_b64(salt)?;
my_hash = argon2::Argon2::default().hash_password(password.as_bytes(), argon2_salt)?.to_string(); // GOOD

References

OWASP: Password Storage Cheat Sheet and Transport Layer Security Cheat Sheet .
GitHub: RustCrypto: Hashes and RustCrypto: Password Hashes .
The RustCrypto Book: Password Hashing .
Common Weakness Enumeration: CWE-327.
Common Weakness Enumeration: CWE-328.
Common Weakness Enumeration: CWE-916.

rust/ql/lib/codeql/rust/security/WeakSensitiveDataHashingExtensions.qll

geoffw0 · 2025-01-10T14:39:54Z

rust/ql/test/library-tests/dataflow/taint/TaintFlowStep.expected

-| file://:0:0:0:0 | [summary param] 0 in lang:alloc::_::crate::fmt::format | file://:0:0:0:0 | [summary] to write: ReturnValue in lang:alloc::_::crate::fmt::format | MaD:9 |
-| file://:0:0:0:0 | [summary param] self in lang:alloc::_::<crate::string::String>::as_str | file://:0:0:0:0 | [summary] to write: ReturnValue in lang:alloc::_::<crate::string::String>::as_str | MaD:7 |
+| file://:0:0:0:0 | [summary param] 0 in lang:alloc::_::crate::fmt::format | file://:0:0:0:0 | [summary] to write: ReturnValue in lang:alloc::_::crate::fmt::format | MaD:14 |
+| file://:0:0:0:0 | [summary param] self in lang:alloc::_::<crate::string::String>::as_str | file://:0:0:0:0 | [summary] to write: ReturnValue in lang:alloc::_::<crate::string::String>::as_str | MaD:12 |


@hvitved this test is unrelated to the PR, apart from that they both use models-as-data. I think that means we shouldn't be outputting these IDs in the tests?

We should probably be translating the IDs, like we do in query tests.

How is that done?

We would have to pull out the relevant bits from the ShowProvenance module.

Do you want to do this, or shall I do this as a follow-up PR?

Let's do it on a separate PR.

geoffw0 · 2025-01-10T19:03:54Z

DCA LGTM (uneventful).

rust/ql/lib/codeql/rust/security/WeakSensitiveDataHashingExtensions.qll

rust/ql/src/queries/security/CWE-328/WeakSensitiveDataHashing.ql

hvitved · 2025-01-13T08:28:13Z

rust/ql/test/library-tests/dataflow/taint/TaintFlowStep.expected

-| file://:0:0:0:0 | [summary param] 0 in lang:alloc::_::crate::fmt::format | file://:0:0:0:0 | [summary] to write: ReturnValue in lang:alloc::_::crate::fmt::format | MaD:9 |
-| file://:0:0:0:0 | [summary param] self in lang:alloc::_::<crate::string::String>::as_str | file://:0:0:0:0 | [summary] to write: ReturnValue in lang:alloc::_::<crate::string::String>::as_str | MaD:7 |
+| file://:0:0:0:0 | [summary param] 0 in lang:alloc::_::crate::fmt::format | file://:0:0:0:0 | [summary] to write: ReturnValue in lang:alloc::_::crate::fmt::format | MaD:14 |
+| file://:0:0:0:0 | [summary param] self in lang:alloc::_::<crate::string::String>::as_str | file://:0:0:0:0 | [summary] to write: ReturnValue in lang:alloc::_::<crate::string::String>::as_str | MaD:12 |


We should probably be translating the IDs, like we do in query tests.

hvitved · 2025-01-13T08:30:26Z

rust/ql/test/query-tests/security/CWE-328/test.rs

+    let mut md5_hasher = md5::Md5::new();
+    md5_hasher.update(b"abc");
+    md5_hasher.update(harmless);
+    md5_hasher.update(password); // $ MISSING: Alert[rust/weak-sensitive-data-hashing]


Do we know why this is missing?

I'm not sure exactly. It's the method calls (rather than function calls) where we're having difficulty.
(MethodCallExpr).getResolvedCrateOrigin() and (MethodCallExpr).getResolvedPath() work as expected.

Is resolving method call crates / paths and method calls as MAD sources something we expect to work (tested?), or are there known to be missing features here?

I can take a look.

The reason it doesn't work is because your logic is restricted to calls where the function is a PathExpr, which is not the case above. In order to extend to the case above, I think we need to know the type of md5_hasher, but that is currently not available.

OK, I'll make a note in the issue for improvements to this query that we're waiting for type inference before we can fix that bit by replacing the PathExpr.

mchammer01

@geoffw0 - Approving this on behalf of Docs ✨
Left a few minor comments/suggestions.

rust/ql/src/queries/security/CWE-328/WeakSensitiveDataHashing.qhelp

Co-authored-by: mc <[email protected]>

geoffw0 · 2025-01-13T10:51:04Z

Thanks for the reviews.

all docs suggestions accepted.
all code suggestions accepted, in discussion above, or both.

geoffw0 · 2025-01-14T11:19:31Z

I think all issues have been resolved, I just need a re-approval (preferably from code).

geoffw0 added 10 commits January 10, 2025 11:56

Rust: Add tests for weak hashing.

509c6ff

Rust: Add query framework.

8f4a520

Rust: Add sensitive data sources.

d72b978

Rust: Add hash function sinks.

ae0f4f1

Rust: Add models for an alternative md5 library.

babfa75

Rust: Add .qhelp.

5938659

Rust: Add another reference.

9b8f561

Rust: Update test for changes on main.

ae26cd6

Rust: Move ModelledHashOperation to a more logical location.

c115169

Rust: Make a type more accurate.

bb4322c

geoffw0 added the Rust Pull requests that update Rust code label Jan 10, 2025

Copilot bot review requested due to automatic review settings January 10, 2025 12:43

github-actions bot added the documentation label Jan 10, 2025

This comment was marked as off-topic.

Sign in to view

Rust: Tweak .qhelp layout.

39a38c4

github-advanced-security bot found potential problems Jan 10, 2025

View reviewed changes

geoffw0 added 3 commits January 10, 2025 12:53

Rust: Address QL-for-QL comments.

ad26822

Rust: Correct the qhelp.

19d3e9d

Rust: Unrelated MaD test impact. :(

1b6c289

geoffw0 commented Jan 10, 2025

View reviewed changes

Rust: Attempt to fix the test on CI.

edd1f25

geoffw0 added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Jan 10, 2025

mchammer01 self-requested a review January 13, 2025 07:51

hvitved reviewed Jan 13, 2025

View reviewed changes

mchammer01 previously approved these changes Jan 13, 2025

View reviewed changes

Apply suggestions from code review

722b7bb

Co-authored-by: mc <[email protected]>

geoffw0 dismissed mchammer01’s stale review via 722b7bb January 13, 2025 10:28

Rust: More suggestions from review.

676141b

Rust: Autoformat.

e61d6ae

hvitved approved these changes Jan 14, 2025

View reviewed changes

geoffw0 merged commit 6402aa5 into github:main Jan 14, 2025
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust: Weak hashing query #18471

Rust: Weak hashing query #18471

geoffw0 commented Jan 10, 2025 •

edited

Loading

github-actions bot commented Jan 10, 2025 •

edited

Loading

Use of a broken or weak cryptographic hashing algorithm on sensitive data

Recommendation

Example

References

This comment was marked as off-topic.

geoffw0 Jan 10, 2025

hvitved Jan 13, 2025

geoffw0 Jan 13, 2025

hvitved Jan 13, 2025

geoffw0 Jan 13, 2025

hvitved Jan 13, 2025

geoffw0 commented Jan 10, 2025

hvitved Jan 13, 2025

hvitved Jan 13, 2025

geoffw0 Jan 13, 2025

geoffw0 Jan 14, 2025

hvitved Jan 14, 2025

hvitved Jan 14, 2025

geoffw0 Jan 14, 2025

geoffw0 Jan 14, 2025

mchammer01 left a comment

geoffw0 commented Jan 13, 2025

geoffw0 commented Jan 14, 2025

Rust: Weak hashing query #18471

Rust: Weak hashing query #18471

Conversation

geoffw0 commented Jan 10, 2025 • edited Loading

github-actions bot commented Jan 10, 2025 • edited Loading

Use of a broken or weak cryptographic hashing algorithm on sensitive data

Recommendation

Example

References

This comment was marked as off-topic.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffw0 commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchammer01 left a comment

Choose a reason for hiding this comment

geoffw0 commented Jan 13, 2025

geoffw0 commented Jan 14, 2025

geoffw0 commented Jan 10, 2025 •

edited

Loading

github-actions bot commented Jan 10, 2025 •

edited

Loading