Rust: new query rust/hardcoded-crytographic-value

[geoffw0]

Adds a "Hard-coded cryptographic value" (rust/hardcoded-crytographic-value) query for Rust, detecting use of hardcoded keys, passwords, salts and initialization vectors. In other languages these have sometimes been implemented as separate queries (e.g. swift/constant-password, swift/hardcoded-key, swift/constant-salt, swift/static-initialization-vector) but I see no benefit to them being separate.

Note that a few sinks have been defined for the query in this pull request but they're a bit sparse. Improving that will be follow-up work.

TODO:

• review DCA
• code review
• docs review

[Copilot]

PR Overview

This pull request adds a new query to detect hardcoded cryptographic values in Rust code along with corresponding test cases and configuration updates for dependencies and library models.

• Introduces query tests for detecting hardcoded keys, salts, and initialization vectors in various cipher configurations.
• Provides both "bad" (hardcoded) and "good" (randomly generated) examples for cryptographic key usage.
• Updates dependency options and model files to support the new query.

Reviewed Changes

File	Description
rust/ql/test/query-tests/security/CWE-798/test_cipher.rs	Adds various test cases for hardcoded cryptographic values in cipher operations.
rust/ql/test/query-tests/security/CWE-798/options.yml	Specifies dependencies and cargo check options required for the tests.
rust/ql/src/queries/security/CWE-798/HardcodedCryptographicValueBad.rs	Introduces an example of bad usage with hardcoded keys.
rust/ql/src/queries/security/CWE-798/HardcodedCryptographicValueGood.rs	Provides an example of good usage using randomly generated keys.
rust/ql/lib/codeql/rust/frameworks/genericarray.model.yml	Updates model to include generic-array methods relevant to the query.
rust/ql/lib/codeql/rust/frameworks/rustcrypto/rustcrypto.model.yml	Enhances rustcrypto model with new mappings for cryptographic key and IV usage.
rust/ql/lib/codeql/rust/frameworks/stdlib/lang-core.model.yml	Adds conversions and constant source taint tracking relevant to key initialization.

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

[github-actions]

QHelp previews:

rust/ql/src/queries/security/CWE-798/HardcodedCryptographicValue.qhelp

Hard-coded cryptographic value

Hard-coded passwords, keys, initialization vectors, and salts should not be used for cryptographic operations.

• Attackers can easily recover hard-coded values if they have access to the source code or compiled executable.
• Some hard-coded values are easily guessable.
• Use of hard-coded values may leave cryptographic operations vulnerable to dictionary attacks, rainbow tables, and other forms of cryptanalysis.

Recommendation

Use randomly generated key material, initialization vectors, and salts. Use strong passwords that are not hard-coded.

Example

The following example shows instantiating a cipher with hard-coded key material, making the encrypted data vulnerable to recovery.

let key: [u8;32] = [0;32]; // BAD: Using hard-coded keys for encryption
let cipher = Aes256Gcm::new(&key.into());

In the fixed code below, the key material is randomly generated and not hard-coded, which protects the encrypted data against recovery. A real application would also need a strategy for secure key management after the key has been generated.

let key = Aes256Gcm::generate_key(aes_gcm::aead::OsRng); // GOOD: Using randomly generated keys for encryption
let cipher = Aes256Gcm::new(&key);

References

• OWASP: Use of hard-coded password.
• OWASP: Key Management Cheat Sheet.
• O'Reilly: Using Salts, Nonces, and Initialization Vectors.
• Common Weakness Enumeration: CWE-259.
• Common Weakness Enumeration: CWE-321.
• Common Weakness Enumeration: CWE-798.
• Common Weakness Enumeration: CWE-1204.

[geoffw0]

Some of these models are bothering me. They're all based on data from getResolvedPath + getResolvedCrateOrigin and they demonstrably work but:

• it would be cleaner and easier to implement the models if they could be expressed in more general terms, e.g. in terms of traits rather than implementations.
• it's also likely (well, certain) we're missing some implementations with these models as a result (AES is not he only block cipher in existence).
• I also observed some weird platform dependence, where everything worked on my local Mac but for CI I had to add a few additional variants of models to get the same results as I was already getting locally - that's what this commit is. I don't know if there was a known quirk affecting me here or something potentially serious and undiagnosed.

@hvitved I would really appreciate your opinion on these issues. I think we will need to address them to model sources and sinks effectively.

[geoffw0]

