Skip to content

Rust: Add model for str.trim #19310

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 17, 2025
Merged

Rust: Add model for str.trim #19310

merged 8 commits into from
Apr 17, 2025

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Apr 15, 2025

Add model for str.trim. Make String and str models consistent. Fixes a couple of test cases.

@geoffw0 geoffw0 added no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code labels Apr 15, 2025
@Copilot Copilot AI review requested due to automatic review settings April 15, 2025 10:56
@geoffw0 geoffw0 requested a review from a team as a code owner April 15, 2025 10:56
Copy link
Contributor

@Copilot 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 adds a model for str.trim to bring consistency between String and str models, and adjusts test cases to reflect the expected security warnings.

  • Updates test cases to include alerts for weak hash usage on password strings.
  • Adds a model for "::trim" in lang-core.model.yml.
  • Adds a model for "crate::string::String::trim" in lang-alloc.model.yml.

Reviewed Changes

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

File Description
rust/ql/test/query-tests/security/CWE-328/test.rs Updates test comments to include alerts for weak-sensitive-data-hashing
rust/ql/lib/codeql/rust/frameworks/stdlib/lang-core.model.yml Adds a model for ::trim (and related string conversion methods)
rust/ql/lib/codeql/rust/frameworks/stdlib/lang-alloc.model.yml Adds models for String parsing and trimming in accordance with str models
Files not reviewed (2)
  • rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected: Language not supported
  • rust/ql/test/query-tests/security/CWE-328/WeakSensitiveDataHashing.expected: Language not supported
Comments suppressed due to low confidence (1)

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

  • Update the comment to remove the '$ MISSING:' prefix for consistency with the other alert comments.
_ = md5::Md5::digest(std::str::from_utf8(password_arr).unwrap()); // $ MISSING: Alert[rust/weak-sensitive-data-hashing]

Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

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

Looks good to me, let mark some steps as value instead of taint.

Comment on lines 43 to 45
- ["lang:core", "<str>::as_str", "Argument[self]", "ReturnValue", "taint", "manual"]
- ["lang:core", "<str>::as_bytes", "Argument[self]", "ReturnValue", "taint", "manual"]
- ["lang:core", "<str>::to_string", "Argument[self]", "ReturnValue", "taint", "manual"]
Copy link
Contributor

Choose a reason for hiding this comment

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

The function <str>::as_str is value preserving, it just returns self. I think as_bytes should be considered value preserving as well. It is defined as and doesn't change the value, only its type.

    pub const fn as_bytes(&self) -> &[u8] {
        // SAFETY: const sound because we transmute two types with the same layout
        unsafe { mem::transmute(self) }
    }

I'm a little in doubt of whether to consider str::to_string to be value preserving as well. I'm not sure whether we consider copies to be taint or value preserving in other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done.

I'm a little in doubt of whether to consider str::to_string to be value preserving as well.

I would say no, because in general to_string is not value preserving.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, it's only value "preserving" for String and str.

aibaars
aibaars previously approved these changes Apr 15, 2025
@geoffw0
Copy link
Contributor Author

geoffw0 commented Apr 16, 2025

CI failure fixed.

@geoffw0 geoffw0 merged commit 7e108a8 into github:main Apr 17, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants