From a2f249cc86d70533a3c8fecb9c7967d2edfacdb0 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Sun, 7 Jun 2020 22:41:11 +0200 Subject: [PATCH] app-layer: handle AppLayerTxData being NULL Http parser can have 'NULL' user data in case of memcap limit getting reached. --- src/app-layer-parser.c | 75 ++++++++++++++++++++---------------------- src/detect.c | 4 +-- src/output-tx.c | 26 ++++++++------- 3 files changed, 53 insertions(+), 52 deletions(-) diff --git a/src/app-layer-parser.c b/src/app-layer-parser.c index 6de88860f5..badc85c1a3 100644 --- a/src/app-layer-parser.c +++ b/src/app-layer-parser.c @@ -696,37 +696,25 @@ uint64_t AppLayerParserGetTransactionInspectId(AppLayerParserState *pstate, uint SCReturnCT(pstate->inspect_id[direction & STREAM_TOSERVER ? 0 : 1], "uint64_t"); } -static inline uint64_t GetTxDetectFlags(const uint8_t ipproto, const AppProto alproto, void *tx, const uint8_t dir) +static inline uint64_t GetTxDetectFlags(AppLayerTxData *txd, const uint8_t dir) { - uint64_t detect_flags; - AppLayerTxData *txd = AppLayerParserGetTxData(ipproto, alproto, tx); - if (txd != NULL) { - detect_flags = (dir & STREAM_TOSERVER) ? txd->detect_flags_ts : txd->detect_flags_tc; - } else { - detect_flags = 0; - } + uint64_t detect_flags = + (dir & STREAM_TOSERVER) ? txd->detect_flags_ts : txd->detect_flags_tc; return detect_flags; } -static inline void SetTxDetectFlags(const uint8_t ipproto, const AppProto alproto, void *tx, const uint8_t dir, const uint64_t detect_flags) +static inline void SetTxDetectFlags(AppLayerTxData *txd, const uint8_t dir, const uint64_t detect_flags) { - AppLayerTxData *txd = AppLayerParserGetTxData(ipproto, alproto, tx); - if (txd != NULL) { - if (dir & STREAM_TOSERVER) { - txd->detect_flags_ts = detect_flags; - } else { - txd->detect_flags_tc = detect_flags; - } + if (dir & STREAM_TOSERVER) { + txd->detect_flags_ts = detect_flags; + } else { + txd->detect_flags_tc = detect_flags; } } -static inline uint32_t GetTxLogged(const Flow *f, void *alstate, void *tx) +static inline uint32_t GetTxLogged(AppLayerTxData *txd) { - AppLayerTxData *txd = AppLayerParserGetTxData(f->proto, f->alproto, tx); - if (txd) { - return txd->logged.flags; - } - return 0; + return txd->logged.flags; } void AppLayerParserSetTransactionInspectId(const Flow *f, AppLayerParserState *pstate, @@ -763,11 +751,12 @@ void AppLayerParserSetTransactionInspectId(const Flow *f, AppLayerParserState *p if (state_progress < state_done_progress) break; - if (tag_txs_as_inspected) { - uint64_t detect_flags = GetTxDetectFlags(ipproto, alproto, tx, flags); + AppLayerTxData *txd = AppLayerParserGetTxData(ipproto, alproto, tx); + if (txd && tag_txs_as_inspected) { + uint64_t detect_flags = GetTxDetectFlags(txd, flags); if ((detect_flags & APP_LAYER_TX_INSPECTED_FLAG) == 0) { detect_flags |= APP_LAYER_TX_INSPECTED_FLAG; - SetTxDetectFlags(ipproto, alproto, tx, flags, detect_flags); + SetTxDetectFlags(txd, flags, detect_flags); SCLogDebug("%p/%"PRIu64" in-order tx is done for direction %s. Flag %016"PRIx64, tx, idx, flags & STREAM_TOSERVER ? "toserver" : "toclient", detect_flags); } @@ -801,15 +790,22 @@ void AppLayerParserSetTransactionInspectId(const Flow *f, AppLayerParserState *p if (state_progress < state_done_progress) break; - uint64_t detect_flags = GetTxDetectFlags(ipproto, alproto, tx, flags); - if ((detect_flags & APP_LAYER_TX_INSPECTED_FLAG) == 0) { - detect_flags |= APP_LAYER_TX_INSPECTED_FLAG; - SetTxDetectFlags(ipproto, alproto, tx, flags, detect_flags); - SCLogDebug("%p/%"PRIu64" out of order tx is done for direction %s. Flag %016"PRIx64, - tx, idx, flags & STREAM_TOSERVER ? "toserver" : "toclient", detect_flags); - - SCLogDebug("%p/%"PRIu64" out of order tx. Update inspect_id? %"PRIu64, - tx, idx, pstate->inspect_id[direction]); + /* txd can be NULL for HTTP sessions where the user data alloc failed */ + AppLayerTxData *txd = AppLayerParserGetTxData(ipproto, alproto, tx); + if (likely(txd)) { + uint64_t detect_flags = GetTxDetectFlags(txd, flags); + if ((detect_flags & APP_LAYER_TX_INSPECTED_FLAG) == 0) { + detect_flags |= APP_LAYER_TX_INSPECTED_FLAG; + SetTxDetectFlags(txd, flags, detect_flags); + SCLogDebug("%p/%"PRIu64" out of order tx is done for direction %s. Flag %016"PRIx64, + tx, idx, flags & STREAM_TOSERVER ? "toserver" : "toclient", detect_flags); + + SCLogDebug("%p/%"PRIu64" out of order tx. Update inspect_id? %"PRIu64, + tx, idx, pstate->inspect_id[direction]); + if (pstate->inspect_id[direction]+1 == idx) + pstate->inspect_id[direction] = idx; + } + } else { if (pstate->inspect_id[direction]+1 == idx) pstate->inspect_id[direction] = idx; } @@ -924,9 +920,10 @@ void AppLayerParserTransactionsCleanup(Flow *f) skipped = true; goto next; } - if (has_tx_detect_flags) { + AppLayerTxData *txd = AppLayerParserGetTxData(ipproto, alproto, tx); + if (txd && has_tx_detect_flags) { if (f->sgh_toserver != NULL) { - uint64_t detect_flags_ts = GetTxDetectFlags(ipproto, alproto, tx, STREAM_TOSERVER); + uint64_t detect_flags_ts = GetTxDetectFlags(txd, STREAM_TOSERVER); if (!(detect_flags_ts & APP_LAYER_TX_INSPECTED_FLAG)) { SCLogDebug("%p/%"PRIu64" skipping: TS inspect not done: ts:%"PRIx64, tx, i, detect_flags_ts); @@ -935,7 +932,7 @@ void AppLayerParserTransactionsCleanup(Flow *f) } } if (f->sgh_toclient != NULL) { - uint64_t detect_flags_tc = GetTxDetectFlags(ipproto, alproto, tx, STREAM_TOCLIENT); + uint64_t detect_flags_tc = GetTxDetectFlags(txd, STREAM_TOCLIENT); if (!(detect_flags_tc & APP_LAYER_TX_INSPECTED_FLAG)) { SCLogDebug("%p/%"PRIu64" skipping: TC inspect not done: tc:%"PRIx64, tx, i, detect_flags_tc); @@ -944,8 +941,8 @@ void AppLayerParserTransactionsCleanup(Flow *f) } } } - if (logger_expectation != 0) { - LoggerId tx_logged = GetTxLogged(f, alstate, tx); + if (txd &&logger_expectation != 0) { + LoggerId tx_logged = GetTxLogged(txd); if (tx_logged != logger_expectation) { SCLogDebug("%p/%"PRIu64" skipping: logging not done: want:%"PRIx32", have:%"PRIx32, tx, i, logger_expectation, tx_logged); diff --git a/src/detect.c b/src/detect.c index 1223720e28..519ae9ea26 100644 --- a/src/detect.c +++ b/src/detect.c @@ -1235,7 +1235,7 @@ static DetectTransaction GetDetectTx(const uint8_t ipproto, const AppProto alpro { uint64_t detect_flags; AppLayerTxData *txd = AppLayerParserGetTxData(ipproto, alproto, tx_ptr); - if (txd != NULL) { + if (likely(txd != NULL)) { detect_flags = (flow_flags & STREAM_TOSERVER) ? txd->detect_flags_ts : txd->detect_flags_tc; } else { detect_flags = 0; @@ -1272,7 +1272,7 @@ static inline void StoreDetectFlags(DetectTransaction *tx, const uint8_t flow_fl const uint8_t ipproto, const AppProto alproto, const uint64_t detect_flags) { AppLayerTxData *txd = (AppLayerTxData *)tx->tx_data_ptr; - if (txd != NULL) { + if (likely(txd != NULL)) { if (flow_flags & STREAM_TOSERVER) { txd->detect_flags_ts = detect_flags; } else { diff --git a/src/output-tx.c b/src/output-tx.c index 9446c39a78..5974168890 100644 --- a/src/output-tx.c +++ b/src/output-tx.c @@ -207,14 +207,13 @@ static TmEcode OutputTxLog(ThreadVars *tv, Packet *p, void *thread_data) break; void * const tx = ires.tx_ptr; tx_id = ires.tx_id; - AppLayerTxData *txd = AppLayerParserGetTxData(ipproto, alproto, tx); - if (txd) { - SCLogDebug("tx %p/%"PRIu64" txd %p: log_flags %x", tx, tx_id, txd, txd->config.log_flags); - if (txd->config.log_flags & BIT_U8(CONFIG_TYPE_TX)) { - SCLogDebug("SKIP tx %p/%"PRIu64, tx, tx_id); - goto next_tx; - } + AppLayerTxData *txd = AppLayerParserGetTxData(ipproto, alproto, tx); + if (txd == NULL) { + /* make sure this tx, which can't be properly logged is skipped */ + logged = 1; + max_id = tx_id; + goto next_tx; } if (list[ALPROTO_UNKNOWN] != 0) { @@ -223,7 +222,13 @@ static TmEcode OutputTxLog(ThreadVars *tv, Packet *p, void *thread_data) goto next_tx; } - LoggerId tx_logged = txd ? txd->logged.flags : 0; + SCLogDebug("tx %p/%"PRIu64" txd %p: log_flags %x", tx, tx_id, txd, txd->config.log_flags); + if (txd->config.log_flags & BIT_U8(CONFIG_TYPE_TX)) { + SCLogDebug("SKIP tx %p/%"PRIu64, tx, tx_id); + goto next_tx; + } + + LoggerId tx_logged = txd->logged.flags; const LoggerId tx_logged_old = tx_logged; SCLogDebug("logger: expect %08x, have %08x", logger_expectation, tx_logged); if (tx_logged == logger_expectation) { @@ -295,9 +300,8 @@ next_logger: if (tx_logged != tx_logged_old) { SCLogDebug("logger: storing %08x (was %08x)", tx_logged, tx_logged_old); - if (txd != NULL) { - txd->logged.flags |= tx_logged; - } + DEBUG_VALIDATE_BUG_ON(txd == NULL); + txd->logged.flags |= tx_logged; } /* If all loggers logged set a flag and update the last tx_id -- 2.47.2