app-layer: improve transaction cleanup handling

The app layers with a custom iterator would skip a tx if during
the ..Cleanup() pass a transaction was removed.

Address this by storing the current index instead of the next
index. Also pass in the next "min_tx_id" to be incremented from
the last TX. Update loops to do this increment.

Also make sure that the min_id is properly updated if the last
TX is removed when out of order.

Finally add a SMB unittest to test this.

Reported by: Ilya Bakhtin

diff --git a/rust/src/applayertemplate/template.rs b/rust/src/applayertemplate/template.rs
index fdb9b3e7744274ec55f05ea5ddc10a93c9d10945..636f75931240a634927987f48e2b1e9397c57c17 100644 (file)
--- a/rust/src/applayertemplate/template.rs
+++ b/rust/src/applayertemplate/template.rs
@@ -221,7 +221,7 @@ impl TemplateState {
                 index += 1;
                 continue;
             }
-            *state = index as u64 + 1;
+            *state = index as u64;
             return Some((tx, tx.tx_id - 1, (len - index) > 1));
         }
 

diff --git a/rust/src/dhcp/dhcp.rs b/rust/src/dhcp/dhcp.rs
index 885f6b62162bbe00cbe780276d704a5e50c47c60..15c06e29ac786887edf647410b5637ef6a387838 100644 (file)
--- a/rust/src/dhcp/dhcp.rs
+++ b/rust/src/dhcp/dhcp.rs
@@ -210,7 +210,7 @@ impl DHCPState {
                 index += 1;
                 continue;
             }
-            *state = index as u64 + 1;
+            *state = index as u64;
             return Some((tx, tx.tx_id - 1, (len - index) > 1));
         }
         

diff --git a/rust/src/nfs/nfs.rs b/rust/src/nfs/nfs.rs
index b2a66f4da0ad67b5818db7e0b49db305c3d369ea..f3eb59f513d835ff7ea8fbde78d5b6a9945ba971 100644 (file)
--- a/rust/src/nfs/nfs.rs
+++ b/rust/src/nfs/nfs.rs
@@ -428,7 +428,10 @@ impl NFSState {
                 index += 1;
                 continue;
             }
-            *state = index as u64 + 1;
+            // store current index in the state and not the next
+            // as transactions might be freed between now and the
+            // next time we are called.
+            *state = index as u64;
             SCLogDebug!("returning tx_id {} has_next? {} (len {} index {}), tx {:?}",
                     tx.id - 1, (len - index) > 1, len, index, tx);
             return Some((tx, tx.id - 1, (len - index) > 1));

diff --git a/rust/src/smb/smb.rs b/rust/src/smb/smb.rs
index 8c854dc04e14bbeca314f11a396c64c3e801424f..9f2f1d42e4f26ab2b49c36142319f2d8528b0344 100644 (file)
--- a/rust/src/smb/smb.rs
+++ b/rust/src/smb/smb.rs
@@ -874,7 +874,10 @@ impl SMBState {
                 index += 1;
                 continue;
             }
-            *state = index as u64 + 1;
+            // store current index in the state and not the next
+            // as transactions might be freed between now and the
+            // next time we are called.
+            *state = index as u64;
             //SCLogDebug!("returning tx_id {} has_next? {} (len {} index {}), tx {:?}",
             //        tx.id - 1, (len - index) > 1, len, index, tx);
             return Some((tx, tx.id - 1, (len - index) > 1));

diff --git a/src/app-layer-parser.c b/src/app-layer-parser.c
index aab63724043397d11be97204a351a974669f5966..1b8fba9ab5856312fb45c3ad16b818cefeb08173 100644 (file)
--- a/src/app-layer-parser.c
+++ b/src/app-layer-parser.c
@@ -768,6 +768,7 @@ void AppLayerParserSetTransactionInspectId(const Flow *f, AppLayerParserState *p
         }
         if (!ires.has_next)
             break;
+        idx++;
     }
     pstate->inspect_id[direction] = idx;
     SCLogDebug("inspect_id now %"PRIu64, pstate->inspect_id[direction]);
@@ -808,6 +809,7 @@ void AppLayerParserSetTransactionInspectId(const Flow *f, AppLayerParserState *p
             }
             if (!ires.has_next)
                 break;
+            idx++;
         }
     }
 
@@ -901,7 +903,7 @@ void AppLayerParserTransactionsCleanup(Flow *f)
             break;
 
         void *tx = ires.tx_ptr;
-        i = ires.tx_id;
+        i = ires.tx_id; // actual tx id for the tx the IterFunc returned
 
         SCLogDebug("%p/%"PRIu64" checking", tx, i);
 
