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
Comments
@SutarSuhas code snippets would help. Also do you use the OpenSSL bindings or not ? |
@SutarSuhas ping |
@SutarSuhas any news here ? |
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
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. |
I will have a look once back from vacation |
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);
}
} |
If you can provide a fix as pr I'm more then happy to review
|
@wolftobias what version of netty this is ? |
This is 4.0.27-final but we have tested with 4.0.33-fonal and same behavior. |
@normanmaurer Any update on this issue? |
@ramahmoo I think I will open a PR today. |
@ramahmoo I was more interested to understand how you detected you not received the alert data. |
@ramahmoo thanks! |
@ramahmoo one last question... you are using OpenSslEngine on the server or client side when you see the issue ? |
As client, but i think is also as server. |
@ramahmoo when you said "using the JDK one" you are refer to use SslProvider.JDK and still netty right ? |
No, just a simple java ssl socket based handshake test without netty. |
can you share the code ?
|
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));
}
} |
@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. |
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 |
@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 ? |
@ramahmoo btw you could try to port the change by yourself but OpenSslEngine has other problems in 4.0.27.Final |
@SutarSuhas maybe you could have a look as well. |
Fixed via #4578 |
@ramahmoo if you find another issue in the latest version of |
@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? |
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();
}
}
} |
@ramahmoo ah makes sense! I think it always worked here as I called Also please ensure you respect our commit message template: |
@ramahmoo actually let me take a stab. There are a few other places which needs some attention. |
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.
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.
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.
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.
…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.
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.
The text was updated successfully, but these errors were encountered: