Topic: crash on malformed cipher change spec message


TLDR: cyassl corrupts memory if client sends cipher change spec with body longer than 1 byte.

* SuSE Linux Enterprise Server 11.1
* gcc 4.3.4 (system default)
* cyassl - latest master from github, as of 3.sept.2012, master 690938f lsr fs macros
* linking statically with libcyassl.a (but see below an example with cyassl bundled examples)
* cyassl configured with: ./configure --prefix=/local/cyassl-dev -enable-hugecache --enable-fastmath --enable-aesni --enable-opensslExtra --disable-debug

Debugging my application ssl-related crashes i've found strange situation when if there is an error during the handshake my app would crash with "heap corruption" glibc message.

So investigating i've narrowed it down to one specifi case: cipher change spec message where header looks like this:

5 byte record header
[14 03 01 00 81]
[random garbage in the body and some data after]

so processing this message yassl first loops over ALL 129 bytes of the body, decrypting byte by byte and updating hashes, etc. and then like nothing has happened proceeds to the next message and bails on it, since it's header is garbage.

But doing that it corrupts memory somewhere, the exact spot i haven't been able to track yet sad

So in pseudocode it looks like

if (-1 == CyaSSL_accept(ssl)) // returns -226, record layer version error
  CyaSSL_free(ssl); // here it crashes with glibc "double free or corruption" message

So i'm not supposed to call CyaSSL_free() there? but it definitely should NOT loop anyway, right ?

Also heres the patch to reproduce it on bundled server example (it basically comments out exiting on SSL_accept() failure and any io after that, jumping straight to CyaSSL_free() call)

diff --git a/examples/server/server.c b/examples/server/server.c
index a33a4ef..6318cb4 100644--- a/examples/server/server.c+++ b/examples/server/server.c
@@ -298,12 +298,14 @@ THREAD_RETURN CYASSL_THREAD server_test(void* args)             int err = SSL_get_error(ssl, 0);             char buffer[80];
             printf("error = %d, %s\n", err, ERR_error_string(err, buffer));-            err_sys("SSL_accept failed");
+            //err_sys("SSL_accept failed");
         }     #else
         NonBlockingSSL_Accept(ssl);     #endif
+#if 0
     idx = SSL_read(ssl, input, sizeof(input));
@@ -316,6 +318,7 @@ THREAD_RETURN CYASSL_THREAD server_test(void* args)
         err_sys("SSL_write failed");

so to reproduce:
* apply the patch, run make
* start server: ./examples/server/server -p 1105 -v 1 -d
* execute the following command imitating the proper handshake with malicious change_cipher_spec:

echo -ne '160301003b0100003703010bf3397a0a912e0ee6398b8513ffa464ad8e69184b93f133c5e29c955c25c4e0000010003900330035002f00050004000a00fd010016030101061000010201002f5cc167dbc143d8a5bdaeb6740c1b89a09a013e8ec3be484b109d746001e0ce328d53999c99b74cfe21c8ca05ad3d8d45e61b2b47ecbdaee93b5ff06bf1e04338fd964152c231d65573d40e33e71c75c6beba1d237ddabe7322a26c6ac13a648c6e71555854c428100434a5237dbb0277c7982ea081dc8edbcbfde8792b6bde58a146380804c23ce4246abb34caf8dfe0c48f87123f80db20c5364e4165006ef2ba7f37fc373b2ee28dd37b26d33073ba932167b5719087eae472ce021832e8d83f5e0ad82f75a257b20294692b7dbbf7c19a0d8e6e302a6cf064373740ea4b619d0688f8b5c23ebd424d56b5110a885c8f002c10688f092522fdfdf1e360c71403010081011603010030b7346eff3b5da42f49afec2df1d1da5c1339493f3f87ef4d9cacff525eb7b2668135a08c6fd80288632897f56b9244d51703010020a682f8e0cb0490324ea9e8317032f08269e7de0c5e302fc76e4690d55f3482af17030101e0c17628395f42964139f16e616da331ac2ae432c88e0ea50b10a35965a15e7ee9fe5ba81e9e58' | xxd -r -p | netcat localhost 1105 | xxd

i'm unsure how to patch this, but for now i've opted to just reject cipher_change_spec messages with length != 1, here's the patch, which seems to solve the problem for me, by bailing early without trying to recover

diff --git a/src/internal.c b/src/internal.c
index 367b97e..c70079e 100644
--- a/src/internal.c
+++ b/src/internal.c
@@ -2731,6 +2731,11 @@ int ProcessReply(CYASSL* ssl)
                             AddLateRecordHeader(&ssl->curRL, &ssl->timeoutInfo);
+                   /* CHANGE CIPHER SPEC is always 1 byte long, just reject the whole thing if it's not */
+                   if (ssl->curSize != 1)
+                       return LENGTH_ERROR;
                     ssl->keys.encryptionOn = 1;

oh and btw, strangely if i rebuild cyassl with --enable-debug it stops crashing, but doesn't stop looping over the whole message.

Thanks for reading, hope to receive some help from the gurus tongue


2 (edited by antoxa 2012-09-04 02:38:08)

Re: crash on malformed cipher change spec message

just noticed that wasn't actually the latest master i tested on.
so updated to "master f8b1066 fix sniffer cipher suite tests with user override"

the problem is still there for me.


Re: crash on malformed cipher change spec message

Thanks antoxa!

I've pushed your fix for corrupted or malicious change cipher spec messages along with a debug message for that case to our embedded SSL repo.  I've also added protection against multiple in-place decryptions of the same record for malicious or corrupted messages.  Thanks again for the help and patch.  What are you working on if you don't mind me asking?



Re: crash on malformed cipher change spec message

You can see the commit here: … ec657b859e


Re: crash on malformed cipher change spec message

Awesome, thanks wink

> What are you working on if you don't mind me asking?
that's the frontend server for Badoo Mobile Application: