Fix buffer overflow handling in dynamic_buffer_prepare #2879
Conversation
|
I believe the exception thrown by the allocator doesn't have the same meaning as |
|
Well, I followed the same logic as in the thread #553 where such a decision (replacing |
That commits points to the old versions of the code which is replaced by: c7a7d16
Not really, I mean suppose you convert that allocation exception to a form of error_code, then what can you do with that error_code in the handler, retry again? If you want to set an upper limit to the maximum size, there is already an interface for it. If you want to read a very large message without allocating too much memory, it is doable by using the This exception complains about an allocation with a size of |
I think just log the error message, as probably for the case with
Sure, as I already said it's this example + ws_server.cpp#include <boost/asio/dispatch.hpp>
#include <boost/asio/strand.hpp>
#include <boost/beast/core.hpp>
#include <boost/beast/websocket.hpp>
#include <algorithm>
#include <cstdlib>
#include <functional>
#include <iostream>
#include <memory>
#include <string>
#include <thread>
#include <vector>
namespace beast = boost::beast; // from <boost/beast.hpp>
namespace http = beast::http; // from <boost/beast/http.hpp>
namespace websocket = beast::websocket; // from <boost/beast/websocket.hpp>
namespace net = boost::asio; // from <boost/asio.hpp>
using tcp = boost::asio::ip::tcp; // from <boost/asio/ip/tcp.hpp>
//------------------------------------------------------------------------------
// Report a failure
void fail(beast::error_code ec, char const* what)
{
std::cerr << what << ": " << ec.message() << "\n";
}
// Echoes back all received WebSocket messages
class session : public std::enable_shared_from_this<session>
{
websocket::stream<beast::tcp_stream> ws_;
beast::flat_buffer buffer_;
public:
// Take ownership of the socket
explicit session(tcp::socket&& socket) : ws_(std::move(socket))
{
}
// Get on the correct executor
void run()
{
// We need to be executing within a strand to perform async operations
// on the I/O objects in this session. Although not strictly necessary
// for single-threaded contexts, this example code is written to be
// thread-safe by default.
net::dispatch(
ws_.get_executor(),
beast::bind_front_handler(&session::on_run, shared_from_this()));
}
// Start the asynchronous operation
void on_run()
{
// Set suggested timeout settings for the websocket
ws_.set_option(websocket::stream_base::timeout::suggested(
beast::role_type::server));
// Set a decorator to change the Server of the handshake
ws_.set_option(websocket::stream_base::decorator(
[](websocket::response_type& res) {
res.set(
http::field::server,
std::string(BOOST_BEAST_VERSION_STRING)
+ " websocket-server-async");
}));
ws_.set_option(websocket::permessage_deflate{
.server_enable = true,
});
// Accept the websocket handshake
ws_.async_accept(
beast::bind_front_handler(&session::on_accept, shared_from_this()));
}
void on_accept(beast::error_code ec)
{
if (ec)
return fail(ec, "accept");
// Read a message
do_read();
}
void do_read()
{
// Read a message into our buffer
ws_.async_read(
buffer_,
beast::bind_front_handler(&session::on_read, shared_from_this()));
}
void on_read(beast::error_code ec, std::size_t bytes_transferred)
{
boost::ignore_unused(bytes_transferred);
// This indicates that the session was closed
if (ec == websocket::error::closed)
return;
if (ec)
fail(ec, "read");
// Echo the message
ws_.text(ws_.got_text());
ws_.async_write(
buffer_.data(),
beast::bind_front_handler(&session::on_write, shared_from_this()));
}
void on_write(beast::error_code ec, std::size_t bytes_transferred)
{
boost::ignore_unused(bytes_transferred);
if (ec)
return fail(ec, "write");
// Clear the buffer
buffer_.consume(buffer_.size());
// Do another read
do_read();
}
};
//------------------------------------------------------------------------------
// Accepts incoming connections and launches the sessions
class listener : public std::enable_shared_from_this<listener>
{
net::io_context& ioc_;
tcp::acceptor acceptor_;
public:
listener(net::io_context& ioc, tcp::endpoint endpoint)
: ioc_(ioc)
, acceptor_(ioc)
{
beast::error_code ec;
// Open the acceptor
acceptor_.open(endpoint.protocol(), ec);
if (ec)
{
fail(ec, "open");
return;
}
// Allow address reuse
acceptor_.set_option(net::socket_base::reuse_address(true), ec);
if (ec)
{
fail(ec, "set_option");
return;
}
// Bind to the server address
acceptor_.bind(endpoint, ec);
if (ec)
{
fail(ec, "bind");
return;
}
// Start listening for connections
acceptor_.listen(net::socket_base::max_listen_connections, ec);
if (ec)
{
fail(ec, "listen");
return;
}
}
// Start accepting incoming connections
void run()
{
do_accept();
}
private:
void do_accept()
{
// The new connection gets its own strand
acceptor_.async_accept(
net::make_strand(ioc_),
beast::bind_front_handler(&listener::on_accept, shared_from_this()));
}
void on_accept(beast::error_code ec, tcp::socket socket)
{
if (ec)
{
fail(ec, "accept");
}
else
{
// Create the session and run it
std::make_shared<session>(std::move(socket))->run();
}
// Accept another connection
do_accept();
}
};
//------------------------------------------------------------------------------
int main(int argc, char* argv[])
{
// Check command line arguments.
if (argc != 4)
{
std::cerr << "Usage: websocket-server-async <address> <port> "
"<threads>\n"
<< "Example:\n"
<< " websocket-server-async 0.0.0.0 8080 1\n";
return EXIT_FAILURE;
}
auto const address = net::ip::make_address(argv[1]);
auto const port = static_cast<unsigned short>(std::atoi(argv[2]));
auto const threads = std::max<int>(1, std::atoi(argv[3]));
// The io_context is required for all I/O
net::io_context ioc{threads};
// Create and launch a listening port
std::make_shared<listener>(ioc, tcp::endpoint{address, port})->run();
// Run the I/O service on the requested number of threads
std::vector<std::thread> v;
v.reserve(threads - 1);
for (auto i = threads - 1; i > 0; --i)
v.emplace_back([&ioc] { ioc.run(); });
ioc.run();
return EXIT_SUCCESS;
}And a malformed input message that I got during fuzzing: crash-d22d762b7748edd0ac549ce67f8907a05c0eba4f.txt How to reproduce:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2879 +/- ##
========================================
Coverage 93.23% 93.23%
========================================
Files 177 177
Lines 13670 13670
========================================
Hits 12745 12745
Misses 925 925
Continue to review full report in Codecov by Sentry.
|
When This means the client shouldn't use compression on frames. However, the file |
|
This is clear. But the question - is it expected that ws_server.cpp crashes for this input? Should it be fixed in the Boost.Beast library or I need to adjust the basic_flat_buffer::max_size / catch the exception? |
A real-life server should set an upper limit on the maximum message size, for example: ws.read_message_max(64 * 1024 * 1024);Alternatively, you can pass a limit for each call to |
|
When the server receives a message from a client with invalid bits in the message frame header, Beast should not throw an exception. Instead, it should signal the invalid condition with an |
|
@vinniefalco Yes, I agree with you, that's why I decided to try to fix this exception. Do I understand correctly that my intention to correct the current behavior makes sense? If so, do you agree with how I fixed it, or is there a better solution? Regardless of the answer, I want to express my opinion about the quality of the library code - it is very high! |
It currently doesn't throw an exception; instead, it completes with |
Definitely not :) The correct way to fix this is to first check for overflow in the dynamic buffer, in the read_op, and return an error_code if there is not enough space. The maximum space can be determined by looking at and possibly other places, @ashtum can you please weigh in? |
Why isn't the server receiving this error before an attempt is made to read the frame payload? |
|
@ashtum we must add two minimal tests:
|
I believe It already does that inside the beast::detail::dynamic_buffer_prepare:
The file |
When Beast is called upon to read a huge frame into a dynamic buffer, it is our responsibility to check that the frame can fit within the maximum of that dynamic buffer. If it cannot, we must return an In this case I believe that the exception is coming from the attempt to allocate via std? If yes, then the exception is correct and the proper solution is for the caller to set a lower maximum size on the dynamic buffer or to set a maximum message size for websocket. Do we already have default maximum websocket message sizes for clients and servers? |
Yes, there is a call stack in the first message. |
Thank you for the answer. I guess I can close this MR |
Yes, this is what the current implementation does.
Interesting, it is 16MB by default. I had assumed it wasn't set by default, so the behavior seemed expected. It appears we only check it when message is not deflated: It is checked in a few other places in the read operation too, which I need to verify. But this is definitely a bug. |
|
If the message is deflated we should check for the limit twice: one, to make sure that the deflated message is not over the limit, and then again to make sure that the decompressed message is not over the limit. |
I believe what is happening is that in the call to beast/include/boost/beast/websocket/impl/read.hpp Lines 791 to 793 in 4bff457 Note that the |
|
This could have happened for an uncompressed message too, but it is prevented earlier in beast/include/boost/beast/websocket/impl/stream_impl.hpp Lines 889 to 898 in 4bff457 |
|
@tyler92, thanks for reporting the bug. The fix is already in the master branch and will be included in the upcoming Boost release. |
I experienced a bad_alloc exception using WebSocket server with enabled compression. A malformed message with a very big length led to the following address sanitizer report:
It's possible to reproduce it with the official example with just one change - enabling compression for the server.
I believe it's a good idea to fix this in the library instead of catching the exception on the user side. I used a similar MR as a base for my one - #558 . This is my first MR for this repository so I would appreciate your feedback.