From 0a8264e7761859f81acacf64cf496b0b47e54c60 Mon Sep 17 00:00:00 2001 From: Alex Hultman Date: Wed, 2 Dec 2020 03:37:56 +0100 Subject: [PATCH] Document & fix PROXY regression/bug --- src/HttpParser.h | 45 +++++++++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/src/HttpParser.h b/src/HttpParser.h index 2917dcc..c63e32f 100644 --- a/src/HttpParser.h +++ b/src/HttpParser.h @@ -34,7 +34,7 @@ namespace uWS { /* We require at least this much post padding */ -static const int MINIMUM_HTTP_POST_PADDING = 32; +static const unsigned int MINIMUM_HTTP_POST_PADDING = 32; struct HttpRequest { @@ -188,7 +188,7 @@ private: // the only caller of getHeaders template - std::pair fenceAndConsumePostPadded(char *data, unsigned int length, void *user, void *reserved, HttpRequest *req, fu2::unique_function &requestHandler, fu2::unique_function &dataHandler) { + std::pair fenceAndConsumePostPadded(char *data, unsigned int length, void *user, void *reserved, HttpRequest *req, fu2::unique_function &requestHandler, fu2::unique_function &dataHandler) { /* How much data we CONSUMED (to throw away) */ unsigned int consumedTotal = 0; @@ -198,8 +198,11 @@ private: ProxyParser *pp = (ProxyParser *) reserved; /* Parse PROXY protocol */ - auto [done, offset] = pp->parse({data, /*(unsigned int)*/ length}); + auto [done, offset] = pp->parse({data, length}); if (!done) { + /* We do not reset the ProxyParser (on filure) since it is tied to this + * connection, which is really only supposed to ever get one PROXY frame + * anyways. We do however allow multiple PROXY frames to be sent (overwrites former). */ return {0, user}; } else { /* We have consumed this data so skip it */ @@ -212,6 +215,9 @@ private: (void) reserved; #endif + /* For tracking whether we managed to parse more than just PROXY */ + unsigned int consumedProxyParser = consumedTotal; + /* Fence one byte past end of our buffer (buffer has post padded margins) */ data[length] = '\r'; @@ -266,6 +272,15 @@ private: break; } } + + /* It is critical for fallback buffering logic that we only return with success + * if we managed to parse a complete HTTP request. Returning success for PROXY means + * we can end up succeeding, yet leaving bytes in the fallback buffer which is then + * removed, and our counters to flip due to overflow and we end up with a crash */ + if (consumedProxyParser == consumedTotal) { + return {0, user}; + } + return {consumedTotal, user}; } @@ -281,7 +296,7 @@ public: // this is exactly the same as below! // todo: refactor this if (remainingStreamingBytes >= length) { - void *returnedUser = dataHandler(user, std::string_view(data, length), remainingStreamingBytes == (unsigned int) length); + void *returnedUser = dataHandler(user, std::string_view(data, length), remainingStreamingBytes == length); remainingStreamingBytes -= length; return returnedUser; } else { @@ -298,26 +313,28 @@ public: } } else if (fallback.length()) { - int had = (int) fallback.length(); + unsigned int had = (unsigned int) fallback.length(); size_t maxCopyDistance = std::min(MAX_FALLBACK_SIZE - fallback.length(), (size_t) length); /* We don't want fallback to be short string optimized, since we want to move it */ - fallback.reserve(fallback.length() + maxCopyDistance + std::max(MINIMUM_HTTP_POST_PADDING, sizeof(std::string))); + fallback.reserve(fallback.length() + maxCopyDistance + std::max(MINIMUM_HTTP_POST_PADDING, sizeof(std::string))); fallback.append(data, maxCopyDistance); // break here on break - std::pair consumed = fenceAndConsumePostPadded(fallback.data(), (unsigned int) fallback.length(), user, reserved, &req, requestHandler, dataHandler); + std::pair consumed = fenceAndConsumePostPadded(fallback.data(), (unsigned int) fallback.length(), user, reserved, &req, requestHandler, dataHandler); if (consumed.second != user) { return consumed.second; } if (consumed.first) { + /* This logic assumes that we consumed everything in fallback buffer. + * This is critically important, as we will get an integer overflow in case + * of "had" being larger than what we consumed, and that we would drop data */ fallback.clear(); - - data += (unsigned int) (consumed.first - had); - length -= (unsigned int) (consumed.first - had); + data += consumed.first - had; + length -= consumed.first - had; if (remainingStreamingBytes) { // this is exactly the same as above! @@ -349,16 +366,16 @@ public: } } - std::pair consumed = fenceAndConsumePostPadded(data, length, user, reserved, &req, requestHandler, dataHandler); + std::pair consumed = fenceAndConsumePostPadded(data, length, user, reserved, &req, requestHandler, dataHandler); if (consumed.second != user) { return consumed.second; } - data += (unsigned int) consumed.first; - length -= (unsigned int) consumed.first; + data += consumed.first; + length -= consumed.first; if (length) { - if ((unsigned int) length < MAX_FALLBACK_SIZE) { + if (length < MAX_FALLBACK_SIZE) { fallback.append(data, length); } else { return errorHandler(user);