Bug 55988

Created attachment 31198 [details]
Proof of concept patch

Here is initial patch to prove the concept. This patch will always try to set parameter useCipherSuitesOrder using reflection.

To test it:

(1) Install JDK 1.8.0 EA (must be B108+, tested with B121) [1]
(2) Install Java 7 JCE Unlimited Strength (it also works with JDK 1.8.0 EA) [2]
(3) Apply patch, build Tomcat
(4) Add JSSE Connector configuration to server.xml:

    <Connector port="443" 
               protocol="org.apache.coyote.http11.Http11Protocol"
               SSLEnabled="true"
               maxThreads="150" 
               scheme="https" 
               secure="true"
               clientAuth="false" 
               sslProtocol="TLS" 
               ciphers="TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384,
                        TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,
                        TLS_DHE_RSA_WITH_AES_256_CBC_SHA,
                        TLS_DHE_RSA_WITH_AES_128_CBC_SHA,
                        TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA,
                        TLS_RSA_WITH_AES_256_CBC_SHA256,
                        TLS_RSA_WITH_AES_256_CBC_SHA,
                        SSL_RSA_WITH_3DES_EDE_CBC_SHA" />

(5) Start Tomcat. Forward Secrecy is enabled (on all clients that support it)

-Ognjen

[1] https://jdk8.java.net/download.html
[2] http://www.oracle.com/technetwork/java/javase/downloads/jce-7-download-432124.html

Created attachment 31272 [details]
Patch to add useCipherSuitesOrder to BIO and NIO connectors

Fully functional patch. Here is an example how to use it for BIO (with OpenJDK 1.8.0 B108+ and JCE Unlimited Strength):

    <Connector port="443" 
               protocol="org.apache.coyote.http11.Http11Protocol"
               SSLEnabled="true"
               maxThreads="150" scheme="https" secure="true"
               clientAuth="false" sslProtocol="TLS" 
               useCipherSuitesOrder="true"
               ciphers="TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384,
                        TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,
                        TLS_DHE_RSA_WITH_AES_256_CBC_SHA,
                        TLS_DHE_RSA_WITH_AES_128_CBC_SHA,
                        SSL_RSA_WITH_3DES_EDE_CBC_SHA" />


To test NIO, just replace Http11Protocol with Http11NioProtocol.

---

You may test Forward Secrecy using 

  https://www.ssllabs.com/ssltest/

It should report "Forward Secrecy -- Yes (with most browsers)"

---

Note: If you try the same with JDK that does not support javax.net.SSLParameters.setUseCipherSuitesOrder(boolean) method, it will log:

WARNING [main] org.apache.tomcat.util.net.jsse.JSSESocketFactory.configureUseCipherSuitesOrder Method setUseCipherSuitesOrder is not supported by the SSL engine

APR connector has the "SSLHonorCipherOrder" attribute. Tomcat has a history of using different SSL-configuration attributes for APR versus other connector types. I would, however, prefer to make the configuration option more clear to the user like "useServerCipherSuitesOrder"... as it stands, the option doesn't really make it clear that the server is in control.

Any objections?

