Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

High CPU usage when TLS client sends TLS Record data exceeding length 17408 #6072

Closed
xiezhaokun opened this issue Mar 18, 2021 · 13 comments · Fixed by #6073 or #6074
Closed

High CPU usage when TLS client sends TLS Record data exceeding length 17408 #6072

xiezhaokun opened this issue Mar 18, 2021 · 13 comments · Fixed by #6073 or #6074
Labels
Sponsored This issue affects a user with a commercial support agreement

Comments

@xiezhaokun
Copy link

xiezhaokun commented Mar 18, 2021

Jetty version
9.4.36.v20210114

Java version
openjdk 8u272

OS type/version
linux

Description

When using a socket to send data with a size greater than 17408, an endless loop will occur in the jetty server.
The following demo can reproduce this problem:

// server 
public class MyServer {
    public static void startServer() throws Exception {
         SslContextFactory sslContextFactory = new SslContextFactory.Server();
         HttpConnectionFactory httpFactory = new HttpConnectionFactory();
         Server server = new Server();
         ServerConnector connector = new 
         ServerConnector(server,null,null,null,1,-1,AbstractConnectionFactory.getFactories(sslContextFactory ,httpFactory));
         connector.setPort(9988);
         connector.setHost("localhost");
         server.setConnectors(new Connector[]{connector});
         ServletContextHandler context = new ServletContextHandler();
         context.setContextPath("/");
         HandlerCollection handlerCollection= new HandlerCollection();
         handlerCollection.setHandlers(new Handler[]{context,new DefaultHandler()});
         server.setHandler(handlerCollection);
         server.start();
         server.join();
    }
     public static void main(String[] args) throws Exception {
        startServer();
     }
}



public class MyClient {
    // client 
    private static byte[] buildMessage() {
       byte[] bytes = new byte[20005];
       bytes[0] = 22;  // record type
       bytes[1] = 3;   // major version
       bytes[2] = 3;   // minor version
       bytes[3] = 78; // record length 2 bytes
       bytes[4] = 32;  // record length
    
       bytes[5] = 1; // message type
       bytes[6] = 0; // message length 3 bytes
       bytes[7] = 78;
       bytes[8] = 23;
      
      for( int i = 9; i < bytes.length; i++) {
          bytes[i] = 1;
      }
      return bytes;
    }
    
    public static void sendMessage() throws Exception {
        byte[] bytes = buildMessage();
        SocketFactory socketFactory = SocketFactory.getDefault();
        Socket socket = socketFactory.createSocket("localhost",9988);
        socket.getOutputStream().write(bytes);
        socket.close();
    }

    public static void main(String[] args) throws Exception {
        sendMessage();
    }
}
joakime added a commit that referenced this issue Mar 18, 2021
…icate

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Copy link
Contributor

joakime commented Mar 18, 2021

Branch jetty-9.4.x-6072-ssl-cpuspin created.
Testcase added in commit f1c3c2e

@joakime
Copy link
Contributor

joakime commented Mar 18, 2021

Per TLS/1.1 spec ...

https://tools.ietf.org/html/rfc4346#section-6.2.2

   length
     The length (in bytes) of the following TLSCompressed.fragment.
     The length should not exceed 2^14 + 1024.

2^14 + 1024 = 17408

We are working out why we are not failing when encountering the example TLS Frame with spec invalid excessive length.

sbordet added a commit that referenced this issue Mar 18, 2021
…408.

Avoid spinning if the input buffer is full.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet pushed a commit that referenced this issue Mar 18, 2021
…408.

Avoid spinning if the input buffer is full.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Co-authored-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
sbordet pushed a commit that referenced this issue Mar 18, 2021
…408.

Avoid spinning if the input buffer is full.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Co-authored-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@xiezhaokun
Copy link
Author

xiezhaokun commented Mar 19, 2021

I think _encryptedInput can be expanded to SSLRecord.maxLargeRecordSize , some SSL/TLS implementation support large fragment upto 2^15, you can sess the more information about maxRecordSize and maxLargeRecordSize in jdk.

