Rust: Update StreamCipherInit to use getCanonicalPath. #20238
Conversation
| .getCanonicalPath() | ||
| .matches(["%::new", "%::new_from_slice", "%::new_with_eff_key_len", "%::new_from_slices"]) and | ||
| // extract the algorithm name from the type of `ce` or its receiver. | ||
| exists(Type t, TypePath tp | |
Check warning
Code scanning / CodeQL
Omittable 'exists' variable Warning
I believe #20243 should fix this. |
|
Thanks for the quick fix - results are much better now! |
There was a problem hiding this comment.
Pull Request Overview
This pull request updates the StreamCipherInit class in RustCrypto.qll to use getCanonicalPath instead of the previous path-based approach for identifying cryptographic algorithm instances. The change aims to improve detection accuracy by leveraging type inference rather than parsing path expressions.
- Refactors algorithm detection to use type inference and canonical paths
- Updates test expectations to reflect improved detection of weak cryptographic algorithms
- Addresses missing results for
crypto_common::KeyInit::newcalls
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rust/ql/lib/codeql/rust/frameworks/rustcrypto/RustCrypto.qll | Replaces path parsing with type inference approach using getCanonicalPath and adds validation against known cryptographic algorithms |
| rust/ql/test/query-tests/security/CWE-327/test_cipher.rs | Adds new test case and updates expected results comments to reflect detection changes |
| rust/ql/test/query-tests/security/CWE-327/BrokenCryptoAlgorithm.expected | Updates expected test results with additional detected weak algorithm instances |
| rust/ql/test/query-tests/security/CWE-327/CONSISTENCY/PathResolutionConsistency.expected | Updates line number reference for consistency test |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| @@ -129,15 +133,15 @@ fn test_cbc( | |||
| _ = aes_cipher1.encrypt_padded_mut::<aes::cipher::block_padding::Pkcs7>(data, data_len).unwrap(); | |||
|
|
|||
| // des (broken) | |||
| let des_cipher1 = cbc::Encryptor::<des::Des>::new(key.into(), iv.into()); // $ Alert[rust/weak-cryptographic-algorithm] | |||
| let des_cipher1 = cbc::Encryptor::<des::Des>::new(key.into(), iv.into()); // $ MISSING: Alert[rust/weak-cryptographic-algorithm] | |||
There was a problem hiding this comment.
I note that this call and the two below (also) do not resolve because we currently do not support blanket implementations.
There was a problem hiding this comment.
I see, and I note the issue tracking this. 👍
Co-authored-by: Tom Hvitved <hvitved@github.com>
Update
StreamCipherInitto usegetCanonicalPath.@hvitved this change introduces a lot of missing results for calls to
crypto_common::KeyInit::new. This is a bit odd as it's an associated function (so no receiver to worry about type inference on) and we do find agetStaticTarget()for thenewcall - just nogetCanonicalPath()for it. The provided methodnew_from_slicein the same trait seems to work fine. If possible I'd really like to have this fixed before we merge the change (🤞).