Fix two buffer overruns reachable from safe code

[jameysharp]

I was auditing the zstd crate's usage of unsafe blocks as part of a larger effort to audit the transitive dependencies of Wasmtime. I haven't audited zstd_safe in detail, but I believe the following two issues are the only ones which impact the few unsafe blocks in the zstd crate. Hopefully I'll have time later to go look at zstd_safe in depth as well.

Issue 1:

When zstd::stream::raw::NoOp::run computes the length of the slice that it should copy, it does not take output.pos() into account. Instead it can write up to offset output.dst.capacity() + output.pos(), if the input is long enough. So even someone who uses NoOp exactly as it's intended to be used may trigger a buffer overrun.

The minimal fix for this issue is to use output.dst.capacity() - output.pos() instead, in the call to min.

Issue 2:

zstd_safe::OutBuffer is documented as having the invariant that "pos <= dst.capacity()". However, it can't reliably enforce that invariant. Because dst is a public field, its capacity can be changed without updating pos. For example, if dst is a Vec, someone could resize it to be smaller than pos and then call shrink_to_fit on it. Additionally, it's possible to entirely replace one Vec with another, which may have a smaller capacity. In either case, the invariant would be violated without using any unsafe blocks.

As a result I believe the zstd_safe::CCtx/DCtx methods which use OutBuffer are currently vulnerable to buffer overruns in the underlying C library even if the caller uses only safe Rust.

In addition: zstd::stream::raw::NoOp::run contains the only unsafe blocks in the zstd crate. The first of the three unsafe blocks there is only safe if OutBuffer's invariant is preserved. Since its invariant can't currently be assumed even if the callers use only safe code, this is a potential buffer overrun as well.

I've audited all uses of the private OutBuffer::pos field and I believe they're safe, so all possibly-unsafe accesses pass through the public fn pos() accessor. Therefore I've added an assertion there to check that the invariant still holds. I believe this method is called infrequently and not performance-critical.

[gyscos]

Hi, and thanks for the investigation!

When zstd::stream::raw::NoOp::run computes the length of the slice that it should copy, it does not take output.pos() into account. Instead it can write up to offset output.dst.capacity() + output.pos(), if the input is long enough. So even someone who uses NoOp exactly as it's intended to be used may trigger a buffer overrun.

Arf indeed, thanks for the fix!

Because dst is a public field, its capacity can be changed without updating pos.

Whoops indeed. I think we probably don't need this field to be public. We only need to access dst as a &mut [u8], and expose the capacity. That'd be a breaking change, but I think we need it to preserve the invariant.

I pushed this proposed change in 3cb6088.

Therefore I've added an assertion there to check that the invariant still holds.

I'm afraid assertions are entirely skipped in release mode, so this would still be vulnerable (but detectable in debug if the user is lucky).

[jameysharp]

Nice, I think making dst private is a much better solution; I just didn't want to propose a breaking change for a security fix if I could avoid it. But for the record, assert! is "always checked in both debug and release builds, and cannot be disabled," according to its documentation. It's only debug_assert! that is disabled in release builds.

Thank you for merging this quickly! Do you have a schedule in mind for making a release containing these fixes?

[gyscos]

But for the record, assert! is "always checked in both debug and release builds, and cannot be disabled," according to its documentation. It's only debug_assert! that is disabled in release builds.

Ah, good to know! Not sure where this misconception came from.

Thank you for merging this quickly! Do you have a schedule in mind for making a release containing these fixes?

Still wondering if I can sneakily not bump the version, as it hopefully affects very few real use cases. In particular most users of the zstd crate should be unaffected by this technically breaking change.
Bumping the version, while more correct, means that both zstd-safe and zstd need a breaking change, and so every user will need to explicitly bump their version to benefit from the soundness fix. :^S

[jameysharp]

Bumping the version, while more correct, means that both zstd-safe and zstd need a breaking change, and so every user will need to explicitly bump their version to benefit from the soundness fix.

I've been thinking about this a bit. Yeah, it would be nice to let cargo automatically pick up the new version, but you'd know better than I do whether making dst private is likely to break anybody's builds. Of course, if nobody accesses that field then everybody is safe from that particular soundness bug. In a way, only people whose builds would break if you don't bump the version could benefit from you not bumping the version. (That's not quite true since there's also the buffer overrun in NoOp, although I imagine that's not widely used so I'm not that concerned about it.)

My suggestion would be to do a patch release with only my assert, leaving dst public so that there's no API break; and also do a version bump release containing your more invasive dst visibility changes. That way existing users at least get a panic instead of a buffer overrun, while people who are willing to update their Cargo.toml are guaranteed not to hit the panic at all.

Whichever approach you pick, I think it'll also help to file a RustSec advisory so that people are more likely to notice the new version. In addition, once there's a release I can record a passing audit for, I'll also add a cargo vet "violation" for older versions to the Bytecode Alliance audit set for anyone who's importing that.