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

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

changed milestone to %Tor: 0.2.4.x-final

added 024-backport component::core tor/tor milestone::Tor: 0.2.4.x-final priority::medium resolution::fixed status::closed tor-client type::defect version::tor 0.2.4.22 labels

Trac:
tor-0.2.4.22-tlsbugfix.patch

patch

Trac:
tor_ssl-as_20140607_symbz.txt

before-patch ASan report

Trac:
verify_tortls.tar

quick test used for testing patch

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?

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;

ssl2_get_cipher_by_char and ssl3_get_cipher_by_char are incompatible it seems

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);

?

Trac:
Milestone: N/A to Tor: 0.2.5.x-final
Keywords: N/A deleted, tor-client 024-backport added
Status: new to needs_review

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.

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.

Trac:
Status: needs_review to needs_revision

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.

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.)

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.

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.

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.

Trac:
Milestone: Tor: 0.2.5.x-final to Tor: 0.2.4.x-final
Status: needs_revision to needs_review

I put it into the upcoming 0.2.4.23.

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

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

closed

moved to tpo/core/tor#12227 (closed)