svn commit: r1799216 - in /tomcat/trunk: java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java webapps/docs/changelog.xml

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

svn commit: r1799216 - in /tomcat/trunk: java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java webapps/docs/changelog.xml

Rémy Maucherat
Author: remm
Date: Mon Jun 19 14:40:46 2017
New Revision: 1799216

URL: http://svn.apache.org/viewvc?rev=1799216&view=rev
Log:
Derived from 60461, protect the SSL session provided by the OpenSSL engine from concurrent destruction.

Modified:
    tomcat/trunk/java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java
    tomcat/trunk/webapps/docs/changelog.xml

Modified: tomcat/trunk/java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java?rev=1799216&r1=1799215&r2=1799216&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java Mon Jun 19 14:40:46 2017
@@ -1041,8 +1041,13 @@ public final class OpenSSLEngine extends
 
         @Override
         public byte[] getId() {
-            // We don't cache that to keep memory usage to a minimum.
-            byte[] id = SSL.getSessionId(ssl);
+            byte[] id;
+            synchronized (OpenSSLEngine.this) {
+                if (destroyed) {
+                    throw new IllegalStateException(sm.getString("engine.noSession"));
+                }
+                id = SSL.getSessionId(ssl);
+            }
             if (id == null) {
                 // The id should never be null, if it was null then the SESSION itself was not valid.
                 throw new IllegalStateException(sm.getString("engine.noSession"));
@@ -1058,7 +1063,14 @@ public final class OpenSSLEngine extends
         @Override
         public long getCreationTime() {
             // We need to multiply by 1000 as OpenSSL uses seconds and we need milliseconds.
-            return SSL.getTime(ssl) * 1000L;
+            long creationTime = 0;
+            synchronized (OpenSSLEngine.this) {
+                if (destroyed) {
+                    throw new IllegalStateException(sm.getString("engine.noSession"));
+                }
+                creationTime = SSL.getTime(ssl);
+            }
+            return creationTime * 1000L;
         }
 
         @Override
@@ -1140,19 +1152,22 @@ public final class OpenSSLEngine extends
             // these are lazy created to reduce memory overhead
             Certificate[] c = peerCerts;
             if (c == null) {
-                if (SSL.isInInit(ssl) != 0) {
-                    throw new SSLPeerUnverifiedException(sm.getString("engine.unverifiedPeer"));
-                }
-                byte[][] chain = SSL.getPeerCertChain(ssl);
                 byte[] clientCert;
-                if (!clientMode) {
-                    // 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;
+                byte[][] chain;
+                synchronized (OpenSSLEngine.this) {
+                    if (destroyed || SSL.isInInit(ssl) != 0) {
+                        throw new SSLPeerUnverifiedException(sm.getString("engine.unverifiedPeer"));
+                    }
+                    chain = SSL.getPeerCertChain(ssl);
+                    if (!clientMode) {
+                        // 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 && clientCert == null) {
                     return null;
@@ -1193,10 +1208,13 @@ public final class OpenSSLEngine extends
             // these are lazy created to reduce memory overhead
             X509Certificate[] c = x509PeerCerts;
             if (c == null) {
-                if (SSL.isInInit(ssl) != 0) {
-                    throw new SSLPeerUnverifiedException(sm.getString("engine.unverifiedPeer"));
+                byte[][] chain;
+                synchronized (OpenSSLEngine.this) {
+                    if (destroyed || SSL.isInInit(ssl) != 0) {
+                        throw new SSLPeerUnverifiedException(sm.getString("engine.unverifiedPeer"));
+                    }
+                    chain = SSL.getPeerCertChain(ssl);
                 }
-                byte[][] chain = SSL.getPeerCertChain(ssl);
                 if (chain == null) {
                     throw new SSLPeerUnverifiedException(sm.getString("engine.unverifiedPeer"));
                 }
@@ -1241,7 +1259,14 @@ public final class OpenSSLEngine extends
                 return INVALID_CIPHER;
             }
             if (cipher == null) {
-                String c = OpenSSLCipherConfigurationParser.openSSLToJsse(SSL.getCipherForSSL(ssl));
+                String ciphers;
+                synchronized (OpenSSLEngine.this) {
+                    if (destroyed) {
+                        return INVALID_CIPHER;
+                    }
+                    ciphers = SSL.getCipherForSSL(ssl);
+                }
+                String c = OpenSSLCipherConfigurationParser.openSSLToJsse(ciphers);
                 if (c != null) {
                     cipher = c;
                 }
@@ -1253,7 +1278,12 @@ public final class OpenSSLEngine extends
         public String getProtocol() {
             String applicationProtocol = OpenSSLEngine.this.applicationProtocol;
             if (applicationProtocol == null) {
-                applicationProtocol = SSL.getNextProtoNegotiated(ssl);
+                synchronized (OpenSSLEngine.this) {
+                    if (destroyed) {
+                        throw new IllegalStateException(sm.getString("engine.noSession"));
+                    }
+                    applicationProtocol = SSL.getNextProtoNegotiated(ssl);
+                }
                 if (applicationProtocol == null) {
                     applicationProtocol = fallbackApplicationProtocol;
                 }
@@ -1263,7 +1293,13 @@ public final class OpenSSLEngine extends
                     OpenSSLEngine.this.applicationProtocol = applicationProtocol = "";
                 }
             }
-            String version = SSL.getVersion(ssl);
+            String version;
+            synchronized (OpenSSLEngine.this) {
+                if (destroyed) {
+                    throw new IllegalStateException(sm.getString("engine.noSession"));
+                }
+                version = SSL.getVersion(ssl);
+            }
             if (applicationProtocol.isEmpty()) {
                 return version;
             } else {

Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1799216&r1=1799215&r2=1799216&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Mon Jun 19 14:40:46 2017
@@ -148,6 +148,11 @@
         <code>AsyncListener</code>s after an I/O error on a non-container
         thread. (markt)
       </fix>
+      <fix>
+        Add additional syncs to the SSL session object provided by the OpenSSL
+        engine so that a concurrent destruction cannot cause a JVM crash.
+        (remm)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Jasper">



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]