CVE-2020-35861

@@ -23,7 +23,8 @@ jobs:
       - bash: |
           set -ex
           export RUST_BACKTRACE=1
-          export CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER=valgrind
+          # Don't leak-check, as Rust globals tend to cause false positives.
+          export CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER="valgrind --suppressions=valgrind.supp --leak-check=no --error-exitcode=1"
           cargo test
           cargo test --all-features
         displayName: "Run `cargo test` under Valgrind"

@@ -48,7 +48,7 @@ dependencies = [
 
 [[package]]
 name = "bumpalo"
-version = "3.2.0"
+version = "3.2.1"
 dependencies = [
  "criterion 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "quickcheck 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)",

@@ -8,7 +8,7 @@ license = "MIT/Apache-2.0"
 name = "bumpalo"
 readme = "./README.md"
 repository = "https://github.com/fitzgen/bumpalo"
-version = "3.2.0"
+version = "3.2.1"
 
 [package.metadata.docs.rs]
 all-features = true

@@ -28,6 +28,48 @@ Released YYYY-MM-DD.
 
 --------------------------------------------------------------------------------
 
+## 3.2.1
+
+Released 2020-03-24.
+
+### Security
+
+* When `realloc`ing, if we allocate new space, we need to copy the old
+  allocation's bytes into the new space. There are `old_size` number of bytes in
+  the old allocation, but we were accidentally copying `new_size` number of
+  bytes, which could lead to copying bytes into the realloc'd space from past
+  the chunk that we're bump allocating out of, from unknown memory.
+
+  If an attacker can cause `realloc`s, and can read the `realoc`ed data back,
+  this could allow them to read things from other regions of memory that they
+  shouldn't be able to. For example, if some crypto keys happened to live in
+  memory right after a chunk we were bump allocating out of, this could allow
+  the attacker to read the crypto keys.
+
+  Beyond just fixing the bug and adding a regression test, I've also taken two
+  additional steps:
+
+  1. While we were already running the testsuite under `valgrind` in CI, because
+     `valgrind` exits with the same code that the program did, if there are
+     invalid reads/writes that happen not to trigger a segfault, the program can
+     still exit OK and we will be none the wiser. I've enabled the
+     `--error-exitcode=1` flag for `valgrind` in CI so that tests eagerly fail
+     in these scenarios.
+
+  2. I've written a quickcheck test to exercise `realloc`. Without the bug fix
+     in this patch, this quickcheck immediately triggers invalid reads when run
+     under `valgrind`. We didn't previously have quickchecks that exercised
+     `realloc` beacuse `realloc` isn't publicly exposed directly, and instead
+     can only be indirectly called. This new quickcheck test exercises `realloc`
+     via `bumpalo::collections::Vec::resize` and
+     `bumpalo::collections::Vec::shrink_to_fit` calls.
+
+  This bug was introduced in version 3.0.0.
+
+  See [#69](https://github.com/fitzgen/bumpalo/issues/69) for details.
+
+--------------------------------------------------------------------------------
+
 ## 3.2.0
 
 Released 209-2-07.

@@ -1,14 +1,5 @@
 steps:
-  - script: |
-      (test -x $HOME/.cargo/bin/cargo-install-update || cargo install cargo-update)
-    displayName: Install `cargo install-update`
-  - script: |
-      (test -x $HOME/.cargo/bin/cargo-readme || cargo install --vers "^3" cargo-readme)
+  - script: cargo install --vers "^3" cargo-readme
     displayName: Install `cargo readme`
-  - script: |
-      cargo install-update -a
-    displayName: Update `cargo install`ed binaries
-  - script: |
-      cargo install-update --version
-      cargo readme --version
-    displayName: Query `cargo install-update` and `cargo readme` versions
+  - script: cargo readme --version
+    displayName: Query `cargo readme` version

@@ -1130,7 +1130,7 @@ unsafe impl<'a> alloc::Alloc for &'a Bump {
             if let Some(p) =
                 self.try_alloc_layout_fast(layout_from_size_align(delta, layout.align()))
             {
-                ptr::copy(ptr.as_ptr(), p.as_ptr(), new_size);
+                ptr::copy(ptr.as_ptr(), p.as_ptr(), old_size);
                 return Ok(p);
             }
         }
@@ -1200,4 +1200,23 @@ mod tests {
             b.reset();
         }
     }
+
+    #[test]
+    fn invalid_read() {
+        use alloc::Alloc;
+
+        let mut b = &Bump::new();
+
+        unsafe {
+            let l1 = Layout::from_size_align(12000, 4).unwrap();
+            let p1 = Alloc::alloc(&mut b, l1).unwrap();
+
+            let l2 = Layout::from_size_align(1000, 4).unwrap();
+            Alloc::alloc(&mut b, l2).unwrap();
+
+            let p1 = b.realloc(p1, l1, 24000).unwrap();
+            let l3 = Layout::from_size_align(24000, 4).unwrap();
+            b.realloc(p1, l3, 48000).unwrap();
+        }
+    }
 }

@@ -50,10 +50,36 @@ fn recursive_vecs() {
 fn test_into_bump_slice_mut() {
     let b = Bump::new();
     let v = bumpalo::vec![in &b; 1, 2, 3];
-    let mut slice = v.into_bump_slice_mut();
+    let slice = v.into_bump_slice_mut();
 
     slice[0] = 3;
     slice[2] = 1;
 
     assert_eq!(slice, [3, 2, 1]);
 }
+
+quickcheck::quickcheck! {
+    fn vec_resizes_causing_reallocs(sizes: std::vec::Vec<usize>) -> () {
+        // Exercise `realloc` by doing a bunch of `resize`s followed by
+        // `shrink_to_fit`s.
+
+        let b = Bump::new();
+        let mut v = bumpalo::vec![in &b];
+
+        for len in sizes {
+            // We don't want to get too big and OOM.
+            const MAX_SIZE: usize = 1 << 15;
+
+            // But we want allocations to get fairly close to the minimum chunk
+            // size, so that we are exercising both realloc'ing within a chunk
+            // and when we need new chunks.
+            const MIN_SIZE: usize = 1 << 7;
+
+            let len = std::cmp::min(len, MAX_SIZE);
+            let len = std::cmp::max(len, MIN_SIZE);
+
+            v.resize(len, 0);
+            v.shrink_to_fit();
+        }
+    }
+}

@@ -0,0 +1,7 @@
+{
+   <oom_instead_of_bump_pointer_overflow has an expected malloc fishy value>
+   Memcheck:FishyValue
+   malloc(size)
+   fun:malloc
+   obj:/**/target/*/deps/tests-*
+}