[Quantal] Regression in TLS 1.2 workarounds

[Tyler Hicks]

openssl 1.0.1c-3ubuntu1 dropped almost all of debian/patches/tls12_workarounds.patch because the upstream 1.0.1c release contained the changes.

However, the dropped pieces of tls12_workarounds.patch had a subtle difference from upstream. In the Ubuntu patch, ssl23_client_hello() checked the *client* TLS version when deciding if the cipher list should be truncated or not for TLS 1.2. The upstream code (http://cvs.openssl.org/chngview?cn=22408) checks the *negotiated* TLS version, which I believe is incorrect since the ServerHello hasn't even occurred yet in order to negotiate the TLS version.

The change from TLS1_get_versions() to TLS1_get_client_versions() was discussed here:

https://bugs.launchpad.net/ubuntu/+source/openssl/+bug/986147/comments/4

This bug can be reproduced with the following command:

$ openssl s_client -connect d2chzxaqi4y7f8.cloudfront.net:443 -CApath /etc/ssl/certs/

It will fail unless -tls1 is specified like so:

$ openssl s_client -connect d2chzxaqi4y7f8.cloudfront.net:443 -CApath /etc/ssl/certs/ -tls1

Making this change fixes the problem (ssl3_client_hello() will probably need the same change):

--- openssl-1.0.1c.orig/ssl/s23_clnt.c 2012-09-17 01:06:06.584617683 -0700
+++ openssl-1.0.1c/ssl/s23_clnt.c 2012-09-17 02:09:01.140540223 -0700
@@ -491,7 +491,7 @@
                         * as hack workaround chop number of supported ciphers
                         * to keep it well below this if we use TLS v1.2
                         */
- if (TLS1_get_version(s) >= TLS1_2_VERSION
+ if (TLS1_get_client_version(s) >= TLS1_2_VERSION
                                && i > OPENSSL_MAX_TLS1_2_CIPHER_LENGTH)
                                i = OPENSSL_MAX_TLS1_2_CIPHER_LENGTH & ~1;
 #endif

[Tyler Hicks (tyhicks)]

I haven't attached a debdiff because it still isn't clear to me if calling TLS1_get_client_version() is the correct thing to do here. We probably need to open an upstream bug and get their opinion. I am willing to do that tomorrow.

[Tyler Hicks (tyhicks)]

I've submitted a bug, with patch, to OpenSSL's request tracker. I'll update this bug with a link when it makes its way through their moderation queue.

[Jessie Morris (jessieamorris)]

Any news on the upstream. This is affecting me as well and the release date for 12.10 looms closer.

[Tyler Hicks (tyhicks)]

On 2012-10-03 17:10:36, Jessie Morris wrote:
> Any news on the upstream. This is affecting me as well and the release
> date for 12.10 looms closer.

I've linked this bug to the upstream bug. It has received no attention
upstream. I'm going to propose a debdiff soon to fix this in Ubuntu.

[Liam Gladdy (liam)]

Affects me too, and this breaks connections to twitter's user stream over SSL, so that's a pretty major service that would be affected if this isn't fixed before release.

[Tyler Hicks (tyhicks)]

Since the upstream bug hasn't received any attention and it is late in our release cycle, I decided to just keep it simple and carry over the simple change that we carry in Precise for ssl23_client_hello().

I still think that we have a strange combination of build options with -DOPENSSL_NO_TLS1_2_CLIENT and -DOPENSSL_MAX_TLS1_2_CIPHER_LENGTH=50. It looks to me like it should be one or the other, but I'm not comfortable making that change at this point in the cycle.

I've added a truncate cipher list test case to test-openssl.py in lp:qa-regression-testing and also ran through test connections to a few of the servers that have been reported as problematic in bug 965371, bug 986147, and this bug.

Here are the results with Quantal's openssl 1.0.1c-3ubuntu1:

Testing www.mediafire.com:443 FAIL
Testing cs3-api.salesforce.com:443 pass
Testing graph.facebook.com:443 pass
Testing www.paypal.com:443 pass
Testing info.vsu.ru:443 FAIL
Testing www.evernote.com:443 FAIL
Testing d3vwyrdyja2n00.cloudfront.net:443 FAIL
Testing d18kq98amm3n6k.cloudfront.net:443 FAIL
Testing userstream.twitter.com:443 FAIL

Here are the results after applying the attached debdiff:

Testing www.mediafire.com:443 FAIL
Testing cs3-api.salesforce.com:443 pass
Testing graph.facebook.com:443 pass
Testing www.paypal.com:443 pass
Testing info.vsu.ru:443 pass
Testing www.evernote.com:443 FAIL
Testing d3vwyrdyja2n00.cloudfront.net:443 pass
Testing d18kq98amm3n6k.cloudfront.net:443 pass
Testing userstream.twitter.com:443 pass

This matches the results in Precise's openssl 1.0.1-4ubuntu5.5:

Testing www.mediafire.com:443 FAIL
Testing cs3-api.salesforce.com:443 pass
Testing graph.facebook.com:443 pass
Testing www.paypal.com:443 pass
Testing info.vsu.ru:443 pass
Testing www.evernote.com:443 FAIL
Testing d3vwyrdyja2n00.cloudfront.net:443 pass
Testing d18kq98amm3n6k.cloudfront.net:443 pass
Testing userstream.twitter.com:443 pass

[Tyler Hicks (tyhicks)]

The www.mediafire.com:443 is expected. See here : https://bugs.launchpad.net/ubuntu/+source/openssl/+bug/965371/comments/22

The www.evernote.com:443 failure may have been a bad test. evernote.com:443 (note the lack of 'www.') passes in all three test environments so I'm not going to look into this any more.

Since the debdiff above solves the regression and causes OpenSSL to operate as well as it does in Precise, I feel it is ready for sponsorship.

[Micah Gersten (micahg)]

debdiff looks good, will upload if it builds

[Micah Gersten (micahg)]

Uploaded, waiting in queue for acceptance

[Adam Conrad (adconrad)]

Rejecting for now, based on the diff, until someone explains to me why the upstream commit adds the same code block to two files (s3_clnt.c and s23_clnt.c), but the Ubuntu patch only swaps the get_version call to get_client_version in one of them (s23_clnt.c). This feels wrong to me, but maybe there's a valid reason for it?

[Tyler Hicks (tyhicks)]

On 2012-10-06 02:27:45, Adam Conrad wrote:
> Rejecting for now, based on the diff, until someone explains to me why
> the upstream commit adds the same code block to two files (s3_clnt.c and
> s23_clnt.c), but the Ubuntu patch only swaps the get_version call to
> get_client_version in one of them (s23_clnt.c). This feels wrong to me,
> but maybe there's a valid reason for it?

There's a reason, but I'm not sure if it is actually valid:

That's how it is in Precise.

I mentioned that s3_clnt.c should probably be changed to
TLS1_get_client_version() in this bug description and the patch that I
proposed to upstream in rt #2881 does make that change. But since
upstream hasn't commented and things seem to be working ok in Precise, I
don't want to rock the boat too much at this point.

FWIW, I did switch ssl3_client_hello() over to use
TLS1_get_client_version() and all of the test results above were the
same. So I'm ok with making the change, but I'd rather not at this
point.

[Tyler Hicks (tyhicks)]

To put a different way... I'm just attempting to fix the regression from Precise to Quantal. That simple change is what fixes it and gets OpenSSL working the same as it is in Precise. If I had more time before release, then I'd probably propose quite a few other changes to the TLS 1.2 workarounds but I'm just focusing on this regression right now.

[Adam Conrad (adconrad)]

Alright, based on the above, I'll pull openssl back out of the rejected queue, but I'd certainly like to see this revisited. Agreed that making it "work like precise" is good enough for now, though.

[Launchpad Janitor (janitor)]

This bug was fixed in the package openssl - 1.0.1c-3ubuntu2

---------------
openssl (1.0.1c-3ubuntu2) quantal; urgency=low

  [ Tyler Hicks <email address hidden> ]
  * debian/patches/tls12_workarounds.patch: Readd the change to check
    TLS1_get_client_version rather than TLS1_get_version to fix incorrect
    client hello cipher list truncation when TLS 1.1 and lower is in use.
    (LP: #1051892)

  [ Micah Gersten <email address hidden> ]
  * Mark Debian Vcs-* as XS-Debian-Vcs-*
    - update debian/control
 -- Tyler Hicks <email address hidden> Thu, 04 Oct 2012 10:34:57 -0700