Data corruption due to interaction between SslHandler and HttpObjectEncoder

[trustin]

TL;DR:

HttpObjectEncoder sends a duplicate of its static final ByteBuf field to encode HTTP chunks, such as 0\r\n\r\n. This reused buffer is neither read-only nor unexpandable, and thus anyone in the pipeline can modify or expand it. As a result, SslHandler can sometimes expand it and append stuff to it, causing unexpected data corruption. It'd be nice if this issue is fixed soon and a new release is out given its severity.

Here's the full analysis from my colleague @alexc-db:

1. When Channel.flush() is called, SslHandler starts getting items from the pendingUnencryptedWrites queue [1] - it gets entries from the internal bufAndListenerPairs and accumulates them in the toReturn buffer [2].
2. For the first entry, toReturn is null and thus composeFirst is called. That method by default just returns the reference to the first entry (it’s overridden in SslHandlerCoalescingBufferQueue to have special handling for CompositeByteBuf, but it’s not relevant to the issue here). The following calls are calling compose and pass toReturn there.
3. Internally, compose calls attemptCopyToCumulation which checks if toReturn is writable and its capacity can be expanded to write new bytes from the following queued buffers - if it passes checks in [3], attemptCopyToCumulation copies bytes from the next entries to the original toReturn buffer (as we saw above it can be the first entry), if it does not pass the checks - SslHandlerCoalescingBufferQueue calls copyAndCompose which creates a new bigger buf and copies bytes from toReturn and next entry there, making it a new toReturn.
4. Given the behavior above - if flush is called and bufAndListenerPairs has multiple buffers queued where the first entry is writable, had enough capacity for the bytes in the next entries, and shared across multiple threads - two threads will start copying bytes into the same memory addresses which will cause corruption. In our local reproducer that has more dependencies that just Netty, it was ZERO_CRLF_CRLF_BUF [4] (duplicate() call doesn’t make a copy, just wraps the original direct buffer).
   I was able to make tests pass by marking ZERO_CRLF_CRLF_BUF read-only [5].
   [1] https://github.com/netty/netty/blob/netty-4.1.68.Final/handler/src/main/java/io/netty/handler/ssl/SslHandler.java#L817
   [2] https://github.com/netty/netty/blob/netty-4.1.68.Final/transport/src/main/java/io/netty/channel/AbstractCoalescingBufferQueue.java#L175
   [3] https://github.com/netty/netty/blob/netty-4.1.68.Final/handler/src/main/java/io/netty/handler/ssl/SslHandler.java#L2326-L2334
   [4] https://github.com/netty/netty/blob/netty-4.1.68.Final/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java#L210
   [5]

diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java (revision 7d34282f9d2ffdd64c91cb4780b09902d9779b92)
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java (date 1635112426650)
@@ -50,9 +50,9 @@
     static final int CRLF_SHORT = (CR << 8) | LF;
     private static final int ZERO_CRLF_MEDIUM = ('0' << 16) | CRLF_SHORT;
     private static final byte[] ZERO_CRLF_CRLF = { '0', CR, LF, CR, LF };
-    private static final ByteBuf CRLF_BUF = unreleasableBuffer(directBuffer(2).writeByte(CR).writeByte(LF));
+    private static final ByteBuf CRLF_BUF = unreleasableBuffer(directBuffer(2).writeByte(CR).writeByte(LF).asReadOnly());
     private static final ByteBuf ZERO_CRLF_CRLF_BUF = unreleasableBuffer(directBuffer(ZERO_CRLF_CRLF.length)
-            .writeBytes(ZERO_CRLF_CRLF));
+            .writeBytes(ZERO_CRLF_CRLF).asReadOnly());
     private static final float HEADERS_WEIGHT_NEW = 1 / 5f;
     private static final float HEADERS_WEIGHT_HISTORICAL = 1 - HEADERS_WEIGHT_NEW;
     private static final float TRAILERS_WEIGHT_NEW = HEADERS_WEIGHT_NEW;

Netty version

4.1.68.Final and 4.1.69.Final and of course possible in older versions.

[normanmaurer]

@trustin can you just do a PR so we can easily merge and attribute ?

[alexc-db]

@normanmaurer @trustin Do you have a preference/opinion on how to prevent this class of issues going forward? There are a few more instances where static buffers are not marked as read-only, quick search found [1][2], there may be more. We can add unit-tests for the affected classes, but it will not prevent new instances in other places. Ideally, there should be a compile-time check or validation that'll catch issues like that during development, before changes are merged.

[1] https://github.com/netty/netty/blob/netty-4.1.68.Final/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocket00FrameEncoder.java#L34-L39
[2] https://github.com/netty/netty/blob/netty-4.1.68.Final/codec-smtp/src/main/java/io/netty/handler/codec/smtp/SmtpRequestEncoder.java#L36-L37

[trustin]

@normanmaurer, @alexc-db will send out a PR.

@alexc-db, I'd recommend 1) making sure all static final ByteBuf instantiation involves a certain new utility method that makes a buffer read-only, e.g. ByteBufUtil.newConstantBuf(ByteBuf) and 2) writing a Checkstyle rule for it, as we discussed offline. What do you think, @normanmaurer?

[normanmaurer]

@trustin sounds good!

[alexc-db]

Added validation check in netty/netty-build#12, seems like I don't have permissions to assign reviewers, @normanmaurer - could you please take a look?