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

SslHandler does not send SSL close notify after receiving an invalid client certificate #3900

Closed
SutarSuhas opened this issue Jun 18, 2015 · 36 comments
Assignees
Labels
Milestone

Comments

@SutarSuhas
Copy link

Our server application implements a simple SSLHandler by extending SslHandler, and implements another handler (ServerHandler) for application message/exception handling. We are trying to get Netty SSLHandler to send an SSL close notify message before closing the channel.

However, whenever our client application sends an invalid SSL certificate, it never receives ssl close notify message before TCP reset. In "exceptionCaught" method of SSLHandler we invoke "close" method of SslHandler and in "exceptionCaught" method of ServerHandler we invoke "ExceptionEvent.getChannel().close()" method.

According to https://netty.io/4.0/api/io/netty/handler/ssl/SslHandler.html#close(), "...the close() method should be called to send the close_notify message to the remote peer. One exception is when you close the Channel - SslHandler intercepts the close request and send the close_notify message before the channel closure automatically...". However, it does not seem to work as expected.

Below are few log snippets:

TLS: Unable to verify Client Certificate for: EMAILADDRESS=someone@somewhere.com, CN=someone@somewhere.com, OU=Enterprise, O=somewhere, L=Sunrise, ST=FL, C=US; with Algorithm: SHA256withRSA; AuthType: RSA; error: sun.security.validator.ValidatorException: No trusted certificate found
New I/O worker #1, fatal error: 46: General SSLEngine problem
sun.security.validator.ValidatorException: No trusted certificate found
%% Invalidated: [Session-1, TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA]
New I/O worker #1, SEND TLSv1 ALERT: fatal, description = certificate_unknown
New I/O worker #1, WRITE: TLSv1 Alert, length = 2
New I/O worker #1, fatal: engine already closed. Rethrowing javax.net.ssl.SSLHandshakeException: General SSLEngine problem
New I/O worker #1, called closeOutbound()
New I/O worker #1, closeOutboundInternal()
New I/O worker #1, called closeInbound()
New I/O worker #1, fatal: engine already closed. Rethrowing javax.net.ssl.SSLException: Inbound closed before receiving peer's close_notify: possible truncation attack?
ServerSSLHandler: exceptionCaught: Exception=javax.net.ssl.SSLHandshakeException: General SSLEngine problem
ServerSSLHandler: exceptionCaught: closing SSL handler
New I/O worker #1, called closeOutbound()
New I/O worker #1, closeOutboundInternal()
[Raw write]: length = 7
0000: 15 03 01 00 02 02 2E .......
ServerSSLHandler: exceptionCaught: closing SSL handler
ServerHandler: exceptionCaught. Exception: javax.net.ssl.SSLHandshakeException: General SSLEngine problem
ServerHandler: exceptionCaught: closing channel
ServerHandler: channelDisconnected
New I/O worker #1, called closeOutbound()
New I/O worker #1, closeOutboundInternal()

ServerHandler: channelClosed

I can provide the code snippets of my sample application if required.

@normanmaurer normanmaurer self-assigned this Jun 27, 2015
@normanmaurer normanmaurer added this to the 4.0.30.Final milestone Jun 27, 2015
@normanmaurer
Copy link
Member

@SutarSuhas code snippets would help. Also do you use the OpenSSL bindings or not ?

@normanmaurer
Copy link
Member

@SutarSuhas ping

@normanmaurer
Copy link
Member

@SutarSuhas any news here ?

@wolftobias
Copy link

I am using Netty (version 4.0.27-final) with OpenSSL implementation provided by Netty. What i have observed that shutdown behavior in bad case is not clean. For example netty acting as Server have to verify the peer certificate and verification is failed e.g. unknown_certificate. OpenSSL sets a handshake error with an alert message "unknown_certificate". But this alert is not sent to the peer. I traced with wireshark. The following code in OpenSSLEngine.unwrap method throws the exception:

int lastPrimingReadResult = SSL.readFromSSL(ssl, EMPTY_ADDR, 0); // priming read

    // check if SSL_read returned <= 0. In this case we need to check the error and see if it was something
    // fatal.
    if (lastPrimingReadResult <= 0) {
        // Check for OpenSSL errors caused by the priming read
        long error = SSL.getLastErrorNumber();
        if (OpenSsl.isError(error)) {
            String err = SSL.getErrorString(error);
            if (logger.isDebugEnabled()) {
                logger.debug(
                        "SSL_read failed: primingReadResult: " + lastPrimingReadResult +
                                "; OpenSSL error: '" + err + '\'');
            }

            // There was an internal error -- shutdown
            shutdown();
            throw new SSLException(err);
        }
    }

