Skip to content

Commit

Permalink
Fix two buffer overruns reachable from safe code (#231)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jameysharp authored Jul 20, 2023
1 parent 025bd7d commit 79592c8
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 3 deletions.
7 changes: 4 additions & 3 deletions src/stream/raw.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,18 +96,19 @@ impl Operation for NoOp {
// Skip the prelude
let src = &input.src[input.pos..];
// Safe because `output.pos() <= output.dst.capacity()`.
let dst = unsafe { output.dst.as_mut_ptr().add(output.pos()) };
let output_pos = output.pos();
let dst = unsafe { output.dst.as_mut_ptr().add(output_pos) };

// Ignore anything past the end
let len = usize::min(src.len(), output.dst.capacity());
let len = usize::min(src.len(), output.dst.capacity() - output_pos);
let src = &src[..len];

// Safe because:
// * `len` is less than either of the two lengths
// * `src` and `dst` do not overlap because we have `&mut` to each.
unsafe { std::ptr::copy_nonoverlapping(src.as_ptr(), dst, len) };
input.set_pos(input.pos() + len);
unsafe { output.set_pos(output.pos() + len) };
unsafe { output.set_pos(output_pos + len) };

Ok(0)
}
Expand Down
1 change: 1 addition & 0 deletions zstd-safe/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1718,6 +1718,7 @@ impl<'a, C: WriteBuf + ?Sized> OutBuffer<'a, C> {

/// Returns the current cursor position.
pub fn pos(&self) -> usize {
assert!(self.pos <= self.dst.capacity());
self.pos
}

Expand Down

0 comments on commit 79592c8

Please sign in to comment.