Update on the third bullet points: the MaD IDs still don't match what I generate locally. That's surely a clue to what's going on. Hopefully I just need to update something I haven't updated on my machine??? 🤞

[hvitved]

If a model applies to all trait implementations, it should be applied to the trait itself and not all of the implementations. This is much like models applied to e.g. interfaces in C#/Java apply to all classes that implement it. This is currently not supported in Rust, but is certainly something we should do.

[geoffw0]

I'll write up an issue for this then.

[hvitved]

We could consider adding a WithReference token, which would allow us to simplify the above slightly as

["repo:https://github.com/fizyk20/generic-array.git:generic-array", "<crate::GenericArray>::from_slice", "Argument[0].WithReference", "ReturnValue", "value", "manual"]

[geoffw0]

I'm not sure I see what's special about WithReference over WithElement or any of a hundred other shortcuts we could potentially create. But if you've had good experiences with this syntactic sugar in other languages, I will defer to your experience.

[hvitved]

WithElement mostly makes a difference when you are tracking precise array indicies, like we do in Ruby, where you would otherwise have to write an "infinite" number of MaD rows (one for index 0, one for index 1, ...). WithElement generates a somewhat simpler synthetic function, which does not consist of a read step followed by a store step.

[hvitved]

If a model applies to all trait implementations, it should be applied to the trait itself and not all of the implementations. This is much like models applied to e.g. interfaces in C#/Java apply to all classes that implement it. This is currently not supported in Rust, but is certainly something we should do.

[mchammer01]

@geoffw0 - LGTM 🚀
Only a couple of very minor suggestions.

[geoffw0]

DCA:

• two results, both false positives because we don't model getrandom as a barrier. I'll add that today. I'm actually quite pleased we got these results as it shows the sources, sinks and flow are working sufficiently well to find results on real world code.
• 68 new sinks across 3 of the 24 analysed projects (again I'm quite pleased with this considering the relatively sparse initial set of models, and that not every project does cryptography anyway).

[geoffw0]

Barrier added for getrandom. I should do another DCA run when I've addressed all comments.

[geoffw0]

Regarding the current test failures, it's clear to me now that it's applying different MaD models to the same lines of code on CI (Linux) vs my local machine (Mac), possibly because the code is actually seen differently by the extractor there. I turned on pretty printing models so we can better see what's going on, and here's how I believe the models correspond between what is applied on my local machine (-) and on CI (+):

-| 2 | Sink: repo:https://github.com/RustCrypto/block-ciphers:aes; <crate::soft::Aes256 as crate::KeyInit>::new; credentials-key; Argument[0] |
+| 2 | Sink: repo:https://github.com/RustCrypto/block-ciphers:aes; <crate::autodetect::Aes256 as crate::KeyInit>::new; credentials-key; Argument[0] |

-| 4 | Sink: repo:https://github.com/RustCrypto/traits:cipher; <crate::stream_wrapper::StreamCipherCoreWrapper as crate::KeyIvInit>::new; credentials-iv; Argument[1] |
+| 7 | Sink: repo:https://github.com/RustCrypto/traits:crypto-common; crate::KeyInit::new; credentials-iv; Argument[1] |

-| 5 | Sink: repo:https://github.com/RustCrypto/traits:cipher; <crate::stream_wrapper::StreamCipherCoreWrapper as crate::KeyIvInit>::new; credentials-key; Argument[0] |
+| 4 | Sink: repo:https://github.com/RustCrypto/traits:cipher; <crate::stream_wrapper::StreamCipherCoreWrapper as crate::KeyInit>::new; credentials-key; Argument[0] |
OR
+| 8 | Sink: repo:https://github.com/RustCrypto/traits:crypto-common; crate::KeyInit::new; credentials-key; Argument[0] |

In other words, a line of code that uses the - model on my local machine is using the + model after it on the CI run. This is repeatable.

@redsun82 - this looks like it could be an extractor or platform / configuration issue?

@hvitved - being able to express MaD models on traits may well help as these look like different implementations of the same traits.

[redsun82]

@redsun82 - this looks like it could be an extractor or platform / configuration issue?

@hvitved - being able to express MaD models on traits may well help as these look like different implementations of the same traits.

I rather think this is indeed an implementation detail that may change between platforms. I think it'd be good as you point out to optionally be able to express models at the trait level rather than their implementations... Do you reckon this is this something doable @hvitved and/or @paldepind?

[hvitved]

Do you reckon this is this something doable @hvitved and/or @paldepind?

Yes, should be doable.