/*
     * SSL has a maximum record size.  It's header, (compressed) data,
     * padding, and a trailer for the message authentication information (MAC
     * for block and stream ciphers, and message authentication tag for AEAD
     * ciphers).
     *
     * Some compression algorithms have rare cases where they expand the data.
     * As we don't support compression at this time, leave that out.
     */
    static final int    maxRecordSize =
                                      headerPlusMaxIVSize   // header + iv
                                    + maxDataSize           // data
                                    + maxPadding            // padding
                                    + maxMacSize;           // MAC or AEAD tag

    /*
     * The maximum large record size.
     *
     * Some SSL/TLS implementations support large fragment upto 2^15 bytes,
     * such as Microsoft. We support large incoming fragments.
     *
     * The maximum large record size is defined as maxRecordSize plus 2^14,
     * this is the amount OpenSSL is using.
     */
    static final int    maxLargeRecordSize =
                maxRecordSize   // Max size with a conforming implementation
              + maxDataSize;    // extra 2^14 bytes for large data packets.

@sbordet
Copy link
Contributor

sbordet commented Mar 19, 2021

@xiezhaokun we don't want to use larger buffers if it's not necessary.
We ask the TLS implementation what size to use and we use that.

If the first buffer we read is legit and really the TLS data does not fit it, we will read the TLS data in 2 reads and then the TLS implementation should adjust to use larger buffers.
The next reads we will use larger buffers.

We want to avoid spinning in case of invalid TLS data that fills the whole buffer.

sbordet added a commit that referenced this issue Mar 22, 2021
…408.

Updates after review.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Mar 22, 2021
…408.

Updates after review.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet pushed a commit that referenced this issue Mar 22, 2021
…408.

Avoid spinning if the input buffer is full.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Co-authored-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
sbordet added a commit that referenced this issue Mar 22, 2021
…408.

Updates after review.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet pushed a commit that referenced this issue Mar 22, 2021
…408.

Avoid spinning if the input buffer is full.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Co-authored-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
sbordet added a commit that referenced this issue Mar 22, 2021
…408.

Updates after review.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@xiezhaokun
Copy link
Author

@xiezhaokun we don't want to use larger buffers if it's not necessary.
We ask the TLS implementation what size to use and we use that.

If the first buffer we read is legit and really the TLS data does not fit it, we will read the TLS data in 2 reads and then the TLS implementation should adjust to use larger buffers.
The next reads we will use larger buffers.

We want to avoid spinning in case of invalid TLS data that fills the whole buffer.

In openjdk8u272 , SSLEngineImpl#readRecord method, when packet length > Record.maxRecordSize && length <= Record.maxLargeRecordSize , it will expand buffer size. But In jetty, I did not find where to expand the _encryptedInput.

// Is this packet bigger than SSL/TLS normally allows?
        if (packetLen > conContext.conSession.getPacketBufferSize()) {
            int largestRecordSize = SSLRecord.maxLargeRecordSize;
            if (packetLen <= largestRecordSize) {
                // Expand the expected maximum packet/application buffer
                // sizes.
                //
                // Only apply to SSL/TLS protocols.

                // Old behavior: shall we honor the System Property
                // "jsse.SSLEngine.acceptLargeFragments" if it is "false"?
                conContext.conSession.expandBufferSizes();
            }

            // check the packet again
            largestRecordSize = conContext.conSession.getPacketBufferSize();
            if (packetLen > largestRecordSize) {
                throw new SSLProtocolException(
                        "Input record too big: max = " +
                        largestRecordSize + " len = " + packetLen);
            }
        }

@gregw
Copy link
Contributor

gregw commented Mar 24, 2021

@xiezhaokun We currently do not expand the buffers beyond the sizes provided by SSLSession::getApplicationBufferSize and SSLSession::getPacketBufferSize
Maybe there is a use-case, but we need to consider it carefully to avoid creating overly large buffers which might result in a DOS.

So for now, we just want to fix the 100% CPU issue.

Then in a later release we need to understand why a frame might be larger than the max buffer size suggested by the session, and if we wish to support that or not.

Do you know why a frame larger than the max size in the session should be accepted?

@joakime joakime changed the title jetty server high CPU when client send data length > 17408 High CPU usage when TLS client sends TLS Record data exceeding length 17408 Mar 30, 2021
@joakime
Copy link
Contributor

joakime commented Mar 30, 2021

Here's some values from constants within the JVM itself.

sun.security.ssl.SSLRecord

SSLRecord.headerPlusMaxIVSize = 21
SSLRecord.headerSize = 5
SSLRecord.maxIVLength = 16
SSLRecord.maxPlaintextPlusSize = 325
SSLRecord.handshakeHeaderSize = 4
SSLRecord.maxMacSize = 48
SSLRecord.maxPadding = 256
SSLRecord.maxRecordSize = 16,709
SSLRecord.maxDataSize = 16,384
SSLRecord.maxLargeRecordSize = 33,093

In order for SSLRecord.maxLargeRecordSize to be used by the JVM, the system property jsse.SSLEngine.acceptLargeFragments needs to be set to true (default is false).
Once that property is set, the JVM will adjust the SSLSession.getApplicationBufferSize() to maxLargeRecordSize (but only if the protocol in use is not DTLS)

@xiezhaokun
Copy link
Author

xiezhaokun commented Apr 9, 2021

Here's some values from constants within the JVM itself.

sun.security.ssl.SSLRecord

SSLRecord.headerPlusMaxIVSize = 21
SSLRecord.headerSize = 5
SSLRecord.maxIVLength = 16
SSLRecord.maxPlaintextPlusSize = 325
SSLRecord.handshakeHeaderSize = 4
SSLRecord.maxMacSize = 48
SSLRecord.maxPadding = 256
SSLRecord.maxRecordSize = 16,709
SSLRecord.maxDataSize = 16,384
SSLRecord.maxLargeRecordSize = 33,093

In order for SSLRecord.maxLargeRecordSize to be used by the JVM, the system property jsse.SSLEngine.acceptLargeFragments needs to be set to true (default is false).
Once that property is set, the JVM will adjust the SSLSession.getApplicationBufferSize() to maxLargeRecordSize (but only if the protocol in use is not DTLS)

The maximumPacketSize value will be set to shc.sslConfig.maximumPacketSize when send ServerHello message , so use jsse.SSLEngine.acceptLargeFragments will be invalid when Send the message after ServerHello.

public int getPacketBufferSize() {
        sessionLock.lock();
        try {
            // Use the bigger packet size calculated from maximumPacketSize
            // and negotiatedMaxFragLen.
            int packetSize = 0;
            if (negotiatedMaxFragLen > 0) {
                packetSize = cipherSuite.calculatePacketSize(
                        negotiatedMaxFragLen, protocolVersion,
                        protocolVersion.isDTLS);
            }

            if (maximumPacketSize > 0) {
                return (maximumPacketSize > packetSize) ?
                        maximumPacketSize : packetSize;
            }

            if (packetSize != 0) {
               return packetSize;
            }

            if (protocolVersion.isDTLS) {
                return DTLSRecord.maxRecordSize;
            } else {
                return acceptLargeFragments ?
                        SSLRecord.maxLargeRecordSize : SSLRecord.maxRecordSize;
            }
        } finally {
            sessionLock.unlock();
        }
    }
SSLSessionImpl session =
                        new SSLSessionImpl(shc, CipherSuite.C_NULL);
                session.setMaximumPacketSize(shc.sslConfig.maximumPacketSize);

@sbordet
Copy link
Contributor

sbordet commented Apr 9, 2021

@xiezhaokun I'm not sure I understand your point?

@yrewzjs
Copy link

yrewzjs commented Apr 10, 2021

The "maximumPacketSize" was set to "shc.sslConfig.maximumPacketSize (16709) " when send ServerHello message. So the function "getPacketBufferSize" can not return "SSLRecord.maxLargeRecordSize" when the config item "-Djsse.SSLEngine.acceptLargeFragments=true".

@gregw
Copy link
Contributor

gregw commented Aug 31, 2021

Need to backport to 9.3

@gregw gregw reopened this Aug 31, 2021
@gregw gregw added the Sponsored This issue affects a user with a commercial support agreement label Aug 31, 2021
olamy added a commit that referenced this issue Sep 2, 2021
Signed-off-by: Olivier Lamy <oliver.lamy@gmail.com>
olamy added a commit that referenced this issue Sep 2, 2021
Signed-off-by: Olivier Lamy <oliver.lamy@gmail.com>
joakime added a commit that referenced this issue Sep 8, 2021
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime pushed a commit that referenced this issue Sep 8, 2021
Co-authored-by: Olivier Lamy <oliver.lamy@gmail.com>
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Sep 10, 2021
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Sep 15, 2021
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Sep 20, 2021
…-tls-fix

Issue #6072 - backport of minimum fix for jetty-9.3.x
@lachlan-roberts
Copy link
Contributor

@joakime @gregw can we close now that this has been merged to 9.3?

@joakime
Copy link
Contributor

joakime commented Sep 30, 2021

@lachlan-roberts yes, this is complete in my mind.

@joakime joakime closed this as completed Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sponsored This issue affects a user with a commercial support agreement
Projects
None yet
6 participants