(In reply to Christopher Schultz from comment #3)
> I would, however, prefer to make the configuration option
> more clear to the user like "useServerCipherSuitesOrder"... as it stands,
> the option doesn't really make it clear that the server is in control.
> 
> Any objections?

No objections. Do I need to provide a new patch with the name you proposed?

Not necessary.

Just a gentle reminder. If you have some free time to review the patch, it would be great.

Ok, since you asked for it, here's a review:
- I don't think this feature justifies a big blob of ugly code, so this should wait for Java 8.
- Regardless of what APR may or may not do, it should be a boolean for the Java connectors, not a String.

(In reply to Remy Maucherat from comment #7)
> Ok, since you asked for it, here's a review:
> - I don't think this feature justifies a big blob of ugly code, so this
> should wait for Java 8.

I'm not sure I'd call this a big ball of ugly code. The only thing that makes it ugly is the fact that we can't have a compilation dependency on Java 8 -- which is where this feature is actually implemented.

> - Regardless of what APR may or may not do, it should be a boolean for the
> Java connectors, not a String.

While the class member is a String, it's treated as a boolean. Ognjen was simply following precedent set for example by "clientAuth".

Yes, the reflection code is ugly by design, it's not like it could look better (although maybe it could use introspection util ?), but the logging shouldn't be there (if some Java 8 features are to be used, there should be a static Java 8 flag somewhere that would avoid trying the reflection). About the "String" this is because the attributes are set using the introspection utils reflection, so that looks ok actually [= like the rest].

I still don't see why this should be added right now, the Java 8 support doesn't look good overall.

(In reply to Remy Maucherat from comment #9)
> I still don't see why this should be added right now, the Java 8 support
> doesn't look good overall.

Because:

a. is is a useful security feature, and as such it should be subject of consideration even if the small number of users may benefit from it, and

b. Oracle Java 1.8.0 is released a week ago, so its adoption is only going to increase, not shrink.

Ognjen, I have a couple of further comments about your proposed patch. I'm leaning towards adding this to Tomcat 8 but not back-porting unless there is significant demand.

1. Most of the 2 configureUseCipherSuitesOrder methods is the same. Consider re-factoring the bulk of that method into a superclass utility method and then extract the SSLParameters object from either SSLEngine or Socket in the subclasses.

2. Since this is a security-related configuration, consider failing totally when server-side ordering is requested but can't be enforced -- e.g. the reflection fails for any reason. You have it logging a warning but continuing which I think isn't appropriate in this case.

(In reply to Ognjen Blagojevic from comment #4)
> No objections. Do I need to provide a new patch with the name you proposed?

If you like my suggestions above, you could make all 3 changes at once and propose a new patch. That would be nice ;)

getting as many clients to choose a forward-secret cipher even if their makers didn't think of putting forward-secret ciphers highest priority is important in today's world of massive eaves-dropping.

Please implement this feature also for non-APR connectors A.S.A.P. - I think it is even worthwhile to backport to Tomcat 7!

Ognjen, if you are still willing to produce a patch, consider writing it against trunk, which will require Java 8 so won't need the reflection. If we decide to back-port to Tomcat 8, the reflection can be re-introduced.

Are you still able to update the patch?

(In reply to Ralf Hauser from comment #13)
> Please implement this feature also for non-APR connectors A.S.A.P. - I think
> it is even worthwhile to backport to Tomcat 7!

This enhancement request is specifically targeted towards the non-APR connectors. The APR connector already supports this capability via the SSLHonorCipherOrder setting.

Chris,

(In reply to Christopher Schultz from comment #14)
> Ognjen, if you are still willing to produce a patch, consider writing it
> against trunk, which will require Java 8 so won't need the reflection. If we
> decide to back-port to Tomcat 8, the reflection can be re-introduced.

Ok. I will attach patch for Tomcat 9. As you suggested:

1. Parameter name is useServerCipherSuitesOrder insted of useCipherSuitesOrder.
2. Code is deduplicated / moved to superclass.

To test it:

(1) Install JDK 1.8.0
(2) Install Java 8 JCE Unlimited Strength
(3) Apply patch, build Tomcat
(4) Add JSSE Connector configuration to server.xml:

    <Connector port="443" 
               protocol="org.apache.coyote.http11.Http11NioProtocol"
               SSLEnabled="true"
               maxThreads="150" 
               scheme="https" 
               secure="true"
               clientAuth="false" 
               sslProtocol="TLS" 
               useServerCipherSuitesOrder="true"
               ciphers="TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384,   
                        TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,      
                        TLS_DHE_RSA_WITH_AES_256_CBC_SHA,        
                        TLS_DHE_RSA_WITH_AES_128_CBC_SHA,        
                        SSL_RSA_WITH_3DES_EDE_CBC_SHA" 
 />

(5) Start Tomcat. Forward Secrecy is enabled (on all clients that support it)

To test with NIO2, just replace Http11NioProtocol with Http11Nio2Protocol.

-Ognjen

Created attachment 32407 [details]
Patch to add useServerCipherSuitesOrder to NIO and NIO2 connectors

Fixed in trunk in r1662614.

I'll start preparing a patch for Tomcat 8.

Fixed in Tomcat 8.0.x in r1662627.
Will be in Tomcat 8.0.21.

Support for BIO connector added in Tomcat 8.0.x in r1662632.

Fixed in Tomcat 7.0.x in r1662633.
Will be in Tomcat 7.0.60.

Refactoring in r1662994 broke the support for 7.0.x. It introduced several issues:

1. Inverted if condition in AbstractEndpoint.testServerCipherSuitesOrderSupport (fixed in r1669346).


2. Steps in AbstractEndpoint.configureUseServerCipherSuitesOrder:

  (a) SSLParameters sslParameters = engine.getSSLParameters();
  (b) sslParameters.setUseCipherSuitesOrder(boolean)
  (c) engine.setSSLParamters(sllParameters)

were refactored omitting step (c).


3. Steps in JSSESocketFactory.configureUseServerCipherSuitesOrder:

  (a) SSLParameters sslParameters = socket.getSSLParameters();
  (b) sslParameters.setUseCipherSuitesOrder(boolean)
  (c) socket.setSSLParamters(sllParameters)

were refactored omitting step (c).

I'm preparing the patch for issues 2. and 3.

-Ognjen

Created attachment 32611 [details]
Patch to add setters for SSLParameters

Thanks for the patch.
The fix will be available for 7.0.61 onwards.

7.0.61 works as expected. Thank you.

see also bug 53481 for SSLHonorCipherOrder (alias for the honorCipherOrder) as per http://tomcat.apache.org/tomcat-9.0-doc/config/http.html#SSL_Support_-_SSLHostConfig

somehow with the current debian stable (tomcat 8.0.14)

https://www.ssllabs.com/ssltest/analyze.html?d=www.privasphere.com&hideResults=on 

still claims "Cipher Suites (sorted by strength as the server has no preference..."