app-layer: fix tx tracking updates in tx cleanup

Fix min_id not getting updated in all cases.

Reported by: Ilya Bakhtin

diff --git a/src/app-layer-dns-udp.c b/src/app-layer-dns-udp.c
index a40377ba494b221d566cf39502de1c413e1128e7..9c59e63e1bf230aa8e2ab10be049fdb4862a815f 100644 (file)
--- a/src/app-layer-dns-udp.c
+++ b/src/app-layer-dns-udp.c
@@ -858,6 +858,114 @@ static int DNSUDPParserTestLostResponse(void)
     PASS;
 }
 
+static int DNSUDPParserTxCleanup(void)
+{
+    uint64_t ret[4];
+
+    /* DNS request:
+     * - Flags: 0x0100 Standard query
+     * - A www.google.com
+     */
+    uint8_t req[] = {
+        0x00, 0x01, 0x01, 0x00, 0x00, 0x01, 0x00, 0x00,
+        0x00, 0x00, 0x00, 0x00, 0x03, 0x77, 0x77, 0x77,
+        0x06, 0x67, 0x6f, 0x6f, 0x67, 0x6c, 0x65, 0x03,
+        0x63, 0x6f, 0x6d, 0x00, 0x00, 0x01, 0x00, 0x01,
+    };
+    size_t reqlen = sizeof(req);
+
+    uint8_t res[] = {
+        0x00, 0x01, 0x81, 0x80, 0x00, 0x01, 0x00, 0x08,
+        0x00, 0x00, 0x00, 0x00, 0x03, 0x77, 0x77, 0x77,
+        0x06, 0x67, 0x6f, 0x6f, 0x67, 0x6c, 0x65, 0x03,
+        0x63, 0x6f, 0x6d, 0x00, 0x00, 0x01, 0x00, 0x01,
+        0xc0, 0x0c, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00,
+        0x01, 0x08, 0x00, 0x04, 0x18, 0xf4, 0x04, 0x38,
+        0xc0, 0x0c, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00,
+        0x01, 0x08, 0x00, 0x04, 0x18, 0xf4, 0x04, 0x39,
+        0xc0, 0x0c, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00,
+        0x01, 0x08, 0x00, 0x04, 0x18, 0xf4, 0x04, 0x34,
+        0xc0, 0x0c, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00,
+        0x01, 0x08, 0x00, 0x04, 0x18, 0xf4, 0x04, 0x35,
+        0xc0, 0x0c, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00,
+        0x01, 0x08, 0x00, 0x04, 0x18, 0xf4, 0x04, 0x36,
+        0xc0, 0x0c, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00,
+        0x01, 0x08, 0x00, 0x04, 0x18, 0xf4, 0x04, 0x3b,
+        0xc0, 0x0c, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00,
+        0x01, 0x08, 0x00, 0x04, 0x18, 0xf4, 0x04, 0x37,
+        0xc0, 0x0c, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00,
+        0x01, 0x08, 0x00, 0x04, 0x18, 0xf4, 0x04, 0x3a
+    };
+    size_t reslen = sizeof(res);
+
+    Flow *f = UTHBuildFlow(AF_INET, "1.1.1.1", "2.2.2.2", 1024, 53);
+    FAIL_IF_NULL(f);
+    f->proto = IPPROTO_UDP;
+    f->protomap = FlowGetProtoMapping(IPPROTO_UDP);
+    f->alproto = ALPROTO_DNS;
+
+    AppLayerParserThreadCtx *alp_tctx = AppLayerParserThreadCtxAlloc();
+    FAIL_IF_NULL(alp_tctx);
+
+    /* First request. */
+    req[1] = 0x01;
+    int r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_DNS,
+                                STREAM_TOSERVER | STREAM_START, req, reqlen);
+    FAIL_IF_NOT(r == 0);
+    /* Second request. */
+    req[1] = 0x02;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_DNS,
+                                STREAM_TOSERVER, req, reqlen);
+    FAIL_IF_NOT(r == 0);
+    /* Third request. */
+    req[1] = 0x03;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_DNS,
+                                STREAM_TOSERVER, req, reqlen);
+    FAIL_IF_NOT(r == 0);
+
+    /* Now respond to the second. */
+    res[1] = 0x02;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_DNS,
+                                STREAM_TOCLIENT, res, reslen);
+    FAIL_IF_NOT(r == 0);
+
+    /* Send a 4th request. */
+    req[1] = 0x04;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_DNS,
+                                STREAM_TOSERVER, req, reqlen);
+    FAIL_IF_NOT(r == 0);
+
+    AppLayerParserTransactionsCleanup(f);
+    UTHAppLayerParserStateGetIds(f->alparser, &ret[0], &ret[1], &ret[2], &ret[3]);
+    FAIL_IF_NOT(ret[0] == 0); // inspect_id[0]
+    FAIL_IF_NOT(ret[1] == 0); // inspect_id[1]
+    FAIL_IF_NOT(ret[2] == 0); // log_id
+    FAIL_IF_NOT(ret[3] == 0); // min_id
+
+    /* Response to the third request. */
+    res[1] = 0x03;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_DNS,
+                                STREAM_TOCLIENT, res, reslen);
+    FAIL_IF_NOT(r == 0);
+    DNSState *state = f->alstate;
+    DNSTransaction *tx = TAILQ_FIRST(&state->tx_list);
+    FAIL_IF_NULL(tx);
+    FAIL_IF(tx->replied);
+    FAIL_IF(!tx->reply_lost);
+
+    AppLayerParserTransactionsCleanup(f);
+    UTHAppLayerParserStateGetIds(f->alparser, &ret[0], &ret[1], &ret[2], &ret[3]);
+    FAIL_IF_NOT(ret[0] == 3); // inspect_id[0]
+    FAIL_IF_NOT(ret[1] == 3); // inspect_id[1]
+    FAIL_IF_NOT(ret[2] == 3); // log_id
+    FAIL_IF_NOT(ret[3] == 3); // min_id
+
+    /* Also free's state. */
+    UTHFreeFlow(f);
+
+    PASS;
+}
+
 void DNSUDPParserRegisterTests(void)
 {
     UtRegisterTest("DNSUDPParserTest01", DNSUDPParserTest01);
@@ -870,6 +978,8 @@ void DNSUDPParserRegisterTests(void)
         DNSUDPParserTestDelayedResponse);
     UtRegisterTest("DNSUDPParserTestLostResponse",
         DNSUDPParserTestLostResponse);
+    UtRegisterTest("DNSUDPParserTxCleanup",
+        DNSUDPParserTxCleanup);
 }
 #endif
 #endif /* HAVE_RUST */

diff --git a/src/app-layer-htp.c b/src/app-layer-htp.c
index ca1f3558a5e9a61fdc130ad22428540d82721700..d01f420fd769a35169046fdf4da7ee19f11c3707 100644 (file)
--- a/src/app-layer-htp.c
+++ b/src/app-layer-htp.c
@@ -6920,6 +6920,115 @@ static int HTPParserTest24(void)
     PASS;
 }
 
+/** \test multi transactions and cleanup */
+static int HTPParserTest25(void)
+{
+    AppLayerParserThreadCtx *alp_tctx = AppLayerParserThreadCtxAlloc();
+    FAIL_IF_NULL(alp_tctx);
+
+    StreamTcpInitConfig(TRUE);
+    TcpSession ssn;
+    memset(&ssn, 0, sizeof(ssn));
+
+    Flow *f = UTHBuildFlow(AF_INET, "1.2.3.4", "1.2.3.5", 1024, 80);
+    FAIL_IF_NULL(f);
+    f->protoctx = &ssn;
+    f->proto = IPPROTO_TCP;
+    f->alproto = ALPROTO_HTTP;
+
+    const char *str = "GET / HTTP/1.1\r\nHost: www.google.com\r\nUser-Agent: Suricata/1.0\r\n\r\n";
+    int r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOSERVER | STREAM_START, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOSERVER, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOSERVER, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOSERVER, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOSERVER, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOSERVER, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOSERVER, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOSERVER, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+
+    str = "HTTP 1.1 200 OK\r\nServer: Suricata/1.0\r\nContent-Length: 8\r\n\r\nSuricata";
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOCLIENT | STREAM_START, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOCLIENT, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOCLIENT, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOCLIENT, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOCLIENT, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOCLIENT, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOCLIENT, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOCLIENT, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+
+    AppLayerParserTransactionsCleanup(f);
+
+    uint64_t ret[4];
+    UTHAppLayerParserStateGetIds(f->alparser, &ret[0], &ret[1], &ret[2], &ret[3]);
+    FAIL_IF_NOT(ret[0] == 8); // inspect_id[0]
+    FAIL_IF_NOT(ret[1] == 8); // inspect_id[1]
+    FAIL_IF_NOT(ret[2] == 8); // log_id
+    FAIL_IF_NOT(ret[3] == 8); // min_id
+
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOSERVER | STREAM_EOF, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+    AppLayerParserTransactionsCleanup(f);
+
+    UTHAppLayerParserStateGetIds(f->alparser, &ret[0], &ret[1], &ret[2], &ret[3]);
+    FAIL_IF_NOT(ret[0] == 8); // inspect_id[0] not updated by ..Cleanup() until full tx is done
+    FAIL_IF_NOT(ret[1] == 8); // inspect_id[1]
+    FAIL_IF_NOT(ret[2] == 8); // log_id
+    FAIL_IF_NOT(ret[3] == 8); // min_id
+
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_HTTP,
+                                STREAM_TOCLIENT | STREAM_EOF, (uint8_t *)str, strlen(str));
+    FAIL_IF_NOT(r == 0);
+    AppLayerParserTransactionsCleanup(f);
+
+    UTHAppLayerParserStateGetIds(f->alparser, &ret[0], &ret[1], &ret[2], &ret[3]);
+    FAIL_IF_NOT(ret[0] == 9); // inspect_id[0]
+    FAIL_IF_NOT(ret[1] == 9); // inspect_id[1]
+    FAIL_IF_NOT(ret[2] == 9); // log_id
+    FAIL_IF_NOT(ret[3] == 9); // min_id
+
+    HtpState *http_state = f->alstate;
+    FAIL_IF_NULL(http_state);
+
+    AppLayerParserThreadCtxFree(alp_tctx);
+    StreamTcpFreeConfig(TRUE);
+    UTHFreeFlow(f);
+
+    PASS;
+}
+
 #endif /* UNITTESTS */
 
 /**
@@ -6976,6 +7085,7 @@ void HTPParserRegisterTests(void)
     UtRegisterTest("HTPParserTest22", HTPParserTest22);
     UtRegisterTest("HTPParserTest23", HTPParserTest23);
     UtRegisterTest("HTPParserTest24", HTPParserTest24);
+    UtRegisterTest("HTPParserTest25", HTPParserTest25);
 
     HTPFileParserRegisterTests();
     HTPXFFParserRegisterTests();

diff --git a/src/app-layer-parser.c b/src/app-layer-parser.c
index c9384c9667818af8675c5bb26493ffbc2c880a0b..aab63724043397d11be97204a351a974669f5966 100644 (file)
--- a/src/app-layer-parser.c
+++ b/src/app-layer-parser.c
@@ -893,6 +893,7 @@ void AppLayerParserTransactionsCleanup(Flow *f)
     uint64_t i = min;
     uint64_t new_min = min;
     SCLogDebug("start min %"PRIu64, min);
+    bool skipped = false;
 
     while (1) {
         AppLayerGetTxIterTuple ires = IterFunc(ipproto, alproto, alstate, i, total_txs, &state);
@@ -907,11 +908,13 @@ void AppLayerParserTransactionsCleanup(Flow *f)
         const int tx_progress_tc = AppLayerParserGetStateProgress(ipproto, alproto, tx, STREAM_TOCLIENT);
         if (tx_progress_tc < tx_end_state_tc) {
             SCLogDebug("%p/%"PRIu64" skipping: tc parser not done", tx, i);
+            skipped = true;
             goto next;
         }
         const int tx_progress_ts = AppLayerParserGetStateProgress(ipproto, alproto, tx, STREAM_TOSERVER);
         if (tx_progress_ts < tx_end_state_ts) {
             SCLogDebug("%p/%"PRIu64" skipping: ts parser not done", tx, i);
+            skipped = true;
             goto next;
         }
         if (f->sgh_toserver != NULL) {
@@ -919,6 +922,7 @@ void AppLayerParserTransactionsCleanup(Flow *f)
             if (!(detect_flags_ts & APP_LAYER_TX_INSPECTED_FLAG)) {
                 SCLogDebug("%p/%"PRIu64" skipping: TS inspect not done: ts:%"PRIx64,
                         tx, i, detect_flags_ts);
+                skipped = true;
                 goto next;
             }
         }
@@ -927,6 +931,7 @@ void AppLayerParserTransactionsCleanup(Flow *f)
             if (!(detect_flags_tc & APP_LAYER_TX_INSPECTED_FLAG)) {
                 SCLogDebug("%p/%"PRIu64" skipping: TC inspect not done: tc:%"PRIx64,
                         tx, i, detect_flags_tc);
+                skipped = true;
                 goto next;
             }
         }
@@ -935,6 +940,7 @@ void AppLayerParserTransactionsCleanup(Flow *f)
             if (tx_logged != logger_expectation) {
                 SCLogDebug("%p/%"PRIu64" skipping: logging not done: want:%"PRIx32", have:%"PRIx32,
                         tx, i, logger_expectation, tx_logged);
+                skipped = true;
                 goto next;
             }
         }
@@ -943,8 +949,9 @@ void AppLayerParserTransactionsCleanup(Flow *f)
         p->StateTransactionFree(alstate, i);
         SCLogDebug("%p/%"PRIu64" freed", tx, i);
 
-        /* if this tx was the minimum, up the minimum */
-        if (i == new_min)
+        /* if we didn't skip any tx so far, up the minimum */
+        SCLogDebug("i %"PRIu64", new_min %"PRIu64, i, new_min);
+        if (!skipped)
             new_min = i + 1;
 
 next: