Skip to content

Commit

Permalink
OpenSslSession#initPeerCerts creates too long almost empty arrays.
Browse files Browse the repository at this point in the history 
Motivation:

#5945

Modifications:

Refactored initialization of arrays. Fixed arrays length

Result:

Cert arrays have proper length. Testing added
  • Loading branch information
@ichaki5748 @normanmaurer
ichaki5748 authored and normanmaurer committed Nov 3, 2016
1 parent 61d448e commit a7662db
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 41 deletions.
84 changes: 44 additions & 40 deletions handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1287,6 +1289,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;
Expand Down Expand Up @@ -1574,9 +1584,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;
Expand Down Expand Up @@ -1726,51 +1736,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]);
}
}

Expand Down Expand Up @@ -1838,7 +1842,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();
Expand All @@ -1856,7 +1860,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();
Expand Down
25 changes: 24 additions & 1 deletion handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down

0 comments on commit a7662db

Please sign in to comment.