search

fitzgen / bumpalo

A fast bump allocation arena for Rust
https://docs.rs/bumpalo
Apache License 2.0
1.39k stars 110 forks source link

Flaw in `realloc` allows reading unknown memory #69

Closed Riey closed 4 years ago

Riey commented 4 years ago

I'm very confused and don't know what is wrong

when I run this code in unit test with cargo test it failed with SIGSEGV

Here is my test code https://github.com/Riey/bumpalo/tree/unit-test-failed

Screenshot from 2020-03-24 14-28-23

but when I run same code in main.rs then it success

Screenshot from 2020-03-24 14-28-54

Riey commented 4 years ago

Also note that

I use Arch Linux and test failed in this line

https://github.com/fitzgen/bumpalo/blob/7d0c4ddf81aa8e5693827015ba531bf85d05a21e/src/lib.rs#L1133

fitzgen commented 4 years ago

Thanks for filing an issue! I can reproduce the segfault. Looking into it.

fitzgen commented 4 years ago

Fix incoming.

fitzgen commented 4 years ago

Thanks again for filing this issue!

It turns out that this is a sec bug, so I've filed this advisory: https://github.com/RustSec/advisory-db/pull/251

I've also just published 3.2.1 with the fix. See the CHANGELOG.md and pull request that fixes this bug for a few more details.

Kixunil commented 4 years ago

My understanding is that the code shouldn't read from the allocated buffer past old_size until it overwrites it, so it's not outright vulnerability, but increases the risk if someone screws up somewhere else. Am I correct?

Ancient123 commented 4 years ago

@fitzgen : 3.2.1 isn't showing up in github releases or tags. Did it not actually get published yet?

fitzgen commented 4 years ago

@Ancient123

@fitzgen : 3.2.1 isn't showing up in github releases or tags. Did it not actually get published yet?

I guess I forgot to do a git tag, but the crates.io release is here: https://crates.io/crates/bumpalo/3.2.1

I'll tag it now.

@Kixunil

My understanding is that the code shouldn't read from the allocated buffer past old_size until it overwrites it, so it's not outright vulnerability, but increases the risk if someone screws up somewhere else. Am I correct?

It would copy invalid memory into the newly allocated block. But yes, this alone is not sufficient to pull off a whole attack because it would also require some Vec or something to allow reading the uninitialized memory between its length and its capacity.

None the less, better safe than sorry, hence the advisory.