@@ -950,13 +952,25 @@ void AppLayerParserTransactionsCleanup(Flow *f)
         SCLogDebug("%p/%"PRIu64" freed", tx, i);
 
         /* if we didn't skip any tx so far, up the minimum */
-        SCLogDebug("i %"PRIu64", new_min %"PRIu64, i, new_min);
+        SCLogDebug("skipped? %s i %"PRIu64", new_min %"PRIu64, skipped ? "true" : "false", i, new_min);
         if (!skipped)
             new_min = i + 1;
+        SCLogDebug("final i %"PRIu64", new_min %"PRIu64, i, new_min);
 
 next:
-        if (!ires.has_next)
+        if (!ires.has_next) {
+            /* this was the last tx. See if we skipped any. If not
+             * we removed all and can update the minimum to the max
+             * id. */
+            SCLogDebug("no next: cur tx i %"PRIu64", total %"PRIu64, i, total_txs);
+            if (!skipped) {
+                new_min = total_txs;
+                SCLogDebug("no next: cur tx i %"PRIu64", total %"PRIu64": "
+                        "new_min updated to %"PRIu64, i, total_txs, new_min);
+            }
             break;
+        }
+        i++;
     }
 
     /* see if we need to bring all trackers up to date. */

diff --git a/src/app-layer-smb-tcp-rust.c b/src/app-layer-smb-tcp-rust.c
index a41658121cfb1d45c3c9f8503a9a5b2ffb50b6b6..3681c124beb69b569defa00e4435ea265982ffee 100644 (file)
--- a/src/app-layer-smb-tcp-rust.c
+++ b/src/app-layer-smb-tcp-rust.c
@@ -208,6 +208,10 @@ static SuricataFileContext sfc = { &sbcfg };
 
 #define SMB_CONFIG_DEFAULT_STREAM_DEPTH 0
 
+#ifdef UNITTESTS
+static void SMBParserRegisterTests(void);
+#endif
+
 static uint32_t stream_depth = SMB_CONFIG_DEFAULT_STREAM_DEPTH;
 
 void RegisterRustSMBTCPParsers(void)
@@ -299,7 +303,177 @@ void RegisterRustSMBTCPParsers(void)
         SCLogInfo("Parsed disabled for %s protocol. Protocol detection"
                   "still on.", proto_name);
     }
+#ifdef UNITTESTS
+    AppLayerParserRegisterProtocolUnittests(IPPROTO_TCP, ALPROTO_SMB, SMBParserRegisterTests);
+#endif
 
     return;
 }
