From: Victor Julien Date: Wed, 5 Dec 2018 21:09:15 +0000 (+0100) Subject: app-layer: fix tx tracking updates in tx cleanup X-Git-Tag: suricata-4.1.1~20 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d34e41068f6070f71bdf1bf509fc807a999a3723;p=thirdparty%2Fsuricata.git 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 a40377ba49..9c59e63e1b 100644 --- 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 ca1f3558a5..d01f420fd7 100644 --- 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 c9384c9667..aab6372404 100644 --- 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: