- Declare the profiling name variable once we know the actual size allowed
for the name instead of before. Prevents an snprintf truncation warning.
- Account for the null byte when calculating how much space is left to
avoid an snprintf format string truncation warning.
CC unix-manager.o
unix-manager.c:258:13: warning: Use of memory after it is freed [unix.Malloc]
258 | if (item->fd == fd) {
| ^~~~~~~~
1 warning generated.
stream-tcp.c:1938:16: warning: Access to field 'next' results in a dereference of a null pointer (loaded from variable 'tail') [core.NullDereference]
1938 | tail->next = old_head;
| ~~~~ ^
1 warning generated.
stream-tcp.c:1982:5: warning: Potential leak of memory pointed to by 'q' [unix.Malloc]
1982 | ssn->queue_len++;
| ^~~
1 warning generated.
RFC 7323 forbids a server to respond with a timestamp option in the
SYN/ACK when the SYN didn't have a timestamp option:
A TCP MAY send the TSopt in an initial <SYN> segment (i.e., segment
containing a SYN bit and no ACK bit), and MAY send a TSopt in
<SYN,ACK> only if it received a TSopt in the initial <SYN> segment
for the connection.
Once TSopt has been successfully negotiated, that is both <SYN> and
<SYN,ACK> contain TSopt, the TSopt MUST be sent in every non-<RST>
segment for the duration of the connection, and SHOULD be sent in an
<RST> segment (see Section 5.2 for details).
However, in the real world this pattern happens on benign traffic. This
would lead to missing logs and detection, and in IPS mode such sessions
would be blocked.
This patch allows this pattern when the `stream.liberal-timestamps` is
enabled (enabled by default).
Bug #4702.
Take SEQ and ACK into account for more scenarios.
SYN on SYN_SENT
In this case the SYN packets with different SEQ and other properties are
queued up. Each packet updates the ssn to reflect the last packet to
come in. The old ssn data is added to a TcpStateQueue entry in
TcpSession::queue. If the max queue length is exceeded, the oldest entry
is evicted. The queue is actually a single linked list, where the list
head reflects the oldest entry.
SYN/ACK on SYN_SENT
In this case the first check is if the SYN/ACK matches the session. If
it doesn't, the queue is checked to see if there SYN's stored. If one is
found that matches, it is used and the session is updated to reflect
that.
SYN/ACK on SYN_RECV
SYN/ACK resent on the SYN_RECV state. In this case the ssn is updated
from the current packet. The old settings are stored in a TcpStateQueue
entry in the TcpSession::queue.
ACK on SYN_RECV
Checks any stored SYN/ACKs before checking the session. If a queued
SYN/ACK was sound, the session is updated to match it.
Ticket: #3844.
Ticket: #7657.
After upgrading from 7.0.6 to 7.0.8, regular ppp packets are getting
dropped when ppp rules in decoder-events.rules were set as drop.
This was caused by commit a8f35806 ("detect: fix decoder only events").
Previously these rules would not be alerted or dropped.
It turns out several PPP protocols in a switch statement were falling
into the PPP_UNSUP_PROTO case. This has always been the case, I assume
the intention was that they don't get further inspected for size and
other decode errors hence unsupported.
But really some of the protocols are fundamentally required for a PPP
connection to take place.
Change some types that we know should be allowed to pass this.
Ticket: 7651
As a part of the commit d096b98 a defensive check was added stating that
the stream must have EOF flag set if it is in TCP_CLOSING state or
above. However, this led to a false positive reported by oss-fuzz whose
analysis showed that this does not hold true for TCP_CLOSING state. It
does hold true only for TCP_CLOSED or if packet has PKT_PSEUDO_STREAM_END
set.
TCP_CLOSING state correspond to an established flow hence the correct
course of action is to remove the assertion.
Bug 7636
Co-authored-by: Philippe Antoine <pantoine@oisf.net>
In util-hash.c there was some behavior that is unexpected and likely
incorrect. To see this behavior, create a hash table 32 entries wide
and use the default hash function. Then add a short string “abc”,
observe the string is stored properly. Now remove a string “iln”, and
observe string “abc” is no longer in the table.
This is because the hash function is not properly handling collisions in
some edge cases.
Includes new unit test:
- UT verifies that the hash function generates a collision for
the selected test data. This must be true for the bug to be present.
Then UT demonstrates the bug by adding two items to the hash table
that collide, and then removing one of them 2x. The bug is that the
other value is removed as well.
Bug #7828 --> https://redmine.openinfosecfoundation.org/issues/7828
Signed-off-by: Charlie Vigue <charlie.vigue@openvpn.com>
de_ctx->rule_file is never NULL inside DetectFileHashParse(); add a comment
stating this fact and remove the superfluous NULL guard.
No functional change – the patch only clarifies the code and trims a few
lines of dead code.
Bug 7769
This commit modifies the call path for registering MT tenants to avoid
deadlocks on the master->lock
When performing tenant operations, e.g., using suricatasc to send a
register-tenant command, a deadlock occurs when
- DetectEngineMTApply: acquires master->lock
- Calls DetectEngineReloadThreads
- Within DetectEngineReloadThreads, calls DetectEngineMultiTenantEnabled
- Which first acquires master->lock
Commit 2bea5af introduced changes to the master->lock usage leading to
the deadlock situation.
Issue: 7819
The failure report was always just written to rules_fast_pattern.txt. In
case that setting is disabled or there's nothing fast-pattern related,
the report should be written to the usual rules_analysis.txt.
Bug 7821
de_ctx->ea->fp_engine_analysis_fp is only initialized if
engine-analysis.rules-fast-pattern is enabled in the configuration. If
this config param is missing, this leads to segfault.
Bug 7822
Remove unused rule prefilter-related stats counters that aren't in use.
94644ac960 (detect: move non-pf rules into special prefilter engines)
removed the logic that made use of and incremented the stats counters:
- det_ctx->counter_fnonmpm_list
- det_ctx->counter_nonmpm_list
Some code was left, registering them, and mentioning them in the
json schema.
Ticket #7834
Fixed counter for ippair.memcap by using the correct function,
IPPairGetMemcap.
Until now IPPairGetMemuse was used both for memuse and memcap.
Ticket: 7827
Since cached_hash was updated through reference (hash), it seems
LTO did not notice this and optimized the whole code block, returning
zero.
This in turn caused all caches to have the same name and to overwrite.
On subsequent runs, only the last cache was loaded for all SGHs
causing wrong MPM assignment.
Ticket: 7824
Functions like ByteExtractStringUint8 return 0 or less on
failure. Many usages of this function treat 0 as successful as its our
common pattern.
Ticket: #7836
Keywords registration should provide a desc and a link to the
documentation. This patch adds desc and/or url for on most keywords
missing that.
This allows better output for list-keywords command line which
is then use in Suricata Language Server.
Instead of enabling signal handlers by default, require the user of
the library to opt-in. This is done with the call to
SCEnableDefaultSignalHandlers, which sets a flag to add the default
signal handlers.
This seems like the least invasive way to do this at this time, but it
will require some re-thinking for 9.0, especially if migrate globals
to engine instances, signal handling will need to be re-thought.
Ticket: #6814
Configuration option `threads: auto` in DPDK's interface node
overassigns available threads to the interface.
Commit 4dfd44d3 changed the signedness of the remaining threads counter,
which caused surpass of the counter initialization.
The if-clause is switched to first initialize and then use the counter.
Ticket: 7798
The condition data_offset == NULL can never be true: data_offset has
already been validated as non-NULL a few lines earlier. The guard seems
to have been intended for the offset argument, yet throughout the
codebase offset is never passed as NULL. (In the unit tests, offset is
NULL, but those tests pass the value parameter as NULL, which causes the
function to return before offset is dereferenced.)
Remove the pointless check to simplify control flow and silence
static-analysis warnings.
No functional change.
Bug 7767
Jason Ish
Victor Julien
Philippe Antoine
Jeff Lucovsky
Shivani Bhardwaj
Thomas Winter
Juliana Fajardini
Charlie Vigue
Boris Tonofa
Adam Kiripolsky
Lukas Sismis
Eric Leblond