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

realloc: Copy old_size number of bytes (not new_size) into new allocation #70

Merged
merged 2 commits into from
Mar 24, 2020

Conversation

fitzgen
Copy link
Owner

@fitzgen fitzgen commented Mar 24, 2020

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 reallocs, and can read the realoced 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

…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 force-pushed the realloc-copy-old-size-bytes branch from b9ebe52 to d08bc37 Compare March 24, 2020 21:05
@fitzgen fitzgen merged commit 02e6d0a into master Mar 24, 2020
@fitzgen fitzgen deleted the realloc-copy-old-size-bytes branch March 24, 2020 21:11
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.

Flaw in realloc allows reading unknown memory
1 participant