buffer: fix vulnerabilities when allocation fails#5468
buffer: fix vulnerabilities when allocation fails#5468ggreenway merged 2 commits intoenvoyproxy:masterfrom
Conversation
If allocations in Buffer::OwnedImpl fail in reserve(), the caller may write to uninitialized pointers. There are various callers that would fall into this path, and it may be possible to develop an exploit based on this. A similar allocation failure could occur in linearize(), which could result in a NULL-pointer dereference. Because this is undefined-behavior in C++, this should be avoided. This could also result in reading/writing to arbitrary memory (NULL + offset) if either offset is large (and thus points to a valid virtual memory address), or very low addresses are mapped in the process. Typically on linux, allocations do not fail, and OOM conditions are resolved by the kernel killing a process to reclaim memory. However, with various ulimit and cgroup options, an allocation could return NULL. Signed-off-by: Greg Greenway <ggreenway@apple.com>
|
Related: should we be calling |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks. re: a new handler, we don't blindly catch std::exception anywhere, so it should be OK and cause a crash, but it can't hurt so seems like a good idea. Open a tracking issue or just do a PR?
| ASSERT(size <= length()); | ||
| return evbuffer_pullup(buffer_.get(), size); | ||
| void* const ret = evbuffer_pullup(buffer_.get(), size); | ||
| RELEASE_ASSERT(ret != nullptr || size == 0, |
There was a problem hiding this comment.
Do we want to fail on size == 0 here? I don't feel strongly but it seems like they should be OK?
There was a problem hiding this comment.
This is explicitly not failing if size == 0 because this case occurs in the tests. And I don't think we should crash when you have something that evaluates to:
void* foo = buf.linearize(0);
for (int i = 0; i < 0; i++) {
x = foo[i];
}
There was a problem hiding this comment.
Sorry, I read this as the reverse that it would crash when size == 0. Yes this makes sense.
|
/retest |
* buffer: fix vulnerabilities when allocation fails If allocations in Buffer::OwnedImpl fail in reserve(), the caller may write to uninitialized pointers. There are various callers that would fall into this path, and it may be possible to develop an exploit based on this. A similar allocation failure could occur in linearize(), which could result in a NULL-pointer dereference. Because this is undefined-behavior in C++, this should be avoided. This could also result in reading/writing to arbitrary memory (NULL + offset) if either offset is large (and thus points to a valid virtual memory address), or very low addresses are mapped in the process. Typically on linux, allocations do not fail, and OOM conditions are resolved by the kernel killing a process to reclaim memory. However, with various ulimit and cgroup options, an allocation could return NULL. Signed-off-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: Fred Douglas <fredlas@google.com>
2 participants
If allocations in Buffer::OwnedImpl fail in reserve(), the caller may write to uninitialized pointers. There are various callers that would fall into this path, and it may be possible to develop an exploit based on this.
A similar allocation failure could occur in linearize(), which could result in a NULL-pointer dereference. Because this is undefined-behavior in C++, this should be avoided. This could also result in reading/writing to arbitrary memory (NULL + offset) if either offset is large (and thus points to a valid virtual memory address), or very low addresses are mapped in the process.
Typically on linux, allocations do not fail, and OOM conditions are resolved by the kernel killing a process to reclaim memory. However, with various ulimit and cgroup options, an allocation could return NULL.
Signed-off-by: Greg Greenway ggreenway@apple.com
Risk Level: Low
Testing: Existing tests pass, but no additional testing was added because there isn't an easy way to make libevent allocations fail in tests, and this code is currently being rewritten (see #5441).
Docs Changes: None
Release Notes: TODO