Repository: netty/netty
Issue: OpenSslSession#initPeerCerts creates too long almost empty arrays
Labels: defect

netty 4.1.6 

io.netty.handler.ssl.ReferenceCountedOpenSslEngine.OpenSslSession#initPeerCerts

```
final byte[] clientCert;
...
int len = clientCert.length + 1; // <------ BUG: raw certificate byte array size is used
peerCerts = new Certificate[len];
x509PeerCerts = new X509Certificate[len];

```

But should probably be

`int len = chain.length + 1; // <------ so we have first entry as client cert + chain.length`


Issue comments:
Maybe want to provide a PR with a fix?

> Am 26.10.2016 um 21:25 schrieb ichaki5748 notifications@github.com:
> 
> netty 4.1.6
> 
> io.netty.handler.ssl.ReferenceCountedOpenSslEngine.OpenSslSession#initPeerCerts
> 
> final byte[] clientCert;
> ...
> int len = clientCert.length + 1; // <------ BUG: raw certificate byte array size is used
> peerCerts = new Certificate[len];
> x509PeerCerts = new X509Certificate[len];
> 
> But should probably be
> 
> int len = chain.length + 1; // <------ so we have first entry as client cert + chain.length
> 
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub, or mute the thread.

Yes, sure, will try to do it this week 

Thx

> Am 26.10.2016 um 23:54 schrieb ichaki5748 notifications@github.com:
> 
> Yes, sure, will try to do it this week
> 
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or mute the thread.

Also `io.netty.handler.ssl.OpenSslJavaxX509Certificate.*`
could theoretically blow in concurrent usage because of lazy init in `io.netty.handler.ssl.OpenSslJavaxX509Certificate#unwrap` because 
`io.netty.handler.ssl.OpenSslJavaxX509Certificate#wrapped` is not `volatile` (certificate would not be fully initialized)? 

@iainmcgin please open separate PRs for the separate issues and ping me once you are ready. I will review then

You probably meant to reference @ichaki5748

@iainmcgin ups yes... sorry I need more ☕️ 

@normanmaurer Hi, please see https://github.com/netty/netty/pull/5967 https://github.com/netty/netty/pull/5966

Fixed by https://github.com/netty/netty/pull/5967


Pull request review comments:
@ichaki5748 can you make this method static

please make static

sure

![MINOR](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/severity-minor.png) Consider using varargs for methods or constructors which take an array the last parameter. [![rule](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/rule.png)](https://garage.netty.io/sonarqube/coding_rules#rule_key=pmd%3AUseVarargs)


Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
index 382f8606fc6..fce7410bb4d 100644
--- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
+++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
@@ -59,6 +59,8 @@
 import javax.security.cert.X509Certificate;
 
 import static io.netty.handler.ssl.OpenSsl.memoryAddress;
+import static io.netty.util.internal.EmptyArrays.EMPTY_CERTIFICATES;
+import static io.netty.util.internal.EmptyArrays.EMPTY_JAVAX_X509_CERTIFICATES;
 import static io.netty.util.internal.ObjectUtil.checkNotNull;
 import static javax.net.ssl.SSLEngineResult.HandshakeStatus.FINISHED;
 import static javax.net.ssl.SSLEngineResult.HandshakeStatus.NEED_UNWRAP;
@@ -1308,6 +1310,14 @@ private static SSLEngineResult.HandshakeStatus pendingStatus(int pendingStatus)
         return pendingStatus > 0 ? NEED_WRAP : NEED_UNWRAP;
     }
 
+    private static boolean isEmpty(Object[] arr) {
+        return arr == null || arr.length == 0;
+    }
+
+    private static boolean isEmpty(byte[] cert) {
+        return cert == null || cert.length == 0;
+    }
+
     private SSLEngineResult.HandshakeStatus handshake() throws SSLException {
         if (handshakeState == HandshakeState.FINISHED) {
             return FINISHED;
@@ -1595,9 +1605,9 @@ private final class OpenSslSession implements SSLSession, ApplicationProtocolAcc
         // These are guarded by synchronized(OpenSslEngine.this) as handshakeFinished() may be triggered by any
         // thread.
         private X509Certificate[] x509PeerCerts;
+        private Certificate[] peerCerts;
         private String protocol;
         private String applicationProtocol;
-        private Certificate[] peerCerts;
         private String cipher;
         private byte[] id;
         private long creationTime;
@@ -1747,51 +1757,45 @@ void handshakeFinished() throws SSLException {
         private void initPeerCerts() {
             // Return the full chain from the JNI layer.
             byte[][] chain = SSL.getPeerCertChain(ssl);
-            final byte[] clientCert;
-            if (!clientMode) {
+            if (clientMode) {
+                if (isEmpty(chain)) {
+                    peerCerts = EMPTY_CERTIFICATES;
+                    x509PeerCerts = EMPTY_JAVAX_X509_CERTIFICATES;
+                } else {
+                    peerCerts = new Certificate[chain.length];
+                    x509PeerCerts = new X509Certificate[chain.length];
+                    initCerts(chain, 0);
+                }
+            } else {
                 // if used on the server side SSL_get_peer_cert_chain(...) will not include the remote peer
                 // certificate. We use SSL_get_peer_certificate to get it in this case and add it to our
                 // array later.
                 //
                 // See https://www.openssl.org/docs/ssl/SSL_get_peer_cert_chain.html
-                clientCert = SSL.getPeerCertificate(ssl);
-            } else {
-                clientCert = null;
-            }
-
-            if (chain == null || chain.length == 0) {
-                if (clientCert == null || clientCert.length == 0) {
-                    peerCerts = EmptyArrays.EMPTY_CERTIFICATES;
-                    x509PeerCerts = EmptyArrays.EMPTY_JAVAX_X509_CERTIFICATES;
+                byte[] clientCert = SSL.getPeerCertificate(ssl);
+                if (isEmpty(clientCert)) {
+                    peerCerts = EMPTY_CERTIFICATES;
+                    x509PeerCerts = EMPTY_JAVAX_X509_CERTIFICATES;
                 } else {
-                    peerCerts = new Certificate[1];
-                    x509PeerCerts = new X509Certificate[1];
-
-                    peerCerts[0] = new OpenSslX509Certificate(clientCert);
-                    x509PeerCerts[0] = new OpenSslJavaxX509Certificate(clientCert);
-                }
-            } else if (clientCert == null || clientCert.length == 0) {
-                peerCerts = new Certificate[chain.length];
-                x509PeerCerts = new X509Certificate[chain.length];
-
-                for (int a = 0; a < chain.length; ++a) {
-                    byte[] bytes = chain[a];
-                    peerCerts[a] = new OpenSslX509Certificate(bytes);
-                    x509PeerCerts[a] = new OpenSslJavaxX509Certificate(bytes);
+                    if (isEmpty(chain)) {
+                        peerCerts = new Certificate[] {new OpenSslX509Certificate(clientCert)};
+                        x509PeerCerts = new X509Certificate[] {new OpenSslJavaxX509Certificate(clientCert)};
+                    } else {
+                        peerCerts = new Certificate[chain.length + 1];
+                        x509PeerCerts = new X509Certificate[chain.length + 1];
+                        peerCerts[0] = new OpenSslX509Certificate(clientCert);
+                        x509PeerCerts[0] = new OpenSslJavaxX509Certificate(clientCert);
+                        initCerts(chain, 1);
+                    }
                 }
-            } else {
-                int len = clientCert.length + 1;
-                peerCerts = new Certificate[len];
-                x509PeerCerts = new X509Certificate[len];
-
-                peerCerts[0] = new OpenSslX509Certificate(clientCert);
-                x509PeerCerts[0] = new OpenSslJavaxX509Certificate(clientCert);
+            }
+        }
 
-                for (int a = 0, i = 1; a < chain.length; ++a, ++i) {
-                    byte[] bytes = chain[a];
-                    peerCerts[i] = new OpenSslX509Certificate(bytes);
-                    x509PeerCerts[i] = new OpenSslJavaxX509Certificate(bytes);
-                }
+        private void initCerts(byte[][] chain, int startPos) {
+            for (int i = 0; i < chain.length; i++) {
+                int certPos = startPos + i;
+                peerCerts[certPos] = new OpenSslX509Certificate(chain[i]);
+                x509PeerCerts[certPos] = new OpenSslJavaxX509Certificate(chain[i]);
             }
         }
 
@@ -1859,7 +1863,7 @@ private String selectApplicationProtocol(List<String> protocols,
         @Override
         public Certificate[] getPeerCertificates() throws SSLPeerUnverifiedException {
             synchronized (ReferenceCountedOpenSslEngine.this) {
-                if (peerCerts == null || peerCerts.length == 0) {
+                if (isEmpty(peerCerts)) {
                     throw new SSLPeerUnverifiedException("peer not verified");
                 }
                 return peerCerts.clone();
@@ -1877,7 +1881,7 @@ public Certificate[] getLocalCertificates() {
         @Override
         public X509Certificate[] getPeerCertificateChain() throws SSLPeerUnverifiedException {
             synchronized (ReferenceCountedOpenSslEngine.this) {
-                if (x509PeerCerts == null || x509PeerCerts.length == 0) {
+                if (isEmpty(x509PeerCerts)) {
                     throw new SSLPeerUnverifiedException("peer not verified");
                 }
                 return x509PeerCerts.clone();


Test patch:
diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
index 32f9c9345ae..b565b3da09d 100644
--- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
+++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
@@ -53,6 +53,7 @@
 import java.io.InputStream;
 import java.net.InetSocketAddress;
 import java.nio.ByteBuffer;
+import java.security.cert.Certificate;
 import java.security.cert.CertificateException;
 import java.util.List;
 import java.util.concurrent.CountDownLatch;
@@ -64,6 +65,7 @@
 import javax.net.ssl.SSLException;
 import javax.net.ssl.SSLHandshakeException;
 import javax.net.ssl.SSLSession;
+import javax.security.cert.X509Certificate;
 
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
@@ -866,7 +868,28 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc
                         if (evt instanceof SslHandshakeCompletionEvent) {
                             Throwable cause = ((SslHandshakeCompletionEvent) evt).cause();
                             if (cause == null) {
-                                promise.setSuccess(null);
+                                SSLSession session = ((SslHandler) ctx.pipeline().first()).engine().getSession();
+                                X509Certificate[] peerCertificateChain = session.getPeerCertificateChain();
+                                Certificate[] peerCertificates = session.getPeerCertificates();
+                                if (peerCertificateChain == null) {
+                                    promise.setFailure(new NullPointerException("peerCertificateChain"));
+                                } else if (peerCertificates == null) {
+                                    promise.setFailure(new NullPointerException("peerCertificates"));
+                                } else if (peerCertificateChain.length + peerCertificates.length != 4) {
+                                    String excTxtFmt = "peerCertificateChain.length:%s, peerCertificates.length:%s";
+                                    promise.setFailure(new IllegalStateException(String.format(excTxtFmt,
+                                            peerCertificateChain.length,
+                                            peerCertificates.length)));
+                                } else {
+                                    for (int i = 0; i < peerCertificateChain.length; i++) {
+                                        if (peerCertificateChain[i] == null || peerCertificates[i] == null) {
+                                            promise.setFailure(
+                                                    new IllegalStateException("Certificate in chain is null"));
+                                            return;
+                                        }
+                                    }
+                                    promise.setSuccess(null);
+                                }
                             } else {
                                 promise.setFailure(cause);
                             }