diff --git a/src/app-layer-ssl.c b/src/app-layer-ssl.c index 6df3ada261..0ff0b080d4 100644 --- a/src/app-layer-ssl.c +++ b/src/app-layer-ssl.c @@ -70,24 +70,20 @@ static SCEnumCharMap tls_state_client_table[] = { static SCEnumCharMap tls_state_server_table[] = { { - "server_in_progress", - TLS_STATE_SERVER_IN_PROGRESS, + "server_started", + TLS_STATE_SERVER_STARTED, }, { "server_hello", TLS_STATE_SERVER_HELLO, }, { - "server_cert_done", - TLS_STATE_SERVER_CERT_DONE, + "server_cert", + TLS_STATE_SERVER_CERT, }, { - "server_hello_done", - TLS_STATE_SERVER_HELLO_DONE, - }, - { - "server_handshake_done", - TLS_STATE_SERVER_HANDSHAKE_DONE, + "server_data", + TLS_STATE_SERVER_DATA, }, { "server_finished", @@ -440,19 +436,15 @@ static void TlsDecodeHSCertificateErrSetEvent(SSLState *ssl_state, uint32_t err) } static inline int TlsDecodeHSCertificateFingerprint( - SSLStateConnp *connp, const uint8_t *input, uint32_t cert_len) + char **out, const uint8_t *input, uint32_t cert_len) { - if (unlikely(connp->cert0_fingerprint != NULL)) - return 0; - - connp->cert0_fingerprint = SCCalloc(1, SHA1_STRING_LENGTH); - if (connp->cert0_fingerprint == NULL) + *out = SCCalloc(1, SHA1_STRING_LENGTH); + if (*out == NULL) return -1; uint8_t hash[SC_SHA1_LEN]; if (SCSha1HashBuffer(input, cert_len, hash, sizeof(hash)) == 1) { - SCToHex_sep( - (uint8_t *)connp->cert0_fingerprint, SHA1_STRING_LENGTH, ':', hash, SC_SHA1_LEN); + SCToHex_sep((uint8_t *)*out, SHA1_STRING_LENGTH, ':', hash, SC_SHA1_LEN); } return 0; } @@ -472,13 +464,23 @@ static inline int TlsDecodeHSCertificateAddCertToChain( } static int TlsDecodeHSCertificate(SSLState *ssl_state, SSLStateConnp *connp, - const uint8_t *const initial_input, const uint32_t input_len, const int certn) + const uint8_t *const initial_input, const uint32_t input_len, const int certn, + bool *extract_event_sent) { const uint8_t *input = (uint8_t *)initial_input; uint32_t err_code = 0; X509 *x509 = NULL; int rc = 0; + /* stashed cert0 extraction: published to connp only when the whole + * certificate decodes (see the cert0 fill below) */ + uint8_t *st_subject = NULL, *st_issuer = NULL, *st_serial = NULL; + SSLSubjectAltName *st_sans = NULL; + char *st_fingerprint = NULL; + uint32_t st_subject_len = 0, st_issuer_len = 0, st_serial_len = 0; + uint16_t st_sans_num = 0; + int64_t st_not_before = 0, st_not_after = 0; + if (!(HAS_SPACE(3))) goto invalid_cert; @@ -493,74 +495,121 @@ static int TlsDecodeHSCertificate(SSLState *ssl_state, SSLStateConnp *connp, connp->cert0_serial == NULL) { x509 = SCX509Decode(input, cert_len, &err_code); if (x509 == NULL) { + /* an undecodable certificate: report it once and let the + * caller skip it - the rest of the chain must still be + * decoded */ TlsDecodeHSCertificateErrSetEvent(ssl_state, err_code); - goto next; + goto fail; } - SCX509GetSubject(x509, &connp->cert0_subject, &connp->cert0_subject_len); - if (connp->cert0_subject == NULL) { + /* extract into the stashes and publish to connp->cert0_* only + * when the whole certificate yields every field: a partial fill + * would otherwise block the next certificate from completing the + * fields */ + SCX509GetSubject(x509, &st_subject, &st_subject_len); + if (st_subject == NULL) { err_code = ERR_EXTRACT_SUBJECT; goto error; } - SCX509GetIssuer(x509, &connp->cert0_issuerdn, &connp->cert0_issuerdn_len); - if (connp->cert0_issuerdn == NULL) { + SCX509GetIssuer(x509, &st_issuer, &st_issuer_len); + if (st_issuer == NULL) { err_code = ERR_EXTRACT_ISSUER; goto error; } - connp->cert0_sans_num = SCX509GetSubjectAltNameLen(x509); - if (connp->cert0_sans_num == TLS_MAX_SAN) { + st_sans_num = SCX509GetSubjectAltNameLen(x509); + if (st_sans_num == TLS_MAX_SAN && !*extract_event_sent) { SSLSetEvent(ssl_state, TLS_DECODER_EVENT_TOO_MANY_SUBJECT_ALTERNATIVE_NAMES); + *extract_event_sent = true; } - connp->cert0_sans = SCCalloc(connp->cert0_sans_num, sizeof(SSLSubjectAltName)); - if (connp->cert0_sans == NULL) { - connp->cert0_sans_num = 0; - goto error; - } - for (uint16_t i = 0; i < connp->cert0_sans_num; i++) { - SCX509GetSubjectAltNameAt( - x509, i, &connp->cert0_sans[i].san, &connp->cert0_sans[i].san_len); + if (st_sans_num > 0) { + st_sans = SCCalloc(st_sans_num, sizeof(SSLSubjectAltName)); + if (st_sans == NULL) { + st_sans_num = 0; + goto error; + } + for (uint16_t i = 0; i < st_sans_num; i++) { + SCX509GetSubjectAltNameAt(x509, i, &st_sans[i].san, &st_sans[i].san_len); + } } - SCX509GetSerial(x509, &connp->cert0_serial, &connp->cert0_serial_len); - if (connp->cert0_serial == NULL) { + SCX509GetSerial(x509, &st_serial, &st_serial_len); + if (st_serial == NULL) { err_code = ERR_INVALID_SERIAL; goto error; } - rc = SCX509GetValidity(x509, &connp->cert0_not_before, &connp->cert0_not_after); + rc = SCX509GetValidity(x509, &st_not_before, &st_not_after); if (rc != 0) { err_code = ERR_EXTRACT_VALIDITY; goto error; } - SCX509Free(x509); - x509 = NULL; - - rc = TlsDecodeHSCertificateFingerprint(connp, input, cert_len); + rc = TlsDecodeHSCertificateFingerprint(&st_fingerprint, input, cert_len); if (rc != 0) { SCLogDebug("TlsDecodeHSCertificateFingerprint failed with %d", rc); goto error; } + + /* the certificate yielded every field: publish it */ + connp->cert0_subject = st_subject; + connp->cert0_subject_len = st_subject_len; + connp->cert0_issuerdn = st_issuer; + connp->cert0_issuerdn_len = st_issuer_len; + connp->cert0_sans = st_sans; + connp->cert0_sans_num = st_sans_num; + connp->cert0_serial = st_serial; + connp->cert0_serial_len = st_serial_len; + connp->cert0_not_before = st_not_before; + connp->cert0_not_after = st_not_after; + connp->cert0_fingerprint = st_fingerprint; + st_subject = st_issuer = st_serial = NULL; + st_sans = NULL; + st_fingerprint = NULL; + + SCX509Free(x509); + x509 = NULL; } + /* every certificate of the chain is kept, also for certn > 0 */ rc = TlsDecodeHSCertificateAddCertToChain(connp, input, cert_len); if (rc != 0) { SCLogDebug("TlsDecodeHSCertificateAddCertToChain failed with %d", rc); goto error; } -next: input += cert_len; return (int)(input - initial_input); error: - if (err_code != 0) + if (err_code != 0 && !*extract_event_sent) { + /* the message no longer aborts at the first failing certificate: + * report the first extraction failure once */ TlsDecodeHSCertificateErrSetEvent(ssl_state, err_code); + *extract_event_sent = true; + } + /* the certificate failed before the publish block: nothing was + * published, release the stashed extraction so the next certificate + * can fill cert0 */ + if (st_subject != NULL) + SCX509ArrayFree(st_subject, st_subject_len); + if (st_issuer != NULL) + SCX509ArrayFree(st_issuer, st_issuer_len); + if (st_serial != NULL) + SCX509ArrayFree(st_serial, st_serial_len); + if (st_fingerprint != NULL) + SCFree(st_fingerprint); + if (st_sans != NULL) { + for (uint16_t i = 0; i < st_sans_num; i++) + SCX509ArrayFree(st_sans[i].san, st_sans[i].san_len); + SCFree(st_sans); + } if (x509 != NULL) SCX509Free(x509); + return -1; +fail: return -1; invalid_cert: @@ -589,10 +638,17 @@ static int TlsDecodeHSCertificates(SSLState *ssl_state, SSLStateConnp *connp, return -1; if (connp->certs_buffer != NULL) { - // TODO should we set an event here? + /* one-shot buffer: a second certificate message must not + * overwrite or invalidate the data of the first one (base + * semantics, renegotiation re-sends the same certificate) */ return -1; } + /* this connp's certificate message: until it decodes fully the + * certificate data is incomplete - only a fully decoded certificate + * message completes it, never a later handshake message */ + connp->cert_data_incomplete = true; + connp->certs_buffer = SCCalloc(1, cert_chain_len); if (connp->certs_buffer == NULL) { return -1; @@ -602,21 +658,42 @@ static int TlsDecodeHSCertificates(SSLState *ssl_state, SSLStateConnp *connp, int cert_cnt = 0; uint32_t processed_len = 0; + bool cert_failed = false; + bool extract_event_sent = false; /* coverity[tainted_data] */ while (processed_len < cert_chain_len) { int rc = TlsDecodeHSCertificate(ssl_state, connp, connp->certs_buffer + processed_len, - connp->certs_buffer_size - processed_len, cert_cnt); - if (rc <= 0) { // 0 should be impossible, but lets be defensive - return -1; - } - DEBUG_VALIDATE_BUG_ON(processed_len + (uint32_t)rc > cert_chain_len); - if (processed_len + (uint32_t)rc > cert_chain_len) { - return -1; + connp->certs_buffer_size - processed_len, cert_cnt, &extract_event_sent); + if (rc > 0) { + DEBUG_VALIDATE_BUG_ON(processed_len + (uint32_t)rc > cert_chain_len); + if (processed_len + (uint32_t)rc > cert_chain_len) { + return -1; + } + processed_len += (uint32_t)rc; + continue; } - processed_len += (uint32_t)rc; + /* a certificate failed to decode: skip its bytes so the rest + * of the chain is still decoded; an invalid length aborts the + * message */ + const uint8_t *cert = connp->certs_buffer + processed_len; + if (processed_len + 3 > cert_chain_len) + return -1; + const uint32_t skip = cert[0] << 16 | cert[1] << 8 | cert[2]; + if (processed_len + 3 + skip > cert_chain_len) + return -1; + processed_len += 3 + skip; + cert_failed = true; } + if (cert_failed) { + /* the message contains an undecodable certificate: the phase + * data is incomplete, so the caller suppresses the cert phase; + * the decoded rest of the chain is kept */ + return -1; + } + connp->cert_data_incomplete = false; + connp->cert_chain_final = true; return processed_len + 3; } @@ -1490,15 +1567,19 @@ static int TLSDecodeHandshakeHello(SSLState *ssl_state, int ret; uint32_t parsed = 0; + /* a failed sub-decode must not fail the parse: the message bytes are + * consumed either way. The phase advance that follows relies on a + * fully parsed message, so suppress it via the connp flag instead. */ + ret = TLSDecodeHSHelloVersion(ssl_state, input, input_len); if (ret < 0) - goto end; + goto fail; parsed += ret; ret = TLSDecodeHSHelloRandom(ssl_state, input + parsed, input_len - parsed); if (ret < 0) - goto end; + goto fail; parsed += ret; @@ -1510,7 +1591,7 @@ static int TLSDecodeHandshakeHello(SSLState *ssl_state, ret = TLSDecodeHSHelloSessionID(ssl_state, input + parsed, input_len - parsed); if (ret < 0) - goto end; + goto fail; parsed += ret; } @@ -1518,7 +1599,7 @@ static int TLSDecodeHandshakeHello(SSLState *ssl_state, ret = TLSDecodeHSHelloCipherSuites(ssl_state, input + parsed, input_len - parsed); if (ret < 0) - goto end; + goto fail; parsed += ret; @@ -1530,7 +1611,7 @@ static int TLSDecodeHandshakeHello(SSLState *ssl_state, ret = TLSDecodeHSHelloCompressionMethods(ssl_state, input + parsed, input_len - parsed); if (ret < 0) - goto end; + goto fail; parsed += ret; } @@ -1538,16 +1619,15 @@ static int TLSDecodeHandshakeHello(SSLState *ssl_state, ret = TLSDecodeHSHelloExtensions(ssl_state, input + parsed, input_len - parsed); if (ret < 0) - goto end; + goto fail; if (SC_ATOMIC_GET(ssl_config.enable_ja3) && ssl_state->curr_connp->ja3_hash == NULL) { ssl_state->curr_connp->ja3_hash = Ja3GenerateHash(ssl_state->curr_connp->ja3_str); } + return 0; - if (ssl_state->curr_connp != &ssl_state->client_connp) { - UpdateServerState(ssl_state, TLS_STATE_SERVER_HELLO); - } -end: +fail: + ssl_state->curr_connp->phase_suppressed = true; return 0; } @@ -1571,10 +1651,12 @@ static inline int SSLv3ParseHandshakeTypeCertificate(SSLState *ssl_state, SSLSta } else if (rc < 0) { SCLogDebug("error parsing cert, reset state"); SSLParserHSReset(connp); - /* fall through to still consume the cert bytes */ - } - if (connp != &ssl_state->client_connp) { - UpdateServerState(ssl_state, TLS_STATE_SERVER_CERT_DONE); + /* the certificate data is incomplete (an undecodable certificate); + * the track is already in the certificate phase (entered at the + * message header), so nothing to suppress here. cert_data_incomplete + * only gates the handshake-done shortcut at the end of SSLDecode + * (the cert-fields based tail), not the record-level data-phase + * advance (app data and CCS records advance the track regardless) */ } return input_len; } @@ -1647,9 +1729,20 @@ static int SSLv3ParseHandshakeType(SSLState *ssl_state, const uint8_t *input, case SSLV3_HS_CLIENT_KEY_EXCHANGE: ssl_state->current_flags = SSL_AL_FLAG_STATE_CLIENT_KEYX; + /* a client flight message: the server flight is over, no + * CertificateRequest can still arrive. Only the client's own + * message settles it - the parser accepts the type from either + * direction. */ + if (!direction) + ssl_state->current_flags |= SSL_AL_FLAG_SERVER_FLIGHT_DECIDED; break; case SSLV3_HS_CERTIFICATE: + if (!direction) { + /* the client certificate: its own flight (and so the + * server's CertificateRequest decision) is past */ + ssl_state->current_flags |= SSL_AL_FLAG_SERVER_FLIGHT_DECIDED; + } rc = SSLv3ParseHandshakeTypeCertificate(ssl_state, direction ? &ssl_state->server_connp : &ssl_state->client_connp, initial_input, input_len); @@ -1661,21 +1754,28 @@ static int SSLv3ParseHandshakeType(SSLState *ssl_state, const uint8_t *input, break; case SSLV3_HS_CERTIFICATE_REQUEST: if (direction) { - ssl_state->current_flags = SSL_AL_FLAG_NEED_CLIENT_CERT; + ssl_state->current_flags = + SSL_AL_FLAG_NEED_CLIENT_CERT | SSL_AL_FLAG_SERVER_FLIGHT_DECIDED; } break; case SSLV3_HS_CERTIFICATE_VERIFY: - case SSLV3_HS_FINISHED: case SSLV3_HS_CERTIFICATE_URL: case SSLV3_HS_CERTIFICATE_STATUS: break; - case SSLV3_HS_NEW_SESSION_TICKET: - SCLogDebug("new session ticket"); + case SSLV3_HS_FINISHED: + /* a finished ends its flight in either direction: the server + * flight (and any CertificateRequest) is past */ + ssl_state->current_flags |= SSL_AL_FLAG_SERVER_FLIGHT_DECIDED; break; case SSLV3_HS_SERVER_HELLO_DONE: - if (direction) { - UpdateServerState(ssl_state, TLS_STATE_SERVER_HELLO_DONE); - } + /* the server flight's last message: only the server direction + * settles it - the parser accepts the type from either + * direction */ + if (direction) + ssl_state->current_flags |= SSL_AL_FLAG_SERVER_FLIGHT_DECIDED; + break; + case SSLV3_HS_NEW_SESSION_TICKET: + SCLogDebug("new session ticket"); break; default: SSLSetEvent(ssl_state, TLS_DECODER_EVENT_INVALID_SSL_RECORD); @@ -1690,21 +1790,41 @@ static int SSLv3ParseHandshakeType(SSLState *ssl_state, const uint8_t *input, return input_len; } -/** - * \brief advance the phase state once a handshake message is fully parsed - * - * A handshake message may span several records: the started states stay - * in place until the final fragment completes the message, so the phase - * data (SNI, certificates) is available when its state is entered. - */ -static void SSLv3HandshakePhaseAdvance(SSLState *ssl_state) +/* the phase of a message is entered when the message starts, so all of its + * fragments belong to it (HTTP1: request_line starts at the first byte of the + * line, not when the line is complete) */ +static void SSLv3HandshakePhaseEnter(SSLState *ssl_state, const uint8_t hs_type) { + /* one message is one phase: the flag is per message, reset at its + * start (the header is read once per message) */ + ssl_state->curr_connp->phase_suppressed = false; if (ssl_state->curr_connp == &ssl_state->client_connp) { - if (ssl_state->curr_connp->handshake_type == SSLV3_HS_CLIENT_HELLO) { + if (hs_type == SSLV3_HS_CLIENT_HELLO) UpdateClientState(ssl_state, TLS_STATE_CLIENT_HELLO); - } else if (ssl_state->curr_connp->handshake_type == SSLV3_HS_CERTIFICATE) { + else if (hs_type == SSLV3_HS_CERTIFICATE) UpdateClientState(ssl_state, TLS_STATE_CLIENT_CERT); - } + } else if (hs_type == SSLV3_HS_SERVER_HELLO) { + UpdateServerState(ssl_state, TLS_STATE_SERVER_HELLO); + } else if (hs_type == SSLV3_HS_CERTIFICATE) { + UpdateServerState(ssl_state, TLS_STATE_SERVER_CERT); + } +} + +/* a complete hello hands the track over to the certificate phase, before any + * certificate byte arrives (HTTP1: request_line -> request_headers at line + * completion). A hello that failed to decode keeps the track in the hello + * phase: its data never existed */ +static void SSLv3HandshakePhaseLeave(SSLState *ssl_state, const uint8_t hs_type) +{ + /* some handshake messages (certificates) clear the connp handshake + * type before returning, so the message type is passed in */ + if (ssl_state->curr_connp->phase_suppressed) + return; + if (ssl_state->curr_connp == &ssl_state->client_connp) { + if (hs_type == SSLV3_HS_CLIENT_HELLO) + UpdateClientState(ssl_state, TLS_STATE_CLIENT_CERT); + } else if (hs_type == SSLV3_HS_SERVER_HELLO) { + UpdateServerState(ssl_state, TLS_STATE_SERVER_CERT); } } @@ -1775,7 +1895,7 @@ static int SSLv3ParseHandshakeProtocol(SSLState *ssl_state, const uint8_t *input } SCLogDebug("retval %d", retval); - SSLv3HandshakePhaseAdvance(ssl_state); + SSLv3HandshakePhaseLeave(ssl_state, ssl_state->curr_connp->hs_buffer_message_type); /* data processed, reset buffer */ SCFree(ssl_state->curr_connp->hs_buffer); @@ -1805,6 +1925,8 @@ static int SSLv3ParseHandshakeProtocol(SSLState *ssl_state, const uint8_t *input ssl_state->curr_connp->handshake_type = input[0]; ssl_state->curr_connp->message_length = input[1] << 16 | input[2] << 8 | input[3]; + /* the fragments of this message all belong to its phase */ + SSLv3HandshakePhaseEnter(ssl_state, ssl_state->curr_connp->handshake_type); SCLogDebug("handshake_type %u message len %u input %p input_len %u", ssl_state->curr_connp->handshake_type, ssl_state->curr_connp->message_length, input, input_len); @@ -1865,14 +1987,17 @@ static int SSLv3ParseHandshakeProtocol(SSLState *ssl_state, const uint8_t *input } else { /* full record, parse it now */ + const uint8_t full_record_hs_type = ssl_state->curr_connp->handshake_type; int retval = SSLv3ParseHandshakeType( ssl_state, input, ssl_state->curr_connp->message_length, direction); if (retval < 0 || retval > (int)input_len) { DEBUG_VALIDATE_BUG_ON(retval > (int)input_len); + if (retval < 0) + SSLParserHSReset(ssl_state->curr_connp); return (retval); } SCLogDebug("retval %d input_len %u", retval, input_len); - SSLv3HandshakePhaseAdvance(ssl_state); + SSLv3HandshakePhaseLeave(ssl_state, full_record_hs_type); input += retval; input_len -= retval; @@ -2327,8 +2452,11 @@ static struct SSLDecoderResult SSLv2Decode(uint8_t direction, SSLState *ssl_stat "direction!"); } else { ssl_state->current_flags = SSL_AL_FLAG_STATE_CLIENT_KEYX; + /* a v2 client certificate message names the client's + * certificate phase, even if the v2 certificate payload is + * not extracted */ + UpdateClientState(ssl_state, TLS_STATE_CLIENT_CERT); } - UpdateServerState(ssl_state, TLS_STATE_SERVER_CERT_DONE); /* fall through */ case SSLV2_MT_SERVER_VERIFY: @@ -2588,7 +2716,7 @@ static struct SSLDecoderResult SSLv3Decode(uint8_t direction, SSLState *ssl_stat if (ssl_state->curr_connp == &ssl_state->client_connp) { UpdateClientState(ssl_state, TLS_STATE_CLIENT_DATA); } else { - UpdateServerState(ssl_state, TLS_STATE_SERVER_HANDSHAKE_DONE); + UpdateServerState(ssl_state, TLS_STATE_SERVER_DATA); } if (ssl_config.encrypt_mode != SSL_CNF_ENC_HANDLE_FULL) { @@ -2821,17 +2949,32 @@ static AppLayerResult SSLDecode(Flow *f, uint8_t direction, void *alstate, counter++; } /* while (input_len) */ - /* mark handshake as done if we have subject and issuer */ + /* mark handshake as done if we have subject and issuer; the + * certificate data must be complete - a certificate message whose + * decode failed (even with the rest of the chain decoded) leaves the + * data incomplete until a certificate message decodes fully, so the + * tracks must not cross the cert phase on partial data */ if ((ssl_state->flags & SSL_AL_FLAG_NEED_CLIENT_CERT) && + !ssl_state->client_connp.cert_data_incomplete && ssl_state->client_connp.cert0_subject && ssl_state->client_connp.cert0_issuerdn) { - /* update both sides to keep existing behavior */ - UpdateClientState(ssl_state, TLS_STATE_CLIENT_DATA); - UpdateServerState(ssl_state, TLS_STATE_SERVER_HANDSHAKE_DONE); - } else if ((ssl_state->flags & SSL_AL_FLAG_NEED_CLIENT_CERT) == 0 && - ssl_state->server_connp.cert0_subject && ssl_state->server_connp.cert0_issuerdn) { - /* update both sides to keep existing behavior */ + /* per track: a track's own certificate data decides its own track; + * pushing the peer track unconditionally would jump it over the + * certificate phase before its own certificate is parsed (and + * progression is monotonic, so the phase could never be visited + * afterwards) */ UpdateClientState(ssl_state, TLS_STATE_CLIENT_DATA); - UpdateServerState(ssl_state, TLS_STATE_SERVER_HANDSHAKE_DONE); + /* an mTLS handshake is complete with the client certificate: by + * the flight order the server certificate phase was entered + * already, so advancing the server track skips nothing (matters + * when its own chain decode failed and no server app data + * follows) */ + if (ssl_state->server_state >= TLS_STATE_SERVER_CERT) { + UpdateServerState(ssl_state, TLS_STATE_SERVER_DATA); + } + } + if (!ssl_state->server_connp.cert_data_incomplete && ssl_state->server_connp.cert0_subject && + ssl_state->server_connp.cert0_issuerdn) { + UpdateServerState(ssl_state, TLS_STATE_SERVER_DATA); } /* flag session as finished if APP_LAYER_PARSER_EOF is set */ @@ -2845,6 +2988,62 @@ static AppLayerResult SSLDecode(Flow *f, uint8_t direction, void *alstate, return APP_LAYER_OK; } +/* tls event content readiness: everything the logger may emit must be + * final. cert0 fields are final as soon as any certificate decoded + * into the slot - a later undecodable one in the same message does not + * invalidate them (base parity: the old completion teleport logged on + * fields alone). Where fields stay empty (resumption, a fully-garbage + * chain), the track advance beyond the certificate phase makes the + * data final; a track stuck below it can only end at close, where the + * end-of-file pass logs. The cert_data_incomplete flag is not needed + * here: it only gates phase advancement, and loggers run between + * parses, never during one. The client certificate decision must also + * be settled: the standard flight order puts the CertificateRequest + * after the server Certificate, so server content readiness alone can + * precede it - logging there would miss the client certificate + * content and the decoder events a client flight may still raise (the + * TLS tx logs once; base logged at server flight end, where both were + * included). The decision is settled by the NEED_CLIENT_CERT flag, by + * any message at or past the CertificateRequest position (server + * flight: CertificateRequest, ServerHelloDone, Finished; client + * flight: its Certificate or ClientKeyExchange - flag + * SERVER_FLIGHT_DECIDED), by a change cipher spec record, or by the + * client's first application data - a TLS 1.3 flight is encrypted + * past the ServerHello, so its encrypted records are the mid-flow + * settlement signal there (anything else leaves the log to close, + * the generic release through SERVER_FINISHED). The mTLS branch is + * separate and unaffected. */ +static bool TlsConnpLogReady(const SSLStateConnp *connp, const uint8_t track_done) +{ + if (connp->cert0_subject && connp->cert0_issuerdn) { + return true; + } + return track_done; +} + +bool SSLV3TxLogReady(void *tx) +{ + SSLState *ssl_state = (SSLState *)tx; + + if (!TlsConnpLogReady( + &ssl_state->server_connp, ssl_state->server_state >= TLS_STATE_SERVER_DATA)) { + return false; + } + if (ssl_state->flags & SSL_AL_FLAG_NEED_CLIENT_CERT) { + if (!TlsConnpLogReady( + &ssl_state->client_connp, ssl_state->client_state >= TLS_STATE_CLIENT_DATA)) { + return false; + } + } else if ((ssl_state->flags & + (SSL_AL_FLAG_SERVER_FLIGHT_DECIDED | SSL_AL_FLAG_CHANGE_CIPHER_SPEC)) == 0 && + ssl_state->server_state < TLS_STATE_SERVER_FINISHED && + ssl_state->client_state < TLS_STATE_CLIENT_DATA) { + /* a CertificateRequest may still arrive in this flight */ + return false; + } + return true; +} + static AppLayerResult SSLParseClientRecord(Flow *f, void *alstate, AppLayerParserState *pstate, StreamSlice stream_slice, void *local_data) { diff --git a/src/app-layer-ssl.h b/src/app-layer-ssl.h index c2f81e015e..6f51368f9a 100644 --- a/src/app-layer-ssl.h +++ b/src/app-layer-ssl.h @@ -85,11 +85,10 @@ enum TlsStateClient { }; enum TlsStateServer { - TLS_STATE_SERVER_IN_PROGRESS = 0, + TLS_STATE_SERVER_STARTED = 0, TLS_STATE_SERVER_HELLO, - TLS_STATE_SERVER_CERT_DONE, - TLS_STATE_SERVER_HELLO_DONE, - TLS_STATE_SERVER_HANDSHAKE_DONE, + TLS_STATE_SERVER_CERT, + TLS_STATE_SERVER_DATA, TLS_STATE_SERVER_FINISHED, }; @@ -140,6 +139,10 @@ enum TlsStateServer { #define TLS_TC_RANDOM_SET BIT_U32(25) #define SSL_AL_FLAG_NEED_CLIENT_CERT BIT_U32(26) +/* a message at or after the CertificateRequest position of the server + * flight was seen (or a client flight message): whether a client + * certificate is required is decided from here */ +#define SSL_AL_FLAG_SERVER_FLIGHT_DECIDED BIT_U32(27) /* config flags */ #define SSL_TLS_LOG_PEM (1 << 0) @@ -197,6 +200,23 @@ typedef struct SSLStateConnp_ { uint32_t cert0_issuerdn_len; uint8_t *cert0_serial; uint32_t cert0_serial_len; + + /* the current handshake message failed to decode: suppress the phase + * advance that follows it (the phase data does not exist); the + * message bytes are consumed and parsing continues; reset per message */ + bool phase_suppressed; + /* this connp's certificate message has not decoded fully (it failed, + * or a certificate in it failed): the certificate data is incomplete, + * so the handshake-done heuristic must not establish the data phase + * from it; set while the first certificate message is being decoded, + * cleared by a fully decoded certificate message only */ + bool cert_data_incomplete; + /* a certificate message has fully decoded at least once: the chain + * content (count included) is final from here on; until then the + * parser state may already sit in the cert phase (entered at + * ServerHello completion or a certificate header) with an + * incomplete chain */ + bool cert_chain_final; int64_t cert0_not_before; int64_t cert0_not_after; char *cert0_fingerprint; @@ -263,6 +283,7 @@ typedef struct SSLState_ { void RegisterSSLParsers(void); void SSLEnableJA3(void); bool SSLJA3IsEnabled(void); +bool SSLV3TxLogReady(void *tx); void SSLEnableJA4(void); bool SSLJA4IsEnabled(void); diff --git a/src/detect-lua.c b/src/detect-lua.c index e52d48940b..6c0c11e657 100644 --- a/src/detect-lua.c +++ b/src/detect-lua.c @@ -101,7 +101,7 @@ void DetectLuaRegister(void) g_lua_ja3s_list_id = DetectBufferTypeRegister("ja3s.lua"); DetectAppLayerInspectEngineRegister("ja3s.lua", ALPROTO_TLS, SIG_FLAG_TOCLIENT, - TLS_STATE_SERVER_HELLO_DONE, DetectEngineInspectGenericList, NULL); + TLS_STATE_SERVER_DATA, DetectEngineInspectGenericList, NULL); DetectAppLayerInspectEngineRegister( "ja3s.lua", ALPROTO_QUIC, SIG_FLAG_TOCLIENT, 1, DetectEngineInspectGenericList, NULL); diff --git a/src/detect-tls-cert-fingerprint.c b/src/detect-tls-cert-fingerprint.c index e1374755ef..ce3c738105 100644 --- a/src/detect-tls-cert-fingerprint.c +++ b/src/detect-tls-cert-fingerprint.c @@ -83,10 +83,10 @@ void DetectTlsFingerprintRegister(void) sigmatch_table[DETECT_TLS_CERT_FINGERPRINT].flags |= SIGMATCH_INFO_STICKY_BUFFER; DetectAppLayerInspectEngineRegister("tls.cert_fingerprint", ALPROTO_TLS, SIG_FLAG_TOCLIENT, - TLS_STATE_SERVER_CERT_DONE, DetectEngineInspectBufferGeneric, GetData); + TLS_STATE_SERVER_CERT, DetectEngineInspectBufferGeneric, GetData); DetectAppLayerMpmRegister("tls.cert_fingerprint", SIG_FLAG_TOCLIENT, 2, - PrefilterGenericMpmRegister, GetData, ALPROTO_TLS, TLS_STATE_SERVER_CERT_DONE); + PrefilterGenericMpmRegister, GetData, ALPROTO_TLS, TLS_STATE_SERVER_CERT); DetectAppLayerInspectEngineRegister("tls.cert_fingerprint", ALPROTO_TLS, SIG_FLAG_TOSERVER, TLS_STATE_CLIENT_CERT, DetectEngineInspectBufferGeneric, GetData); diff --git a/src/detect-tls-cert-issuer.c b/src/detect-tls-cert-issuer.c index 02c7f0bb38..4ccb4a9a67 100644 --- a/src/detect-tls-cert-issuer.c +++ b/src/detect-tls-cert-issuer.c @@ -87,10 +87,10 @@ void DetectTlsIssuerRegister(void) GetData, ALPROTO_TLS, TLS_STATE_CLIENT_CERT); DetectAppLayerInspectEngineRegister("tls.cert_issuer", ALPROTO_TLS, SIG_FLAG_TOCLIENT, - TLS_STATE_SERVER_CERT_DONE, DetectEngineInspectBufferGeneric, GetData); + TLS_STATE_SERVER_CERT, DetectEngineInspectBufferGeneric, GetData); DetectAppLayerMpmRegister("tls.cert_issuer", SIG_FLAG_TOCLIENT, 2, PrefilterGenericMpmRegister, - GetData, ALPROTO_TLS, TLS_STATE_SERVER_CERT_DONE); + GetData, ALPROTO_TLS, TLS_STATE_SERVER_CERT); DetectBufferTypeSetDescriptionByName("tls.cert_issuer", "TLS certificate issuer"); diff --git a/src/detect-tls-cert-serial.c b/src/detect-tls-cert-serial.c index 0fd9467989..da8839f3a3 100644 --- a/src/detect-tls-cert-serial.c +++ b/src/detect-tls-cert-serial.c @@ -85,10 +85,10 @@ void DetectTlsSerialRegister(void) sigmatch_table[DETECT_TLS_CERT_SERIAL].flags |= SIGMATCH_INFO_STICKY_BUFFER; DetectAppLayerInspectEngineRegister("tls.cert_serial", ALPROTO_TLS, SIG_FLAG_TOCLIENT, - TLS_STATE_SERVER_CERT_DONE, DetectEngineInspectBufferGeneric, GetData); + TLS_STATE_SERVER_CERT, DetectEngineInspectBufferGeneric, GetData); DetectAppLayerMpmRegister("tls.cert_serial", SIG_FLAG_TOCLIENT, 2, PrefilterGenericMpmRegister, - GetData, ALPROTO_TLS, TLS_STATE_SERVER_CERT_DONE); + GetData, ALPROTO_TLS, TLS_STATE_SERVER_CERT); DetectAppLayerInspectEngineRegister("tls.cert_serial", ALPROTO_TLS, SIG_FLAG_TOSERVER, TLS_STATE_CLIENT_CERT, DetectEngineInspectBufferGeneric, GetData); diff --git a/src/detect-tls-cert-subject.c b/src/detect-tls-cert-subject.c index 6d9335c94f..f14ebac981 100644 --- a/src/detect-tls-cert-subject.c +++ b/src/detect-tls-cert-subject.c @@ -87,10 +87,10 @@ void DetectTlsSubjectRegister(void) GetData, ALPROTO_TLS, TLS_STATE_CLIENT_CERT); DetectAppLayerInspectEngineRegister("tls.cert_subject", ALPROTO_TLS, SIG_FLAG_TOCLIENT, - TLS_STATE_SERVER_CERT_DONE, DetectEngineInspectBufferGeneric, GetData); + TLS_STATE_SERVER_CERT, DetectEngineInspectBufferGeneric, GetData); DetectAppLayerMpmRegister("tls.cert_subject", SIG_FLAG_TOCLIENT, 2, PrefilterGenericMpmRegister, - GetData, ALPROTO_TLS, TLS_STATE_SERVER_CERT_DONE); + GetData, ALPROTO_TLS, TLS_STATE_SERVER_CERT); DetectBufferTypeSetDescriptionByName("tls.cert_subject", "TLS certificate subject"); diff --git a/src/detect-tls-cert-validity.c b/src/detect-tls-cert-validity.c index 1a26d2a276..acc620e67e 100644 --- a/src/detect-tls-cert-validity.c +++ b/src/detect-tls-cert-validity.c @@ -124,7 +124,7 @@ void DetectTlsValidityRegister (void) DetectSetupParseRegexes(PARSE_REGEX, &parse_regex); - g_tls_validity_buffer_id = DetectBufferTypeGetByName("tls:server_cert_done:generic"); + g_tls_validity_buffer_id = DetectBufferTypeGetByName("tls:server_cert:generic"); } /** diff --git a/src/detect-tls-certs.c b/src/detect-tls-certs.c index 3e8bdc4b52..1c3c847be9 100644 --- a/src/detect-tls-certs.c +++ b/src/detect-tls-certs.c @@ -114,8 +114,8 @@ void DetectTlsCertsRegister(void) sigmatch_table[DETECT_TLS_CERTS].flags |= SIGMATCH_INFO_STICKY_BUFFER | SIGMATCH_INFO_MULTI_BUFFER; - DetectAppLayerMultiRegister("tls.certs", ALPROTO_TLS, SIG_FLAG_TOCLIENT, - TLS_STATE_SERVER_CERT_DONE, TlsCertsGetData, 2); + DetectAppLayerMultiRegister( + "tls.certs", ALPROTO_TLS, SIG_FLAG_TOCLIENT, TLS_STATE_SERVER_CERT, TlsCertsGetData, 2); DetectAppLayerMultiRegister( "tls.certs", ALPROTO_TLS, SIG_FLAG_TOSERVER, TLS_STATE_CLIENT_CERT, TlsCertsGetData, 2); @@ -149,7 +149,7 @@ static int DetectTlsCertsSetup(DetectEngineCtx *de_ctx, Signature *s, } static int g_tls_cert_buffer_id = 0; -#define BUFFER_NAME "tls:server_cert_done:generic" +#define BUFFER_NAME "tls:server_cert:generic" #define KEYWORD_ID DETECT_TLS_CHAIN_LEN #define KEYWORD_NAME "tls.cert_chain_len" #define KEYWORD_DESC "match TLS certificate chain length" @@ -179,6 +179,18 @@ static int DetectTLSCertChainLenMatch(DetectEngineThreadCtx *det_ctx, Flow *f, u SSLState *ssl_state = state; if (flags & STREAM_TOCLIENT) { SSLStateConnp *connp = &ssl_state->server_connp; + /* count the chain only when the certificate data is final: + * a Certificate message that failed to decode leaves a partial + * chain with cert_data_incomplete set, sticky across later + * phases (an application-data record advances the track and + * rules at server_cert are still evaluated mid-walk); + * within the cert phase the queue may also simply not be + * parsed yet (the track enters it at ServerHello completion, + * before any certificate byte) */ + if (connp->cert_data_incomplete || + (ssl_state->server_state == TLS_STATE_SERVER_CERT && !connp->cert_chain_final)) { + SCReturnInt(0); + } uint32_t cnt = 0; SSLCertsChain *cert; TAILQ_FOREACH (cert, &connp->certs, next) { diff --git a/src/detect-tls-subjectaltname.c b/src/detect-tls-subjectaltname.c index ab6c1e0bea..fa0fef3526 100644 --- a/src/detect-tls-subjectaltname.c +++ b/src/detect-tls-subjectaltname.c @@ -92,7 +92,7 @@ void DetectTlsSubjectAltNameRegister(void) SIGMATCH_INFO_STICKY_BUFFER | SIGMATCH_INFO_MULTI_BUFFER; DetectAppLayerMultiRegister("tls.subjectaltname", ALPROTO_TLS, SIG_FLAG_TOCLIENT, - TLS_STATE_SERVER_CERT_DONE, TlsSubjectAltNameGetData, 2); + TLS_STATE_SERVER_CERT, TlsSubjectAltNameGetData, 2); DetectBufferTypeSetDescriptionByName("tls.subjectaltname", "TLS Subject Alternative Name"); diff --git a/src/detect-tls.c b/src/detect-tls.c index 6a4dd567fa..4e44535336 100644 --- a/src/detect-tls.c +++ b/src/detect-tls.c @@ -144,7 +144,7 @@ void DetectTlsRegister (void) g_tls_cert_fingerprint_list_id = DetectBufferTypeRegister("tls.cert_fingerprint"); DetectAppLayerInspectEngineRegister("tls_cert", ALPROTO_TLS, SIG_FLAG_TOCLIENT, - TLS_STATE_SERVER_CERT_DONE, DetectEngineInspectGenericList, NULL); + TLS_STATE_SERVER_CERT, DetectEngineInspectGenericList, NULL); DetectAppLayerInspectEngineRegister("tls_cert", ALPROTO_TLS, SIG_FLAG_TOSERVER, TLS_STATE_CLIENT_CERT, DetectEngineInspectGenericList, NULL); diff --git a/src/output-json-tls.c b/src/output-json-tls.c index 2f92eccad2..134346ed5d 100644 --- a/src/output-json-tls.c +++ b/src/output-json-tls.c @@ -729,10 +729,19 @@ static OutputInitResult OutputTlsLogInitSub(SCConfNode *conf, OutputCtx *parent_ return result; } +static bool TlsTxLogCondition( + ThreadVars *tv, const Packet *p, void *state, void *tx, uint64_t tx_id) +{ + /* log once the event's content is final, not when both directions + * report handshake-done: state completeness is not a logging + * dependency (EOF still flushes unlogged txes) */ + return SSLV3TxLogReady(tx); +} + void JsonTlsLogRegister (void) { /* register as child of eve-log */ - OutputRegisterTxSubModuleWithProgress(LOGGER_JSON_TX, "eve-log", "JsonTlsLog", "eve-log.tls", - OutputTlsLogInitSub, ALPROTO_TLS, JsonTlsLogger, TLS_STATE_SERVER_HANDSHAKE_DONE, - TLS_STATE_CLIENT_DATA, JsonTlsLogThreadInit, JsonTlsLogThreadDeinit); + OutputRegisterTxSubModuleWithCondition(LOGGER_JSON_TX, "eve-log", "JsonTlsLog", "eve-log.tls", + OutputTlsLogInitSub, ALPROTO_TLS, JsonTlsLogger, TlsTxLogCondition, + JsonTlsLogThreadInit, JsonTlsLogThreadDeinit); } diff --git a/src/output-lua.c b/src/output-lua.c index 93506e1ecc..d1cf06c3e1 100644 --- a/src/output-lua.c +++ b/src/output-lua.c @@ -78,6 +78,13 @@ static TmEcode LuaLogThreadDeinit(ThreadVars *t, void *data); * * NOTE: The flow (f) also referenced by p->flow is locked. */ +static bool LuaTlsTxLogCondition( + ThreadVars *tv, const Packet *p, void *state, void *tx, uint64_t tx_id) +{ + /* keep in sync with the json-tls logger gate */ + return SSLV3TxLogReady(tx); +} + static int LuaTxLogger(ThreadVars *tv, void *thread_data, const Packet *p, Flow *f, void *alstate, void *txptr, uint64_t tx_id) { SCEnter(); @@ -793,8 +800,7 @@ static OutputInitResult OutputLuaLogInit(SCConfNode *conf) } else if (opts.alproto == ALPROTO_TLS) { om->TxLogFunc = LuaTxLogger; om->alproto = ALPROTO_TLS; - om->tc_log_progress = TLS_STATE_SERVER_HANDSHAKE_DONE; - om->ts_log_progress = TLS_STATE_CLIENT_DATA; + om->TxLogCondition = LuaTlsTxLogCondition; SCAppLayerParserRegisterLogger(IPPROTO_TCP, ALPROTO_TLS); } else if (opts.alproto == ALPROTO_DNS) { om->TxLogFunc = LuaTxLogger;