+
+#ifdef UNITTESTS
+#include "stream-tcp.h"
+#include "util-unittest-helper.h"
+
+/** \test multi transactions and cleanup */
+static int SMBParserTxCleanupTest(void)
+{
+    uint64_t ret[4];
+    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, 445);
+    FAIL_IF_NULL(f);
+    f->protoctx = &ssn;
+    f->proto = IPPROTO_TCP;
+    f->alproto = ALPROTO_SMB;
+
+    char req_str[] ="\x00\x00\x00\x79\xfe\x53\x4d\x42\x40\x00\x01\x00\x00\x00\x00\x00" \
+                     "\x05\x00\xe0\x1e\x10\x00\x00\x00\x00\x00\x00\x00\x0b\x00\x00\x00" \
+                     "\x00\x00\x00\x00\x00\x00\x00\x00\x10\x72\xd2\x9f\x36\xc2\x08\x14" \
+                     "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" \
+                     "\x00\x00\x00\x00\x39\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00" \
+                     "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x80\x00\x00\x00" \
+                     "\x00\x00\x00\x00\x07\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00" \
+                     "\x78\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00";
+    req_str[28] = 0x01;
+    int r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOSERVER | STREAM_START, (uint8_t *)req_str, sizeof(req_str));
+    FAIL_IF_NOT(r == 0);
+    req_str[28]++;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOSERVER, (uint8_t *)req_str, sizeof(req_str));
+    FAIL_IF_NOT(r == 0);
+    req_str[28]++;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOSERVER, (uint8_t *)req_str, sizeof(req_str));
+    FAIL_IF_NOT(r == 0);
+    req_str[28]++;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOSERVER, (uint8_t *)req_str, sizeof(req_str));
+    FAIL_IF_NOT(r == 0);
+    req_str[28]++;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOSERVER, (uint8_t *)req_str, sizeof(req_str));
+    FAIL_IF_NOT(r == 0);
+    req_str[28]++;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOSERVER, (uint8_t *)req_str, sizeof(req_str));
+    FAIL_IF_NOT(r == 0);
+    req_str[28]++;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOSERVER, (uint8_t *)req_str, sizeof(req_str));
+    FAIL_IF_NOT(r == 0);
+    req_str[28]++;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOSERVER, (uint8_t *)req_str, sizeof(req_str));
+    FAIL_IF_NOT(r == 0);
+    req_str[28]++;
+
+    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
+
+    char resp_str[] = "\x00\x00\x00\x98\xfe\x53\x4d\x42\x40\x00\x01\x00\x00\x00\x00\x00" \
+                       "\x05\x00\x21\x00\x11\x00\x00\x00\x00\x00\x00\x00\x0b\x00\x00\x00" \
+                       "\x00\x00\x00\x00\x00\x00\x00\x00\x10\x72\xd2\x9f\x36\xc2\x08\x14" \
+                       "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" \
+                       "\x00\x00\x00\x00\x59\x00\x00\x00\x01\x00\x00\x00\x48\x38\x40\xb3" \
+                       "\x0f\xa8\xd3\x01\x84\x9a\x2b\x46\xf7\xa8\xd3\x01\x48\x38\x40\xb3" \
+                       "\x0f\xa8\xd3\x01\x48\x38\x40\xb3\x0f\xa8\xd3\x01\x00\x00\x00\x00" \
+                       "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x10\x00\x00\x00" \
+                       "\x00\x00\x00\x00\x9e\x8f\xb8\x91\x00\x00\x00\x00\x01\x5b\x11\xbb" \
+                       "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00";
+
+    resp_str[28] = 0x01;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOCLIENT | STREAM_START, (uint8_t *)resp_str, sizeof(resp_str));
+    FAIL_IF_NOT(r == 0);
+    resp_str[28] = 0x04;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOCLIENT, (uint8_t *)resp_str, sizeof(resp_str));
+    FAIL_IF_NOT(r == 0);
+    resp_str[28] = 0x05;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOCLIENT, (uint8_t *)resp_str, sizeof(resp_str));
+    FAIL_IF_NOT(r == 0);
+    resp_str[28] = 0x06;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOCLIENT, (uint8_t *)resp_str, sizeof(resp_str));
+    FAIL_IF_NOT(r == 0);
+    resp_str[28] = 0x08;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOCLIENT, (uint8_t *)resp_str, sizeof(resp_str));
+    FAIL_IF_NOT(r == 0);
+    resp_str[28] = 0x02;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOCLIENT, (uint8_t *)resp_str, sizeof(resp_str));
+    FAIL_IF_NOT(r == 0);
+    resp_str[28] = 0x07;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOCLIENT, (uint8_t *)resp_str, sizeof(resp_str));
+    FAIL_IF_NOT(r == 0);
+    AppLayerParserTransactionsCleanup(f);
+
+    UTHAppLayerParserStateGetIds(f->alparser, &ret[0], &ret[1], &ret[2], &ret[3]);
+    FAIL_IF_NOT(ret[0] == 2); // inspect_id[0]
+    FAIL_IF_NOT(ret[1] == 2); // inspect_id[1]
+    FAIL_IF_NOT(ret[2] == 2); // log_id
+    FAIL_IF_NOT(ret[3] == 2); // min_id
+
+    resp_str[28] = 0x03;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOCLIENT, (uint8_t *)resp_str, sizeof(resp_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]
+    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
+
+    req_str[28] = 0x09;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOSERVER | STREAM_EOF, (uint8_t *)req_str, sizeof(req_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
+
+    resp_str[28] = 0x09;
+    r = AppLayerParserParse(NULL, alp_tctx, f, ALPROTO_SMB,
+                                STREAM_TOCLIENT | STREAM_EOF, (uint8_t *)resp_str, sizeof(resp_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
+
+    AppLayerParserThreadCtxFree(alp_tctx);
+    StreamTcpFreeConfig(TRUE);
+    UTHFreeFlow(f);
+
+    PASS;
+}
+
+static void SMBParserRegisterTests(void)
+{
+    UtRegisterTest("SMBParserTxCleanupTest", SMBParserTxCleanupTest);
+}
+
+#endif /* UNITTESTS */
 #endif /* HAVE_RUST */

diff --git a/src/output-tx.c b/src/output-tx.c
index b061b89d0358040f6e1d5fdbbdc8fc5af6799f1c..33fb8786acdd8ee8fc86f7dcdaad2cb2b44e6026 100644 (file)
--- a/src/output-tx.c
+++ b/src/output-tx.c
@@ -270,6 +270,7 @@ next_logger:
 next_tx:
         if (!ires.has_next)
             break;
+        tx_id++;
     }
 
     /* Update the the last ID that has been logged with all