Victor Julien [Fri, 18 Mar 2022 18:02:45 +0000 (12:02 -0600)]
nfs3: fix partial write record handling
Note: This was more of a manual cherry-pick converting some parsers from
named macros into functions in order to handle more arguments than just
the input data -- Jason Ish
Jeff Lucovsky [Fri, 11 Feb 2022 14:02:39 +0000 (09:02 -0500)]
threads: Honor per-thread stack size setting
Issue: 4550
This commit adjusts the per-thread stack size if a size has been
configured. If the setting has not been configured, the default
per-thread stack size provided by the runtime mechanisms are used.
This commit documents the new per-thread stack-size setting. Some
systems have a small default value that is not suitable for Suricata's
multi-threaded architecture and adjustment may be required.
Jeff Lucovsky [Thu, 6 May 2021 13:49:55 +0000 (09:49 -0400)]
proto: Remove dependency on /etc/protocols
This commit eliminates the dependency on /etc/protocols and equivalent
on other platforms by using a static table of IANA assigned protocol
values (names, description).
Victor Julien [Thu, 17 Feb 2022 12:32:17 +0000 (13:32 +0100)]
radix: improve address range handling
Handle non-exact address ranges from string. This can come directly
from user input, so here it is accepted but the address is converted
to the address range start. A warning will be issued.
Debug validation checks are added to catch this.
This issue could lead to bad input from iprep (with cidr), defrag config
and htp server personalities to produce a bad radix tree.
Victor Julien [Tue, 15 Feb 2022 19:43:27 +0000 (20:43 +0100)]
detect/iponly: fix netmask handling
If the ipaddress was not the address range start, it was not masked to turn
it into that. So 1.2.3.4/24 was not stored as address 1.2.3.0 with netmask 24,
but as 1.2.3.4 with netmask 24. This was then propagated into the radix tree,
where it was used as an exact key in exact lookups, giving unexpected results.
This patch implements the netmask handling for IPv4 and IPv6, and adds a set
of tests for it.
Victor Julien [Fri, 11 Feb 2022 14:50:01 +0000 (15:50 +0100)]
radix: fix FP/FN issue in IP-only
A bug was reported about the IP-only rules not correctly matching. This was
traced to the rules in question not getting recorded into the IP-only radix
tree correctly.
Sequence:
- 100.117.241.0/25 inserted into the tree
- 100.117.241.0/26 inserted into the tree
Both are part of the same radix node, but recorded by their different netmasks
in the user data portion.
The IP-only engine first does a search to get to the user data it may need to
include. It does so for with `SCRadixFindKeyIPV4ExactMatch` for single IPs, or
using `SCRadixFindKeyIPV4Netblock` in case of a netblock. Any "match" from
either of these is considered an "exact match" by the IP-only setup code.
This exact match expectation turned out to be wrong and
`SCRadixFindKeyIPV4Netblock` behaved more like "best match" instead, which is
a non-exact match, but its the next best match if no exact match is found.
The way the look up for 100.117.241.64/26 went wrong, is that it returned
the user data for 100.117.241.0/26. This happens as follows:
- first it would do an exact find, which didn't give a result
- then it removed bits from the keystream until it found a matching node
and explore if any of the netmasks it contained matched. Here the first
step of the bug started:
it considered the netmask (with user data) a match that matched the
number of bits of the matching key, but not of the actual range netmask cidr
value.
So in this case the number of shared bits between `100.117.241.0/25` and
`100.117.241.64/26` was 25, so it assumed that the user data for the
netmask 25 was the match.
To summarize this step, there are 2 problems with this:
1. it returns a match on something that isn't an exact match
2. it considered the wrong netmask value
- the radix code then took the returned node, and did the netmask check
again. This time it did use its own netmask value, so this time
it did find the netmask 26 (+ user data). However because of the node that
was returned, this netmask (+user data) belongs to `100.117.241.0`, not to
`100.117.241.64`.
- the IP-only detection code was satisfied with what it assumed to be
"exact match" and just updated the user data to include the user data that
should have been associated with `100.117.241.64/26` to `100.117.241.0/26`.
This patch addresses the issue as follows:
It makes `SCRadixFindKeyIPV4Netblock` also return an exact match by propagating
the netmask in the search and in the evaluation of the stored netmasks.
It does away with the secondary netmask (+user data) evaluation.
`SCRadixFindKeyIPV4Netblock` is expected to handle this correctly.
The IP-only engine will fall back to the "not found" path, which does an explicit
"best match" lookup and then insert a new entry into the radix tree based on
the user data of the "best match".
Victor Julien [Fri, 18 Feb 2022 09:19:04 +0000 (10:19 +0100)]
output: fix timestamp missing usecs
On ARM 32bit with Musl `tv_usecs` is defined as `int64_t` which lead to
CreateIsoTimeString() printing all zeros on the usecs. Work around this
by first assigning to a `int64_t` and then updating the expected format
string to accept `int64_t`.
Jason Ish [Thu, 20 Jan 2022 18:08:33 +0000 (12:08 -0600)]
logging: change ownership of application log if needed
When running with privilege dropping, the application log file
is opened before privileges are dropped resulting in Suricata
failing to re-open the file for file rotation.
If needed, chown the application to the run-as user/group after
opening.
Jason Ish [Thu, 20 Jan 2022 17:40:24 +0000 (11:40 -0600)]
startup: initialize run as user info sooner
Initialize the run-as user info after loading the config, but
before setting up logging (previously it was done while initializing
signal handlers). This will allow the log file to be given the
correct permissions if Suricata is configured to run as a non-root
user.
>>> CID 1499365: (UNINIT)
>>> Using uninitialized value "infstream.total_out" when calling "inflate".
98 int result = inflate(&infstream, Z_NO_FLUSH);
99 switch(result) {
100 case Z_STREAM_END:
101 break;
102 case Z_OK:
103 break;
>>> CID 1499365: (UNINIT)
>>> Using uninitialized value "infstream.total_out" when calling "inflate".
98 int result = inflate(&infstream, Z_NO_FLUSH);
99 switch(result) {
100 case Z_STREAM_END:
101 break;
102 case Z_OK:
103 break;
*** CID 1499363: Error handling issues (CHECKED_RETURN)
/src/util-file-swf-decompression.c: 97 in FileSwfZlibDecompression()
91
92 infstream.avail_in = (uInt)compressed_data_len;
93 infstream.next_in = (Bytef *)compressed_data;
94 infstream.avail_out = (uInt)decompressed_data_len;
95 infstream.next_out = (Bytef *)decompressed_data;
96
>>> CID 1499363: Error handling issues (CHECKED_RETURN)
>>> Calling "inflateInit_(&infstream, "1.2.11", 112)" without checking return value. This library function may fail and return an error code.
97 inflateInit(&infstream);
98 int result = inflate(&infstream, Z_NO_FLUSH);
99 switch(result) {
100 case Z_STREAM_END:
101 break;
102 case Z_OK:
Philippe Antoine [Mon, 29 Nov 2021 09:59:10 +0000 (10:59 +0100)]
ftp: do not set alproto if one was already found
Ticket: 4857
If a pattern such as GET is seen ine the beginning of the
file transferred over ftp-data, this flow will get recognized
as HTTP, and a HTTP state will be created during parsing.
Thus, we cannot override directly alproto's values
This solves the segfault, but not the logical bug that the flow
should be classified as FTP-DATA instead of HTTP
Angelo Mirabella [Thu, 20 Jan 2022 14:52:33 +0000 (14:52 +0000)]
stream-tcp-reassemble: fix reassembly direction for FIN packets
Suricata invokes the stream reassembly logic only for the current packet
direction if the packet contains a FIN flag. However, this does not
handle the case in which the packet ACKs data from the opposing direction.
This patch forces the invocation of the stream reassembly logic
on both direction when Suricata sees a FIN packet.
Victor Julien [Fri, 5 Nov 2021 19:05:43 +0000 (20:05 +0100)]
packetpool: reset PacketRelease on return to pool
Reset PacketRelease callback to make sure its not set to a capture
specific callback.
As an example:
0x000055e00af09d35 in AFPReleaseDataFromRing (p=0x7f1d884cb830) at source-af-packet.c:653
0x000055e00af09dd0 in AFPReleasePacket (p=0x7f1d884cb830) at source-af-packet.c:678
0x000055e00ab53d7e in TmqhOutputPacketpool (t=0x55e00fb79250, p=0x7f1d884cb830) at tmqh-packetpool.c:465
0x000055e00af08dec in TmThreadsSlotProcessPkt (tv=0x55e00fb79250, s=0x55e012134790, p=0x7f1d884cb830) at tm-threads.h:201
0x000055e00af08e70 in TmThreadsCaptureInjectPacket (tv=0x55e00fb79250, p=0x7f1d884cb830) at tm-threads.h:221
0x000055e00af08f2e in TmThreadsCaptureHandleTimeout (tv=0x55e00fb79250, p=0x0) at tm-threads.h:245
0x000055e00af0ba76 in ReceiveAFPLoop (tv=0x55e00fb79250, data=0x7f1d884ccb60, slot=0x55e01198e4b0) at source-af-packet.c:1321
0x000055e00ab55257 in TmThreadsSlotPktAcqLoop (td=0x55e00fb79250) at tm-threads.c:312
0x00007f1dca9d5609 in start_thread (arg=<optimized out>) at pthread_create.c:477
0x00007f1dca7c6293 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
Here the packet was a pseudo packet to handle a timeout condition. But
the ReleasePacket callback was still set to AFPReleasePacket from a
previous use of the Packet.
Jason Ish [Fri, 16 Oct 2020 15:43:29 +0000 (09:43 -0600)]
af-packet: use configured cluster-id when checking for fanout
When testing for fanout support a cluster-id of 1 was always being
used instead of the configured cluster-id. This limited fanout
support to only one Suricata instance.
Instead of hardcoding an ID of 1, use the configured cluster-id.
Also make cluster_id a uint16_t instead of an int in AFPThreadVars.
Victor Julien [Thu, 13 Jan 2022 11:13:43 +0000 (12:13 +0100)]
stream: fix stream pruning being too aggressive
Pruning of StreamBufferBlocks could remove blocks that fell entirely
after the target offset due to a logic error. This could lead to data
being evicted that was still meant to be processed in theapp-layer
parsers.
Jason Ish [Tue, 21 Dec 2021 22:34:05 +0000 (16:34 -0600)]
dns: create transaction even if z-bit was set
It appears that DNS servers will still process a DNS request even if the
z-bit is set, our parser will fail the transaction. So create the
transaction, but still set the event.
Eric Leblond [Wed, 17 Nov 2021 15:43:23 +0000 (16:43 +0100)]
profiling: fix profiling with sample rate
Rules profiling was returning invalid results when used with sample
rate. The problem was that the sample condition was run twice in the
packet flow. As a result, the second pass was not initializing the
variable storing the initial CPU ticks and the resulting performance
counters were reporting invalid values.
Jason Ish [Wed, 10 Nov 2021 22:38:35 +0000 (16:38 -0600)]
smtp: log transaction even if no email present
The SMTP transaction logger was not writing the log if the email
portion of the logger failed, such as in the case of STARTTLS
where this is no email decoded.
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.
projects / thirdparty / suricata.git / log
