Skip to content

Add ruzstd uninit/out-of-bounds memory reads advisory #2147

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 3 commits into from
Nov 28, 2024

Conversation

paolobarbolini
Copy link
Contributor

@paolobarbolini paolobarbolini commented Nov 27, 2024

Creates the advisory for the vulnerability reported at KillingSpark/zstd-rs#75, fixed by KillingSpark/zstd-rs#76, and released as https://github.com/KillingSpark/zstd-rs/releases/tag/v0.7.3.

@Shnatsel
Copy link
Member

Thank you for the report!

We normally use the informational = unsound variant for unsound APIs that can be misused, but I see the function is not exposed via the public API.

AFAIK it's not settled whether it is OK to copy uninitialized u8 values without ever observing them, and the compiler currently does not exploit that for optimization purposes.

However, I am concerned about the risk of exposing contents of uninitialized memory via a crafted input. I would like to hear from the maintainers about the potential impact of this issue.

@KillingSpark
Copy link

KillingSpark commented Nov 28, 2024

Hi, maintainer of ruzstd here:

As far as I can tell this could, with specially crafted inputs, result in the decoded output containing up to 15 bytes of heap memory that didn't belong to the allocation of the ringbuffer used during decoding. More specifically up to 15 bytes directly after that allocation.

So this could maybe be used to exfiltrate data from a process, if the process

  • Decodes an attacker controlled archive
  • Passes the decoded contents back to the attacker

Edit: The 15 bytes are the result of copying data using 128bit registers on SSE platforms. On other platforms a usize is used so on those it would be size_of(usize) - 1 bytes that could be included in the output

Edit 2: That's 15 bytes per case that the UB is hit which can happen multiple times per archive. The Buffer will re-allocate if it gets full, similar to Vec but it won't if the user drains it regularly. So the attacker might read a few different locations multiple times over the course of the decoding process, depending on how ringbuffer is drained.

@KillingSpark
Copy link

Can I ask what the recommended/common next steps are here? This is the first handling such a situation as a maintainer.

My guess would be:

  • Find the released versions that include the error
  • Yank them from crates.io
  • Make new releases with a bumped minor that only fix this and change nothing else
  • Delete the affected releases on github

Am I missing something?

@paolobarbolini
Copy link
Contributor Author

paolobarbolini commented Nov 28, 2024

I've updated the advisory with some of the feedback.

Find the released versions that include the error

I think the issue was introduced by KillingSpark/zstd-rs@2ee37fd. I've run miri on a checkout of v0.6.0 and it found no issues. Because of this I put < 0.7.0 as unaffected.

Make new releases with a bumped minor that only fix this and change nothing else

I've put >= 0.7.3 into the patched versions of the advisory.

So this could maybe be used to exfiltrate data from a process, if the process

  • Decodes an attacker controlled archive

  • Passes the decoded contents back to the attacker

I added the memory-exposure category.

@paolobarbolini paolobarbolini marked this pull request as ready for review November 28, 2024 10:01
@paolobarbolini
Copy link
Contributor Author

Cross posting here too that the v0.7.3 release fixing the issue is out

paolobarbolini added a commit to paolobarbolini/rust that referenced this pull request Nov 28, 2024
This fixes the yet to be published advisory for uninit/out-of-bounds
memory reads and potential exposure.

See rustsec/advisory-db#2147
Co-authored-by: Paolo Barbolini <paolo@paolo565.org>
@Shnatsel Shnatsel merged commit c91b809 into rustsec:main Nov 28, 2024
1 check passed
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.

3 participants