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

@@ -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;
+        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 < connp->cert0_sans_num; i++) {
-            SCX509GetSubjectAltNameAt(
-                    x509, i, &connp->cert0_sans[i].san, &connp->cert0_sans[i].san_len);
+            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;
-        }
+                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;
+        }
+
+        /* 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 */
+        /* 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);
-    } 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 */
-        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)
 {

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);
 

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);
 

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);

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");

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);

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");

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");
 }
 
 /**

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) {

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");
 

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);

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);
 }

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;