SslHandler and OpenSslEngine miscalculation of wrap destination buffe… #6492

Scottmitch · 2017-03-03T19:26:47Z

…r size

Motivation:
When we do a wrap operation we calculate the maximum size of the destination buffer ahead of time, and return a BUFFER_OVERFLOW exception if the destination buffer is not big enough. However if there is a CompositeByteBuf the wrap operation may consist of multiple ByteBuffers and each incurs its own overhead during the encryption. We currently don't account for the overhead required for encryption if there are multiple ByteBuffers and we assume the overhead will only apply once to the entire input size. If there is not enough room to write an entire encrypted packed into the BIO SSL_write will return -1 despite having actually written content to the BIO. We then attempt to retry the write with a bigger buffer, but because SSL_write is stateful the remaining bytes from the previous operation are put into the BIO. This results in sending the second half of the encrypted data being sent to the peer which is not of proper format and the peer will be confused and ultimately not get the expected data (which may result in a fatal error). In this case because SSL_write returns -1 we have no way to know how many bytes were actually consumed and so the best we can do is ensure that we always allocate a destination buffer with enough space so we are guaranteed to complete the write operation synchronously.

Modifications:

SslHandler#allocateNetBuf should take into account how many ByteBuffers will be wrapped and apply the encryption overhead for each
Include the TLS header length in the overhead computation

Result:
Fixes #6481

normanmaurer · 2017-03-03T19:28:26Z

handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java

+
+    private void compositeBufSizeEstimationGuaranteesSynchronousWrite(SslProvider serverProvider,
+                                                                      SslProvider clientProvider)
+            throws CertificateException, SSLException {


formatting looks strange

how would you prefer this be formatted?

private void compositeBufSizeEstimationGuaranteesSynchronousWrite( SslProvider serverProvider, SslProvider clientProvider) throws CertificateException, SSLException {

Something like this maybe ?

normanmaurer · 2017-03-03T19:29:08Z

handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java

+            group.shutdownGracefully();
+
+            ReferenceCountUtil.release(sslServerCtx);
+            ReferenceCountUtil.release(sslClientCtx);


call ssc.delete();

normanmaurer · 2017-03-03T19:30:35Z

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

@@ -107,11 +107,12 @@
     * allow up to 255 bytes. 16 bytes is the max for PKC#5 (which handles it the same way as PKC#7) as we use a block
     * size of 16. See <a href="https://tools.ietf.org/html/rfc5652#section-6.3">rfc5652#section-6.3</a>.
     *
-     * 16 (IV) + 48 (MAC) + 1 (Padding_length field) + 15 (Padding) + 1 (ContentType) + 2 (ProtocolVersion) + 2 (Length)
+     * 5 (TLS Header) + 16 (IV) + 48 (MAC) + 1 (Padding_length field) + 15 (Padding) + 1 (ContentType) +


So we missed the 5 bytes ?

normanmaurer · 2017-03-03T19:32:53Z

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

-    static int calculateOutNetBufSize(int pendingBytes) {
-        return min(MAX_ENCRYPTED_PACKET_LENGTH, MAX_ENCRYPTION_OVERHEAD_LENGTH
+    static int calculateOutNetBufSize(int pendingBytes, int numComponents) {
+        return min(MAX_ENCRYPTED_PACKET_LENGTH, MAX_ENCRYPTION_OVERHEAD_LENGTH * numComponents


why do we need to multiply this by the number of components ? Shouldn't it not matter as we only need to worry about the summary of the readable bytes of all of them as we can only create one record with all of the components in it.

This is described in the commit message but if we have a CompositeByteBuf we break that down into ByteBuffer[] and call SSL_write for each of these. So each ByteBuffer will incur the tls record overhead. I wanted to review this more because it may be the case that we can apply the encryption overhead to all the bytes, and other apply the tls record overhead (header) for each ByteBuffer. ~~I think this case ... so let me refine this estimate.~~

normanmaurer · 2017-03-03T19:34:03Z

handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java

@@ -560,4 +564,127 @@ public void operationComplete(Future<Channel> future) {
            ReferenceCountUtil.release(sslClientCtx);
        }
    }
+
+    @Test(timeout = 30000)
+    public void testCompositeBufSizeEstimationGuaranteesSynchronousWrite() throws CertificateException, SSLException {


also would be nice if we had this as test that not depends on the SslHandler but only uses the SSLEngine itself

SslHandler is involved in the estimation process so it has a role to play in this failure.

Scottmitch · 2017-03-03T21:32:15Z

@normanmaurer - PTAL

Scottmitch · 2017-03-03T22:06:17Z

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

-        return min(MAX_ENCRYPTED_PACKET_LENGTH, MAX_ENCRYPTION_OVERHEAD_LENGTH
-                + min(MAX_ENCRYPTION_OVERHEAD_DIFF, pendingBytes));
+    static int calculateOutNetBufSize(int pendingBytes, int numComponents) {
+        int amt = pendingBytes + MAX_TLS_RECORD_OVERHEAD_LENGTH * numComponents;


@normanmaurer - I consulted with @davidben regarding the question in #6492 (comment) ... each call to SSL_write should account for the encryption overhead + tls header length amount of overhead.

Is Integer.MAX_VALUE safe?

@Scottmitch ok thanks for verify

johnou · 2017-03-03T22:13:24Z

handler/src/main/java/io/netty/handler/ssl/SslHandler.java

-                return TCNATIVE;
-            }
-            return JDK;
+            return engine instanceof ReferenceCountedOpenSslEngine ? TCNATIVE : JDK;


JDK SslEngine when using OpenSslEngine and finalizer? 😕 Shouldn't this instance of check remain OpenSslEngine?

JDK SslEngine when using OpenSslEngine and finalizer?

I don't understand this statement. Can you explain.

Basically ReferenceCountedOpenSslEngine is the base class of OpenSslEngine and is the class which provides the interface which SslEngineType#TCNATIVE depends upon.

eugh, I had it in my head that ReferenceCountedOpenSslEngine extends OpenSslEngine.

johnou · 2017-03-03T22:15:57Z

handler/src/main/java/io/netty/handler/ssl/SslHandler.java

@@ -193,7 +193,7 @@ SSLEngineResult unwrap(SslHandler handler, ByteBuf in, int readerIndex, int len,
                     * we can use a special {@link OpenSslEngine#unwrap(ByteBuffer[], ByteBuffer[])} method
                     * that accepts multiple {@link ByteBuffer}s without additional memory copies.
                     */
-                    OpenSslEngine opensslEngine = (OpenSslEngine) handler.engine;
+                    ReferenceCountedOpenSslEngine opensslEngine = (ReferenceCountedOpenSslEngine) handler.engine;


classcastexception when using openssl with finalizer?

I don't think so. ReferenceCountedOpenSslEngine is the base class of OpenSslEngine.

…r size Motivation: When we do a wrap operation we calculate the maximum size of the destination buffer ahead of time, and return a BUFFER_OVERFLOW exception if the destination buffer is not big enough. However if there is a CompositeByteBuf the wrap operation may consist of multiple ByteBuffers and each incurs its own overhead during the encryption. We currently don't account for the overhead required for encryption if there are multiple ByteBuffers and we assume the overhead will only apply once to the entire input size. If there is not enough room to write an entire encrypted packed into the BIO SSL_write will return -1 despite having actually written content to the BIO. We then attempt to retry the write with a bigger buffer, but because SSL_write is stateful the remaining bytes from the previous operation are put into the BIO. This results in sending the second half of the encrypted data being sent to the peer which is not of proper format and the peer will be confused and ultimately not get the expected data (which may result in a fatal error). In this case because SSL_write returns -1 we have no way to know how many bytes were actually consumed and so the best we can do is ensure that we always allocate a destination buffer with enough space so we are guaranteed to complete the write operation synchronously. Modifications: - SslHandler#allocateNetBuf should take into account how many ByteBuffers will be wrapped and apply the encryption overhead for each - Include the TLS header length in the overhead computation Result: Fixes netty#6481

Scottmitch · 2017-03-06T16:15:43Z

4.1 (53fc693) 4.0 (ccd6098)

Scottmitch added the defect label Mar 3, 2017

Scottmitch added this to the 4.0.45.Final milestone Mar 3, 2017

Scottmitch self-assigned this Mar 3, 2017

Scottmitch requested a review from normanmaurer March 3, 2017 19:26

normanmaurer reviewed Mar 3, 2017

View reviewed changes

Scottmitch force-pushed the ssl_bug branch 2 times, most recently from d750e50 to 05df78b Compare March 3, 2017 19:30

normanmaurer requested changes Mar 3, 2017

View reviewed changes

normanmaurer reviewed Mar 3, 2017

View reviewed changes

Scottmitch mentioned this pull request Mar 3, 2017

SingleThreadEventLoopTest#scheduleTaskAtFixedRateB test failure #5924

Open

Scottmitch force-pushed the ssl_bug branch from 05df78b to e991b8e Compare March 3, 2017 21:23

Scottmitch mentioned this pull request Mar 3, 2017

SSL_read failed: OpenSSL error #6481

Closed

Scottmitch force-pushed the ssl_bug branch from e991b8e to 5b57fa0 Compare March 3, 2017 22:04

Scottmitch commented Mar 3, 2017

View reviewed changes

johnou suggested changes Mar 3, 2017

View reviewed changes

Scottmitch force-pushed the ssl_bug branch from 5b57fa0 to f545529 Compare March 3, 2017 23:33

Scottmitch force-pushed the ssl_bug branch from f545529 to 677f5e2 Compare March 4, 2017 00:11

normanmaurer approved these changes Mar 4, 2017

View reviewed changes

Scottmitch closed this Mar 6, 2017

Scottmitch deleted the ssl_bug branch March 6, 2017 16:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SslHandler and OpenSslEngine miscalculation of wrap destination buffe… #6492

SslHandler and OpenSslEngine miscalculation of wrap destination buffe… #6492

Scottmitch commented Mar 3, 2017

normanmaurer Mar 3, 2017

Scottmitch Mar 3, 2017

normanmaurer Mar 3, 2017

Scottmitch Mar 3, 2017

normanmaurer Mar 3, 2017

Scottmitch Mar 3, 2017

normanmaurer Mar 3, 2017

Scottmitch Mar 3, 2017

normanmaurer Mar 3, 2017

Scottmitch Mar 3, 2017 •

edited

Loading

normanmaurer Mar 3, 2017

Scottmitch Mar 3, 2017

Scottmitch commented Mar 3, 2017

Scottmitch Mar 3, 2017 •

edited

Loading

johnou Mar 3, 2017

normanmaurer Mar 4, 2017

johnou Mar 3, 2017 •

edited

Loading

Scottmitch Mar 3, 2017

johnou Mar 4, 2017

johnou Mar 3, 2017

Scottmitch Mar 3, 2017

Scottmitch commented Mar 6, 2017

SslHandler and OpenSslEngine miscalculation of wrap destination buffe… #6492

SslHandler and OpenSslEngine miscalculation of wrap destination buffe… #6492

Conversation

Scottmitch commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottmitch Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottmitch commented Mar 3, 2017

Scottmitch Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnou Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottmitch commented Mar 6, 2017

Scottmitch Mar 3, 2017 •

edited

Loading

Scottmitch Mar 3, 2017 •

edited

Loading

johnou Mar 3, 2017 •

edited

Loading