Skip to content
New issue

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

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

Already on GitHub? # to your account

Private function prefix() is unsound #29

Closed
Shnatsel opened this issue May 26, 2019 · 1 comment · Fixed by #30
Closed

Private function prefix() is unsound #29

Shnatsel opened this issue May 26, 2019 · 1 comment · Fixed by #30

Comments

@Shnatsel
Copy link
Contributor

The following function may perform out-of-bounds reads if used incorrectly:

fn prefix(buf: &[u8]) -> [u8; 3] {
unsafe {
[
*buf.get_unchecked(0),
*buf.get_unchecked(1),
*buf.get_unchecked(2),
]
}
}

There have already been two known cases of out-of-bounds reads due to misuse of this function: #16, #21.

In the current implementation it's the caller's responsibility to ensure no out-of-bounds reads occur. If left as-is, this function must be marked unsafe. A better option would be getting rid of unsafety entirely.

@Shnatsel
Copy link
Contributor Author

Shnatsel commented May 26, 2019

I feel memory safety is really critical for libflate for two reasons:

  1. It's used in the enormously popular reqwest crate, where it's subjected to untrusted data from the network. Assuming the limitation of reading only 2 bytes out of bounds at a time can be bypassed by supplying inputs of varying sizes, this issue skirts dangerously close to heartbleed. Actually no, libflate is a development dependency of reqwest.
  2. If I were okay with sacrificing memory safety for performance, I'd just use the popular C libraries. Memory safety should be the differentiating factor of a Rust implementation.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant