Skip to content

Add advisory for segmentation fault in grcov and fast-float#2189

Closed
nyw0102 wants to merge 2 commits intorustsec:mainfrom
nyw0102:main
Closed

Add advisory for segmentation fault in grcov and fast-float#2189
nyw0102 wants to merge 2 commits intorustsec:mainfrom
nyw0102:main

Conversation

@nyw0102
Copy link
Contributor

@nyw0102 nyw0102 commented Jan 13, 2025

No description provided.

@nyw0102 nyw0102 changed the title Add advisory for segmentation fault in grcov Add advisory for segmentation fault in grcov and fast-float Jan 13, 2025
Copy link
Member

@Shnatsel Shnatsel left a comment

Choose a reason for hiding this comment

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

Thank you for the report!

I think the fast-float advisory will need some work. Could you split the grcov advisory into a separate PRs, so we could merge each one as soon as it's ready?

functions = { "fast_float::common::AsciiStr::first" = ["<=0.2.2"] }

[versions]
patched = [">0.2.2"]
Copy link
Member

Choose a reason for hiding this comment

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

I checked crates.io, and the fast-float crate was last updated 4 years ago. A patched version has never been published. Only fast-float2 crate has been patched.

So we would need 2 advisories - one for fast-float without a patched version, the other for fast-float2 with a patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the advice. It would be good to just make PR without the patched version at this moment?

Copy link
Member

Choose a reason for hiding this comment

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

For fast_float, yes. We'll also need an advisory for fast_float2 with a patched version. It's okay to put both of those in the same PR since those are basically the same issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well noted. I will also make PR for fast_float2!

Comment on lines +15 to +19
# Segmentation fault due to lack of bound check
In this case, the "fast_float::common::AsciiStr::first" method within the "AsciiStr" struct
uses the unsafe keyword to access memory without performing bounds checking.
Specifically, it directly dereferences a pointer offset by "self.ptr".
This approach violates Rust’s memory safety guarantees, as it can lead to invalid memory access if empty buffer is provided.
Copy link
Member

Choose a reason for hiding this comment

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

The memory access is only read, not write, which is worth noting. And I understand it always dereferences the null pointer, is that correct? Or is there a large offset that can be appended to read arbitrary memory?

cc @Alexhuszagh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Frist of all, the null pointer dereference occurs when the function "fast_float::common::AsciiStr::first" method takes empty string "". Since the method dereference the value of "self.ptr" without bound checking, the null pointer dereference occurs. I think it is quite similar to buffer overflow when to read index 0 in object with size 0. So, the answer of the question is that the dereference occurs when the input of the "fast_float::common::AsciiStr::first" method is empty string.

I'm thankful to Rust community tring to ensure memory safety in various ways and I think this case is might be the one to ensure the memory safety in the crate written in Rust language! I think there are similar RUSTSEC cases to this crash. I attached some references for your consideration.

RUSTSEC-2024-0400
RUSTSEC-2023-0044

@nyw0102
Copy link
Contributor Author

nyw0102 commented Jan 15, 2025

Thank you for the reply! Sorry for the combined advisory of two crates. I will split this PR into each program!

@nyw0102 nyw0102 closed this by deleting the head repository Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants