Skip to content

buffer: fix vulnerabilities when allocation fails #5468

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

Merged
ggreenway merged 2 commits into from
Jan 3, 2019
Merged

buffer: fix vulnerabilities when allocation fails #5468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26f25f0
buffer: fix vulnerabilities when allocation fails
ggreenway Jan 3, 2019
e3d82c1
relnote
ggreenway Jan 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/root/intro/version_history.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ Version history
1.10.0 (pending)
================
* access log: added a new flag for upstream retry count exceeded.
* buffer: fix vulnerabilities when allocation fails
* config: removed deprecated_v1 sds_config from :ref:`Bootstrap config <config_overview_v2_bootstrap>`.
* tls: enabled TLS 1.3 on the server-side (non-FIPS builds).

Expand Down
14 changes: 9 additions & 5 deletions source/common/buffer/buffer_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,10 @@ uint64_t OwnedImpl::length() const { return evbuffer_get_length(buffer_.get());

void* OwnedImpl::linearize(uint32_t size) {
ASSERT(size <= length());
return evbuffer_pullup(buffer_.get(), size);
void* const ret = evbuffer_pullup(buffer_.get(), size);
RELEASE_ASSERT(ret != nullptr || size == 0,

@mattklein123 mattklein123 Jan 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to fail on size == 0 here? I don't feel strongly but it seems like they should be OK?

@ggreenway ggreenway Jan 3, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];
}

@mattklein123 mattklein123 Jan 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I read this as the reverse that it would crash when size == 0. Yes this makes sense.

"Failure to linearize may result in buffer overflow by the caller.");
return ret;
}

void OwnedImpl::move(Instance& rhs) {
Expand Down Expand Up @@ -148,10 +151,11 @@ Api::SysCallIntResult OwnedImpl::read(int fd, uint64_t max_length) {
}

uint64_t OwnedImpl::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) {
uint64_t ret = evbuffer_reserve_space(buffer_.get(), length,
reinterpret_cast<evbuffer_iovec*>(iovecs), num_iovecs);
ASSERT(ret >= 1);
return ret;
int ret = evbuffer_reserve_space(buffer_.get(), length, reinterpret_cast<evbuffer_iovec*>(iovecs),
num_iovecs);
RELEASE_ASSERT(ret >= 1, "Failure to allocate may result in callers writing to uninitialized "
"memory, buffer overflows, etc");
return static_cast<uint64_t>(ret);
}

ssize_t OwnedImpl::search(const void* data, uint64_t size, size_t start) const {
Expand Down