Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

smallvec: Buffer overflow in insert_many #552

Merged
merged 1 commit into from Jan 8, 2021

Conversation

mbrubeck
Copy link
Contributor

@mbrubeck mbrubeck commented Jan 8, 2021

A bug in the SmallVec::insert_many method caused it to allocate a buffer that was smaller than needed. It then wrote past the end of the buffer, causing a buffer overflow and memory corruption on the heap.

This bug was only triggered if the iterator passed to insert_many yielded more items than the lower bound returned from its size_hint method.

The flaw was corrected in smallvec 0.6.14 and 1.6.1, by ensuring that additional space is always reserved for each item inserted. The fix also simplified the implementation of insert_many to use less unsafe code, so it is easier to verify its correctness.

Thank you to Yechan Bae (@Qwaz) and the Rust group at Georgia Tech’s SSLab for finding and reporting this bug. See servo/rust-smallvec#252 for more details.

@Shnatsel
Copy link
Member

Shnatsel commented Jan 8, 2021

Thank you!

@Shnatsel Shnatsel merged commit 5851ec6 into rustsec:master Jan 8, 2021
@Shnatsel
Copy link
Member

Shnatsel commented Jan 8, 2021

cargo audit does not flag this advisory on a test crate depending on smallvec 1.4.2, even though CI has passed. I suspect something is wrong with version resolution logic, we had some fallout from the semver crate changes. I'm investigating in more detail. Sorry about that.

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jan 8, 2021

cargo audit correctly gives errors for me in a project with both smallvec 0.6.13 and smallvec 1.2.0 in the Cargo.lock file, though it fails to print the dependency tree.

Crate:         smallvec
Version:       0.6.13
Title:         Buffer overflow in SmallVec::insert_many
Date:          2021-01-08
ID:            RUSTSEC-2021-0003
URL:           https://rustsec.org/advisories/RUSTSEC-2021-0003
Solution:      Upgrade to >=0.6.14, <1.0.0 OR >=1.6.1
Dependency tree: 
smallvec 0.6.13

Crate:         smallvec
Version:       1.2.0
Title:         Buffer overflow in SmallVec::insert_many
Date:          2021-01-08
ID:            RUSTSEC-2021-0003
URL:           https://rustsec.org/advisories/RUSTSEC-2021-0003
Solution:      Upgrade to >=0.6.14, <1.0.0 OR >=1.6.1
Dependency tree: 
smallvec 1.2.0

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jan 8, 2021

In an empty test project with nothing but smallvec 1.4.2 in Cargo.lock, it works correctly:

    Fetching advisory database from `https://github.com/RustSec/advisory-db.git`
      Loaded 175 security advisories (from /Users/mattbrubeck/.cargo/advisory-db)
    Updating crates.io index
    Scanning Cargo.lock for vulnerabilities (2 crate dependencies)
Crate:         smallvec
Version:       1.4.2
Title:         Buffer overflow in SmallVec::insert_many
Date:          2021-01-08
ID:            RUSTSEC-2021-0003
URL:           https://rustsec.org/advisories/RUSTSEC-2021-0003
Solution:      Upgrade to >=0.6.14, <1.0.0 OR >=1.6.1
Dependency tree: 
smallvec 1.4.2
└── foo 0.1.0

error: 1 vulnerability found!

@Shnatsel
Copy link
Member

Shnatsel commented Jan 8, 2021

Interesting! It doesn't appear to work for me. Could you post the output of cargo audit --version ?

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jan 8, 2021

$ cargo audit --version
cargo-audit 0.13.1

cargo-deny is also working correctly for me.

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jan 8, 2021

Have you double-checked your Cargo.lock? Specifying smallvec = "1.4.2" in Cargo.toml is not sufficient, because Cargo will choose the highest compatible version, which includes the fix.

@Shnatsel
Copy link
Member

Shnatsel commented Jan 8, 2021

I think this was caused by workspace handling in my tests. I have a weird configuration where a binary is both in and not in a workspace. When testing on a stand-alone "hello world" it works fine.

I have a report from one other user, but they're using an experimental data extraction pipeline, which is probably the culprit.

This leads me to conclude that this advisory is entirely correct, and I've just run into an unrelated usability issue. Thanks for confirming!

@lopopolo
Copy link
Contributor

lopopolo commented Jan 8, 2021

The advisory link that cargo-deny reports 404s: https://rustsec.org/advisories/RUSTSEC-2021-0003

Output of cargo-deny:

error[A001]: Buffer overflow in SmallVec::insert_many
   ┌─ /home/runner/work/artichoke/artichoke/Cargo.lock:60:1
   │
60 │ smallvec 1.6.0 registry+https://github.com/rust-lang/crates.io-index
   │ -------------------------------------------------------------------- security vulnerability detected
   │
   = ID: RUSTSEC-2021-0003
   = Advisory: https://rustsec.org/advisories/RUSTSEC-2021-0003
   = A bug in the `SmallVec::insert_many` method caused it to allocate a buffer that was smaller than needed.  It then wrote past the end of the buffer, causing a buffer overflow and memory corruption on the heap.
     
     This bug was only triggered if the iterator passed to `insert_many` yielded more items than the lower bound returned from its `size_hint` method.
      
     The flaw was corrected in smallvec 0.6.14 and 1.6.1, by ensuring that additional space is always reserved for each item inserted.  The fix also simplified the implementation of `insert_many` to use less unsafe code, so it is easier to verify its correctness.
     
     Thank you to Yechan Bae (@Qwaz) and the Rust group at Georgia Tech’s SSLab for finding and reporting this bug.
   = Announcement: https://github.com/servo/rust-smallvec/issues/252
   = Solution: Upgrade to >=0.6.14, <1.0.0 OR >=1.6.1
   = smallvec v1.6.0
     └── spinoso-array v0.4.0
         └── artichoke-backend v0.1.0
             └── artichoke v0.1.0-pre.0

 advisories FAILED: 1 errors, 0 warnings, 0 notes
           bans ok: 0 errors, 0 warnings, 1 notes
       licenses ok: 0 errors, 0 warnings, 85 notes
        sources ok: 0 errors, 0 warnings, 0 notes
Error: Process completed with exit code 1.

@tarcieri
Copy link
Member

tarcieri commented Jan 8, 2021

I just regenerated the web site. It should work now.

FintanH added a commit to FintanH/parking_lot that referenced this pull request Jan 27, 2021
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 <fintan.halpenny@gmail.com>
FintanH added a commit to FintanH/parking_lot that referenced this pull request Jan 27, 2021
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 <fintan.halpenny@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants