From 79592c882c5c3212e727fed55054241f5f891d17 Mon Sep 17 00:00:00 2001
From: Jamey Sharp <jamey@minilop.net>
Date: Wed, 19 Jul 2023 18:43:03 -0700
Subject: [PATCH] Fix two buffer overruns reachable from safe code (#231)

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.
---
 src/stream/raw.rs    | 7 ++++---
 zstd-safe/src/lib.rs | 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/stream/raw.rs b/src/stream/raw.rs
index ee1d292a..65a6d06c 100644
--- a/src/stream/raw.rs
+++ b/src/stream/raw.rs
@@ -96,10 +96,11 @@ 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:
@@ -107,7 +108,7 @@ impl Operation for NoOp {
         // * `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)
     }
diff --git a/zstd-safe/src/lib.rs b/zstd-safe/src/lib.rs
index 51564bdd..1c555674 100644
--- a/zstd-safe/src/lib.rs
+++ b/zstd-safe/src/lib.rs
@@ -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
     }