June 2, 2014

CVE-2014-1361: SecureTransport buffer overflow


Today, Apple released a fix to CVE-2014-1361 in SecureTransport. The essence of this bug is this: the TLS record parser would interpret a DTLS record even when using normal TLS, causing a buffer overflow when parsing a record header. I reported this issue to Apple on May 28th.

To summarize, the impact of this bug is small: it can disclose 2 specific bytes of plain text to an attacker. Doing this will also cause the connection to be closed. It can also give an attacker the ability of carrying out a replay attack, with a probability of success of 2-16 (~0.0015%).

TLS vs DTLS

DTLS and TLS send their payloads in separate records of up to 2^14 bytes, where each record has a header. For TLS this header is 5 bytes: 1 byte payload type, 2 bytes TLS version number and 2 bytes indicating length of the rest of the record.

(Aside: Why every record includes two extra bytes to include the version is not exactly clear to me. I haven’t ever seen it legitimately change except during the handshake, where the client would initiate with a TLS 1.0 record, but include that it supports up to TLS 1.2, and then switch to TLS 1.2 after the server replies using that version.)

DTLS records are similar, but these are 13 bytes instead: in between the version number and the length it includes a sequence counter. Contrary to TLS, DTLS was designed to use datagrams (like UDP), so it doesn’t require reliable or in-order delivery. To still be able to decrypt records and know their intended order, the sequence counter is included on every record. TLS also uses a sequence counter (to prevent attackers from reordering messages), but it is implicit. Both parties simply count how many messages they have received or sent.

Record parsing in SecureTransport

This is how Apple’s code used to parse these records:

static int SSLRecordReadInternal(SSLRecordContextRef ref, SSLRecord *rec)
{   int        err;
    size_t          len, contentLen;
    uint8_t           *charPtr;
    SSLBuffer       readData, cipherFragment;
    size_t          head=5;
    int             skipit=0;
    struct SSLRecordInternalContext *ctx = ref;

    if(ctx->isDTLS)
        head+=8;

    if (!ctx->partialReadBuffer.data || ctx->partialReadBuffer.length < head)
    {   if (ctx->partialReadBuffer.data)
        if ((err = SSLFreeBuffer(&ctx->partialReadBuffer)) != 0)
        {
            return err;
        }
        if ((err = SSLAllocBuffer(&ctx->partialReadBuffer,
                                  DEFAULT_BUFFER_SIZE)) != 0)
        {
            return err;
        }
    }

    if (ctx->negProtocolVersion == SSL_Version_Undetermined) {
        if (ctx->amountRead < 1)
        {   readData.length = 1 - ctx->amountRead;
            readData.data = ctx->partialReadBuffer.data + ctx->amountRead;
            len = readData.length;
            err = sslIoRead(readData, &len, ctx);
            if(err != 0)
            {   if (err == errSSLRecordWouldBlock) {
                ctx->amountRead += len;
                return err;
            }
            else {
                /* abort */
                err = errSSLRecordClosedAbort;
#if 0 // TODO: revisit this in the transport layer
                if((ctx->protocolSide == kSSLClientSide) &&
                   (ctx->amountRead == 0) &&
                   (len == 0)) {
                    /*
                     * Detect "server refused to even try to negotiate"
                     * error, when the server drops the connection before
                     * sending a single byte.
                     */
                    switch(ctx->state) {
                        case SSL_HdskStateServerHello:
                            sslHdskStateDebug("Server dropped initial connection\n");
                            err = errSSLConnectionRefused;
                            break;
                        default:
                            break;
                    }
                }
#endif
                return err;
            }
            }
            ctx->amountRead += len;
        }
    }

    if (ctx->amountRead < head)
    {   readData.length = head - ctx->amountRead;
        readData.data = ctx->partialReadBuffer.data + ctx->amountRead;
        len = readData.length;
        err = sslIoRead(readData, &len, ctx);
        if(err != 0)
        {
            switch(err) {
                case errSSLRecordWouldBlock:
                    ctx->amountRead += len;
                    break;
#if SSL_ALLOW_UNNOTICED_DISCONNECT
                case errSSLClosedGraceful:
                    /* legal if we're on record boundary and we've gotten past
                     * the handshake */
                    if((ctx->amountRead == 0) &&                /* nothing pending */
                       (len == 0) &&                            /* nothing new */
                       (ctx->state == SSL_HdskStateClientReady)) {  /* handshake done */
                        /*
                         * This means that the server has disconnected without
                         * sending a closure alert notice. This is technically
                         * illegal per the SSL3 spec, but about half of the
                         * servers out there do it, so we report it as a separate
                         * error which most clients - including (currently)
                         * URLAccess - ignore by treating it the same as
                         * a errSSLClosedGraceful error. Paranoid
                         * clients can detect it and handle it however they
                         * want to.
                         */
                        SSLChangeHdskState(ctx, SSL_HdskStateNoNotifyClose);
                        err = errSSLClosedNoNotify;
                        break;
                    }
                    else {
                        /* illegal disconnect */
                        err = errSSLClosedAbort;
                        /* and drop thru to default: fatal alert */
                    }
#endif  /* SSL_ALLOW_UNNOTICED_DISCONNECT */
                default:
                    break;
            }
            return err;
        }
        ctx->amountRead += len;
    }

    check(ctx->amountRead >= head);

    charPtr = ctx->partialReadBuffer.data;
    rec->contentType = *charPtr++;
    if (rec->contentType < SSL_RecordTypeV3_Smallest ||
        rec->contentType > SSL_RecordTypeV3_Largest)
        return errSSLRecordProtocol;

    rec->protocolVersion = (SSLProtocolVersion)SSLDecodeInt(charPtr, 2);
    charPtr += 2;

    if(rec->protocolVersion == DTLS_Version_1_0)
    {
        sslUint64 seqNum;
        SSLDecodeUInt64(charPtr, 8, &seqNum);
        charPtr += 8;
        sslLogRecordIo("Read DTLS Record %016llx (seq is: %016llx)",
                       seqNum, ctx->readCipher.sequenceNum);

        /* if the epoch of the record is different of current read cipher, just drop it */
        if((seqNum>>48)!=(ctx->readCipher.sequenceNum>>48)) {
            skipit=1;
        } else {
            ctx->readCipher.sequenceNum=seqNum;
        }
    }

    contentLen = SSLDecodeInt(charPtr, 2);
    charPtr += 2;
    if (contentLen > (16384 + 2048))    /* Maximum legal length of an
                                         * SSLCipherText payload */
    {
        return errSSLRecordRecordOverflow;
    }

    if (ctx->partialReadBuffer.length < head + contentLen)
    {   if ((err = SSLReallocBuffer(&ctx->partialReadBuffer, head + contentLen)) != 0)
    {
        return err;
    }
    }

    if (ctx->amountRead < head + contentLen)
    {   readData.length = head + contentLen - ctx->amountRead;
        readData.data = ctx->partialReadBuffer.data + ctx->amountRead;
        len = readData.length;
        err = sslIoRead(readData, &len, ctx);
        if(err != 0)
        {   if (err == errSSLRecordWouldBlock)
            ctx->amountRead += len;
            return err;
        }
        ctx->amountRead += len;
    }

    check(ctx->amountRead >= head + contentLen);

    cipherFragment.data = ctx->partialReadBuffer.data + head;
    cipherFragment.length = contentLen;

    ctx->amountRead = 0;        /* We've used all the data in the cache */

    /* We dont decrypt if we were told to skip this record */
    if(skipit) {
        return errSSLRecordUnexpectedRecord;
    }
    /*
     * Decrypt the payload & check the MAC, modifying the length of the
     * buffer to indicate the amount of plaintext data after adjusting
     * for the block size and removing the MAC */
    check(ctx->sslTslCalls != NULL);
    if ((err = ctx->sslTslCalls->decryptRecord(rec->contentType,
                                               &cipherFragment, ctx)) != 0)
        return err;

    /*
     * We appear to have sucessfully received a record; increment the
     * sequence number
     */
    IncrementUInt64(&ctx->readCipher.sequenceNum);

    /* Allocate a buffer to return the plaintext in and return it */
    if ((err = SSLAllocBuffer(&rec->contents, cipherFragment.length)) != 0)
    {
        return err;
    }
    memcpy(rec->contents.data, cipherFragment.data, cipherFragment.length);


    return 0;
}

head determines how many bytes the header should contain. charPtr points to the current position in the record. rec is a structure describing the record we’re parsing. ctx is the session context.

Line 195 correctly uses ctx->isDTLS, but line 309 uses rec->protocolVersion, which got parsed on line 306. This is data that just came from the network and has not been validated in any way. There are no checks to make sure rec->protocolVersion == DTLS_Version_1_0 is only true when ctx->isDTLS.

This means that an attacker can change the version number on a single record from a TLS version to DTLS 1.0 to make a user execute the if block on line 309, even though they are using a TLS connection. That might make it possible to modify the sequence counter.

Reordering attacks

The sequence counter in TLS is used to make it impossible for an attacker to remove messages, reorder messages or replay previous messages. The sequence counter is included in the MAC, which means the message will not validate when it isn’t in its original place in the sequence. Due to the bug in the code above, the attacker may be able to modify this sequence counter. What an attacker can do with that is hard to determine: it depends a lot on the exact fragmentation of the payload into records.

In HTTPS, for example, an attacker may try to make some JavaScript execute differently, but if the entire script fits in one record then there’s not much an attacker could do. The most efficient way to send webpages or scripts would be to make as few records as possible, as padding and MAC add overhead per record. This means fragmenting the data every 2^14 bytes = 16 KiB (except for a bit of room for the MAC). By comparison, the current version of jquery is 82 KiB. That would fit in 6 records, giving any attacker very few options to shuffle those fragments around, many of these will probably not even parse as valid JavaScript.

In more real-time protocols like IRC or XMPP (yes, of course I have to bring up XMPP again), the fragmentation is a lot easier to understand: these will include a few complete protocol packets within each record (often just 1). Having a malicious impact here will be a lot easier: an attacker would be able to drop a single chat message, retransmit one, reorder them, etc.

Rewriting the sequence number

Trying to exploit this, I quickly ran into the following problem: only 5 bytes of the record had been copied from the socket, so the SSLDecodeUInt64 call will read 2 bytes from the record, but 6 bytes past that too. This does makes it possible to make sure the epoch matches (the two highest bytes of the sequence number), but the 6 next bytes are “random” data.

Looking a little closer, the next 6 bytes didn’t turn out to be random at all. The buffer records are read into gets reused (except when a record has too much payload to fit in the current buffer, then a new one is allocated) and decryption of the record happens in-place in this buffer. So when I tried to exploit this using a HTTPS server which had previously sent a reply starting with HTTP/1.1 200 OK, Safari ended up interpreting HTTP/1 as the sequence number. The length field of the record should follow the sequence number, so it interpreted .1 as its length.

TLS 1.0

I tried a lot of variations, setting up some plaintext in the buffer first and then trying to reinterpret that as the sequence counter, until finally I realized what I was trying to do wasn’t possible with TLS 1.0: all the ciphers I was trying used more inter-record state than just the sequence counter. CBC mode means the decryption of every record depends on the ciphertext of the previous record, so reordering would never work. RC4 keystreams are also inherently statefull. As TLS uses MAC-then-Encrypt (MtE), these records will decrypt to gibberish and then fail the MAC. If TLS had used Encrypt-then-MAC (EtM) here (which a lot of cryptographers nowadays consider the better choice), the MAC would have succeeded, after which the record would have decrypted to gibberish. That gibberish would’ve been passed to the application, as the TLS layer would not have been able to detect anything wrong with it.

TLS 1.1+

TLS 1.1 and TLS 1.2 don’t have that problem: these add an explicit IV to every record to prevent attacks like BEAST. For compatibility with TLS 1.0, this is usually implemented by prepending a block of random data to the plaintext and including that in the encryption. The IV that is used to encrypt this new first block doesn’t matter: it only influences the plaintext of the first block, which is deleted by the receiver after decryption. It doesn’t even need to be the case that the receiver decrypts the first block to the same thing as the sender used. So here every record can be decrypted independently, even when inserted at a random other position in the sequence. In practice, the IV that is used as the IV for the first block is often still the ciphertext of the last block of the previous record, as that makes it easier to be compatible with TLS 1.0 while not being vulnerable to BEAST.

However, this also meant that the sequence number was no longer the ASCII encoding of HTTP/1 (or the first 6 bytes of whatever record was last), but it is now the decryption of the IV block. As this block gets chosen randomly and the server and client don’t even need to decrypt it to the same thing, trying to influence this block to contain just the sequence number I want turned out to be impossible.

My next thought would be to send a record with a wrong epoch first, which would be used to fill the buffer with the data I need and then send another record with a DTLS header that would be used to overwrite the sequence counter. In DTLS, the epoch is indicated by the two upper bytes of the sequence counter. Records with an epoch different from the epoch of the current sequence counter are skipped (decryption or authentication isn’t attempted).

However, this just moved the problem backwards: the length of this new record is still taken from the data still in the buffer, so the decrypted IV of the previous record. Even though this new record will not be decrypted, SecureTransport must read it completely first, and I don’t know what length it expects. Guessing would have a 1 in 2^16 chance of succeeding, which is large cryptographically speaking, but not quite practical. It might be possible to increase this chance by repeating the inserted record over and over, but then the attacker can only insert one record, as the next copy will fail to decrypt.

AES-GCM

I believe AES-GCM would be vulnerable to this, as it uses the sequence number as an implicit IV, though I haven’t checked. While SecureTransport has an (at least partial) implementation of AES-GCM, it wasn’t advertised by Safari, so I’m assuming its unfinished.

Leaking bytes

Another avenue of exploitation would be to try to retrieve some information about the plaintext still in the buffer. As I mentioned in my HTTPS example .1 from HTTP/1.1 200 OK was interpreted as the length of the next record. The ASCII representation of .1 interpreted as a number gives 11825. This means SecureTransport will try to read 11825 more bytes before starting to decrypt it (which will then fail the MAC, causing it to send an alert and close the connection). We can also do this the other way around: we write bytes one by one until SecureTransport closes the connection and from that we will know the 7th and 8th byte of the plaintext of the previous record!

However, the value of the two bytes has to be less than the maximum record size of 214 (while it can be up to 216), as otherwise SecureTransport will reject it for being too large. This means that the first character must have an ASCII representation of less than @, which means it can’t be any of the upper- or lowercase letters, but numbers and a few other punctuation characters would work.

Closing thoughts

After heartbleed, this is another bug that exploits a DTLS code path that should never be used when using TLS. Impact is even similar too: disclosing some contents of the other side’s memory. However, this is only limited to 2 bytes, while heartbleed could retrieve 64 KiB per heartbeat. I guess DTLS has its uses, but maybe implementors should consider whether covering both DTLS and TLS in one library is worth the extra complexity of security-critical code.

A discovery that surprised me is the way SecureTransport deals with its internal buffers. The buffer records are read into and where the result of their decryption is stored are never erased, there’s only malloc and free. Buffers grow when they need to receive a larger record, but they never decrease in size again for as long as the connection is open. This means long-lived TLS connections waste a lot of memory when they receive a single large record. The plaintext of that record will stay in memory for as long as the connection is open.