dhcp: mark transactions single-directional to fix tx leak

DHCP is a stateless parser where each datagram is its own standalone,
single-direction transaction. It was creating transactions with
AppLayerTxData::new(), which leaves both SKIP_INSPECT bits clear, so the
engine treats every tx as still needing inspection in both directions.

For a flow that only ever carries one direction (broadcast DHCP, or a
relay seeing one side), the never-observed direction's inspect bit can
never be set, so AppLayerParserTransactionsCleanup() never frees the tx.
The per-flow transaction Vec then grows without bound and every packet
re-scans the whole list, giving O(n^2) CPU and unbounded memory on a
busy DHCP aggregation point.

Use AppLayerTxData::for_direction() like every other stateless parser
(DNS, SNMP, NTP, IKE, KRB5, MQTT, QUIC, SIP, WebSocket, bittorrent-dht)
so the tx carries SKIP_INSPECT for the direction it will never be seen
in. This lets cleanup free completed transactions and also stops the tx
from being inspected (and alerting) twice, once per direction.

Issue: 8621
pull/15740/head
Jeff Lucovsky 1 month ago committed by Victor Julien
parent a6fcbb1ab7
commit 3f86c56c80

@ -1,4 +1,4 @@
/* Copyright (C) 2018-2021 Open Information Security Foundation
/* Copyright (C) 2018-2026 Open Information Security Foundation
*
* You can copy, redistribute or modify this Program under the terms of
* the GNU General Public License version 2 as published by the Free
@ -23,6 +23,7 @@ use suricata_sys::sys::{
use crate::applayer::{self, *};
use crate::core::{ALPROTO_UNKNOWN, IPPROTO_UDP};
use crate::dhcp::parser::*;
use crate::direction::Direction;
use crate::flow::Flow;
use std;
use std::ffi::CString;
@ -89,11 +90,11 @@ pub struct DHCPTransaction {
}
impl DHCPTransaction {
pub fn new(id: u64, message: DHCPMessage) -> DHCPTransaction {
pub fn new(id: u64, message: DHCPMessage, direction: Direction) -> DHCPTransaction {
DHCPTransaction {
tx_id: id,
message,
tx_data: applayer::AppLayerTxData::new(),
tx_data: applayer::AppLayerTxData::for_direction(direction),
}
}
}
@ -132,13 +133,13 @@ impl DHCPState {
Default::default()
}
pub fn parse(&mut self, input: &[u8]) -> bool {
pub fn parse(&mut self, input: &[u8], direction: Direction) -> bool {
match parse_dhcp(input) {
Ok((_, message)) => {
let malformed_options = message.malformed_options;
let truncated_options = message.truncated_options;
self.tx_id += 1;
let transaction = DHCPTransaction::new(self.tx_id, message);
let transaction = DHCPTransaction::new(self.tx_id, message, direction);
self.transactions.push(transaction);
if malformed_options {
self.set_event(DHCPEvent::MalformedOptions);
@ -227,12 +228,23 @@ unsafe extern "C" fn dhcp_state_get_tx_count(state: *mut std::os::raw::c_void) -
return state.tx_id;
}
unsafe extern "C" fn dhcp_parse(
unsafe extern "C" fn dhcp_parse_request(
_flow: *mut Flow, state: *mut std::os::raw::c_void, _pstate: *mut AppLayerParserState,
stream_slice: StreamSlice, _data: *mut std::os::raw::c_void,
) -> AppLayerResult {
let state = cast_pointer!(state, DHCPState);
if state.parse(stream_slice.as_slice()) {
if state.parse(stream_slice.as_slice(), Direction::ToServer) {
return AppLayerResult::ok();
}
return AppLayerResult::err();
}
unsafe extern "C" fn dhcp_parse_response(
_flow: *mut Flow, state: *mut std::os::raw::c_void, _pstate: *mut AppLayerParserState,
stream_slice: StreamSlice, _data: *mut std::os::raw::c_void,
) -> AppLayerResult {
let state = cast_pointer!(state, DHCPState);
if state.parse(stream_slice.as_slice(), Direction::ToClient) {
return AppLayerResult::ok();
}
return AppLayerResult::err();
@ -275,8 +287,8 @@ pub unsafe extern "C" fn SCRegisterDhcpParser() {
state_new: dhcp_state_new,
state_free: dhcp_state_free,
tx_free: dhcp_state_tx_free,
parse_ts: dhcp_parse,
parse_tc: dhcp_parse,
parse_ts: dhcp_parse_request,
parse_tc: dhcp_parse_response,
get_tx_count: dhcp_state_get_tx_count,
get_tx: dhcp_state_get_tx,
tx_comp_st_ts: 1,
@ -310,3 +322,32 @@ pub unsafe extern "C" fn SCRegisterDhcpParser() {
SCLogDebug!("Protocol detector and parser disabled for DHCP.");
}
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn test_tx_skip_inspect_direction() {
let pcap = include_bytes!("discover.pcap");
let payload = &pcap[24 + 16 + 42..];
// a to-server tx should be marked to skip the to-client inspection it
// will never be observed in, and a to-client tx the reverse
let (_rem, message) = parse_dhcp(payload).unwrap();
let ts_tx = DHCPTransaction::new(1, message, Direction::ToServer);
assert_eq!(
APP_LAYER_TX_SKIP_INSPECT_TC,
ts_tx.tx_data.0.flags & APP_LAYER_TX_SKIP_INSPECT_TC
);
assert_eq!(0, ts_tx.tx_data.0.flags & APP_LAYER_TX_SKIP_INSPECT_TS);
let (_rem, message) = parse_dhcp(payload).unwrap();
let tc_tx = DHCPTransaction::new(2, message, Direction::ToClient);
assert_eq!(
APP_LAYER_TX_SKIP_INSPECT_TS,
tc_tx.tx_data.0.flags & APP_LAYER_TX_SKIP_INSPECT_TS
);
assert_eq!(0, tc_tx.tx_data.0.flags & APP_LAYER_TX_SKIP_INSPECT_TC);
}
}

Loading…
Cancel
Save