From 1fef3872fbe12d222c356d6975a4f488017a1168 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Sat, 27 Aug 2016 20:49:16 +0200 Subject: [PATCH] Update CertificateRequestCallback usage to match new tcnative Motiviation: Previously the way how CertificateRequestCallback was working had some issues which could cause memory leaks and segfaults. Due of this tcnative code was updated to change the signature of the method provided by the interface. Modifications: Update CertificateRequestCallback implementations to match new interface signature. Result: No more segfaults / memory leaks when using boringssl or openssl >= 1.1.0 --- .../ssl/OpenSslKeyMaterialManager.java | 76 ++++++++++++++----- .../ReferenceCountedOpenSslClientContext.java | 5 +- pom.xml | 2 +- 3 files changed, 62 insertions(+), 21 deletions(-) diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java index ef6e1cef95..f699e686f7 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java @@ -16,6 +16,7 @@ package io.netty.handler.ssl; import io.netty.buffer.ByteBufAllocator; +import org.apache.tomcat.jni.CertificateRequestedCallback; import org.apache.tomcat.jni.SSL; import javax.net.ssl.SSLException; @@ -84,9 +85,46 @@ class OpenSslKeyMaterialManager { } } - void setKeyMaterial(ReferenceCountedOpenSslEngine engine, String[] keyTypes, - X500Principal[] issuer) throws SSLException { - setKeyMaterial(engine.sslPointer(), chooseClientAlias(engine, keyTypes, issuer)); + CertificateRequestedCallback.KeyMaterial keyMaterial(ReferenceCountedOpenSslEngine engine, String[] keyTypes, + X500Principal[] issuer) throws SSLException { + String alias = chooseClientAlias(engine, keyTypes, issuer); + long keyBio = 0; + long keyCertChainBio = 0; + long pkey = 0; + long certChain = 0; + + try { + // TODO: Should we cache these and so not need to do a memory copy all the time ? + X509Certificate[] certificates = keyManager.getCertificateChain(alias); + if (certificates == null || certificates.length == 0) { + return null; + } + + PrivateKey key = keyManager.getPrivateKey(alias); + keyCertChainBio = toBIO(certificates); + certChain = SSL.parseX509Chain(keyCertChainBio); + if (key != null) { + keyBio = toBIO(key); + pkey = SSL.parsePrivateKey(keyBio, password); + } + CertificateRequestedCallback.KeyMaterial material = new CertificateRequestedCallback.KeyMaterial( + certChain, pkey); + + // Reset to 0 so we do not free these. This is needed as the client certificate callback takes ownership + // of both the key and the certificate if they are returned from this method, and thus must not + // be freed here. + certChain = pkey = 0; + return material; + } catch (SSLException e) { + throw e; + } catch (Exception e) { + throw new SSLException(e); + } finally { + freeBio(keyBio); + freeBio(keyCertChainBio); + SSL.freePrivateKey(pkey); + SSL.freeX509Chain(certChain); + } } private void setKeyMaterial(long ssl, String alias) throws SSLException { @@ -96,26 +134,28 @@ class OpenSslKeyMaterialManager { try { // TODO: Should we cache these and so not need to do a memory copy all the time ? - PrivateKey key = keyManager.getPrivateKey(alias); X509Certificate[] certificates = keyManager.getCertificateChain(alias); + if (certificates == null || certificates.length == 0) { + return; + } - if (certificates != null && certificates.length != 0) { - // Only encode one time - PemEncoded encoded = PemX509Certificate.toPEM(ByteBufAllocator.DEFAULT, true, certificates); - try { - keyCertChainBio = toBIO(ByteBufAllocator.DEFAULT, encoded.retain()); - keyCertChainBio2 = toBIO(ByteBufAllocator.DEFAULT, encoded.retain()); + PrivateKey key = keyManager.getPrivateKey(alias); - if (key != null) { - keyBio = toBIO(key); - } - SSL.setCertificateBio(ssl, keyCertChainBio, keyBio, password); + // Only encode one time + PemEncoded encoded = PemX509Certificate.toPEM(ByteBufAllocator.DEFAULT, true, certificates); + try { + keyCertChainBio = toBIO(ByteBufAllocator.DEFAULT, encoded.retain()); + keyCertChainBio2 = toBIO(ByteBufAllocator.DEFAULT, encoded.retain()); - // We may have more then one cert in the chain so add all of them now. - SSL.setCertificateChainBio(ssl, keyCertChainBio2, false); - } finally { - encoded.release(); + if (key != null) { + keyBio = toBIO(key); } + SSL.setCertificateBio(ssl, keyCertChainBio, keyBio, password); + + // We may have more then one cert in the chain so add all of them now. + SSL.setCertificateChainBio(ssl, keyCertChainBio2, false); + } finally { + encoded.release(); } } catch (SSLException e) { throw e; diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java index e7ae41c490..7ea0ded1ba 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java @@ -234,7 +234,7 @@ public final class ReferenceCountedOpenSslClientContext extends ReferenceCounted } @Override - public void requested(long ssl, byte[] keyTypeBytes, byte[][] asn1DerEncodedPrincipals) { + public KeyMaterial requested(long ssl, byte[] keyTypeBytes, byte[][] asn1DerEncodedPrincipals) { final ReferenceCountedOpenSslEngine engine = engineMap.get(ssl); try { final Set keyTypesSet = supportedClientKeyTypes(keyTypeBytes); @@ -248,12 +248,13 @@ public final class ReferenceCountedOpenSslClientContext extends ReferenceCounted issuers[i] = new X500Principal(asn1DerEncodedPrincipals[i]); } } - keyManagerHolder.setKeyMaterial(engine, keyTypes, issuers); + return keyManagerHolder.keyMaterial(engine, keyTypes, issuers); } catch (Throwable cause) { logger.debug("request of key failed", cause); SSLHandshakeException e = new SSLHandshakeException("General OpenSslEngine problem"); e.initCause(cause); engine.handshakeException = e; + return null; } } diff --git a/pom.xml b/pom.xml index df556cd38f..0ffae8f89e 100644 --- a/pom.xml +++ b/pom.xml @@ -224,7 +224,7 @@ fedora netty-tcnative - 1.1.33.Fork20 + 1.1.33.Fork21 ${os.detected.classifier} ${os.detected.name}-${os.detected.arch} ${project.basedir}/../common/src/test/resources/logback-test.xml