Philippe Antoine [Fri, 27 Jun 2025 07:31:24 +0000 (09:31 +0200)]
rust/dns: fix clippy char_indices_as_byte_indices
error: indexing into a string with a character position where a byte index is expected
--> src/dns/detect.rs:45:39
|
45 | let code: u8 = opcode[i..].parse().map_err(|_| ())?;
| ^
|
= note: a character can take up more than one byte, so they are not interchangeable
note: position comes from the enumerate iterator
--> src/dns/detect.rs:36:10
|
36 | for (i, c) in opcode.chars().enumerate() {
| ^ ^^^^^^^^^^^
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#char_indices_as_byte_indices
= note: `#[deny(clippy::char_indices_as_byte_indices)]` on by default
help: consider using `.char_indices()` instead
|
36 - for (i, c) in opcode.chars().enumerate() {
36 + for (i, c) in opcode.char_indices() {
Victor Julien [Tue, 10 Jun 2025 09:33:03 +0000 (11:33 +0200)]
threading: fix shutdown of IPS autofp modes
For IPS modes with a verdict thread in autofp there was an issue with
the verdict thread not shutting down, leading to a long shutdown time
until an error condition was reached.
The problem was that when the packet threads, of which the verdict
thread is one, were told to enter their flow timeout loop the verdict
thread got stuck as it immediately progressed to THV_RUNNING_DONE
instead of the expected THV_FLOW_LOOP.
This patch updates the shutdown logic to only apply the flow timeout
logic to the relevant threads, and skip the verdict thread(s).
Add TM_FLAG_VERDICT_TM to indicate a thread has a verdict module to more
explicitly shut it down.
Victor Julien [Wed, 30 Apr 2025 08:20:10 +0000 (10:20 +0200)]
threads: fix autofp shutdown race condition
Sometimes a single flow pcap would log 2 flows. It turns out FlowWorkToDoCleanup
ran before all the packet threads had processed their "wire" packets. It then
removed a flow that a wire packet would still have needed, leading to the worker
thread creating a new flow for it.
This could happen due to the logic in TmThreadDisableReceiveThreads which calls
TmThreadDrainPacketThreads to made sure it only returns when all autofp-workers
have processed all the packets the autofp-capture thread fed to them.
However, the way it checked this is by checking the size of the autofp-worker's
input queue. If 0, it assumes it is done.
What this missed, is that a worker thread could have just taken the last packet
from the input queue, but it is not yet done processing it. If then the
FlowWorkToDoCleanup is ran as well, it would race the worker thread to the flow
handling logic. When it won, the flow was evicted and the packet thread
created a new flow.
This patch improves the shutdown logic to force the worker threads to
enter a "flow loop" (THV_FLOW_LOOP) state before moving on to the
FlowWorkToDoCleanup step. This makes sure that any in progress packets
in the worker threads have been processed.
flow-manager: move time check after RUNNNING state change
When running in pcap-file mode and with a continous directory
reading, the provided directory can be empty.
By having no packets and being in offline mode, the initial packet timestamp
is never set. However, Flow Manager waited until the timestamp was set
to proceed with transferring its state to "RUNNING".
Other pcap-related threads (RX / workers) are set in RUNNING state while
waiting for the PCAP to appear in the directory.
As a result, the main Suricata-Main thread timed out after the default
60 seconds budget for threads to turn from INIT_DONE to RUNNING state.
Jeff Lucovsky [Tue, 17 Dec 2024 12:56:42 +0000 (07:56 -0500)]
detect/content: account for distance variables
Under some cases (below), the depth and offset values are used
twice. This commit disregards the distance variable (if any), when
computing the final depth.
These rules are logically equivalent::
1. alert tcp any any -> any 8080 (msg:"distance name"; flow:to_server; content:"Authorization:"; content:"5f71ycy"; distance:0; byte_extract:1,0,option_len,string,relative; content:!"|38|"; distance:option_len; within:1; content:"|37|"; distance:-1; within:1; content:"|49|"; distance:option_len; within:1; sid:1;)
2. alert tcp any any -> any 8080 (msg:"distance number"; flow:to_server; content:"Authorization:"; content:"5f71ycy"; distance:0; byte_extract:1,0,option_len,string,relative; content:!"|38|"; distance:7; within:1; content:"|37|"; distance:-1; within:1; content:"|49|"; distance:option_len; within:1; sid:2;)
The differences:
Rule 1: content:!"|38|"; distance:option_len; within:1; //option_len == 7
Rule 2: content:!"|38|"; distance:7; within:1;
Without this commit, rule 2 triggers an alert but rule 1 doesn't.
warning: using `contains()` instead of `iter().any()` is more efficient
--> src/http2/http2.rs:267:20
|
267 | if block.value.iter().any(|&x| x == b'@') {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `block.value.contains(&b'@')`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_contains
= note: `#[warn(clippy::manual_contains)]` on by default
Victor Julien [Mon, 17 Mar 2025 20:19:13 +0000 (21:19 +0100)]
datasets: set higher hashsize limits
To avoid possible upgrade issues, allow higher defaults than in the
master branch. Add some upgrade guidance and a note that defaults will
probably be further reduced.
Philippe Antoine [Tue, 18 Mar 2025 09:55:39 +0000 (10:55 +0100)]
detect: add configurable limits for datasets
Ticket: 7615
Avoids signatures setting extreme hash sizes, which would lead to very
high memory use.
Default to allowing:
- 65536 per dataset
- 16777216 total
To override these built-in defaults:
```yaml
datasets:
# Limits for per rule dataset instances to avoid rules using too many
# resources.
limits:
# Max value for per dataset `hashsize` setting
#single-hashsize: 65536
# Max combined hashsize values for all datasets.
#total-hashsizes: 16777216
```
Jason Ish [Mon, 17 Mar 2025 16:35:57 +0000 (10:35 -0600)]
af-packet: delay setting default-packet-size for af-packet
AF_PACKET needs more information about its configuration before we can
set the default packet size, so on startup, leave unset in suricata.c
if in AF_PACKET mode.
If defrag is enabled, use a default packet size of 9k for tpacket-v2.
This can still lead to truncation events, then the user can increase
their 'default-packet-size'.
Tpacket-v3 does not need an increased packet size as it will handle
any size of packet that is smaller than the configured block size
which now has a default of 128k.
9k for the snap is somewhat arbitrary but is large enough for the
common 9000 jumbo frame plus some extra headers including tpacket
headers.
Jason Ish [Wed, 12 Mar 2025 21:58:43 +0000 (15:58 -0600)]
af-packet: make tpacket-v2 block size configurable
With the change of the default tpacket-v2 block size from 32k to 128k,
allow it to be configurable for users who may want to make it larger,
or revert it back to the pre 7.0.9 default of 32k.
Jason Ish [Wed, 12 Mar 2025 18:34:31 +0000 (12:34 -0600)]
af-packet: warn if defrag not suitable for mode
AF_PACKET defrag should not be used for inline modes. Its possible that
a packet received could be larger than can be set when defrag is
enabled, so warn if disabled for inline use.
Likewise, warn if defrag is disabled for IDS use, or non-inline mode.
Jason Ish [Wed, 12 Mar 2025 18:31:08 +0000 (12:31 -0600)]
af-packet: check defrag value even if cluster-type not set
If cluster-type was not set we default to "cluster_flow" with defrag
always on. Instead check for defrag value and disable defrag if disabled
by the user.
Philippe Antoine [Tue, 17 Dec 2024 14:06:25 +0000 (15:06 +0100)]
detect: limit base64_decode `bytes` to 64KiB
Ticket: 7613
Avoids potential large per-thread memory allocation. A buffer with the
size of the largest decode_base64 buffer size setting would be allocated
per thread. As this was a u32, it could mean a per-thread 4GiB memory
allocation.
64KiB was already the built-in default for cases where bytes size wasn't
specified.
Victor Julien [Fri, 29 Nov 2024 13:37:08 +0000 (14:37 +0100)]
stream: RST no longer acks all data
Since forever (1578ef1e3e8a24d0cc615430c4e6bec1fefdad28) a valid RST
would update the internal `last_ack` representation to include all
unack'd data. This was originally done to make sure the unACK'd data was
inspected/processed at flow timeout.
It was observed however, that if GAPs existed in this unACK'd data, a
GAP could be reported in the stats and a GAP event would be raised. This
doesn't make sense, as missing segments in the unACK'd part of the
stream are completely normal. Segments simply do not all arrive in
order.
It turns out that the original behavior of updating `last_ack` to
include all unACK'd data is no longer needed.
For raw stream inspection, the detection engine will already include the
unACK'd data on flow end.
For app-layer updates the unACK'd data is often harmful, as the data
often has GAPs. Parser like the http parser would report these GAPs and
could also get confused about the post-GAP data being a new transaction
including a file. This lead to many reported errors and fantom txs and
files.
Since the GAP detection uses `last_ack` to determine GAPs, not moving
`last_ack` addresses the GAP false positives.
Philippe Antoine [Tue, 25 Feb 2025 09:54:13 +0000 (10:54 +0100)]
detect: delay tx cleanup in some edge case
Ticket: 7552
f->sgh_toserver may be NULL but because FLOW_SGH_TOSERVER is unset
and thus, we want to delay cleanup until detection has really been
run with the right signature group head.
This may happen for a rule using
`alert tcp any any -> any any` and
a app-layer keyword to client
with a app-layer supporting both udp and tcp
with stream.midstream=true
and with the first packet of a flow being a server response
In this case, we swap the flow and reset its signature group heads
Philippe Antoine [Tue, 25 Feb 2025 09:49:41 +0000 (10:49 +0100)]
detect: reset signature groups when reversing flow
Ticket: 7552
When we use midstream, and the first packet we see of a flow is
a response from server, and we want to match on some signature
to client :
- we had first set sgh_toserver/FLOW_SGH_TOSERVER as we first
thought this was a packet to server
- we then swap/reverse the flow, so sgh_toclient becomes sgh_toserver
but it contains signatures to server and cannot match our
to_client signature
The detect engine with DetectRunSetup will set again the
signatures group heads properly
files: append data on closing even with FILE_NOSTORE
Ticket: 7577
When HTTP1 post multipart handles a small file, it will call
HTPFileClose with some data
This data needs to be appended to the streaming buffer for usage
by file.data keyword even if we do not end up storing the file
Philippe Antoine [Fri, 31 May 2024 08:39:16 +0000 (10:39 +0200)]
app-layer: track modified/processed txs
To optimize detection, and logging, to avoid going through
all the live transactions when only a few were modified.
Two boolean fields are added to the tx data: updated_tc and ts
The app-layer parsers are now responsible to set these when
needed, and the logging and detection uses them to skip
transactions that were not updated.
There may some more optimization remaining by when we set
both updated_tc and updated_ts in functions returning
a mutable transaction, by checking if all the callers
are called in one direction only (request or response)
Philippe Antoine [Fri, 21 Feb 2025 09:38:06 +0000 (10:38 +0100)]
quic: discard late retry packets
Ticket: 7556
See RFC 9000 section 17.2.5.2 :
After the client has received and processed an Initial
or Retry packet from the server,
it MUST discard any subsequent Retry packets that it receives.
Philippe Antoine [Mon, 17 Feb 2025 10:13:20 +0000 (11:13 +0100)]
quic: handle fragmented hello over multiple packets
Ticket: 7556
To do so, we need to add 2 buffers (one for each direction)
to the QuicState structure, so that on parsing the second packet
with hello/crypto fragment, we still have the data of the first
hello/crypto fragment.
Use a hardcoded limit so that these buffers cannot grow indefinitely
and set an event when reaching the limit
Victor Julien [Thu, 13 Feb 2025 13:30:39 +0000 (14:30 +0100)]
tls: more permissive empty data eof check
If not all data is ACK'd during the FIN session shutdown, the last calls
to the parser can be with a non-NULL data pointer, but a input length of
0. This wasn't considered by the EOF check, which then lead to it being
seen as an error. No event was raised, but the tls error stats were
incremented.
Jeff Lucovsky [Thu, 12 Dec 2024 13:49:20 +0000 (08:49 -0500)]
output/log: Remove extraneous error message
Issue: 7447
When the output file can't be opened, 2 error messages are displayed
for the same problem. The second message doesn't add value and lacks
context (error reason, e.g., "Permission denied").
Retaining the second message as a debug message.
Without this commit:
Error: logopenfile: Error opening file: "/home/jlucovsky/src/jal/suricata-verify/tests/bug-5198/output/noperms/eve.1.json": Permission denied [SCLogOpenFileFp:util-logopenfile.c:428]
Error: logopenfile: Unable to open slot 1 for file /home/jlucovsky/src/jal/suricata-verify/tests/bug-5198/output/noperms/eve.json [LogFileEnsureExists:util-logopenfile.c:737]
Error: runmodes: unable to initialize sub-module eve-log.stats [RunModeInitializeEveOutput:runmodes.c:692]
Jeff Lucovsky [Wed, 11 Dec 2024 13:26:01 +0000 (08:26 -0500)]
output/log: Improve error handling
This commit improves error handling for cases when file(s) cannot be
opened.
- Return NULL if file object can't be opened
- checks whether the file object has been opened before
dereferencing the per-file context.
Giuseppe Longo [Wed, 20 Mar 2024 17:31:42 +0000 (18:31 +0100)]
output/json: check 5-tuple values prior to logging
This commit enhances the JSON output by introducing a feature for conditional port logging.
Now, port logging is dependent on the underlying protocol
(such as TCP, UDP, or SCTP), where port information is pertinent, while it
avoids unnecessary logging for protocols where a port is not utilized (e.g. ARP).
Furthermore, this update ensures that IP addresses and the protocol have
meaningful values set, rather than being logged as empty strings.
These changes will make each log entry more precise, eliminating cases where
5-tuple fields are empty or set to zero, indicating the absence of a field.
Backported to address ticket: https://redmine.openinfosecfoundation.org/issues/7460
Victor Julien [Tue, 3 Dec 2024 10:36:27 +0000 (11:36 +0100)]
flow/manager: fix multi instance row tracking
In multi instance flow manager setups, each flow manager gets a slice
of the hash table to manage. Due to a logic error in the chunked
scanning of the hash slice, instances beyond the first would always
rescan the same (first) subslice of their slice.
The `pos` variable that is used to keep the state of what the starting
position for the next scan was supposed to be, was treated as if it held
a relative value. Relative to the bounds of the slice. It was however,
holding an absolute position. This meant that when doing it's bounds
check it was always considered out of bounds. This would reset the sub-
slice to be scanned to the first part of the instances slice.
This patch addresses the issue by correctly handling the fact that the
value is absolute.
projects / thirdparty / suricata.git / log
