Bug 55988 - Add parameter useCipherSuitesOrder to JSSE (BIO and NIO) connectors [PATCH]
Add parameter useCipherSuitesOrder to JSSE (BIO and NIO) connectors [PATCH]
Status: CLOSED FIXED
Product: Tomcat 9
Classification: Unclassified
Component: Connectors
unspecified
All All
: P2 enhancement with 27 votes (vote)
: -----
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2014-01-11 14:45 UTC by Ognjen Blagojevic
Modified: 2016-06-25 13:45 UTC (History)
3 users (show)



Attachments
Proof of concept patch (1.38 KB, patch)
2014-01-11 16:43 UTC, Ognjen Blagojevic
Details | Diff
Patch to add useCipherSuitesOrder to BIO and NIO connectors (9.47 KB, patch)
2014-01-30 12:20 UTC, Ognjen Blagojevic
Details | Diff
Patch to add useServerCipherSuitesOrder to NIO and NIO2 connectors (3.69 KB, patch)
2015-01-29 00:52 UTC, Ognjen Blagojevic
Details | Diff
Patch to add setters for SSLParameters (2.00 KB, patch)
2015-03-26 23:09 UTC, Ognjen Blagojevic
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ognjen Blagojevic 2014-01-11 14:45:34 UTC
Starting with Oracle Java 1.8.0 B108, JSSE supports server-side cipher ordering [1]. Server-side cipher ordering is useful for enabling Forward Secrecy and for preventing certain attacks. Appropriate JSSE parameter is called useCipherSuitesOrder [2].

Is it possible to add that same attribute to Tomcat JSSE connectors?

The problem is that parameter useCipherSuitesOrder is only available starting with Java 1.8 B108, while Tomcat 8 requires only Java 1.7. Therefore the proposal is, if Tomcat 8:

(a) runs using Java 1.7 / 1.8 pre-B108, parameter useCipherSuitesOrder would be ignored, and if 
(b) runs using Java 1.8 B108+, JSSE parameter useCipherSuitesOrder would be appropriately set.

It might be a precedence to support parameter from non-required version of Java, albeit -- due to the usefulness of such configuration option -- it might be worthwhile considering.

Note that similar attribute already exists for APR connector -- SSLHonorCipherOrder.

-Ognjen

[1] https://bugs.openjdk.java.net/browse/JDK-7188657
[2] http://download.java.net/jdk8/docs/api/javax/net/ssl/SSLParameters.html#setUseCipherSuitesOrder-boolean-
Comment 1 Ognjen Blagojevic 2014-01-11 16:43:11 UTC
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
Comment 2 Ognjen Blagojevic 2014-01-30 12:20:23 UTC
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
Comment 3 Christopher Schultz 2014-01-30 16:42:47 UTC
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?
Comment 4 Ognjen Blagojevic 2014-01-30 22:15:40 UTC
(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?
Comment 5 Christopher Schultz 2014-01-31 01:27:20 UTC
Not necessary.
Comment 6 Ognjen Blagojevic 2014-03-25 13:17:48 UTC
Just a gentle reminder. If you have some free time to review the patch, it would be great.
Comment 7 Remy Maucherat 2014-03-25 13:45:07 UTC
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.
Comment 8 Christopher Schultz 2014-03-25 15:05:57 UTC
(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".
Comment 9 Remy Maucherat 2014-03-25 15:41:40 UTC
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.
Comment 10 Ognjen Blagojevic 2014-03-26 00:36:17 UTC
(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.
Comment 11 Christopher Schultz 2014-05-26 18:24:20 UTC
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.
Comment 12 Christopher Schultz 2014-05-26 18:26:22 UTC
(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 ;)
Comment 13 Ralf Hauser 2015-01-28 07:18:19 UTC
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!
Comment 14 Christopher Schultz 2015-01-28 14:49:16 UTC
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.
Comment 15 Ognjen Blagojevic 2015-01-29 00:50:14 UTC
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
Comment 16 Ognjen Blagojevic 2015-01-29 00:52:05 UTC
Created attachment 32407 [details]
Patch to add useServerCipherSuitesOrder to NIO and NIO2 connectors
Comment 17 Christopher Schultz 2015-02-27 01:38:47 UTC
Fixed in trunk in r1662614.

I'll start preparing a patch for Tomcat 8.
Comment 18 Christopher Schultz 2015-02-27 02:50:01 UTC
Fixed in Tomcat 8.0.x in r1662627.
Will be in Tomcat 8.0.21.
Comment 19 Christopher Schultz 2015-02-27 04:06:13 UTC
Support for BIO connector added in Tomcat 8.0.x in r1662632.
Comment 20 Christopher Schultz 2015-02-27 04:14:56 UTC
Fixed in Tomcat 7.0.x in r1662633.
Will be in Tomcat 7.0.60.
Comment 21 Ognjen Blagojevic 2015-03-26 23:01:09 UTC
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
Comment 22 Ognjen Blagojevic 2015-03-26 23:09:19 UTC
Created attachment 32611 [details]
Patch to add setters for SSLParameters
Comment 23 Violeta Georgieva 2015-03-27 07:50:10 UTC
Thanks for the patch.
The fix will be available for 7.0.61 onwards.
Comment 24 Ognjen Blagojevic 2015-03-27 20:12:49 UTC
7.0.61 works as expected. Thank you.
Comment 25 Ralf Hauser 2016-06-25 13:45:07 UTC
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..."