The exception is thrown because OpenSSL has already set an error for verification failed. But there is still data (alert data with first byte 21, can see from eclipse debug) in BIO that has not been transferred.

How can i resolve the issue? For my application a clean shutdown is a critical requirement.

@normanmaurer
Copy link
Member

I will have a look once back from vacation

@ramahmoo
Copy link

ramahmoo commented Dec 4, 2015

In case if error is there and Network bio has pending data(in this case alert data), i tried just return with NEED_WRAP and in next wrap() the pending data was consumed and written to wire. I can see the Alert data in Wireshark trace. This was just a quick test. We have to consume the pending bio data and afterwards exception must be thrown to inform SSLHandler about the problem.

 if (lastPrimingReadResult <= 0) {
        // Check for OpenSSL errors caused by the priming read
        long error = SSL.getLastErrorNumber();
        if (OpenSsl.isError(error)) {
            String err = SSL.getErrorString(error);
            if (logger.isDebugEnabled()) {
                logger.debug(
                        "SSL_read failed: primingReadResult: " + lastPrimingReadResult +
                                "; OpenSSL error: '" + err + '\'');
            }
           if (pendingWrittenBytesInBIO(networkBIO) > 0) {
             return NEED_WRAP_OK;
           } else {
             // There was an internal error -- shutdown
                 shutdown();
                 throw new SSLException(err);
           }
}

@normanmaurer
Copy link
Member

If you can provide a fix as pr I'm more then happy to review

Am 04.12.2015 um 12:09 schrieb ramahmoo notifications@github.com:

In case if error is there and Network bio has pending data(in this case alert data), i tried just return with NEED_WRAP and in next wrap() the pending data was consumed and written to wire. I can see the Alert data in Wireshark trace. This was just a quick test. We have to consume the pending bio data and afterwards exception must be thrown to inform SSLHandler about the problem.

if (lastPrimingReadResult <= 0) {
// Check for OpenSSL errors caused by the priming read
long error = SSL.getLastErrorNumber();
if (OpenSsl.isError(error)) {
String err = SSL.getErrorString(error);
if (logger.isDebugEnabled()) {
logger.debug(
"SSL_read failed: primingReadResult: " + lastPrimingReadResult +
"; OpenSSL error: '" + err + ''');
}
if (pendingWrittenBytesInBIO(networkBIO) > 0) {
return NEED_WRAP_OK;
} else {
// There was an internal error -- shutdown
shutdown();
throw new SSLException(err);
}
}

Reply to this email directly or view it on GitHub.

@normanmaurer
Copy link
Member

@wolftobias what version of netty this is ?

@ramahmoo
Copy link

This is 4.0.27-final but we have tested with 4.0.33-fonal and same behavior.

@ramahmoo
Copy link

@normanmaurer Any update on this issue?

@normanmaurer
Copy link
Member

@ramahmoo I think I will open a PR today.

@normanmaurer
Copy link
Member

@ramahmoo I was more interested to understand how you detected you not received the alert data.

@ramahmoo
Copy link

traced with wireshark, see the following wireshark trace screenshot
fin without tls alert

@ramahmoo
Copy link

here is the result with Java socket based impl
image

@normanmaurer
Copy link
Member

@ramahmoo thanks!

@normanmaurer
Copy link
Member

@ramahmoo one last question... you are using OpenSslEngine on the server or client side when you see the issue ?

@ramahmoo
Copy link

As client, but i think is also as server.

@normanmaurer
Copy link
Member

@ramahmoo when you said "using the JDK one" you are refer to use SslProvider.JDK and still netty right ?

@ramahmoo
Copy link

No, just a simple java ssl socket based handshake test without netty.

@normanmaurer
Copy link
Member

can you share the code ?

On 15 Dec 2015, at 12:28, ramahmoo notifications@github.com wrote:

No, just a simple java ssl socket based handshake test without netty.


Reply to this email directly or view it on GitHub #3900 (comment).

@ramahmoo
Copy link

Test Client based on socket

import java.io.BufferedReader;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.io.OutputStream;
import java.net.URL;
import java.security.KeyStore;
import java.security.KeyStoreException;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.security.UnrecoverableKeyException;

import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.HttpsURLConnection;
import javax.net.ssl.KeyManager;
import javax.net.ssl.KeyManagerFactory;
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLSession;
import javax.net.ssl.SSLSocketFactory;
import javax.net.ssl.TrustManager;
import javax.net.ssl.TrustManagerFactory;
import javax.net.ssl.X509ExtendedKeyManager;

import org.junit.Assert;
import org.junit.BeforeClass;
import org.junit.Test;

public class TestHttpsConnection {

    public static final String TEST_URL = "https://test-server:8443/test.html";
    public static final String TEST_REQUEST = "Test Request";
    public static KeyManager[] kms;
    public static TrustManager[] tms;


    @BeforeClass
    public static void init() throws Exception {

        // load client private key
        KeyStore clientKeys = KeyStore.getInstance("PKCS12");
        clientKeys
                .load(new FileInputStream(
                        "test-keystore.p12"),
                        "password".toCharArray());

        kms = getKeyManagers(clientKeys, "password".toCharArray(),
                "test-key-alias");

        // load server public key
        KeyStore serverPub = KeyStore.getInstance("JKS");
        serverPub
                .load(new FileInputStream(
                        "test-truststore.jks"),
                        "password".toCharArray());
        TrustManagerFactory trustManager = TrustManagerFactory
                .getInstance("SunX509");
        trustManager.init(serverPub);
        tms = trustManager.getTrustManagers();
    }

    private static KeyManager[] getKeyManagers(KeyStore keystore,
            char[] keyPassword, String keyAlias)
            throws NoSuchAlgorithmException, UnrecoverableKeyException,
            KeyStoreException {

        KeyManagerFactory keyManagerFactory;
        KeyManager[] keyManagers;
        KeyManager keyManager;

        keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory
                .getDefaultAlgorithm());
        keyManagerFactory.init(keystore, keyPassword);
        keyManagers = keyManagerFactory.getKeyManagers();

        if (keyAlias == null || "".equals(keyAlias))
            throw new KeyStoreException("There was no alias found!");

        if (keyManagers != null) {
            for (int i = 0; i < keyManagers.length; i++) {
                keyManager = keyManagers[i];
                if (keyManager instanceof X509ExtendedKeyManager) {
                    keyManagers[i] = new AliasSelectorKeyManager(
                            (X509ExtendedKeyManager) keyManager, keyAlias);
                }
            }
        }

        return keyManagers;
    }

    @Test
    public void testSSLClientWithSocket() {
        HttpsURLConnection httpsURLConnection = null;
        try {
            URL url = new URL(TEST_URL);
            SSLContext sc = getSSLContext();
            SSLSocketFactory socketFactory = sc
                    .getSocketFactory();

            HttpsURLConnection.setDefaultSSLSocketFactory(socketFactory);
            HttpsURLConnection
                    .setDefaultHostnameVerifier(new HostnameVerifier() {
                        @Override
                        public boolean verify(String arg0, SSLSession arg1) {
                            return true;
                        }
                    });
            httpsURLConnection = (HttpsURLConnection) url.openConnection();
            httpsURLConnection.setDoOutput(true);
            httpsURLConnection.setRequestMethod("POST");
            httpsURLConnection.setRequestProperty("Content-type",
                    "text/xml; charset=utf-8");
            httpsURLConnection.setRequestProperty("Connection", "close");
            OutputStream reqStream = httpsURLConnection.getOutputStream();
            reqStream.write(TEST_REQUEST.getBytes());

            StringBuffer sb = this.getContent(httpsURLConnection
                    .getInputStream());

            if (sb != null) {
                Assert.assertTrue(sb.length() > 0);
            } else
                Assert.fail();
        } catch (Exception e) {
            e.printStackTrace();
            Assert.fail();
        } finally {
            if (httpsURLConnection != null)
                httpsURLConnection.disconnect();
        }
    }

    private StringBuffer getContent(InputStream inputStream) {
        if (inputStream != null) {
            try {
                StringBuffer sb = new StringBuffer();
                String s;
                BufferedReader br = new BufferedReader(new InputStreamReader(
                        inputStream));

                while ((s = br.readLine()) != null) {
                    sb.append(s);
                }
                br.close();
                return sb;
            } catch (IOException e) {
                e.printStackTrace();
            }
        }

        return null;
    }

    private SSLContext getSSLContext() throws Exception {
        SSLContext sc = SSLContext.getInstance("TLSv1.2");
        sc.init(kms, tms, new SecureRandom());
        return sc;
    }
}

Alias Selector Key Manager

import java.net.Socket;
import java.security.Principal;
import java.security.PrivateKey;
import java.security.cert.X509Certificate;

import javax.net.ssl.SSLEngine;
import javax.net.ssl.X509ExtendedKeyManager;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

public class AliasSelectorKeyManager extends X509ExtendedKeyManager {

    private static Logger log = LogManager
            .getLogger(AliasSelectorKeyManager.class);

    private final X509ExtendedKeyManager sourceKeyManager;
    private final String expectedAlias;

    public AliasSelectorKeyManager(X509ExtendedKeyManager keyManager,
            String expectedAlias) {
        if (keyManager == null)
            throw new IllegalArgumentException("Source Keymanger can not be null!");
        this.sourceKeyManager = keyManager;
        this.expectedAlias = expectedAlias;
    }

    @Override
    public String chooseEngineServerAlias(String keyType, Principal[] issuers,
            SSLEngine engine) {
        log.entry(keyType, issuers, engine);
        return log.exit(expectedAlias == null ? this.sourceKeyManager
                .chooseEngineServerAlias(keyType, issuers, engine)
                : expectedAlias);
    }

    @Override
    public String chooseEngineClientAlias(String[] keyTypes,
            Principal[] issuers, SSLEngine engine) {
        log.entry(keyTypes, issuers, engine);
        return log.exit(expectedAlias == null ? this.sourceKeyManager
                .chooseEngineClientAlias(keyTypes, issuers, engine)
                : expectedAlias);
    }

    @Override
    public String chooseServerAlias(String keyType, Principal[] issuers,
            Socket socket) {
        log.entry(keyType, issuers, socket);
        return log.exit(expectedAlias == null ? this.sourceKeyManager
                .chooseServerAlias(keyType, issuers, socket) : expectedAlias);
    }

    @Override
    public String chooseClientAlias(final String[] keyTypes,
            Principal[] issuers, final Socket socket) {
        log.entry(keyTypes, issuers, socket);
        return log.exit(expectedAlias == null ? this.sourceKeyManager
                .chooseClientAlias(keyTypes, issuers, socket) : expectedAlias);
    }

    @Override
    public X509Certificate[] getCertificateChain(String alias) {
        log.entry(alias);
        return log.exit(sourceKeyManager.getCertificateChain(alias));
    }

    @Override
    public String[] getClientAliases(final String keyType,
            final Principal[] issuers) {
        log.entry();
        return log.exit(this.sourceKeyManager
                .getClientAliases(keyType, issuers));
    }

    @Override
    public PrivateKey getPrivateKey(String alias) {
        log.entry();
        return log.exit(sourceKeyManager.getPrivateKey(alias));
    }

    @Override
    public String[] getServerAliases(String keyType, Principal[] issuers) {
        log.entry();
        return log.exit(sourceKeyManager.getServerAliases(keyType,
                issuers));
    }

}

@normanmaurer
Copy link
Member

@ramahmoo thanks again for all your help. With your help I was able to reproduce it and fix it 👍 (at least it seems so). I want to do some more tests here but expect a PR today or tomorrow.

normanmaurer added a commit that referenced this issue Dec 16, 2015
Motivation:

We need to ensure we consume all pending data in the BIO on error to correctly send the close notify for the remote peer.

Modifications:

Correctly force the user to call wrap(...) if there is something left in the BIO.

Result:

close_notify is not lost.
@normanmaurer
Copy link
Member

@ramahmoo can you please verify if the fix works for you as well:
#4578

After I apply it I can see everything in wireshark :)

@ramahmoo
Copy link

@normanmaurer
Unfortunately, we can't take this fix from version 4.0.3x while there was a high CPU usage(maybe somewhere a Race condition) reported with OpenSSLEngine implementation from this version. We are still using 4.0.27-final. Is there any chance of porting the fix to 4.0.27 version?

@normanmaurer
Copy link
Member

@ramahmoo unfortunally no... we will not back port the fix as a lot of internal changed. Any chance to open an issue with the details of the race ? also are you sure it is related to the OpenSslEngine ?

@normanmaurer
Copy link
Member

@ramahmoo btw you could try to port the change by yourself but OpenSslEngine has other problems in 4.0.27.Final

@normanmaurer
Copy link
Member

@SutarSuhas maybe you could have a look as well.

@normanmaurer
Copy link
Member

Fixed via #4578

@normanmaurer
Copy link
Member

@ramahmoo if you find another issue in the latest version of OpenSslEngine please open another issue.

@ramahmoo
Copy link

ramahmoo commented Jan 7, 2016

@normanmaurer tested today with the revision where fixed. But could not see the alert(s) in wireshark. In handshake() NEED_WRAP is returned if something is pending in BIO. In next handshake() call the last stored exception is thrown if there is not more pending data. If i remove throwing of exception then i can see the alert in wireshark trace but in this the other side of the connection will abort first. Can you attach your wireshark trace, test case and any java log file?

@ramahmoo
Copy link

I found that Netty was not flushing the TLS alert data filled by last wrap() call if subsequent wrap() call throws an SSLException. In catch case of the following method of SSLHandler, i just added ctx.flush() and i can see the now TLS alert data in wireshark trace as expected. @normanmaurer any comment?

private void wrapNonAppData(ChannelHandlerContext ctx, boolean inUnwrap)
            throws SSLException {
        ByteBuf out = null;
        ByteBufAllocator alloc = ctx.alloc();
        try {
            for (;;) {
                if (out == null) {
                    out = allocateOutNetBuf(ctx, 0);
                }
                SSLEngineResult result = wrap(alloc, engine,
                        Unpooled.EMPTY_BUFFER, out);

                if (result.bytesProduced() > 0) {
                    ctx.write(out);
                    if (inUnwrap) {
                        needsFlush = true;
                    }
                    out = null;
                }

                switch (result.getHandshakeStatus()) {
                case FINISHED:
                    setHandshakeSuccess();
                    break;
                case NEED_TASK:
                    runDelegatedTasks();
                    break;
                case NEED_UNWRAP:
                    if (!inUnwrap) {
                        unwrapNonAppData(ctx);
                    }
                    break;
                case NEED_WRAP:
                    break;
                case NOT_HANDSHAKING:
                    setHandshakeSuccessIfStillHandshaking();
                    // Workaround for TLS False Start problem reported at:
                    // https://github.com/netty/netty/issues/1108#issuecomment-14266970
                    if (!inUnwrap) {
                        unwrapNonAppData(ctx);
                    }
                    break;
                default:
                    throw new IllegalStateException(
                            "Unknown handshake status: "
                                    + result.getHandshakeStatus());
                }

                if (result.bytesProduced() == 0) {
                    break;
                }

                // It should not consume empty buffers when it is not
                // handshaking
                // Fix for Android, where it was encrypting empty buffers even
                // when not handshaking
                if (result.bytesConsumed() == 0
                        && result.getHandshakeStatus() == HandshakeStatus.NOT_HANDSHAKING) {
                    break;
                }
            }
        } catch (SSLException e) {
            /** TLS ALERT FIX START */
            ctx.flush();
            /** TLS ALERT FIX END */
            setHandshakeFailure(ctx, e);
            throw e;
        } finally {
            if (out != null) {
                out.release();
            }
        }
    }

@normanmaurer
Copy link
Member

@ramahmoo ah makes sense! I think it always worked here as I called ctx.flush()in channelReadComplete(...). Could you do a quick PR for this after sign our ICLA (if you not done this already):

http://netty.io/s/icla

Also please ensure you respect our commit message template:
http://netty.io/wiki/writing-a-commit-message.html

@normanmaurer
Copy link
Member

@ramahmoo actually let me take a stab. There are a few other places which needs some attention.

normanmaurer added a commit that referenced this issue Jan 19, 2016
Motivation:

We need to ensure we flush out all pending data when an SslException accours so the remote peer receives all alerts.

Modifications:

Ensure we call ctx.flush() when needed.

Result:

Correctly receive alerts in all cases on the remote peer.
normanmaurer added a commit that referenced this issue Jan 20, 2016
Motivation:

We need to ensure we flush out all pending data when an SslException accours so the remote peer receives all alerts.

Modifications:

Ensure we call ctx.flush() when needed.

Result:

Correctly receive alerts in all cases on the remote peer.
normanmaurer added a commit that referenced this issue Jan 20, 2016
Motivation:

We need to ensure we flush out all pending data when an SslException accours so the remote peer receives all alerts.

Modifications:

Ensure we call ctx.flush() when needed.

Result:

Correctly receive alerts in all cases on the remote peer.
normanmaurer added a commit that referenced this issue Jan 20, 2016
Motivation:

We need to ensure we flush out all pending data when an SslException accours so the remote peer receives all alerts.

Modifications:

Ensure we call ctx.flush() when needed.

Result:

Correctly receive alerts in all cases on the remote peer.
pulllock pushed a commit to pulllock/netty that referenced this issue Oct 19, 2023
…ty#3900]

Motivation:

We need to ensure we flush out all pending data when an SslException accours so the remote peer receives all alerts.

Modifications:

Ensure we call ctx.flush() when needed.

Result:

Correctly receive alerts in all cases on the remote peer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants