Created attachment 29433[details]
patch for tomcat trunk that adds support for newer TLS versions
It would be nice to have support for newer versions of TLS protocol. Due to BEAST attack, the only usable ciphersuites supported by TLS version 1.0 are those based on RC4.
I'll attach compile-tested patches for both tcnative and tomcat.

This is not the case, because the parts of code which depend on the newer library version are #ifdef'ed. Actually, the patches improve compatibility with newer openssl versions, as the library may be compiled without SSL2 support (for example, current Debian testing contains such a version).

(In reply to comment #3)
> This introduces a compile-time dependency on OpenSSL 1.0.1+.
Retracted: I have successfully built (but not tested) this patch against tcnative 1.1.x with both OpenSSL 0.9.8o and OpenSSL 1.0.0j.

I like this patch, but since security is involved, I think I'd like to see a check in the Java code against the (likely) tcnative version that can support TLSv1.1 and TLSv1.2. We don't want people using "TLSv1+TLSv1.1+TLSv1.2" as their protocol string and thinking that they can get access to TLSv1.2 if tcnative isn't up to the task.
Similarly, there should probably be a check at the JNI level to check to see that the underlying OpenSSL supports TLSv1.1 or TLSv1.2 when attempting to use them. The existing patch will allow a user to request "TLSv1+TLSv1.1+TLSv1.2" and silently implement only TLSv1.
Java code can check org.apache.tomcat.jni.Library.TCN_MAJOR_VERSION, etc. and the C code can use #ifdef checks.

Created attachment 29459[details]
patch for tomcat trunk that adds support for newer TLS versions
Ok, I agree with your comments, and I've reworked the patches.
In the tcnative part, it should be sufficient to move ifdefs inside the if blocks. When newer TLS versions are not available, the variable ctx remains null, and an error is emitted.
In the tomcat part, I rely on the SSL.hasOp functionality to check whether the tcnative library supports newer protocols. I needed to change both AprEndpoint and AprSocketContext, which resulted in some code duplication. I think the ssl protocol parsing should be implemented in one place only. Now AprSocketContext doesn't support more protocols (via +), and it produces no error when the string is invalid.

Now when there is a known practical attack against RC4 in SSL, we have no secure ciphersuite in TLS 1.0, and this issue has probably higher priority than before. What is the reason for not applying this patch for half a year? Is there anything I can do to have this patch merged?

Created attachment 30111[details]
Patch for tomcat native adding support for newer TLS versions
Ok, I've tested the patches and found an error in tcnative part. Here is a fixed patch. The problem was that OpenSSL API is quite counter-intuitive. If one wants more than one protocol to be supported, SSLv23_server_method() should be called and unwanted protocols should then be disabled by SSL_OP_NO_*. Other *_server_methods() always make available just one specific version of SSL/TLS.
To be precise, I've tested tcnative not with Tomcat, but with JBoss and analogical patch for jboss-web. The reason is that I'm primarily interested in JBoss and I don't know how to configure Tomcat.

Oops, there seems to be a problem with OpenSSL 0.9.8. Previously, I've tested 1.0.1e and that worked, but the older version seems to have problems with default protocol set. I currently have no time to find the precise culprit, so in a week or so, I will attach fixed patches that works with all OpenSSL versions.

The problem is following. OpenSSL 0.9.8y defines SSL_OP_PKCS1_CHECK_{1,2} as 0x08000000L and 0x10000000L while OpenSSL 1.0.1e uses the same values for SSL_OP_NO_TLSv1_{1,2}, and defines SSL_OP_PKCS1_CHECK_{1,2} as zero. Therefore, java code calling hasOp with SSL_OP_NO_TLSv1_{1,2} succeeds against both 0.9.8 and 1.0.1. I see no perfect solution, but quite a good way to overcome this issue is to drop SSL_OP_PKCS1_CHECK_* from supported_ssl_opts. Then, these OP's cannot be tested via hasOp, but the flags seem to be unused anyway, according to the comment in 1.0.1e:
/* These next two were never actually used for anything since SSLeay
* zap so we have some more flags.
*/
I'll send fixed patches in a moment. They have been tested (with JBoss, as before) against both 0.9.8y and 1.0.1e. I've also tested newer java against old tcnative, and it works correctly (enabling one of the newer protocols causes a failure).

Given the comment in OpenSSL that SSL_OP_PKCS1_CHECK_{1,2} were never used, I think it's reasonable to use the new symbolic names and remove the old ones. Note that it will also require a patch to Tomcat trunk as well.
Interestingly, there is this comment in o.a.t.jni.SSL:
/* The next flag deliberately changes the ciphertest, this is a check
* for the PKCS#1 attack */
public static final int SSL_OP_PKCS1_CHECK_1 = 0x08000000;
public static final int SSL_OP_PKCS1_CHECK_2 = 0x10000000;
Neither of these constants are used anywhere in Tomcat trunk, so I'm not sure a) what that comment means and b) whether there is anything to be concerned about.
That comment is attributed to mturk, but so is nearly the entire file, so I suspect that his commit r423920 just ended up touching every line in the file or something.
tcnative's code has the same comment in the same place (SSL.java) attributed to mturk in r300716, where it seems those constants were actually added. That was way back in 2005. I wonder if Mladen remembers whether that comment is relevant anymore.

Actually, the comment came from OpenSSL. Here is part of 1.0.1e ssl.h:
/* These next two were never actually used for anything since SSLeay
* zap so we have some more flags.
*/
/* The next flag deliberately changes the ciphertest, this is a check
* for the PKCS#1 attack */
#define SSL_OP_PKCS1_CHECK_1 0x0
#define SSL_OP_PKCS1_CHECK_2 0x0
OpenSSL 0.9.8y contains the same comment, but different values.

Created attachment 30166[details]
patch for tomcat trunk that adds support for newer TLS versions
I've updated the patch for tomcat that can be applied to current trunk. It also drops PKCS* constants from SSL.java.

I've taken another look at the (updated) patches. I'm confused by the changes to sslcontext.c. It looks like there is no provision for combinations of SSL/TLS protocols.
For instance, if I request (TLSv1_1 | TLSv1_2) then I don't get a configured SSL engine because of this:
+#ifndef SSL_OP_NO_TLSv1_2
+ } else if (protocol & SSL_PROTOCOL_TLSV1_2) {
+ /* requested but not supported */
+#endif
Or is this because (TLSv1_1 | TLSv1_2) is not a supported protocol definition? I could only find these TLS-related server-method functions in the OpenSSL API:
const SSL_METHOD *TLSv1_server_method(void); /* TLSv1.0 */
const SSL_METHOD *TLSv1_1_server_method(void); /* TLSv1.1 */
const SSL_METHOD *TLSv1_2_server_method(void); /* TLSv1.2 */

(In reply to Christopher Schultz from comment #23)
> I've taken another look at the (updated) patches. I'm confused by the
> changes to sslcontext.c. It looks like there is no provision for
> combinations of SSL/TLS protocols.
>
> For instance, if I request (TLSv1_1 | TLSv1_2) then I don't get a configured
> SSL engine because of this:
>
> +#ifndef SSL_OP_NO_TLSv1_2
> + } else if (protocol & SSL_PROTOCOL_TLSV1_2) {
> + /* requested but not supported */
> +#endif
>
> Or is this because (TLSv1_1 | TLSv1_2) is not a supported protocol
> definition? I could only find these TLS-related server-method functions in
> the OpenSSL API:
>
> const SSL_METHOD *TLSv1_server_method(void); /* TLSv1.0 */
> const SSL_METHOD *TLSv1_1_server_method(void); /* TLSv1.1 */
> const SSL_METHOD *TLSv1_2_server_method(void); /* TLSv1.2 */
Well, I'm no longer interested in merging the patches upstream. In particular, I'm not going to update them anymore. However, I feel that I should explain current patches.
If I remember it correctly, I found out by experiments that the only method supporting any combination of protocol versions is SSLv23_server_method. So whenever more protocols are requested, this method should be used. Don't be confused by its name, it actually supports all TLS versions.
The code
+#ifndef SSL_OP_NO_TLSv1_2
...
means that whenever SSL library agains which tcnative is built is old enough so that it doesn't support newer TLS versions, and the user requested any of these versions, an error is returned.

(In reply to Mudassir Aftab from comment #27)
> Comment on attachment 29433[details]
> patch for tomcat trunk that adds support for newer TLS versions
>
> This patch is not working for me
>
> /opt/apache-tomcat-7.0.47-src# patch -R < patch
> can't find file to patch at input line 5
> Perhaps you should have used the -p or --strip option?
You have not used 'patch' correctly.

Created attachment 32114[details]
patch for the issue.
The patch works for me.
Basically the SSL.java needs the new SSL_PROTOCOL_TLS11 and SSL_PROTOCOL_TLS12 and add to ALL.
To set the protocol I have set it to SSL.SSL_PROTOCOL_ALL; and use !protocol.contains("java name") to allow support the java syntax for protocol.

I was looking at the code for the patch in Comment #32 and noticed that you introduced a regression. SSLv2 was removed from the ALL list sometime back so that the default was to not support SSLv2. This is also how it is documented on the Tomcat website.
Please remove SSLv2 from the list of ALL protocols.
Might I suggest that our new default for ALL also not include SSLv3, since it is now basically a useless protocol?

(In reply to jfclere from comment #31)
> Created attachment 32114[details]
> patch for the issue.
>
> The patch works for me.
> Basically the SSL.java needs the new SSL_PROTOCOL_TLS11 and
> SSL_PROTOCOL_TLS12 and add to ALL.
>
> To set the protocol I have set it to SSL.SSL_PROTOCOL_ALL; and use
> !protocol.contains("java name") to allow support the java syntax for
> protocol.
To be clear, this also requires a patch to tcnative as well.
Sounds like it's time to pull the trigger on this.

I'm looking at Marcel's attachment #30150[details] and the protocol selection is a bit verbose though methodical.
It took a bit of thinking to understand why the code does what it does. Specifically, it does not explicitly cover all possible combinations of values for "protocol". Instead, it takes a top-down approach assuming that the user will want the highest-available protocol to be supported.
Checks for exact matches are performed for TLSv1.2, TLSv1.1, TLSv1(.0), SSLv3, and SSLv2 are performed and the client gets the requested version unless the library doesn't support that version, in which case the client gets an inert SSL engine. It's debatable whether or not this should throw some kind of error.
After the exact checks, there are checks for "anything including TLSv1.2" and "anything including TLSv1.1", except that those checks are not even compiled if OpenSSL does not support them. (Of those, the highest protocol supported by the library is used.)
Failing the above, SSL2/3 is selected.
I see a consistency problem, here: if TLSv1.2 is not supported by OpenSSL but the client requests is specifically, then they will get an inert engine. If the client requests TLSv1.2 + SSLv3 and TLSv1.2 is not supported, they'll get the SSLv2/3 engine instead instead of the SSLv3 engine. It's not clear to me whether this was intentional.
I will be committing attachment #30150[details] without modification and we can debate the correct behavior later.
What's interesting (or awful: you decide) about OpenSSL is that you can't choose the exact set of protocols to support when choosing an engine method. Instead, you have to choose the engine method that makes the most sense (usually the highest version-number that is supported and requested by the client) and then you have to go back and black-list all the protocols that the selected method may support but that you don't want. A perfect case is that of requesting TLS1.2+TLS1.1 and nothing else. For that, you have to ask for the TLSv1.2 method in OpenSSL, but that method also provides TLS1, SSLv3, and SSLv2. So you have to call SSL_CTX_set_options and *enable* the *disable flags* for those other protocols. It's not straightforward at all and worth mentioning this to those who would like to review the patch.

Comment on attachment 32114[details]
patch for the issue.
Marking jfclere's patch as obsolete because I like the changes to the protocol selection that Marcel made. He also correctly included support for the SSL_OP_NO_TLS* options in supported_ssl_opts.

(In reply to Ralf Hauser from comment #43)
> I guess comment 30 ff. refers to
> https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-3566 ?
Yes.
Patches are available for all supported versions of Tomcat as well as tcnative. Voting is in process for tcnative 1.1.32 and I have voted to release (successfully tested with Tomcat 8-trunk which will be Tomcat 8.0.15). Feedback on the tcnative release candidate is welcome even for non-committers. Please reply to the [VOTE] thread on dev@tomcat.apache.org for tcnative 1.1.32.

This is ASF Bugzilla: the Apache Software Foundation bug system. In case
of problems with the functioning of ASF Bugzilla, please contact
bugzilla-admin@apache.org.
Please Note: this e-mail address is only for reporting problems
with ASF Bugzilla. Mail about any other subject will be silently
ignored.