From e7f22da9c01286a2b0e1e5fbdf3ec9ab3aa128ff Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Wed, 4 Oct 2023 18:13:16 -0500 Subject: [PATCH] Fix inconsistent decision about whether a WebSocket message is compressed. When processing the header, we decide whether the mesasge is compressed based on the presence of the appropriate flag in the header. However, after receiving the whole message, we were expecting it to be compressed based only on whether the session had negotiated compression upfront. With this patch we only rely on the bits in the header (and throw if the header says it is compressed, but we didn't negotiate compression.) --- c++/src/kj/compat/http.c++ | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/c++/src/kj/compat/http.c++ b/c++/src/kj/compat/http.c++ index b1968f3576..1c4be8e807 100644 --- a/c++/src/kj/compat/http.c++ +++ b/c++/src/kj/compat/http.c++ @@ -2574,6 +2574,7 @@ public: } bool isFin = recvHeader.isFin(); + bool isCompressed = false; kj::Array message; // space to allocate byte* payloadTarget; // location into which to read payload (size is payloadLen) @@ -2584,6 +2585,7 @@ public: // Add 4 since we append 0x00 0x00 0xFF 0xFF to the tail of the payload. // See: https://datatracker.ietf.org/doc/html/rfc7692#section-7.2.2 amountToAllocate = payloadLen + 4; + isCompressed = true; } else { // Add space for NUL terminator when allocating text message. amountToAllocate = payloadLen + (opcode == OPCODE_TEXT && isFin); @@ -2632,7 +2634,8 @@ public: Mask mask = recvHeader.getMask(); auto handleMessage = - [this,opcode,payloadTarget,payloadLen,mask,isFin,maxSize,originalMaxSize,message=kj::mv(message)]() mutable + [this,opcode,payloadTarget,payloadLen,mask,isFin,maxSize,originalMaxSize, + isCompressed,message=kj::mv(message)]() mutable -> kj::Promise { if (!mask.isZero()) { mask.apply(kj::arrayPtr(payloadTarget, payloadLen)); @@ -2651,8 +2654,10 @@ public: KJ_UNREACHABLE; case OPCODE_TEXT: #if KJ_HAS_ZLIB - KJ_IF_MAYBE(config, compressionConfig) { + if (isCompressed) { + auto& config = KJ_ASSERT_NONNULL(compressionConfig); auto& decompressor = KJ_ASSERT_NONNULL(decompressionContext); + KJ_ASSERT(message.size() >= 4); auto tail = message.slice(message.size() - 4, message.size()); // Note that we added an additional 4 bytes to `message`s capacity to account for these // extra bytes. See `amountToAllocate` in the if(recvHeader.isCompressed()) block above. @@ -2660,7 +2665,7 @@ public: memcpy(tail.begin(), tailBytes, sizeof(tailBytes)); // We have to append 0x00 0x00 0xFF 0xFF to the message before inflating. // See: https://datatracker.ietf.org/doc/html/rfc7692#section-7.2.2 - if (config->inboundNoContextTakeover) { + if (config.inboundNoContextTakeover) { // We must reset context on each message. decompressor.reset(); } @@ -2675,8 +2680,10 @@ public: return Message(kj::String(message.releaseAsChars())); case OPCODE_BINARY: #if KJ_HAS_ZLIB - KJ_IF_MAYBE(config, compressionConfig) { + if (isCompressed) { + auto& config = KJ_ASSERT_NONNULL(compressionConfig); auto& decompressor = KJ_ASSERT_NONNULL(decompressionContext); + KJ_ASSERT(message.size() >= 4); auto tail = message.slice(message.size() - 4, message.size()); // Note that we added an additional 4 bytes to `message`s capacity to account for these // extra bytes. See `amountToAllocate` in the if(recvHeader.isCompressed()) block above. @@ -2684,7 +2691,7 @@ public: memcpy(tail.begin(), tailBytes, sizeof(tailBytes)); // We have to append 0x00 0x00 0xFF 0xFF to the message before inflating. // See: https://datatracker.ietf.org/doc/html/rfc7692#section-7.2.2 - if (config->inboundNoContextTakeover) { + if (config.inboundNoContextTakeover) { // We must reset context on each message. decompressor.reset(); }