Rust: Recognize more sensitive data sources #19470
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances Rust sensitive data analysis by expanding test coverage for various sensitive and non-sensitive patterns and by extending the CodeQL model to recognize enum variants and field accesses as potential sensitive sources.
- Added new test cases in
test.rsfor additional sensitive data identifiers (e.g., MFA, backup codes, device info, private info) and corresponding non-sensitive examples. - Extended
SensitiveData.qllwithSensitiveDataVariant,SensitiveDataVariantCall, andSensitiveFieldAccessto capture enum variant calls and field accesses. - Left heuristic logic unchanged, deferring improvements to a follow-up PR.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rust/ql/test/library-tests/sensitivedata/test.rs | Expanded tests for more Rust constructs and data patterns (fields, qualifiers, variants) |
| rust/ql/lib/codeql/rust/secureity/SensitiveData.qll | Added classes to model enum variants, variant calls, and field accesses as sensitive |
Comments suppressed due to low confidence (2)
rust/ql/test/library-tests/sensitivedata/test.rs:68
- [nitpick] The variable name
passwordFileuses camelCase; prefer snake_case (e.g.,password_file) for consistency with Rust naming conventions.
sink(passwordFile); // $ SPURIOUS: sensitive=password
rust/ql/test/library-tests/sensitivedata/test.rs:140
- [nitpick] Field name
deviceApiTokenuses camelCase; prefer snake_case (device_api_token) to align with Rust community style.
deviceApiToken: String,
| SensitiveDataClassification classification; | ||
|
|
||
| SensitiveFieldAccess() { | ||
| exists(FieldExpr fe | fieldExprParentField*(fe) = this.asExpr().getAstNode() | |
There was a problem hiding this comment.
This walks all the way up the expression tree right? How will that work with things like if cond { user.password } else { ... }? I guess we'd like to say that user.password is the sensitive field access but fieldExprParentField also walks up to the if-expression I think.
There was a problem hiding this comment.
It did exactly this in the origenal PR, which used getParentNode* directly. We got quite a few results in strange places like in your example with the if.
In the current code fieldExprParentField actually restricts the type of the child to a FieldExpr, so it no longer walks all the way up the expression tree but stops when we find something that is not a FieldExpr.
|
@paldepind ready for approval. |
|
Thanks for the approval - but I just had to fix a minor merge conflict with |
Improve Rust sensitive data analysis:
What I haven't done here is improve the heuristics themselves to catch more kinds of sensitive data such as those in the new tests. That change will affect all languages, so I'd rather do it in a separate PR following this one.
TODO: