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.

Sign up for GitHub

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

Jump to bottom

Flaw in realloc allows reading unknown memory #69

Closed
Riey opened this issue Mar 24, 2020 · 7 comments · Fixed by #70
Closed

Flaw in realloc allows reading unknown memory #69

Riey opened this issue Mar 24, 2020 · 7 comments · Fixed by #70
Labels
bug Something isn't working

Comments

@Riey
Copy link

Riey commented Mar 24, 2020 •
edited
Loading

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
Copy link
Author

Riey commented Mar 24, 2020

Also note that

I use Arch Linux and test failed in this line

bumpalo/src/lib.rs

Line 1133 in 7d0c4dd

ptr::copy(ptr.as_ptr(), p.as_ptr(), new_size);

@fitzgen
Copy link
Owner

fitzgen commented Mar 24, 2020

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

@fitzgen fitzgen added the bug Something isn't working label Mar 24, 2020
@fitzgen
Copy link
Owner

fitzgen commented Mar 24, 2020

Fix incoming.

fitzgen added a commit that referenced this issue Mar 24, 2020
@fitzgen
realloc: Copy old_size number of bytes (not new_size) into new al…
b9ebe52 
…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 #69
@fitzgen fitzgen mentioned this issue Mar 24, 2020
Merged
fitzgen added a commit that referenced this issue Mar 24, 2020
@fitzgen
realloc: Copy old_size number of bytes (not new_size) into new al…
d08bc37 
…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 #69
@fitzgen
Copy link
Owner

fitzgen commented Mar 24, 2020

Thanks again for filing this issue!

It turns out that this is a sec bug, so I've filed this advisory: rustsec/advisory-db#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.

@fitzgen fitzgen changed the title segfault only in unit test Flaw in realloc allows reading unknown memory Mar 24, 2020
@Kixunil
Copy link

Kixunil commented Mar 28, 2020

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
Copy link

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

@fitzgen
Copy link
Owner

fitzgen commented Mar 31, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

realloc: Copy old_size number of bytes (not new_size) into new allocation fitzgen/bumpalo
4 participants
@fitzgen @Kixunil @Ancient123 @Riey