[Quantal] Regression in TLS 1.2 workarounds

Bug #1051892 reported by Tyler Hicks on 2012-09-17
38
This bug affects 7 people
Affects Status Importance Assigned to Milestone
OpenSSL
Fix Released
Unknown
openssl (Ubuntu)
High
Unassigned
Quantal
High
Unassigned

Bug Description

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

Related branches

Tyler Hicks (tyhicks) wrote :

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.

Changed in openssl (Ubuntu):
assignee: nobody → Tyler Hicks (tyhicks)
Tyler Hicks (tyhicks) on 2012-09-18
description: updated
Tyler Hicks (tyhicks) wrote :

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) wrote :

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

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.

James Troup (elmo) on 2012-10-03
tags: added: rls-q-incoming
Liam Gladdy (liam) wrote :

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) wrote :

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

Changed in openssl (Ubuntu):
assignee: Tyler Hicks (tyhicks) → nobody
Tyler Hicks (tyhicks) wrote :

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) wrote :

debdiff looks good, will upload if it builds

tags: added: quantal regression-release
Changed in openssl (Ubuntu Quantal):
assignee: nobody → Micah Gersten (micahg)
status: Triaged → In Progress
Micah Gersten (micahg) wrote :

Uploaded, waiting in queue for acceptance

Changed in openssl (Ubuntu Quantal):
assignee: Micah Gersten (micahg) → nobody
status: In Progress → Fix Committed
Adam Conrad (adconrad) 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?

Tyler Hicks (tyhicks) wrote :

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) wrote :

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) wrote :

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) wrote :

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

Changed in openssl (Ubuntu Quantal):
status: Fix Committed → Fix Released
Changed in openssl:
status: Unknown → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.