Opened 6 years ago

Closed 6 years ago

#12227 closed defect (fixed)

ASan stack-buffer-overflow in prune_v2_cipher_list -- not exploitable

Reported by: starlight Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.22
Severity: Keywords: tor-client 024-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Found a minor buffer overflow when
running live relay with 'tor' and
'openssl' both compiled with
AddressSanitizer.

tortls.c:1492: unsigned char cipherid[2];

should be three characters and the
final byte initialized to zero for

ssl2_get_cipher_by_char()

to function correctly and to avoid
an ASan access exception.

Tested patch that resolves this
issue is attached.

Compiled with gcc 4.8.1 and with
these added options:

tor-0.2.4.22

-O1 # instead of -O2
-fsanitize=address
-fno-omit-frame-pointer

openssl-1.0.1h

-fstack-protector-all
--param ssp-buffer-size=1
-fsanitize=address
-fno-omit-frame-pointer
-DOPENSSL_NO_BUF_FREELIST

Child Tickets

Attachments (3)

tor-0.2.4.22-tlsbugfix.patch (913 bytes) - added by starlight 6 years ago.
patch
tor_ssl-as_20140607_symbz.txt (3.3 KB) - added by starlight 6 years ago.
before-patch ASan report
verify_tortls.tar (10.0 KB) - added by starlight 6 years ago.
quick test used for testing patch

Download all attachments as: .zip

Change History (16)

Changed 6 years ago by starlight

patch

Changed 6 years ago by starlight

before-patch ASan report

Changed 6 years ago by starlight

Attachment: verify_tortls.tar added

quick test used for testing patch

comment:1 Changed 6 years ago by starlight

After spending a day fixing this bug, I have
to wonder, why is SSLv2 still active in the
code?

I gather SSLv2 is usually kept around as a way to
force older software peers to negotiate to SSLv3
or TLS. However Tor OR relays only communicate
with other OR relays and since SSLv2 has been
deprecated for so long, why not just disable
it entirely?

Or is this bit of code strictly internal and
exists to underpin the newer protocols?

comment:2 Changed 6 years ago by cypherpunks

or just?

 prune_v2_cipher_list(void)
 {
   uint16_t *inp, *outp;
-  const SSL_METHOD *m = SSLv23_method();
+  const SSL_METHOD *m = SSLv3_method();

   inp = outp = v2_cipher_list;

comment:3 Changed 6 years ago by cypherpunks

Deleted.

Version 1, edited 6 years ago by cypherpunks (previous) (next) (diff)

comment:4 Changed 6 years ago by nickm

Keywords: tor-client 024-backport added
Milestone: Tor: 0.2.5.x-final
Status: newneeds_review

The SSLv23_method() call is later modified by the SSL_OP_NO_SSLv2 flag.

As for the patch, why can't it just be:

diff --git a/src/common/tortls.c b/src/common/tortls.c
index a6444b8..05e0b50 100644
--- a/src/common/tortls.c
+++ b/src/common/tortls.c
@@ -1477,10 +1477,11 @@ prune_v2_cipher_list(void)
 
   inp = outp = v2_cipher_list;
   while (*inp) {
-    unsigned char cipherid[2];
+    unsigned char cipherid[3];
     const SSL_CIPHER *cipher;
     /* Is there no better way to do this? */
     set_uint16(cipherid, htons(*inp));
+    cipherid[3] = 0;
     cipher = m->get_cipher_by_char(cipherid);
     if (cipher) {
       tor_assert((cipher->id & 0xffff) == *inp);

?

comment:5 Changed 6 years ago by cypherpunks

get_cipher_by_char for SSLv23 is wrong and shouldn't be used. It messes bytes and the same bytes shifts differently inside of cipher id.

comment:6 in reply to:  4 Changed 6 years ago by nickm

Replying to nickm:

The SSLv23_method() call is later modified by the SSL_OP_NO_SSLv2 flag.

Also: Using SSLv3_method() for our SSL objects would mean (IIUC) no TLS1; and using TLSv1_method() would mean no accepting SSLv3. ISTR we had a ticket about that long ago.

But IIUC, using SSLv3_method() or TLSv1_method() in prune_v2_cipher_lists() would seem to be a fine thing to do.

Replying to cypherpunks:

get_cipher_by_char for SSLv23 is wrong and shouldn't be used. It messes bytes and the same bytes shifts differently inside of cipher id.

Can we just switch prune_v2_cipher_list() to use SSLv3_method or TLSv1_method() ? It looks to me like that would work, but it's late, my brain is tired, and I haven't tested it yet.

comment:7 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

comment:8 Changed 6 years ago by cypherpunks

It looks to me like that would work

I think it must to work. The only thing is about updating openssl code in future? What if new methods for new tls version will use something different than ssl3_get_cipher_by_char. With SSLv23_method we at least sure it will support any new tls version while openssl exist.

comment:9 Changed 6 years ago by nickm

But SSLv23_method() exposes ssl23_get_cipher_by_char(). To get ssl3_get_cipher_by_char(), we either need to poke inside openssl internals, or get it from SSLv3_method() or TLSv1_method(), I think.

(In the long term, I think the right decision is to drop support for the Tor V2 link protocol handshake entirely, so we can remove this whole function.)

comment:10 Changed 6 years ago by cypherpunks

In the long term, I think the right decision is to drop support for the Tor V2 link protocol handshake entirely, so we can remove this whole function.

Then no difference to use SSLv3_method() or to pass 3 bytes if SSLv23_method() used. Both are kludge.

Last edited 6 years ago by cypherpunks (previous) (diff)

comment:11 in reply to:  4 Changed 6 years ago by starlight

Replying to nickm:

As for the patch, why can't it just be. . .

It can. But replaced htons() because it is
logically incompatible with the SSLv2
char cipher ID construction. Was mislead
at first and thought changing it to

set_uint32(cipherid, htonl(*inp))

would work but this is incorrect. Never
liked those BSD byte-order functions
and don't use them myself. The proposed
patch is clearer.

comment:12 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.4.x-final
Status: needs_revisionneeds_review

Okay. I put the revised patch into a branch called "bug12227_024", and merged it to master. Marking this for backport to 0.2.4.

comment:13 Changed 6 years ago by arma

Resolution: fixed
Status: needs_reviewclosed

I put it into the upcoming 0.2.4.23.

Here's hoping that wasn't a bad idea. :)

Note: See TracTickets for help on using tickets.