Jeff Lucovsky [Sun, 19 Dec 2021 18:08:59 +0000 (13:08 -0500)]
output/json: Eliminate dangling XFF reference
This commit eliminates a dangling reference caused by the use of
json_object_set. This function adds a reference to the final parameter
-- in this case the object returned by json_string() whereas
json_object_set_new doesn't add the additional reference to the
final parameter.
Jeff Lucovsky [Thu, 16 Dec 2021 14:32:52 +0000 (09:32 -0500)]
rust/dns: Ensure JSON object doesn't get leaked
Ensure js_answers isn't leaked when detailed logging is not in use. This
commit changes how js_answers allocation is performed. Previously, it
was allocated regardless of whether detailed logging was enabled. Now,
it's only allocated if detailed logging is enabled.
Jason Ish [Wed, 17 Nov 2021 01:34:11 +0000 (19:34 -0600)]
rust: fix urls in comments
rustdoc was complaining about the format of the URL in a comment
while trying to generate documentation. Convert the comment to a
non-rustdoc comment for now to satisfy rustdoc.
Philippe Antoine [Thu, 16 Sep 2021 14:54:37 +0000 (16:54 +0200)]
tcp: rejects FIN+SYN packets as invalid
Ticket: #4569
If a FIN+SYN packet is sent, the destination may keep the
connection alive instead of starting to close it.
In this case, a later SYN packet will be ignored by the
destination.
Previously, Suricata considered this a session reuse, and thus
used the sequence number of the last SYN packet, instead of
using the one of the live connection, leading to evasion.
This commit errors on FIN+SYN so that they do not get
processed as regular FIN packets.
Victor Julien [Tue, 5 Oct 2021 12:48:27 +0000 (14:48 +0200)]
stream/tcp: handle RST with MD5 or AO header
Special handling for RST packets if they have an TCP MD5 or AO header option.
The options hash can't be validated. The end host might be able to validate
it, as it can have a key/password that was communicated out of band.
The sender could use this to move the TCP state to 'CLOSED', leading to
a desync of the TCP session.
This patch builds on top of 843d0b7a10bb ("stream: support RST getting lost/ignored")
It flags the receiver as having received an RST and moves the TCP state
into the CLOSED state. It then reverts this if the sender continues to
send traffic. In this case it sets the following event:
Victor Julien [Sun, 31 Oct 2021 21:13:19 +0000 (22:13 +0100)]
af-packet: fix soft lockup issues
The Suricata AF_PACKET code opens a socket per thread, then after some minor
setup enters a loop where the socket is poll()'d with a timeout. When the
poll() call returns a non zero positive value, the AF_PACKET ring will be
processed.
The ringbuffer processing logic has a pointer into the ring where we last
checked the ring. From this position we will inspect each frame until we
find a frame with tp_status == TP_STATUS_KERNEL (so essentially 0). This
means the frame is currently owned by the kernel.
There is a special case handling for starting the ring processing but
finding a TP_STATUS_KERNEL immediately. This logic then skip to the next
frame, rerun the check, etc until it either finds an initialized frame or
the last frame of the ringbuffer.
The problem was, however, that the initial uninitialized frame was possibly
(likely?) still being initialized by the kernel. A data race between the
notification through the socket (the poll()) and the updating of the
`tp_status` field in the frame could lead to a valid frame getting skipped.
Of note is that for example libpcap does not do frame scanning. Instead it
simply exits it ring processing loop. Also interesting is that libpcap uses
atomic loads and stores on the tp_status field.
This skipping of frames had 2 bad side effects:
1. in most cases, the buffer would be full enough that the frame would
be processed in the next pass of the ring, but now the frame would
out of order. This might have lead to packets belong to the same
flow getting processed in the wrong order.
2. more severe is the soft lockup case. The skipped frame sits at ring
buffer index 0. The rest of the ring has been cleared, after the
initial frame was skipped. As our pass of the ring stops at the end
of the ring (ptv->frame_offset + 1 == ptv->req.v2.tp_frame_nr) the code
exits the ring processing loop at goes back to poll(). However, poll()
will not indicate that there is more data, as the stale frame in the
ring blocks the kernel from populating more frames beyond it. This
is now a dead lock, as the kernel waits for Suricata and Suricata
never touches the ring until it hears from the kernel.
The scan logic will scan the whole ring at most once, so it won't
reconsider the stale frame either.
This patch addresses the issues in several ways:
1. the startup "discard" logic was fixed to not skip over kernel
frames. Doing so would get us in a bad state at start up.
2. Instead of scanning the ring, we now enter a busy wait loop
when encountering a kernel frame where we didn't expect one. This
means that if we got a > 0 poll() result, we'll busy wait until
we get at least one frame.
3. Error handling is unified and cleaned up. Any frame error now
returns the frame to the kernel and progresses the frame pointer.
4. If we find a frame that is owned by us (TP_STATUS_USER_BUSY) we
yield to poll() immediately, as the next expected status of that
frame is TP_STATUS_KERNEL.
5. the ring is no longer processed until the "end" of the ring (so
highest index), but instead we process at most one full ring size
per run.
6. Work with a copy of `tp_status` instead of accessing original touched
also by the kernel.
Jason Ish [Tue, 5 Oct 2021 16:44:03 +0000 (10:44 -0600)]
github-ci: pin macos build to 10.15
There is currently a build failure with macos-latest (recently updated)
to 11 in the libhtp test suite code. Not sure if there are other
build issues in libhtp or Suricata at this time.
Victor Julien [Tue, 28 Sep 2021 10:28:54 +0000 (12:28 +0200)]
detect: fix FNs in case of too many prefilter engines
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.
This is only a partial fix however. It's not hard to craft a ruleset that
exceeds the 63-bit space available. A more complete fix is in:
932cf0b6a6ad ("detect: track prefilter by progress, not engine")
However this seems like a too high risk change for a backport into
5.0.
This patch does issue a warning if the condition is detected at start
up, and `-T` does error out on it.
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.
Victor Julien [Wed, 21 Apr 2021 13:20:49 +0000 (15:20 +0200)]
app-layer/pd: only consider actual available data
For size limit checks consider only available data at the stream start
and before any GAPS.
The old check would consider too much data if there were temporary gaps,
like when a data packet was in-window but (far) ahead of the expected
segment.
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.
Victor Julien [Thu, 25 Feb 2021 19:06:40 +0000 (20:06 +0100)]
detect: track base id for xform buffers
Buffers with transforms are based on the non-transformed "base"
buffer, with a new ID assigned and the transform callbacks added.
This patch stores the id of the original buffer in the new buffer
inspect and prefilter structures. This way the buffers with and
without transforms can share some of the logic are progression
of file and body inspection trackers.
Fix handling of file progress tracking for regular file.data along
with transform combinations for the part of the implementation that
uses the File API.
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.
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.
Luke Coughlan [Mon, 26 Apr 2021 12:05:15 +0000 (13:05 +0100)]
flow/bypass: Properly set the ICMP emergency-bypassed value
Currently the ICMP emergency-bypassed value defined in suricata.conf is
overwriting the UDP value rather than correctly setting it for ICMP.
This commit corrects this bug so that the ICMP value can be set as
expected.
Victor Julien [Mon, 1 Feb 2021 21:23:47 +0000 (22:23 +0100)]
app-layer: fix transaction cleanup
Fix a 'skipped' transaction early in the list leading to all further
transactions getting skipped, even if they were fully processed and
ready to be cleaned up.
Jason Ish [Fri, 18 Dec 2020 17:34:30 +0000 (11:34 -0600)]
filestore: fix global counter init in unix socket mode
Move initialization of filestore global counter to PreRunInit,
so they get registered during program initialization, or as
required in unix-socket mode, initialized for each file run.