Skip to content

Rust: Update legacy MaD models 2 #19942

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 14 commits into from
Jul 10, 2025
Merged

Rust: Update legacy MaD models 2 #19942

merged 14 commits into from
Jul 10, 2025

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Jul 1, 2025

Update some more legacy MaD models to the new model format (continues from #19934 , but should be independent of that).

@geoffw0 geoffw0 added no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code labels Jul 1, 2025
@geoffw0
Copy link
Contributor Author

geoffw0 commented Jul 2, 2025

Accepted test regressions. There are almost no results left for rust/weak-sensitive-data-hashing, this needs addressing before the PR can be merged (@hvitved please advise).

We also lose some results in a test case involving a loop, similar to the one discussed in #19934 (comment) .

@@ -11,8 +11,8 @@ fn test_hash_algorithms(

// MD5
_ = md5::Md5::digest(harmless);
_ = md5::Md5::digest(credit_card_no); // $ Alert[rust/weak-sensitive-data-hashing]
_ = md5::Md5::digest(password); // $ Alert[rust/weak-sensitive-data-hashing]
_ = md5::Md5::digest(credit_card_no); // $ MISSING: Alert[rust/weak-sensitive-data-hashing]
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently getStaticTarget() fails on md5::Md5::digest. This are the relevant lines:

pub type Md5 = CoreWrapper<Md5Core>;

impl<D: FixedOutput + Default + Update + HashMarker> Digest for D {
    fn digest(data: impl AsRef<[u8]>) -> Output<Self> { ... }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@hvitved I guess the type variable in for D is the cause of the trouble, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we don't currently handle impl<T> Foo for T; I have added this instance to our internal issue tracking this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I've just created an issue to track test results we are losing but expect to get back with type inference improvements - the idea being we can merge this before we get it back if we want to.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jul 10, 2025

I think the only remaining issue is the CWE-328 test regressions, which we now have an issue two issues tracking. I'm taking this out of draft and starting a DCA run...

@geoffw0 geoffw0 marked this pull request as ready for review July 10, 2025 10:10
@geoffw0 geoffw0 requested a review from a team as a code owner July 10, 2025 10:10
@geoffw0
Copy link
Contributor Author

geoffw0 commented Jul 10, 2025

DCA LGTM, apart from lost query sinks and cryptographic operations. Likely all are the CWE-328 issue we picked up in tests.

(there's some discussion going on elsewhere about whether to temporarily accept these regressions, or put in a temporary fix; other than that I think we're good to merge now)

aibaars
aibaars previously approved these changes Jul 10, 2025
@geoffw0
Copy link
Contributor Author

geoffw0 commented Jul 10, 2025

Merge conflict fixed.

@geoffw0 geoffw0 merged commit 439cf7a into github:main Jul 10, 2025
17 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.

3 participants