Author: markt
Date: Thu Nov  6 11:38:59 2014
New Revision: 1637083

URL: http://svn.apache.org/r1637083
Log:
Mitigate POODLE by disabling SSLv3 by default for JSSE
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=57116
Do not fallback to default protocol list for HTTPS BIO connector if 
sslEnabledProtocols has no matches.
I'll look at a separate patch for BZ 56780

Modified:
    tomcat/tc6.0.x/trunk/STATUS.txt
    tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java
    
tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java
    
tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties
    tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml

Modified: tomcat/tc6.0.x/trunk/STATUS.txt
URL: 
http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?rev=1637083&r1=1637082&r2=1637083&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/STATUS.txt (original)
+++ tomcat/tc6.0.x/trunk/STATUS.txt Thu Nov  6 11:38:59 2014
@@ -48,34 +48,6 @@ PATCHES PROPOSED TO BACKPORT:
   +1: kkolinko, markt, remm
   -1:
 
-* Mitigate POODLE by disabling SSLv3 by default for JSSE
-  http://people.apache.org/~markt/patches/2014-10-21-poodle-tc6-v2.patch
-  +1: markt, schultz
-  +1: kkolinko (several comments below)
-  -1:
-   kkolinko:
-     Good.
-     I think this makes BZ 57116 fixed as well.
-     Several notes:
-      1) From BZ 56780 the static{} block in JSSESocketFactory
-       needs try/catch(IllegalArgumentException),
-       like it is already done in Tomcat 7 in r1615951
-
-      2) In getEnabledProtocols() the
-        "if (requestedProtocols == null) { return DEFAULT_SERVER_PROTOCOLS; }"
-       block can be moved several lines earlier.
-
-      3) From BZ 56780 the DEFAULT_SERVER_PROTOCOLS value might result as
-      null. I am afraid that passing that null to Java APIs will result in
-      some cryptic messages. This question may be addressed later via BZ 56780.
-        https://issues.apache.org/bugzilla/show_bug.cgi?id=56780#c9
-
-      schultz: it's not clear from the code what will happen if
-               DEFAULT_SERVER_PROTOCOLS remains null. Would it be more clear
-               to use an empty string array instead of null? I seem to recall
-               slightly different "null" behavior in Oracle/OpenJDK versus
-               IBM JVMs.
-
 
 PATCHES/ISSUES THAT ARE STALLED:
 

Modified: tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java
URL: 
http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java?rev=1637083&r1=1637082&r2=1637083&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java 
(original)
+++ tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java Thu 
Nov  6 11:38:59 2014
@@ -59,6 +59,7 @@ import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
 import org.apache.tomcat.util.IntrospectionUtils;
 import org.apache.tomcat.util.net.SecureNioChannel.ApplicationBufferHandler;
+import org.apache.tomcat.util.net.jsse.JSSESocketFactory;
 import org.apache.tomcat.util.net.jsse.NioX509KeyManager;
 import org.apache.tomcat.util.res.StringManager;
 
@@ -1139,7 +1140,13 @@ public class NioEndpoint extends Abstrac
         }
         engine.setUseClientMode(false);
         if ( ciphersarr.length > 0 ) engine.setEnabledCipherSuites(ciphersarr);
-        if ( sslEnabledProtocolsarr.length > 0 ) 
engine.setEnabledProtocols(sslEnabledProtocolsarr);
+        if (sslEnabledProtocolsarr.length > 0) {
+            engine.setEnabledProtocols(sslEnabledProtocolsarr);
+        } else {
+            // Filter out the insecure protocols from the defaults
+            
engine.setEnabledProtocols(JSSESocketFactory.filterInsecureProcotols(
+                    engine.getEnabledProtocols()));
+        }
 
         return engine;
     }

Modified: 
tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java
URL: 
http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java?rev=1637083&r1=1637082&r2=1637083&view=diff
==============================================================================
--- 
tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java
 (original)
+++ 
tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java
 Thu Nov  6 11:38:59 2014
@@ -40,7 +40,11 @@ import java.security.cert.CertificateFac
 import java.security.cert.CollectionCertStoreParameters;
 import java.security.cert.PKIXBuilderParameters;
 import java.security.cert.X509CertSelector;
+import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
 import java.util.Vector;
 
 import javax.net.ssl.CertPathTrustManagerParameters;
@@ -86,6 +90,8 @@ public class JSSESocketFactory
 
     private static final boolean RFC_5746_SUPPORTED;
 
+    public static final String[] DEFAULT_SERVER_PROTOCOLS;
+
     // defaults
     static String defaultProtocol = "TLS";
     static boolean defaultClientAuth = false;
@@ -102,6 +108,7 @@ public class JSSESocketFactory
     static {
         boolean result = false;
         SSLContext context;
+        String[] protocols = null;
         try {
             context = SSLContext.getInstance("TLS");
             context.init(null, null, new SecureRandom());
@@ -113,12 +120,26 @@ public class JSSESocketFactory
                     break;
                 }
             }
+
+            // There is no API to obtain the default server protocols and 
cipher
+            // suites. Having inspected the OpenJDK code there the same results
+            // can be achieved via the standard API but there is no guarantee
+            // that every JVM implementation determines the defaults the same
+            // way. Therefore the defaults are determined by creating a server
+            // socket and requested the configured values.
+
+            SSLServerSocket socket = (SSLServerSocket) 
ssf.createServerSocket();
+            // Filter out all the insecure protocols
+            protocols = filterInsecureProcotols(socket.getEnabledProtocols());
         } catch (NoSuchAlgorithmException e) {
             // Assume no RFC 5746 support
         } catch (KeyManagementException e) {
             // Assume no RFC 5746 support
+        } catch (IOException e) {
+            // Unable to determine default ciphers/protocols so use none
         }
         RFC_5746_SUPPORTED = result;
+        DEFAULT_SERVER_PROTOCOLS = protocols;
     }
 
     protected boolean initialized;
@@ -698,7 +719,9 @@ public class JSSESocketFactory
      * @param protocols the protocols to use.
      */
     protected void setEnabledProtocols(SSLServerSocket socket, String 
[]protocols){
-        if (protocols != null) {
+        if (protocols == null) {
+            socket.setEnabledProtocols(DEFAULT_SERVER_PROTOCOLS);
+        } else {
             socket.setEnabledProtocols(protocols);
         }
     }
@@ -715,67 +738,28 @@ public class JSSESocketFactory
      */
     protected String[] getEnabledProtocols(SSLServerSocket socket,
                                            String requestedProtocols){
-        String[] supportedProtocols = socket.getSupportedProtocols();
-
-        String[] enabledProtocols = null;
+        Set<String> supportedProtocols = new HashSet<String>();
+        for (String supportedProtocol : socket.getSupportedProtocols()) {
+            supportedProtocols.add(supportedProtocol);
+        }
 
-        if (requestedProtocols != null) {
-            Vector vec = null;
-            String protocol = requestedProtocols;
-            int index = requestedProtocols.indexOf(',');
-            if (index != -1) {
-                int fromIndex = 0;
-                while (index != -1) {
-                    protocol = requestedProtocols.substring(fromIndex, 
index).trim();
-                    if (protocol.length() > 0) {
-                        /*
-                         * Check to see if the requested protocol is among the
-                         * supported protocols, i.e., may be enabled
-                         */
-                        for (int i=0; supportedProtocols != null
-                                     && i<supportedProtocols.length; i++) {
-                            if (supportedProtocols[i].equals(protocol)) {
-                                if (vec == null) {
-                                    vec = new Vector();
-                                }
-                                vec.addElement(protocol);
-                                break;
-                            }
-                        }
-                    }
-                    fromIndex = index+1;
-                    index = requestedProtocols.indexOf(',', fromIndex);
-                } // while
-                protocol = requestedProtocols.substring(fromIndex);
-            }
+        if (requestedProtocols == null) {
+            return DEFAULT_SERVER_PROTOCOLS;
+        }
 
-            if (protocol != null) {
-                protocol = protocol.trim();
-                if (protocol.length() > 0) {
-                    /*
-                     * Check to see if the requested protocol is among the
-                     * supported protocols, i.e., may be enabled
-                     */
-                    for (int i=0; supportedProtocols != null
-                                 && i<supportedProtocols.length; i++) {
-                        if (supportedProtocols[i].equals(protocol)) {
-                            if (vec == null) {
-                                vec = new Vector();
-                            }
-                            vec.addElement(protocol);
-                            break;
-                        }
-                    }
-                }
-            }
+        String[] requestedProtocolsArr = requestedProtocols.split(",");
+        List<String> enabledProtocols = new 
ArrayList<String>(requestedProtocolsArr.length);
 
-            if (vec != null) {
-                enabledProtocols = new String[vec.size()];
-                vec.copyInto(enabledProtocols);
+        for (String requestedProtocol : requestedProtocolsArr) {
+            String requestedProtocolTrim = requestedProtocol.trim();
+            if (supportedProtocols.contains(requestedProtocolTrim)) {
+                enabledProtocols.add(requestedProtocolTrim);
+            } else {
+                log.warn(sm.getString("jsse.unsupportedProtocol", 
requestedProtocolTrim));
             }
         }
 
-        return enabledProtocols;
+        return enabledProtocols.toArray(new String[enabledProtocols.size()]);
     }
 
     /**
@@ -816,8 +800,7 @@ public class JSSESocketFactory
         }
 
         String requestedProtocols = (String) attributes.get("protocols");
-        setEnabledProtocols(socket, getEnabledProtocols(socket,
-                                                         requestedProtocols));
+        socket.setEnabledProtocols(getEnabledProtocols(socket, 
requestedProtocols));
 
         // we don't know if client auth is needed -
         // after parsing the request we may re-handshake
@@ -864,6 +847,22 @@ public class JSSESocketFactory
                 socket.close();
             }
         }
+    }
+
+
+    public static String[] filterInsecureProcotols(String[] protocols) {
+        if (protocols == null) {
+            return null;
+        }
 
+        List<String> result = new ArrayList<String>(protocols.length);
+        for (String protocol : protocols) {
+            if (protocol == null || protocol.contains("SSL")) {
+                log.debug(sm.getString("jsse.excludeDefaultProtocol", 
protocol));
+            } else {
+                result.add(protocol);
+            }
+        }
+        return result.toArray(new String[result.size()]);
     }
 }

Modified: 
tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties
URL: 
http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties?rev=1637083&r1=1637082&r2=1637083&view=diff
==============================================================================
--- 
tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties
 (original)
+++ 
tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties
 Thu Nov  6 11:38:59 2014
@@ -17,6 +17,8 @@ jsse.alias_no_key_entry=Alias name {0} d
 jsse.keystore_load_failed=Failed to load keystore type {0} with path {1} due 
to {2}
 jsse.invalid_truststore_password=The provided trust store password could not 
be used to unlock and/or validate the trust store. Retrying to access the trust 
store with a null password which will skip validation.
 jsse.invalidTrustManagerClassName=The trustManagerClassName provided [{0}] 
does not implement javax.net.ssl.TrustManager
+jsse.excludeDefaultProtocol=The SSL protocol [{0}] which is enabled by default 
in this JRE was excluded from the defaults used by Tomcat
+jsse.unsupportedProtocol=The specified SSL protocol [{0}] is not supported
 jsseSupport.clientCertError=Error trying to obtain a certificate from the 
client
 jseeSupport.certTranslationError=Error translating certificate [{0}]
 jsseSupport.noCertWant=No client certificate sent for want

Modified: tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml
URL: 
http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml?rev=1637083&r1=1637082&r2=1637083&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml Thu Nov  6 11:38:59 2014
@@ -62,6 +62,15 @@
         Do not increase remaining counter at end of stream in
         IdentityInputFilter. (kkolinko)
       </fix>
+      <fix>
+        Disable SSLv3 by default (along with SSLv2 which was already disabled
+        by default) in light of the recently announced POODLE vulnerability
+        (CVE-2014-3566). (markt)
+      </fix>
+      <fix>
+        <bug>57116</bug>: Do not fallback to default protocol list for HTTPS 
BIO
+        connector if <code>sslEnabledProtocols</code> has no matches. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Web applications">



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Reply via email to