Skip to content

Conversation

@geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Dec 1, 2025

Add heuristic sinks for passwords, initialization vectors, nonces and salts. I haven't pushed these too far (e.g. using partial string matches) as they already work quite well without spending lots of time tuning them.

On the MRVA-1000 we find 2,071 new results across 85 projects as a result of these sinks - more than the query finds with models-as-data sinks. These sinks LGTM (there are quite a few results in tests on certain projects, which I don't think is a problem; and I found one spurious result due to arithmetic involving a constant, test case added below; I'm pretty confident of the new sinks overall).

Copilot AI review requested due to automatic review settings December 1, 2025 12:44
@geoffw0 geoffw0 requested a review from a team as a code owner December 1, 2025 12:44
@geoffw0 geoffw0 added the Rust Pull requests that update Rust code label Dec 1, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the rust/hard-coded-cryptographic-value query with new heuristic sinks that identify parameters named password, iv (initialization vector), nonce, and salt. The heuristics use exact parameter name matching to avoid false positives.

  • Adds heuristic sink detection for cryptographic parameters based on parameter names
  • Implements barrier logic to report only the closest sink instance along a flow path
  • Provides comprehensive test coverage including edge cases and known limitations

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
rust/ql/lib/codeql/rust/security/HardcodedCryptographicValueExtensions.qll Adds HeuristicSinks class that matches function parameters named password, iv, nonce, or salt and treats them as cryptographic value sinks
rust/ql/src/queries/security/CWE-798/HardcodedCryptographicValue.ql Adds isBarrierOut predicate to prevent reporting multiple sinks along the same flow path
rust/ql/test/query-tests/security/CWE-798/test_heuristic.rs Adds comprehensive test cases for the new heuristic sinks, including positive cases and edge cases with arithmetic operations
rust/ql/test/query-tests/security/CWE-798/HardcodedCryptographicValue.expected Updates expected test results with new alerts and flow edges from the heuristic sinks
rust/ql/src/change-notes/2025-12-01-hard-coded-cryptographic-value.md Documents the addition of heuristic sinks for passwords, initialization vectors, nonces, and salts

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@paldepind paldepind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really great! That's a fantastic amount of new results. I have just one minor comment.

Also, an idea for the future: We could do the same for struct and enum fields whose name match the heuristic.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Dec 2, 2025

We could do the same for struct and enum fields whose name match the heuristic.

I've noted this idea on the heuristic models issue. I agree we could find more sinks this way, across this and other heuristic sinks - though it will need another round of testing.

DCA run started.

Copy link
Contributor

@paldepind paldepind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@geoffw0
Copy link
Contributor Author

geoffw0 commented Dec 3, 2025

And DCA LGTM. Merging...

@geoffw0 geoffw0 merged commit 2665d83 into github:main Dec 3, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants