Skip to content

Commit

Permalink
app-layer: add flag to skip detection on TX
Browse files Browse the repository at this point in the history
Stamus team did discover a problem were a signature can shadow
other signatures.

For example, on a PCAP only containing Kerberos protocol and where the
following signature is matching:

alert krb5 $HOME_NET any -> any any (msg:"krb match"; krb5_cname; content:"marlo"; sid:3; rev:1;)

If we add the following signature to the list of signature

alert ssh $HOME_NET any -> any any (msg:"rr"; content:"rr"; flow:established,to_server; sid:4; rev:2;)

Then the Kerberos signature is not matching anymore.

To understand this case, we need some information:

- The krb5_cname is a to_client keyword
- The signal on ssh is to_server
- Kerberos has unidirectional transaction
- kerberos application state progress is a function always returning 1

As the two signatures are in opposite side, they end up in separate
sig group head.

Another fact is that, in the PCAP, the to_server side of the session
is sent first to the detection. It thus hit the sig group head of
the SSH signature. When Suricata runs detection in this direction
the Kerberos application layer send the transaction as it is existing
and because the alstate progress function just return 1 if the transaction
exists. So Suricata runs DetectRunTx() and stops when it sees that
sgh->tx_engines is NULL.

But the transaction is consumed by the engine as it has been evaluated
in one direction and the kerberos transaction are unidirectional so
there is no need to continue looking at it.

This results in no matching of the kerberos signature as the match
should occur in the evaluation of the other side but the transaction
with the data is already seen has been handled.

This problem was discovered on this Kerberos signature but all
the application layer with unidirectional transaction are impacted.

This patch introduces a flag that can be used by application layer
to signal that the TX should not be inspected. By using this flag
on the directional detect_flags_[ts|tc] the application layer can
prevent the TX to be consumed in the wrong direction.

Application layers with unidirectional TX will be updated
in separate commits to set the flag on the direction opposite
to the one they are.

Ticket: OISF#5799
  • Loading branch information
regit authored and victorjulien committed Mar 31, 2023
1 parent 236869b commit 5aaf507
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 3 deletions.
12 changes: 11 additions & 1 deletion rust/src/applayer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
//! Parser registration functions and common interface

use std;
use crate::core::{self,DetectEngineState,Flow,AppLayerEventType,AppProto};
use crate::core::{self,DetectEngineState,Flow,AppLayerEventType,AppProto,Direction};
use crate::filecontainer::FileContainer;
use crate::applayer;
use std::os::raw::{c_void,c_char,c_int};
Expand Down Expand Up @@ -180,6 +180,14 @@ impl AppLayerTxData {
self.file_flags |= state_flags;
}
}

pub fn set_inspect_direction(&mut self, direction: Direction) {
if direction == Direction::ToClient {
self.detect_flags_ts |= APP_LAYER_TX_SKIP_INSPECT_FLAG;
} else {
self.detect_flags_tc |= APP_LAYER_TX_SKIP_INSPECT_FLAG;
}
}
}

#[macro_export]
Expand Down Expand Up @@ -474,6 +482,8 @@ pub const APP_LAYER_PARSER_TRUNC_TC : u16 = BIT_U16!(8);
pub const APP_LAYER_PARSER_OPT_ACCEPT_GAPS: u32 = BIT_U32!(0);
pub const APP_LAYER_PARSER_OPT_UNIDIR_TXS: u32 = BIT_U32!(1);

pub const APP_LAYER_TX_SKIP_INSPECT_FLAG: u64 = BIT_U64!(62);

pub type AppLayerGetTxIteratorFn = unsafe extern "C" fn (ipproto: u8,
alproto: AppProto,
alstate: *mut c_void,
Expand Down
5 changes: 3 additions & 2 deletions src/app-layer-parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@
#define APP_LAYER_TX_RESERVED12_FLAG BIT_U64(59)
#define APP_LAYER_TX_RESERVED13_FLAG BIT_U64(60)
#define APP_LAYER_TX_RESERVED14_FLAG BIT_U64(61)
#define APP_LAYER_TX_RESERVED15_FLAG BIT_U64(62)

#define APP_LAYER_TX_RESERVED_FLAGS \
(APP_LAYER_TX_RESERVED1_FLAG | APP_LAYER_TX_RESERVED2_FLAG | APP_LAYER_TX_RESERVED3_FLAG | \
Expand All @@ -75,8 +74,10 @@
APP_LAYER_TX_RESERVED8_FLAG | APP_LAYER_TX_RESERVED9_FLAG | \
APP_LAYER_TX_RESERVED10_FLAG | APP_LAYER_TX_RESERVED11_FLAG | \
APP_LAYER_TX_RESERVED12_FLAG | APP_LAYER_TX_RESERVED13_FLAG | \
APP_LAYER_TX_RESERVED14_FLAG | APP_LAYER_TX_RESERVED15_FLAG)
APP_LAYER_TX_RESERVED14_FLAG)

/** should inspection be skipped in that direction */
#define APP_LAYER_TX_SKIP_INSPECT_FLAG BIT_U64(62)
/** is tx fully inspected? */
#define APP_LAYER_TX_INSPECTED_FLAG BIT_U64(63)
/** other 63 bits are for tracking which prefilter engine is already
Expand Down
16 changes: 16 additions & 0 deletions src/detect.c
Original file line number Diff line number Diff line change
Expand Up @@ -1256,6 +1256,22 @@ static DetectTransaction GetDetectTx(const uint8_t ipproto, const AppProto alpro
DetectTransaction no_tx = { NULL, 0, NULL, NULL, 0, 0, 0, 0, 0, };
return no_tx;
}
if (detect_flags & APP_LAYER_TX_SKIP_INSPECT_FLAG) {
SCLogDebug("%" PRIu64 " tx should not be inspected in direction %s. Flags %016" PRIx64,
tx_id, flow_flags & STREAM_TOSERVER ? "toserver" : "toclient", detect_flags);
DetectTransaction no_tx = {
NULL,
0,
NULL,
NULL,
0,
0,
0,
0,
0,
};
return no_tx;
}

const int tx_progress = AppLayerParserGetStateProgress(ipproto, alproto, tx_ptr, flow_flags);
const int dir_int = (flow_flags & STREAM_TOSERVER) ? 0 : 1;
Expand Down

0 comments on commit 5aaf507

Please sign in to comment.