Project

General

Profile

Actions

Bug #3774

closed

Assert failed in TLS due to integer underflow

Added by Philippe Antoine over 4 years ago. Updated almost 4 years ago.

Status:
Closed
Priority:
Normal
Target version:
Affected Versions:
Effort:
Difficulty:
Label:
Needs backport

Description

Found by oss-fuzz :
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=22564

Stack trace is :

    #8 0x51f4b0 in SSLv3ParseHandshakeType suricata/src/app-layer-ssl.c:1666:9
    #9 0x51e780 in SSLv3ParseHandshakeProtocol suricata/src/app-layer-ssl.c:1736:18
    #10 0x51cef6 in SSLv3Decode suricata/src/app-layer-ssl.c:2421:26
    #11 0x51b6b7 in SSLDecode suricata/src/app-layer-ssl.c:2608:26
    #12 0x51a9d1 in SSLParseServerRecord suricata/src/app-layer-ssl.c:2658:12
    #13 0x50da1a in AppLayerParserParse suricata/src/app-layer-parser.c:1228:30
    #14 0x4c3860 in LLVMFuzzerTestOneInput suricata/src/tests/fuzz/fuzz_applayerparserparse.c:146:16


Files


Related issues 2 (0 open2 closed)

Copied to Suricata - Bug #3791: Assert failed in TLS due to integer underflowClosedShivani BhardwajActions
Copied to Suricata - Bug #3792: Assert failed in TLS due to integer underflowClosedJeff LucovskyActions
Actions #1

Updated by Philippe Antoine over 4 years ago

Bug analysis :
In SSLv3ParseHandshakeType, we use the condition

if (ssl_state->curr_connp->message_start + ssl_state->curr_connp->message_length <
            ssl_state->curr_connp->bytes_processed + input_len)

before assigning
write_len = (ssl_state->curr_connp->message_start + ssl_state->curr_connp->message_length) -
            ssl_state->curr_connp->bytes_processed;

But with this case, we end up having
  • ssl_state->curr_connp->message_length = 9
  • ssl_state->curr_connp->bytes_processed = 10
    Leading to bad write_len = 0xFFFFFFFF

We seem to need the 5 packets of the reproducer to get this :

First packet (from server to client) has 9 bytes payload
After this packet, we have
  • ssl_state->curr_connp->message_length = 9
  • ssl_state->curr_connp->bytes_processed = 9

Second packet is from client to server to progress handshake somehow

Third packet is from server
It does not reach SSLv3ParseHandshakeType because of SSLv3Decode lines get into the break case :
    if (ssl_state->flags & SSL_AL_FLAG_CHANGE_CIPHER_SPEC) {
                /* In TLSv1.3, ChangeCipherSpec is only used for middlebox
                   compability (rfc8446, appendix D.4). */
                if ((ssl_state->client_connp.version > TLS_VERSION_12) &&
                       ((ssl_state->flags & SSL_AL_FLAG_STATE_SERVER_HELLO) == 0)) {
                    /* do nothing */
                } else {
                    break;
                }
            }

After this packet, we get
  • ssl_state->curr_connp->message_length = 9
  • ssl_state->curr_connp->bytes_processed = 10

Fourth packet is from the client and modifies ssl_state->client_connp.version so as to ssl_state->client_connp.version > TLS_VERSION_12 becomes true

Fifth and last packet from server triggers the integer underflow

Actions #2

Updated by Philippe Antoine over 4 years ago

  • Status changed from New to In Review
  • Assignee set to Philippe Antoine

PR in private Gitlab

Actions #3

Updated by Philippe Antoine over 4 years ago

  • Target version set to 6.0.0beta1
Actions #4

Updated by Jeff Lucovsky over 4 years ago

  • Copied to Bug #3791: Assert failed in TLS due to integer underflow added
Actions #5

Updated by Jeff Lucovsky over 4 years ago

  • Copied to Bug #3792: Assert failed in TLS due to integer underflow added
Actions #6

Updated by Victor Julien over 4 years ago

  • Subject changed from Assert failed in TLS dur to integer underflow to Assert failed in TLS due to integer underflow
Actions #8

Updated by Victor Julien about 4 years ago

  • Target version changed from 6.0.0beta1 to 6.0.0rc1
Actions #9

Updated by Victor Julien about 4 years ago

  • Target version changed from 6.0.0rc1 to 6.0.0
Actions #10

Updated by Victor Julien about 4 years ago

  • Status changed from In Review to Closed
Actions #11

Updated by Victor Julien almost 4 years ago

  • Private changed from Yes to No
Actions

Also available in: Atom PDF