Fix large content error codes
When pushing multi-part payloads, it's quite helpful if the server
supports the header field of "Expect: 100-Continue". What this does, is
on a large file push, allows the server to possibly reject a request
before the payload is actually sent, thereby saving bandwidth, and
giving the user more information.
Bmcweb, since commit 3909dc82a003893812f598434d6c4558107afa28 by James
(merged July 2020) has simply closed the connection if a user attempts
to send too much data, thereby making the bmcweb implementation simpler.
Unfortunately, to a security tester, this has the appearance on the
network as a crash, which will likely then get filed as a "verify this
isn't failing" bug.
In addition, the default args on curl multipart upload enable the
Expect: 100-Continue behavior, so folks testing must've just been
disabling that behavior.
Bmcweb should just support the right thing here. Unfortunately, closing
a connection uncleanly is easy. Closing a connection cleanly is
difficult. This requires a pretty large refactor of the http connection
class to accomplish.
Tested:
Create files of various size and try to send them (Note, default body
limit is 30 MB) and upload them with an without a username.
```
dd if=/dev/zero of=zeros-file bs=1048576 count=16 of=16mb.txt
curl -k --location POST https://192.168.7.2/redfish/v1/UpdateService/update -F 'UpdateParameters={"Targets":["/redfish/v1/Managers/bmc"]} ;type=application/json' -F UpdateFile=@32mb.txt -v
```
No Username:
32MB returns < HTTP/1.1 413 Payload Too Large
16MB returns < HTTP/1.1 401 Unauthorized
With Username
32MB returns < HTTP/1.1 413 Payload Too Large
16MB returns < HTTP/1.1 400 Bad Request
Note, in all cases except the last one, the payload is never sent from
curl.
Redfish protocol validator fails no new tests (SSE failure still
present).
Redfish service validator passes.
Change-Id: I72bc8bbc49a05555c31dc7209292f846ec411d43
Signed-off-by: Ed Tanous <ed@tanous.net>
diff --git a/http/http_connection.hpp b/http/http_connection.hpp
index bd8cf31..d0aa5d5 100644
--- a/http/http_connection.hpp
+++ b/http/http_connection.hpp
@@ -42,9 +42,9 @@
constexpr uint64_t httpReqBodyLimit = 1024UL * 1024UL *
bmcwebHttpReqBodyLimitMb;
-constexpr uint64_t loggedOutPostBodyLimit = 4096;
+constexpr uint64_t loggedOutPostBodyLimit = 4096U;
-constexpr uint32_t httpHeaderLimit = 8192;
+constexpr uint32_t httpHeaderLimit = 8192U;
template <typename>
struct IsTls : std::false_type
@@ -68,9 +68,7 @@
handler(handlerIn), timer(std::move(timerIn)),
getCachedDateStr(getCachedDateStrF)
{
- parser.emplace(std::piecewise_construct, std::make_tuple());
- parser->body_limit(httpReqBodyLimit);
- parser->header_limit(httpHeaderLimit);
+ initParser();
#ifdef BMCWEB_ENABLE_MUTUAL_TLS_AUTHENTICATION
prepareMutualTls();
@@ -78,13 +76,13 @@
connectionCount++;
- BMCWEB_LOG_DEBUG("{} Connection open, total {}", logPtr(this),
+ BMCWEB_LOG_DEBUG("{} Connection created, total {}", logPtr(this),
connectionCount);
}
~Connection()
{
- res.setCompleteRequestHandler(nullptr);
+ res.releaseCompleteRequestHandler();
cancelDeadlineTimer();
connectionCount--;
@@ -104,13 +102,7 @@
// don't require auth
if (preverified)
{
- boost::asio::ip::address ipAddress;
- if (getClientIp(ipAddress))
- {
- return true;
- }
-
- mtlsSession = verifyMtlsUser(ipAddress, ctx);
+ mtlsSession = verifyMtlsUser(ip, ctx);
if (mtlsSession)
{
BMCWEB_LOG_DEBUG("{} Generating TLS session: {}", logPtr(this),
@@ -159,15 +151,19 @@
void start()
{
+ BMCWEB_LOG_DEBUG("{} Connection started, total {}", logPtr(this),
+ connectionCount);
if (connectionCount >= 200)
{
- BMCWEB_LOG_CRITICAL("{}Max connection count exceeded.",
+ BMCWEB_LOG_CRITICAL("{} Max connection count exceeded.",
logPtr(this));
return;
}
startDeadline();
+ readClientIp();
+
// TODO(ed) Abstract this to a more clever class with the idea of an
// asynchronous "start"
if constexpr (IsTls<Adaptor>::value)
@@ -216,6 +212,19 @@
doReadHeaders();
}
+ void initParser()
+ {
+ boost::beast::http::request_parser<bmcweb::HttpBody>& instance =
+ parser.emplace(std::piecewise_construct, std::make_tuple());
+
+ // reset header limit for newly created parser
+ instance.header_limit(httpHeaderLimit);
+
+ // Initially set no body limit. We don't yet know if the user is
+ // authenticated.
+ instance.body_limit(boost::none);
+ }
+
void handle()
{
std::error_code reqEc;
@@ -234,7 +243,7 @@
req.session = userSession;
// Fetch the client IP address
- readClientIp();
+ req.ipAddress = ip;
// Check for HTTP version 1.1.
if (req.version() == 11)
@@ -319,22 +328,41 @@
handler->handle(req, asyncResp);
}
- void close()
+ void hardClose()
{
+ BMCWEB_LOG_DEBUG("{} Closing socket", logPtr(this));
+ boost::beast::get_lowest_layer(adaptor).close();
+ }
+
+ void tlsShutdownComplete(const std::shared_ptr<self_type>& self,
+ const boost::system::error_code& ec)
+ {
+ if (ec)
+ {
+ BMCWEB_LOG_WARNING("{} Failed to shut down TLS cleanly {}",
+ logPtr(self.get()), ec);
+ }
+ self->hardClose();
+ }
+
+ void gracefulClose()
+ {
+ BMCWEB_LOG_DEBUG("{} Socket close requested", logPtr(this));
+ if (mtlsSession != nullptr)
+ {
+ BMCWEB_LOG_DEBUG("{} Removing TLS session: {}", logPtr(this),
+ mtlsSession->uniqueId);
+ persistent_data::SessionStore::getInstance().removeSession(
+ mtlsSession);
+ }
if constexpr (IsTls<Adaptor>::value)
{
- adaptor.next_layer().close();
- if (mtlsSession != nullptr)
- {
- BMCWEB_LOG_DEBUG("{} Removing TLS session: {}", logPtr(this),
- mtlsSession->uniqueId);
- persistent_data::SessionStore::getInstance().removeSession(
- mtlsSession);
- }
+ adaptor.async_shutdown(std::bind_front(
+ &self_type::tlsShutdownComplete, this, shared_from_this()));
}
else
{
- adaptor.close();
+ hardClose();
}
}
@@ -355,19 +383,7 @@
void readClientIp()
{
- boost::asio::ip::address ip;
- boost::system::error_code ec = getClientIp(ip);
- if (ec)
- {
- return;
- }
- req.ipAddress = ip;
- }
-
- boost::system::error_code getClientIp(boost::asio::ip::address& ip)
- {
boost::system::error_code ec;
- BMCWEB_LOG_DEBUG("Fetch the client IP address");
if constexpr (!std::is_same_v<Adaptor, boost::beast::test::stream>)
{
@@ -380,19 +396,83 @@
// will be empty.
BMCWEB_LOG_ERROR(
"Failed to get the client's IP Address. ec : {}", ec);
- return ec;
+ return;
}
ip = endpoint.address();
}
- return ec;
}
private:
+ uint64_t getContentLengthLimit()
+ {
+#ifndef BMCWEB_INSECURE_DISABLE_AUTHX
+ if (userSession == nullptr)
+ {
+ return loggedOutPostBodyLimit;
+ }
+#endif
+
+ return httpReqBodyLimit;
+ }
+
+ // Returns true if content length was within limits
+ // Returns false if content length error has been returned
+ bool handleContentLengthError()
+ {
+ if (!parser)
+ {
+ BMCWEB_LOG_CRITICAL("Paser was null");
+ return false;
+ }
+ const boost::optional<uint64_t> contentLength =
+ parser->content_length();
+ if (!contentLength)
+ {
+ BMCWEB_LOG_DEBUG("{} No content length available", logPtr(this));
+ return true;
+ }
+
+ uint64_t maxAllowedContentLength = getContentLengthLimit();
+
+ if (*contentLength > maxAllowedContentLength)
+ {
+ // If the users content limit is between the logged in
+ // and logged out limits They probably just didn't log
+ // in
+ if (*contentLength > loggedOutPostBodyLimit &&
+ *contentLength < httpReqBodyLimit)
+ {
+ BMCWEB_LOG_DEBUG(
+ "{} Content length {} valid, but greater than logged out"
+ " limit of {}. Setting unauthorized",
+ logPtr(this), *contentLength, loggedOutPostBodyLimit);
+ res.result(boost::beast::http::status::unauthorized);
+ }
+ else
+ {
+ // Otherwise they're over both limits, so inform
+ // them
+ BMCWEB_LOG_DEBUG(
+ "{} Content length {} was greater than global limit {}."
+ " Setting payload too large",
+ logPtr(this), *contentLength, httpReqBodyLimit);
+ res.result(boost::beast::http::status::payload_too_large);
+ }
+
+ keepAlive = false;
+ doWrite();
+ return false;
+ }
+
+ return true;
+ }
+
void doReadHeaders()
{
BMCWEB_LOG_DEBUG("{} doReadHeaders", logPtr(this));
if (!parser)
{
+ BMCWEB_LOG_CRITICAL("Parser was not initialized.");
return;
}
// Clean up any previous Connection.
@@ -408,54 +488,57 @@
{
cancelDeadlineTimer();
+ if (ec == boost::beast::http::error::header_limit)
+ {
+ BMCWEB_LOG_ERROR("{} Header field too large, closing",
+ logPtr(this), ec.message());
+
+ res.result(boost::beast::http::status::
+ request_header_fields_too_large);
+ keepAlive = false;
+ doWrite();
+ return;
+ }
if (ec == boost::beast::http::error::end_of_stream)
{
- BMCWEB_LOG_WARNING("{} Error while reading: {}",
- logPtr(this), ec.message());
+ BMCWEB_LOG_WARNING("{} End of stream, closing {}",
+ logPtr(this), ec);
+ hardClose();
+ return;
}
- else
- {
- BMCWEB_LOG_ERROR("{} Error while reading: {}", logPtr(this),
- ec.message());
- }
- close();
- BMCWEB_LOG_DEBUG("{} from read(1)", logPtr(this));
+
+ BMCWEB_LOG_DEBUG("{} Closing socket due to read error {}",
+ logPtr(this), ec.message());
+ gracefulClose();
+
return;
}
- readClientIp();
-
- boost::asio::ip::address ip;
- if (getClientIp(ip))
+ std::string_view expect =
+ req.getHeaderValue(boost::beast::http::field::expect);
+ if (bmcweb::asciiIEquals(expect, "100-continue"))
{
- BMCWEB_LOG_DEBUG("Unable to get client IP");
+ res.result(boost::beast::http::status::continue_);
+ doWrite();
+ return;
}
+
if constexpr (!std::is_same_v<Adaptor, boost::beast::test::stream>)
{
#ifndef BMCWEB_INSECURE_DISABLE_AUTHX
boost::beast::http::verb method = parser->get().method();
userSession = crow::authentication::authenticate(
ip, res, method, parser->get().base(), mtlsSession);
-
- bool loggedIn = userSession != nullptr;
- if (!loggedIn)
- {
- const boost::optional<uint64_t> contentLength =
- parser->content_length();
- if (contentLength &&
- *contentLength > loggedOutPostBodyLimit)
- {
- BMCWEB_LOG_DEBUG("Content length greater than limit {}",
- *contentLength);
- close();
- return;
- }
-
- BMCWEB_LOG_DEBUG("Starting quick deadline");
- }
#endif // BMCWEB_INSECURE_DISABLE_AUTHX
}
+ if (!handleContentLengthError())
+ {
+ return;
+ }
+
+ parser->body_limit(getContentLengthLimit());
+
if (parser->is_done())
{
handle();
@@ -486,8 +569,22 @@
{
BMCWEB_LOG_ERROR("{} Error while reading: {}", logPtr(this),
ec.message());
- close();
- BMCWEB_LOG_DEBUG("{} from read(1)", logPtr(this));
+ if (ec == boost::beast::http::error::body_limit)
+ {
+ if (handleContentLengthError())
+ {
+ BMCWEB_LOG_CRITICAL("Body length limit reached, "
+ "but no content-length "
+ "available? Should never happen");
+ res.result(
+ boost::beast::http::status::internal_server_error);
+ keepAlive = false;
+ doWrite();
+ }
+ return;
+ }
+
+ gracefulClose();
return;
}
@@ -513,8 +610,8 @@
const boost::system::error_code& ec,
std::size_t bytesTransferred)
{
- BMCWEB_LOG_DEBUG("{} async_write {} bytes", logPtr(this),
- bytesTransferred);
+ BMCWEB_LOG_DEBUG("{} async_write wrote {} bytes, ec=", logPtr(this),
+ bytesTransferred, ec);
cancelDeadlineTimer();
@@ -523,19 +620,27 @@
BMCWEB_LOG_DEBUG("{} from write(2)", logPtr(this));
return;
}
+
+ if (res.result() == boost::beast::http::status::continue_)
+ {
+ // Reset the result to ok
+ res.result(boost::beast::http::status::ok);
+ doRead();
+ return;
+ }
+
if (!keepAlive)
{
- close();
- BMCWEB_LOG_DEBUG("{} from write(1)", logPtr(this));
+ BMCWEB_LOG_DEBUG("{} keepalive not set. Closing socket",
+ logPtr(this));
+
+ gracefulClose();
return;
}
BMCWEB_LOG_DEBUG("{} Clearing response", logPtr(this));
res.clear();
- parser.emplace(std::piecewise_construct, std::make_tuple());
- parser->body_limit(httpReqBodyLimit); // reset body limit for
- // newly created parser
- buffer.consume(buffer.size());
+ initParser();
userSession = nullptr;
@@ -581,6 +686,13 @@
weakSelf.lock();
if (!self)
{
+ if (ec == boost::asio::error::operation_aborted)
+ {
+ BMCWEB_LOG_DEBUG(
+ "{} Timer canceled on connection being destroyed",
+ logPtr(self.get()));
+ return;
+ }
BMCWEB_LOG_CRITICAL("{} Failed to capture connection",
logPtr(self.get()));
return;
@@ -588,21 +700,21 @@
self->timerStarted = false;
- if (ec == boost::asio::error::operation_aborted)
- {
- // Canceled wait means the path succeeded.
- return;
- }
if (ec)
{
- BMCWEB_LOG_CRITICAL("{} timer failed {}", logPtr(self.get()),
+ if (ec == boost::asio::error::operation_aborted)
+ {
+ BMCWEB_LOG_DEBUG("{} Timer canceled", logPtr(self.get()));
+ return;
+ }
+ BMCWEB_LOG_CRITICAL("{} Timer failed {}", logPtr(self.get()),
ec);
}
- BMCWEB_LOG_WARNING("{}Connection timed out, closing",
+ BMCWEB_LOG_WARNING("{} Connection timed out, hard closing",
logPtr(self.get()));
- self->close();
+ self->hardClose();
});
timerStarted = true;
@@ -611,6 +723,9 @@
Adaptor adaptor;
Handler* handler;
+
+ boost::asio::ip::address ip;
+
// Making this a std::optional allows it to be efficiently destroyed and
// re-created on Connection reset
std::optional<boost::beast::http::request_parser<bmcweb::HttpBody>> parser;