Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#4822 closed defect (fixed)

Avoid vulnerability CVE-2011-4576 : Disable SSL3?

Reported by: nickm Owned by:
Priority: Very High Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

According to http://openssl.org/news/secadv_20120104.txt , there is an information leakage vulnerability when making SSL3 connections with SSL_MODE_RELEASE_BUFFERS, where uninitialized memory can get leaked, up to 15 bytes at a time. The bug is fixed in openssl 1.0.0f and 0.9.8s.

(I'm told this was found by wanoskarnet, reported by asn to agl, who got it fixed in openssl.)

On Tor's side, the easiest fix is to just require TLS1 only, and not support SSL3 any more. But that could create problems with our cipher lists.

Child Tickets

TicketStatusOwnerSummaryComponent
#7070closedtor disables the SSLv3 for OpenSSL 1.0.0j on FedoraCore Tor/Tor

Attachments (2)

demo.sh (225 bytes) - added by nickm 8 years ago.
demoprog.c (3.6 KB) - added by nickm 8 years ago.

Download all attachments as: .zip

Change History (43)

comment:1 Changed 8 years ago by nickm

Of course, we should ship packages with updated openssls. I think that is a good fix no matter what Tor does.

comment:2 Changed 8 years ago by nickm

(I'm told this was found by wanoskarnet, reported by asn to agl, who got it fixed in openssl.)

Ah, I misunderstood CVE-2011-4619 was the misattributed one.

comment:3 Changed 8 years ago by nickm

Have a look at branch "bug4822_021" in my public repo.

I considered an approach where we would allow any handshake, but disallow any SSL3 ciphers so that the handshake would fail if the ssl3 handshake were actually tried. Problem was, openssl allows tls1 ciphers with the ssl3 handshake, so that wouldn't have worked. (Thanks to asn for testing that.)

This needs review and a changes file.

comment:4 Changed 8 years ago by nickm

Status: newneeds_review

comment:5 Changed 8 years ago by rransom

nickm's bug4822_021 is a compile-time version check; we need to do a runtime version check.

This needs to be tested to determine whether a server which uses TLSv1_method can receive a connection from a client which uses SSLv23_method. (When I read the OpenSSL documentation for those, it sort of hinted that that would not work.)

comment:6 in reply to:  5 ; Changed 8 years ago by rransom

Replying to rransom:

This needs to be tested to determine whether a server which uses TLSv1_method can receive a connection from a client which uses SSLv23_method. (When I read the OpenSSL documentation for those, it sort of hinted that that would not work.)

BZZZT! Wrong. The documentation (SSL_CTX_new(3ssl)) says quite explicitly that that will not work:

       TLSv1_method(void), TLSv1_server_method(void),
       TLSv1_client_method(void)
           A TLS/SSL connection established with these methods will only
           understand the TLSv1 protocol. A client will send out TLSv1 client
           hello messages and will indicate that it only understands TLSv1. A
           server will only understand TLSv1 client hello messages. This
           especially means, that it will not understand SSLv2 client hello
           messages which are widely used for compatibility reasons, see
           SSLv23_*_method(). It will also not understand SSLv3 client hello
           messages.

       SSLv23_method(void), SSLv23_server_method(void),
       SSLv23_client_method(void)
           A TLS/SSL connection established with these methods will understand
           the SSLv2, SSLv3, and TLSv1 protocol. A client will send out SSLv2
           client hello messages and will indicate that it also understands
           SSLv3 and TLSv1. A server will understand SSLv2, SSLv3, and TLSv1
           client hello messages. This is the best choice when compatibility
           is a concern.

Can we afford to throw away all relays run on RHEL/CentOS and other crap OSes whose OpenSSL packages lie about their versions?

comment:7 in reply to:  6 ; Changed 8 years ago by nickm_mobile

Replying to rransom:

Replying to rransom:

This needs to be tested to determine whether a server which uses TLSv1_method can receive a connection from a client which uses SSLv23_method. (When I read the OpenSSL documentation for those, it sort of hinted that that would not work.)

BZZZT! Wrong. The documentation (SSL_CTX_new(3ssl)) says quite explicitly that that will not work:

Ugh. So that patch would break compatibility with existing tors. Yuck.

We could have clients use TLSv1_method, at least. That would be something. Servers would probably need some disgusting hackery of the info_cb variety.

Any other ideas?

Can we afford to throw away all relays run on RHEL/CentOS and other crap OSes whose OpenSSL packages lie about their versions?

Does anybody claim to have a later openssl version than they really have? I thought they all were claiming too early.or about right. Any documentation there?

(Using flyspray from phone is pretty hard, ow)

comment:8 Changed 8 years ago by nickm_mobile

Maybe SSL_OP_NO_SSLv3? It seems to exist; haven't tested when it appeared.

comment:9 Changed 8 years ago by nickm

Okay, please look at branch bug4822_021_v2 in my public repository. It takes the SSL_OP_NO_SSLv3 approach.

comment:10 in reply to:  8 Changed 8 years ago by rransom

Replying to nickm_mobile:

Maybe SSL_OP_NO_SSLv3? It seems to exist; haven't tested when it appeared.

This appears to be sufficient to keep us from ever calling ssl3_enc (the function in OpenSSL which contains The Bug), because it ensures that we will never use an SSL 3 ‘method’.

comment:11 in reply to:  9 ; Changed 8 years ago by rransom

Replying to nickm:

Okay, please look at branch bug4822_021_v2 in my public repository. It takes the SSL_OP_NO_SSLv3 approach.

The EVERYONE_HAS_AES change probably belongs in a separate commit, although I don't think leaving it in the same commit as the disable-SSL3 change will ever cause any trouble.

I still think we should use a runtime version check, not a compile-time version check -- people will build Tor against an old OpenSSL shared library and run it with a new one.

Other than that, looks good.

comment:12 in reply to:  7 Changed 8 years ago by rransom

Replying to nickm_mobile:

We could have clients use TLSv1_method, at least. That would be something. Servers would probably need some disgusting hackery of the info_cb variety.

This would make Tor clients' ClientHello messages distinguishable from most other applications'. I don't think we're going to want to do that until SSL 2 and 3 are really dead, and I'm not convinced that that will ever happen.

comment:13 Changed 8 years ago by nickm

Just pushed an updated version with a changes file.

comment:14 in reply to:  11 ; Changed 8 years ago by nickm

Replying to rransom:

Replying to nickm:

Okay, please look at branch bug4822_021_v2 in my public repository. It takes the SSL_OP_NO_SSLv3 approach.

The EVERYONE_HAS_AES change probably belongs in a separate commit, although I don't think leaving it in the same commit as the disable-SSL3 change will ever cause any trouble.

Right; I'm inclined to leave it.

I still think we should use a runtime version check, not a compile-time version check -- people will build Tor against an old OpenSSL shared library and run it with a new one.

Hm. That case is actually safe; the problem would be if they built with a newer one and ran with an older one. Does that seem likely? And shouldn't a decent library system prevent this?

comment:15 in reply to:  14 ; Changed 8 years ago by rransom

Replying to nickm:

Replying to rransom:

Replying to nickm:

Okay, please look at branch bug4822_021_v2 in my public repository. It takes the SSL_OP_NO_SSLv3 approach.

I still think we should use a runtime version check, not a compile-time version check -- people will build Tor against an old OpenSSL shared library and run it with a new one.

Hm. That case is actually safe; the problem would be if they built with a newer one and ran with an older one. Does that seem likely? And shouldn't a decent library system prevent this?

The dangerous case could happen with packages for one Linux distribution used on a different distribution (e.g. packages built on and for Ubuntu used on Mint, before Mint updates its OpenSSL packages). But if there is no reason to try to enable SSL 3 whenever it is safe to do so, we shouldn't make this change depend on OpenSSL's version at all.

comment:16 in reply to:  15 ; Changed 8 years ago by nickm

Replying to rransom:

The dangerous case could happen with packages for one Linux distribution used on a different distribution (e.g. packages built on and for Ubuntu used on Mint, before Mint updates its OpenSSL packages).

Okay. Then I'd say, "do this whenever the runtime version looks bad or the compile-time version looks bad."

But if there is no reason to try to enable SSL 3 whenever it is safe to do so, we shouldn't make this change depend on OpenSSL's version at all.

I think we think it might help for profiling resistance. I don't want to make extra changes to our default SSL profile back to 0.2.1 and 0.2.2 as part of this ticket without significant further analysis. This is a "let's make sure that the SSL vulnerability doesn't bite our users" ticket, not a "and while we're here, let's throw out parts of our SSL profile that we think we can do without" ticket.

comment:17 in reply to:  16 ; Changed 8 years ago by rransom

Replying to nickm:

Replying to rransom:

The dangerous case could happen with packages for one Linux distribution used on a different distribution (e.g. packages built on and for Ubuntu used on Mint, before Mint updates its OpenSSL packages).

Okay. Then I'd say, "do this whenever the runtime version looks bad or the compile-time version looks bad."

See my bug4822_021_v2 branch for fixups.

comment:18 in reply to:  17 Changed 8 years ago by rransom

Replying to rransom:

Replying to nickm:

Replying to rransom:

The dangerous case could happen with packages for one Linux distribution used on a different distribution (e.g. packages built on and for Ubuntu used on Mint, before Mint updates its OpenSSL packages).

Okay. Then I'd say, "do this whenever the runtime version looks bad or the compile-time version looks bad."

See my bug4822_021_v2 branch for fixups.

nickm approves these fixups. (Currently, that branch points to commit e0795667f924dec650bc4f8c1be93787776c2408.)

comment:19 Changed 8 years ago by rransom

Should we emit a log message when we disable SSLv3? If we don't, there will be no easy way to determine whether a particular Tor instance supports SSLv3.

comment:20 Changed 8 years ago by nickm

Sounds like a reasonable thing to do.

comment:21 in reply to:  19 Changed 8 years ago by rransom

Replying to rransom:

Should we emit a log message when we disable SSLv3?

I've pushed a commit to log at info level when disabling SSLv3 to my bug4822_021_v2 branch; the previous (nickm-approved) commits are on my bug4822_021_v2-nickm-approved branch.

comment:22 Changed 8 years ago by nickm

For logging version numbers, prefer OPENSSL_VERSION_TEXT and SSLeay_version(SSLEAY_VERSION), I think.

And a better message would be IMO "Disabling SSLv3 because this OpenSSL version might otherwise be vulnerable to CVE-foo." In other words, make it clear that this is a problem that stems from the openssl version, and that disabling SSLv3 will solve the problem if it exists.

This could be at NOTICE, I think. But I'm not sure it has to be. Others can decide.

comment:23 in reply to:  22 ; Changed 8 years ago by rransom

Replying to nickm:

For logging version numbers, prefer OPENSSL_VERSION_TEXT

OK.

and SSLeay_version(SSLEAY_VERSION), I think.

No.

char *SSLeay_version(t)
int t;
	{
	if (t == SSLEAY_VERSION)
		return("SSLeay 0.9.1a 06-Jul-1998");

That is impressively bogus.

And a better message would be IMO "Disabling SSLv3 because this OpenSSL version might otherwise be vulnerable to CVE-foo." In other words, make it clear that this is a problem that stems from the openssl version, and that disabling SSLv3 will solve the problem if it exists.

OK.

This could be at NOTICE, I think. But I'm not sure it has to be. Others can decide.

Currently we emit the log message whenever we disable SSLv3 for some TLS context, which we do at least once on every SIGHUP. Do you still think we should emit this at notice level? If so, should we try to emit it less often?

comment:24 Changed 8 years ago by rransom

Fixups pushed to improve the log message text and include OPENSSL_VERSION_TEXT.

comment:25 in reply to:  6 Changed 8 years ago by rransom

Replying to rransom:

Replying to rransom:

This needs to be tested to determine whether a server which uses TLSv1_method can receive a connection from a client which uses SSLv23_method. (When I read the OpenSSL documentation for those, it sort of hinted that that would not work.)

BZZZT! Wrong. The documentation (SSL_CTX_new(3ssl)) says quite explicitly that that will not work:

That chunk of documentation is out of date for clients using OpenSSL 1.0.0 or later. From the CHANGES file:

 Changes between 0.9.8n and 1.0.0  [29 Mar 2010]
  *) If no SSLv2 ciphers are used don't use an SSLv2 compatible client hello:
     this allows the use of compression and extensions. Change default cipher
     string to remove SSLv2 ciphersuites. This effectively avoids ancient SSLv2
     by default unless an application cipher string requests it.
     [Steve Henson]

But I expect that clients using SSLv23_method still won't be able to connect to servers using TLSv1_method.

comment:26 in reply to:  23 ; Changed 8 years ago by nickm

Replying to rransom:

Replying to nickm:

For logging version numbers, prefer OPENSSL_VERSION_TEXT

OK.

and SSLeay_version(SSLEAY_VERSION), I think.

No.

char *SSLeay_version(t)
int t;
	{
	if (t == SSLEAY_VERSION)
		return("SSLeay 0.9.1a 06-Jul-1998");

That is impressively bogus.

Weird; what openssl version are you looking at? Mine have

   crypto/cversion.c:	if (t == SSLEAY_VERSION)
   crypto/cversion.c-		return OPENSSL_VERSION_TEXT;

comment:27 in reply to:  26 Changed 8 years ago by rransom

Replying to nickm:

Replying to rransom:

Replying to nickm:

For logging version numbers, prefer OPENSSL_VERSION_TEXT

OK.

and SSLeay_version(SSLEAY_VERSION), I think.

No.

char *SSLeay_version(t)
int t;
	{
	if (t == SSLEAY_VERSION)
		return("SSLeay 0.9.1a 06-Jul-1998");

That is impressively bogus.

Weird; what openssl version are you looking at? Mine have

   crypto/cversion.c:	if (t == SSLEAY_VERSION)
   crypto/cversion.c-		return OPENSSL_VERSION_TEXT;

http://repo.or.cz/w/mirror-openssl.git/blob/OpenSSL_1_0_0-stable:/crypto/cversion.c

comment:28 Changed 8 years ago by nickm

That's so weird that I want to blame it on the git conversion or something.

comment:29 in reply to:  28 Changed 8 years ago by rransom

Replying to nickm:

That's so weird that I want to blame it on the git conversion or something.

I've pushed a fixup commit to include SSLeay_version(SSLEAY_VERSION) in the log message. Hopefully you're right, and that won't embarrass us.

comment:30 Changed 8 years ago by nickm

Now all squashed in bug4822_021_v2_squashed

comment:31 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged after review. Thanks, all!

comment:32 Changed 8 years ago by arma

Resolution: fixed
Status: closedreopened

comment:33 Changed 8 years ago by arma

<wanoskarnet> Tor client which uses SSLv23_method must work with a server
which uses TLSv1_method. You missed "SSL_OP_NO_SSLv2" while disscuss #4822.

comment:34 Changed 8 years ago by nickm

I don't understand the comment. We don't actually use TLSv1_method server-side, as far as I understand, and we're not planning to, since it would make us reject non-TLS1 handshakes?

Also, what did we miss about SSL_OP_NO_SSLv2? We already set that option.

comment:35 Changed 8 years ago by asn

This is what wanoskarnet said, before the comment that arma pasted in comment:33.

< wanoskarnet> "Tell OpenSSL to only use TLS1. This would actually break compatibility with clients that are configured to use SSLv23_method()". it is wrong statement. SSLv23 client
               sends ProtocolVersion that indicates understanding TLSv1 so server well understand it. Docs means SSLv3 clients that never sends ProtocolVersion == TLSv1 only SSLv3, not
               a SSLv23 clients.
< wanoskarnet> Tor never used SSLv2 compatiblity so client hello exactly SSLv3.1(TLSv1) looking like.
< wanoskarnet> Tor client which uses SSLv23_method can work with a server which uses TLSv1_method. You missed "SSL_OP_NO_SSLv2" while disscuss #4822.

comment:36 Changed 8 years ago by nickm

Resolution: fixed
Status: reopenedclosed

Okay, so if I understand correctly, wanoskarnet is saying that our reading of the TLSv1_method() documentation and the SSLv23_method() documentation is wrong: that a TLSv1_method() client can connect perfectly well to a SSLv23_method() server, and vice versa.

I'm attaching a quick&dirty test program to demonstrate this (using some code from libevent and some from the openssl docs).

This doesn't mean that we need any changes in the code, except for fixing the comment to be correct. I'll do that after I attach the demo code.

Changed 8 years ago by nickm

Attachment: demo.sh added

Changed 8 years ago by nickm

Attachment: demoprog.c added

comment:37 Changed 8 years ago by fermenthor

tortls.c misidentifies the vulnerability as CVE-2011-4657 while this is CVE-2011-4576 (twice in comments, once in logging).

comment:38 in reply to:  37 Changed 8 years ago by rransom

Replying to fermenthor:

tortls.c misidentifies the vulnerability as CVE-2011-4657 while this is CVE-2011-4576 (twice in comments, once in logging).

Moved to #5066.

comment:39 Changed 7 years ago by nickm

Keywords: tor-client added

comment:40 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:41 Changed 7 years ago by nickm

Milestone: Tor: 0.2.1.x-final

Milestone Tor: 0.2.1.x-final deleted

Note: See TracTickets for help on using tickets.