Skip to content

Commit

Permalink
app-layer-parser: give direction to progress func
Browse files Browse the repository at this point in the history
The tx progress functions are expecting a direction and were given
a flow flags. As a result, they were not reporting correctly the
status if a DetectRunScratchPad flow_flags was containing some other
bits in the flag.

One case was when a signature was alterating the stream analysis
and triggering the addition of the STREAM_FLUSH flags.

The consequences are quite severe as the transactions are pilling
up waiting to be inspected causing sometimes a 10x performance hit
on pcap parsing. Also as the inspection was not done, Suricata is
missing a part of the alerts.

This was discovered when working on the following set of signatures:

alert ssh $HOME_NET any -> any any (msg:"pcre without content"; pcre:"/rabbit/"; sid:1; rev:1;)
alert smb $HOME_NET any -> any any (msg:"smb share content"; smb.share; content:"C"; sid:2; rev:1;)

When the first one is present the second is not triggering even
though the pcap file had no ssh inside. This is due to the fact
that the ssh signature was triggering the STREAM_FLUSH flag to
be set on the flowflags of the packet. But the application
layer will ask the smb state progress via

r = alp_ctx.ctxs[FlowGetProtoMapping(ipproto)][alproto].
        StateGetProgress(alstate, flags);

passing it the flow flags but the smb function is expecting
a direction so we end up in a unplanned case

pub unsafe extern "C" fn rs_smb_tx_get_alstate_progress(tx: *mut ffi::c_void,
                                                  direction: u8)
...
if direction == Direction::ToServer as u8 && tx.request_done {

This leads the signature to not be evaluated correctly.

Ticket: OISF#5799
  • Loading branch information
regit authored and victorjulien committed Mar 31, 2023
1 parent 578f328 commit 29e7027
Showing 1 changed file with 3 additions and 2 deletions.
5 changes: 3 additions & 2 deletions src/app-layer-parser.c
Original file line number Diff line number Diff line change
Expand Up @@ -1127,8 +1127,9 @@ int AppLayerParserGetStateProgress(uint8_t ipproto, AppProto alproto,
if (unlikely(IS_DISRUPTED(flags))) {
r = StateGetProgressCompletionStatus(alproto, flags);
} else {
r = alp_ctx.ctxs[FlowGetProtoMapping(ipproto)][alproto].
StateGetProgress(alstate, flags);
uint8_t direction = flags & (STREAM_TOCLIENT | STREAM_TOSERVER);
r = alp_ctx.ctxs[FlowGetProtoMapping(ipproto)][alproto].StateGetProgress(
alstate, direction);
}
SCReturnInt(r);
}
Expand Down

0 comments on commit 29e7027

Please sign in to comment.