From 6147347a01568873189b2e762098b1dd0fb13370 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Thu, 17 Sep 2026 22:52:51 +0000 Subject: [PATCH] tls: make the server states active phases The server states were completion milestones: server_hello landed when the server hello record was fully parsed and server_cert_done when the certificate message was done, so the states named moments at which the phase data was complete instead of the phase being processed. The flight-completion state (server_hello_done) named a moment at which no data is published and no decision can act. The states now denote the record phase being processed: server_hello is entered when the server hello message begins (its header is read once, the message may span several records) and server_cert when the certificate message begins, so the state names the message being parsed and a message that fails to decode leaves the track in the state that names it. The record that completes a hello hands the track over to the certificate phase before a single certificate byte arrives. The enter/leave is the shared handshake phase mechanism, so the client track follows: client_hello is entered at the start of the client hello as well, and the completing client hello is decided at client_cert. The flight-done state is removed and renumbers the tail (server_data, server_finished), leaving both tracks with the same five-phase shape and an absorbing finished state. Hello and certificate decode failures suppress the phase advance of their own message only: the decoders keep their pre-existing return contract (the message bytes are consumed and the error events are raised as before) and set a per-connp suppression flag that is reset at the start of the next handshake message and checked at the phase advance. A decode failure is not an app-layer parse error: the flow is neither dropped nor blinded, and the next message that decodes cleanly still advances. An undecodable certificate in a chain is skipped, not fatal: one decoder event is raised for the bad certificate and the remaining certificates are still decoded - the rest of the chain is kept and the certificate data is filled from the first fully decodable certificate. The certificate phase is suppressed for such a message (the phase data is incomplete). The incomplete-data condition is sticky per connp: it is set while the certificate message is being decoded, cleared by a fully decoded certificate message only, and consulted by the handshake-done heuristic that would otherwise establish the data phase from the certificate data - a later handshake message does not complete the data, so the tracks do not cross the cert phase on partial data. The certificate field extraction is commit-on-success: the fields are extracted into stashes and published to the connp only when the whole certificate yields every field, so a certificate that fails a late extraction leaves no partial data behind and the next certificate can fill the fields. An extraction failure raises its decoder event once per message: the message no longer aborts at the first failing certificate, and the event is not repeated for the later failing entries. The tls.cert_chain_len match is gated on the chain being final (the per-connp flag set by a fully decoded certificate message), so it does not match while the track sits in server_cert with a chain that is still arriving. TOCLIENT keyword registrations and the tc log threshold follow the renamed constants (values 1/2 unchanged; the threshold is triggered by the same writes as before, so the log fires on the same packets). The ja3s lua registration moves to server_data: lua keeps a numeric hook, so it must follow the renamed state the old hook value pointed at (server hello done was 3, server data is 3). The ja3s content is final once the ServerHello parses, which is why the C keywords (ja3s.string, ja3s.hash) stay registered at server_hello. Two string-based generic buffer lookups and the user guide follow the renamed states. Ticket: #9047. --- src/app-layer-ssl.c | 391 ++++++++++++++++++++++-------- src/app-layer-ssl.h | 29 ++- src/detect-lua.c | 2 +- src/detect-tls-cert-fingerprint.c | 4 +- src/detect-tls-cert-issuer.c | 4 +- src/detect-tls-cert-serial.c | 4 +- src/detect-tls-cert-subject.c | 4 +- src/detect-tls-cert-validity.c | 2 +- src/detect-tls-certs.c | 18 +- src/detect-tls-subjectaltname.c | 2 +- src/detect-tls.c | 2 +- src/output-json-tls.c | 15 +- src/output-lua.c | 10 +- 13 files changed, 367 insertions(+), 120 deletions(-) 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;