Victor Julien [Tue, 14 Sep 2021 08:35:18 +0000 (10:35 +0200)]
detect: track prefilter by progress, not engine
Fix FNs in case of too many prefilter engines. A transaction was tracking
which engines have run using a u64 bit array. The engines 'local_id' was
used to set and check this bit. However the bit checking code didn't
handle int types correctly, leading to an incorrect left shift result of
a u32 to a u64 bit value.
This commit addresses that by fixing the int handling, but also by
changing how the engines are tracked.
To avoid wasting prefilter engine tracking bit space, track what
ran by the progress they are registered at, instead of the individual
engine id's. While we can have many engines, the protocols use far
fewer unique progress values. So instead of tracking for dozens of
prefilter id's, we track for the handful of progress values.
To allow for this the engine array is sorted by tx_min_progress, then
app_proto and finally local_id. A new field is added to "know" when
the last relevant engine for a progress value is reached, so that we
can set the prefilter bit then.
A consquence is that the progress values have a ceiling now that
needs to fit in a 64 bit bitarray. The values used by parsers currently
does not exceed 5, so that seems to be ok.
Victor Julien [Fri, 3 Sep 2021 15:04:02 +0000 (17:04 +0200)]
detect: unify alert handling; fix bugs
Unify handling of signature matches between various rule types and
between noalert and regular rules.
"noalert" sigs are added to the alert queue initially, but removed
from it after handling their actions. This way all actions are applied
from a single place.
Make sure flow drop and pass are mutually exclusive.
The above addresses issue with pass and drops not getting applied
correctly in various cases.
If an HTTP2 file was within only ont DATA frame, the filetracker
would open it and close it in the same call, preventing the
firther call to incr_files_opened
Victor Julien [Thu, 18 Mar 2021 10:05:35 +0000 (11:05 +0100)]
nfs: don't reuse file transactions
After a file has been closed (CLOSE, COMMIT command or EOF/SYNC part of
READ/WRITE data block) mark it as such so that new file commands on that
file do not reuse the transaction.
When a file transfer is completed it will be flagged as such and not be
found anymore by the NFSState::get_file_tx_by_handle() method. This forces
a new transaction to be created.
Victor Julien [Thu, 18 Mar 2021 13:38:33 +0000 (14:38 +0100)]
filestore: store chunks in packet direction
Storing too early can lead to files being considered TRUNCATED if the
TCP state is not yet CLOSED when logging is triggered. This has been
observed with FTP-DATA and might also be an issue with simple HTTP.
Philippe Antoine [Sun, 28 Mar 2021 15:53:50 +0000 (17:53 +0200)]
rust: bump bitflags dependency version
So that lexical-core, needed by nom, and using bitflags
is used with version 0.7.5 instead of version 0.7.0
which fixed the fact that BITS is now a reserved keyword
in nightly version
Victor Julien [Wed, 18 Aug 2021 18:14:48 +0000 (20:14 +0200)]
threading: don't pass locked flow between threads
Previously the flow manager would share evicted flows with the workers
while keeping the flows mutex locked. This reduced the number of unlock/
lock cycles while there was guaranteed to be no contention.
This turns out to be undefined behavior. A lock is supposed to be locked
and unlocked from the same thread. It appears that FreeBSD is stricter on
this than Linux.
This patch addresses the issue by unlocking before handing a flow off
to another thread, and locking again from the new thread.
Issue was reported and largely analyzed by Bill Meeks.
Eric Leblond [Sun, 15 Aug 2021 10:17:23 +0000 (12:17 +0200)]
flow: fix a debug assert
As the FlowBypassedTimeout function is interacting with the capture
method it is possible that the return changes between the call that
did trigger the timeout and the actual state (ie if packets arrive
in between the two calls). So we should not use the call to
FlowBypassedTimeout in the assert.
Eric Leblond [Sat, 14 Aug 2021 21:05:03 +0000 (23:05 +0200)]
flow: more accurate flow counters
Don't increment the flow timeout counter for flows that are not
really timeout (as use_cnt is non zero). And also don't take into
account bypassed flows in the counter for flow timeout in use.
Victor Julien [Mon, 30 Aug 2021 08:53:49 +0000 (10:53 +0200)]
flow/worker: handle timeout edge case
In the flow worker timeout path it is assumed that we can hand off
flows to the recycler after processing, implying that `Flow::use_cnt` is 0.
However, there was a case where this assumption was incorrect.
When during flow timeout handling the last processed data would trigger a
protocol upgrade, two additional pseudo packets would be created that were
then pushed all the way through the engine packet paths. This would mean
they both took a flow reference and would hold that until after the flow
was handed off to the recycler. Thread safety implementation would make
sure this didn't lead to crashes.
This patch handles this case returning these packets to the pool from
the timeout handling.
Jason Ish [Wed, 30 Jun 2021 16:03:35 +0000 (10:03 -0600)]
rust/ike: suppress some compile warnings when not debug
Due to ef5755338fa6404b60e7f90bfbaca039b2bfda1e, the variables
that are only used for debug output now emit unused variable
warnings when Suricata is not built with debug. Prefix these
variables with _ to suppress these warnings.
Victor Julien [Thu, 13 May 2021 06:06:11 +0000 (08:06 +0200)]
detect: set event if max inspect buffers exceeded
If a parser exceeds 1024 buffers we stop processing them and
set a detect event instead. This is to avoid parser bugs as well as
crafted bad traffic leading to resources starvation due to excessive
loops.
Sascha Steinbiss [Mon, 10 May 2021 12:54:47 +0000 (14:54 +0200)]
detect/mqtt: add topic inspection limit
We add a new 'mqtt.(un)subscribe-topic-match-limit' option
to allow a user to specify the maximum number of topics in
a MQTT SUBSCRIBE or UNSUBSCRIBE message to be evaluated
in detection.
Eric Leblond [Fri, 28 May 2021 09:38:18 +0000 (11:38 +0200)]
stream/tcp: avoid evasion linked to ACK handling
Actual code will completely discard TCP analysis of a packet that
don't have the ACK bit set but have a ACK value set. This will be
for example the case of all SYN packets that have a ACK value.
Problem is that these type of packets are legit for the operating
systems and for the RFC. The consequence is that an attacker
sending a SYN packet with a non null ACK value will open succesfully
a TCP session to its target and this session will have no protocol
discovery, no TCP streaming and no application layer analysis.
Result is a quasi full evasion of the TCP stream that will only
appear in the flow log if this log is enable or alert on tcp-pkt
signature that are uncommon.
The patch is updating the code to only discard packets that do not
have the SYN flag set. This prevents the evasion and complies with the
RFC that states that the ACK bit should always be set once the
TCP session is established.
Jason Ish [Mon, 21 Jun 2021 20:00:45 +0000 (14:00 -0600)]
rust/template: suppress unread variable warning
Suppress the warning about an unused variable in the template
parser. As this is just a template I think this is OK, however
master should make sure this variable is used, if only to be
more self documenting.
Mats Klepsland [Thu, 27 May 2021 10:02:55 +0000 (12:02 +0200)]
thresholds: Fix buffer overflow in threshold context
th_entry is resized using ThresholdHashRealloc() every time a rule with
a threshold using by_rule tracking is added. The problem is that this is
done before the rules are reordered, so occasionally a rule with by_rule
tracking gets a higher signature number (after reordering) than the
number of th_entries allocated, causing Suricata to crash.
This commit fixes this by allocating th_entries after all the rules are
loaded and reordered.
Backtrace from core dump:
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x000000000051b381 in ThresholdHandlePacket (p=p@entry=0x7fb0080f3960, lookup_tsh=0x51, new_tsh=new_tsh@entry=0x7fb016c316e0, td=td@entry=0x14adedf0, sid=9800979, gid=1, pa=0x7fb0080f3b18)
at detect-engine-threshold.c:415
415>---- if (TIMEVAL_DIFF_SEC(p->ts, lookup_tsh->tv1) < td->seconds) {
Jason Ish [Thu, 25 Feb 2021 17:16:28 +0000 (11:16 -0600)]
unix-socket: reset to ready state on startup
As part of commit ea15282f47c6ff781533e3a063f9c903dd6f1afb,
some initialization was moved to happen even in unix socket mode,
however, this initialization does setup some loggers that can only have
one instance enabled (anomaly, drop, file-store).
This will cause these loggers to error out on the first pcap, but work
on subsequent runs of the pcap as some deinitialization is done after
each pcap.
This fix just runs the post pcap-file deinitialization routine to
reset some of the initialization done on startup, like is done after
running each pcap in unix socket mode.