Buffer overflow in `insert_many()` #252

Qwaz · 2021-01-08T05:18:24Z

Hello fellow Rustacean,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

Issue Description

rust-smallvec/src/lib.rs

Lines 1009 to 1070 in 9cf1176

    
               /// Insert multiple elements at position `index`, shifting all following elements toward the 
        
               /// back. 
        
               pub fn insert_many<I: IntoIterator<Item = A::Item>>(&mut self, index: usize, iterable: I) { 
        
                   let iter = iterable.into_iter(); 
        
                   if index == self.len() { 
        
                       return self.extend(iter); 
        
                   } 
        
                   let (lower_size_bound, _) = iter.size_hint(); 
        
                   assert!(lower_size_bound <= core::isize::MAX as usize); // Ensure offset is indexable 
        
                   assert!(index   lower_size_bound >= index); // Protect against overflow 
        
                   self.reserve(lower_size_bound); 
        
                   unsafe { 
        
                       let old_len = self.len(); 
        
                       assert!(index <= old_len); 
        
                       let start = self.as_mut_ptr(); 
        
                       let mut ptr = start.add(index); 
        
                       // Move the trailing elements. 
        
                       ptr::copy(ptr, ptr.add(lower_size_bound), old_len - index); 
        
                       // In case the iterator panics, don't double-drop the items we just copied above. 
        
                       self.set_len(0); 
        
                       let mut guard = DropOnPanic { 
        
                           start, 
        
                           skip: index..(index   lower_size_bound), 
        
                           len: old_len   lower_size_bound, 
        
                       }; 
        
                       let mut num_added = 0; 
        
                       for element in iter { 
        
                           let mut cur = ptr.add(num_added); 
        
                           if num_added >= lower_size_bound { 
        
                               // Iterator provided more elements than the hint.  Move trailing items again. 
        
                               self.reserve(1); 
        
                               let start = self.as_mut_ptr(); 
        
                               ptr = start.add(index); 
        
                               cur = ptr.add(num_added); 
        
                               ptr::copy(cur, cur.add(1), old_len - index); 
        
                               guard.start = start; 
        
                               guard.len  = 1; 
        
                               guard.skip.end  = 1; 
        
                           } 
        
                           ptr::write(cur, element); 
        
                           guard.skip.start  = 1; 
        
                           num_added  = 1; 
        
                       } 
        
                       mem::forget(guard); 
        
                       if num_added < lower_size_bound { 
        
                           // Iterator provided fewer elements than the hint 
        
                           ptr::copy( 
        
                               ptr.add(lower_size_bound), 
        
                               ptr.add(num_added), 
        
                               old_len - index, 
        
                           ); 
        
                       } 
        
                       self.set_len(old_len   num_added); 
        
                   }

insert_many() overflows the buffer when an iterator yields more items than the lower bound of size_hint().

The problem is in line 1044. reserve(n) reserves capacity for n more elements to be inserted. This is done by comparing the length and the capacity. Since the length of the buffer is set to 0 in line 1032, line 1044 will be always no-op and the following code will overflow the buffer.

Reproduction

Below is an example program that exhibits undefined behavior using safe APIs of smallvec.

#![forbid(unsafe_code)]

use smallvec::SmallVec;

fn main() {
    let mut v: SmallVec<[u8; 0]> = SmallVec::new();

    // Spill on heap
    v.push(123);

    // Allocate string on heap
    let s = String::from("Hello!");
    println!("{}", s);

    // Prepare an iterator with small lower bound
    let mut iter = (0u8..=255).filter(|n| n % 2 == 0);
    assert_eq!(iter.size_hint().0, 0);

    // Triggering the bug
    v.insert_many(0, iter);

    // Uh oh, heap overflow made smallvec and string to overlap
    assert!(v.as_ptr_range().contains(&s.as_ptr()));

    // String is corrupted
    println!("{}", s);
}

Output:

Hello!
@BDFHJ
double free or corruption (out)

Terminated with signal 6 (SIGABRT)

Tested Environment

Crate: smallvec
Version: 1.6.0
OS: Ubuntu 20.04.1 LTS
Rustc version: rustc 1.48.0 (7eac88abb 2020-11-16)

The text was updated successfully, but these errors were encountered:

Fixes servo#252.

mbrubeck · 2021-01-08T05:31:46Z

Thanks for the report! I have submitted a fix in #254. I can submit a RustSec advisory after the fixed version is released, or if you would like to write the advisory, that would be fine too.

Qwaz · 2021-01-08T05:37:04Z

I believe the bug is older than that. Passing 0 as index will call self.set_len(0) and trigger the same bug.

mbrubeck · 2021-01-08T05:40:06Z

You're right. This was introduced in #103, commit 26b2490, so it affects version 0.6.3 through 1.6.0.

(I deleted a previous comment where I said it was introduced in #213, because I realized it was wrong right after posting it.)

Fixes servo#252.

Qwaz · 2021-01-08T05:50:36Z

I agree that that commit introduced the bug. Thank you for the quick fix!
Regarding the RustSec advisory, I think it's better to be written by the crate maintainer if possible.

Fixes servo#252.

Fix potential buffer overflow in `insert_many` Fixes #252.

mbrubeck · 2021-01-08T18:03:33Z

Fixed in smallvec 0.6.14 and 1.6.1.

RustSec advisory submitted in rustsec/advisory-db#552.

Fixes a buffer overflow: servo/rust-smallvec#252. `spinoso-array` is not impacted because it does not use the vulnerable `SmallVec::insert_many` API.

2506: chore(deps): bump smallvec from 0.6.13 / 1.3.0 to 0.6.14 / 1.6.1 to fix RUSTSEC-2021-0003 r=driftluo,zhangsoledad a=yangby-cryptape Ref: - [RUSTSEC-2021-0003: smallvec: Buffer overflow in SmallVec::insert_many](https://rustsec.org/advisories/RUSTSEC-2021-0003.html) - [Rust-SmallVec Issue-252: Buffer overflow in `insert_many()](servo/rust-smallvec#252) Co-authored-by: Boyu Yang <[email protected]>

There was a vulnerability found in smallvec as described in: * servo/rust-smallvec#252 * rustsec/advisory-db#552 This patch update the package version to 1.6 which is deemed safe to use. Signed-off-by: Fintan Halpenny <[email protected]>

There was a vulnerability found in smallvec as described in: * servo/rust-smallvec#252 * rustsec/advisory-db#552 This patch update the package version to 1.6.1 which is deemed safe to use. Signed-off-by: Fintan Halpenny <[email protected]>

mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this issue Jan 8, 2021

Fix potential buffer overflow in insert_many

f41aaaa

Fixes servo#252.

mbrubeck mentioned this issue Jan 8, 2021

Fix potential buffer overflow in insert_many #253

Closed

Qwaz mentioned this issue Jan 8, 2021

Memory safety problem in insert_many() Alexhuszagh/rust-lexical#53

Closed

mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this issue Jan 8, 2021

Fix potential buffer overflow in insert_many

1d67c2e

Fixes servo#252.

mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this issue Jan 8, 2021

Fix potential buffer overflow in insert_many

3e60240

Fixes servo#252.

mbrubeck closed this as completed in 85593a4 Jan 8, 2021

mbrubeck mentioned this issue Jan 8, 2021

Fix potential buffer overflow in insert_many #254

Merged

mbrubeck reopened this Jan 8, 2021

mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this issue Jan 8, 2021

Fix potential buffer overflow in insert_many

ba79ac7

Fixes servo#252.

mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this issue Jan 8, 2021

Fix potential buffer overflow in insert_many

74dc120

Fixes servo#252.

bors-servo added a commit that referenced this issue Jan 8, 2021

Auto merge of #254 - mbrubeck:overflow, r=emilio

4e53e07

Fix potential buffer overflow in `insert_many` Fixes #252.

bors-servo closed this as completed in 9998ba0 Jan 8, 2021

bors-servo closed this as completed in #254 Jan 8, 2021

mbrubeck mentioned this issue Jan 8, 2021

smallvec: Buffer overflow in insert_many rustsec/advisory-db#552

Merged

mbrubeck mentioned this issue Jan 8, 2021

Deprecate insert_many (and add splice?) #255

Open

kjvalencik mentioned this issue Jan 8, 2021

Convert to tinyvec which does not include unsafe neon-bindings/neon#665

Open

lopopolo added a commit to artichoke/artichoke that referenced this issue Jan 8, 2021

Update SmallVec to 1.6.1

cc127a4

Fixes a buffer overflow: servo/rust-smallvec#252. `spinoso-array` is not impacted because it does not use the vulnerable `SmallVec::insert_many` API.

lopopolo mentioned this issue Jan 8, 2021

Update SmallVec to 1.6.1 artichoke/artichoke#1022

Merged

HCastano mentioned this issue Jan 9, 2021

Update vulnerable smallvec crate (RUSTSEC-2021-0003) paritytech/parity-bridges-common#632

Merged

yangby-cryptape mentioned this issue Jan 11, 2021

chore(deps): bump smallvec from 0.6.13 / 1.3.0 to 0.6.14 / 1.6.1 to fix RUSTSEC-2021-0003 nervosnetwork/ckb#2506

Merged

drtychai mentioned this issue Jan 14, 2021

[Rust] Vulnerability in dependency: smallvec google/flatbuffers#6405

Closed

FintanH mentioned this issue Jan 27, 2021

cargo audit alert for smallvec 1.6.0 Amanieu/parking_lot#274

Closed

FintanH mentioned this issue Jan 27, 2021

Update smallvec to 1.6 Amanieu/parking_lot#276

Merged

FintanH mentioned this issue Jan 27, 2021

smallvec/parking_lot vulnerability boinkor-net/governor#60

Closed

puzzlewolf mentioned this issue Feb 1, 2021

Consider yanking versions affected by #252 #256

Closed

ammaraskar mentioned this issue Feb 19, 2021

Library retains soundness issue fixed in upstream smallvec ColonelPhantom/rust-smallvec#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Buffer overflow in `insert_many()` #252

Buffer overflow in `insert_many()` #252

Qwaz commented Jan 8, 2021

mbrubeck commented Jan 8, 2021 •

edited

Loading

Qwaz commented Jan 8, 2021

mbrubeck commented Jan 8, 2021 •

edited

Loading

Qwaz commented Jan 8, 2021

mbrubeck commented Jan 8, 2021

Buffer overflow in insert_many() #252

Buffer overflow in insert_many() #252

Comments

Qwaz commented Jan 8, 2021

Issue Description

Reproduction

Tested Environment

mbrubeck commented Jan 8, 2021 • edited Loading

Qwaz commented Jan 8, 2021

mbrubeck commented Jan 8, 2021 • edited Loading

Qwaz commented Jan 8, 2021

mbrubeck commented Jan 8, 2021

Buffer overflow in `insert_many()` #252

Buffer overflow in `insert_many()` #252

mbrubeck commented Jan 8, 2021 •

edited

Loading

mbrubeck commented Jan 8, 2021 •

edited

Loading