realloc: Copy old_size number of bytes (not new_size) into new al…

…location

When reallocating, 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.

Fixes fitzgen#69

CHANGELOG.md

@@ -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.

Cargo.toml

@@ -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

azure-pipelines.yml

@@ -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"

ci/install-cargo-readme.yml

@@ -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

src/lib.rs

@@ -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();
+        }
+    }
 }

tests/vec.rs

@@ -57,3 +57,29 @@ fn test_into_bump_slice_mut() {
 
     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();
+        }
+    }
+}

valgrind.supp

@@ -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-*
+}