Fragment validation accounts for the minimum 20-byte IPv4 header. When the
first fragment contains IPv4 options, adding its actual header length during
reassembly can make the packet exceed the 16-bit IPv4 total-length limit.
Use a u32 to check the size, rejecting and setting an event for
reassembled packets that are too large.
Ticket: #8818
Fragmented IPv6 packets with an unfragmentable extension header can
exceed the maximum allowed size. Use a u32 to check the size to prevent
wrap-around, rejecting the packet and setting an event if too large.
Ticket: #8818.
CopyCommandLine allocated a buffer for the whole request line, stripped
the trailing whitespace and returned the shorter length. That shorter
length is what the caller stores in tx->request_length and what
FTPTransactionFree later hands to FTPFree, so one byte for every stripped
whitespace character is never subtracted from ftp_memuse. The memory
itself is released -- SCFree ignores the size it is given -- but the
counter keeps climbing for the life of the process, and a client that
pads its commands with spaces can push it up with every command it sends.
Once the counter reaches app-layer.protocols.ftp.memcap, FTPCheckMemcap
refuses any allocation that would cross it, and request buffers are the
first to be refused: the transaction is still created, but tx->request
stays NULL, so command_data is missing from the log and rules that
inspect it stop matching. Whether a new flow can still allocate its state
depends on how much room is left below the cap. With no memcap
configured, the symptom is an ftp.memuse counter that is permanently
wrong.
Strip the whitespace before allocating, so the size accounted for is the
size returned to the caller:
- CopyCommandLine shortens line->len first, then allocates and copies
that length.
- A line that is nothing but whitespace returns without allocating. It
used to get a buffer that was stored on the transaction and freed with
a length of zero, leaving the whole allocation on the counter.
- When the allocation fails, line->len has already been shortened, so
port_line, the transfer file name and the too-long-line check see the
same length there that they see when it succeeds.
- FTPParserTest13 covers the accounting and the stored request;
FTPParserTest14 covers the line the parser uses afterwards.
Issue: 8848
Add a note to the pcre extraction section stating that a flow: capture
is dropped when the packet is inspected without a flow, and that such a
packet still alerts.
ICMPv4 error messages are one way to get such a packet: FlowCreateCheck
returns false for them, while DecodeICMPV4 has already set p->payload,
so they reach the detection engine with something to inspect and nowhere
to store a flow variable. A packet the flow engine could not give a flow
to once the memcap was reached and no used flow could be reclaimed is
the other.
The pkt: and alert: scopes described in the same section are unaffected,
because a packet variable is stored on the packet and an alert variable
on the alert.
Issue: 8862
DetectPcrePayloadMatch dispatched each capture group through a chain of
captype tests whose flow-variable arm also required a non-NULL flow, and
closed with an else that called BUG_ON(1) on the grounds that the
captype was impossible. A capture typed VAR_TYPE_FLOW_VAR on a packet
inspected without a flow, such as an ICMPv4 error message, matched no
arm of that chain and reached the else. BUG_ON expands to assert()
wherever assert.h is available and NDEBUG is unset and to
exit(EXIT_FAILURE) otherwise, so one pcre rule whose flow capture
matched such a packet killed the process.
Test the captype before the substring is extracted and skip a flow
capture when there is no flow, so nothing is allocated for a capture
that has nowhere to go. The remaining capture groups of the same pcre
still run: a rule that captures one group into a flow variable and the
next into a packet variable still stores the packet variable.
The SCFree(str_ptr) below BUG_ON(1) could never run, because BUG_ON
terminates the process first. Use DEBUG_VALIDATE_BUG_ON instead, which
compiles out unless debug validation is enabled, so a captype the parser
cannot produce releases its capture rather than ending the process.
Issue: 8862
In "tx:answers()" don't attempt to push each OPT as its own string,
instead concatenate into one string like we do with TXT.
The previous method was broken and didn't work anyways, so we're not
breaking something that previously worked.
Ticket: 8843
SSLv2Decode() consumed six fixed CLIENT_HELLO body bytes (version +
cipher_spec_length + session_id_length) guarded only by input_len >= 6,
which checks the buffer remainder but not the record itself. When an
attacker sends a record whose declared record_length is < 7 (e.g. 1),
bytes_processed advances past record_lengths_length + record_length,
leading an integer underflow.
Address by adding a record_length >= 7 check to the CLIENT_HELLO case.
Also add another check: bail if bytes_processed > record_length +
record_lengths_length.
Ticket: #8853.
The fallback memrchr() implementation initialized the search pointer to
the one-past-end position and dereferenced it before decrementing, which
is undefined behavior in C.
Update the implementation to decrement the pointer before dereferencing
while preserving memrchr() semantics. Compare bytes using unsigned char
semantics to ensure values with the high bit set are handled correctly.
Refactor the fallback into a local SCMemrchrFallback() helper so it can
be exercised by unit tests on platforms that provide a native memrchr()
implementation.
Expand the unit test to cover first, middle, last, duplicate, single-byte,
zero-length, absent-element, and high-byte values.
Ticket: 9010
Signed-off-by: Urval Kheni <kheniurval777@gmail.com>
Ticket: 8731
Adds a configurable limit to string lengths to avoid to retain
too much memory for too long, and avoid producing log events
that are too big
Previously, hook sequences in both rules and default policies
allowed `pass` to be combined with incompatible hooks (e.g.
reject or drop).
This commit clamps pass to accept only.
Ticket: 8954
A secondary action given without an explicit scope inherits the scope of
the primary action, but was never validated against the scopes it
supports itself. `pass` only supports packet and flow scope but
the inheritance was never verified.
Ticket: 8954
A server can reject a BDAT command before the client sends the
advertised chunk. Suricata would keep waiting for those bytes and treat
the rest of the SMTP session as chunk data.
Keeps subsequent SMTP commands and message data in sync with the
server.
Ticket: 8995
To detect bug like 8856 of overread, which did not trigger
ASAN as we alloc default-packet-size or MAX_PAYLOAD_SIZE
for each packet, instead of just what we need
Suricata never removed the unix command socket file after shutdown.
The socket path (sockettarget) is now preserved in the struct
UnixCommand_, because it is needed on shutdown to close the socket.
The cleanup is done in the shutdown path of UnixManager(), which is
reached uniformly whether shutdown is triggered by a fatal error or
a signal (e.g. SIGTERM) via THV_KILL. This also applies for a clean
shutdown via 'suricatasc -c shutdown'.
Ticket: 8799
Substitute an empty string for a NULL key or reference in
AlertJsonReference() before the write, so the output no longer depends
on what a libc does with a null pointer and "%s", which the standard
leaves undefined. glibc prints "(null)"; nothing requires that.
Assert both are set alongside the substitution. DetectReferenceParse()
is the only producer of DetectReference and sets each string with its
length, so the assert cannot fire today -- it names the invariant for a
second producer that does not hold it, while the substitution covers a
release build, where DEBUG_VALIDATE_BUG_ON compiles out.
A release build that reaches the substitution logs whichever half it
has rather than crashing, and nothing records that it happened.
Issue: 8849
Assign ref->key for a reference key that is not in reference.config, so
the fallback that registers a synthetic entry produces the same key and
key_len as an ordinary lookup. The assignment sat in the branch taken
when SCRConfGetReference() found the key; the fallback registered the
entry, looked it up again, and fell through with ref->key still at the
NULL SCCalloc() left.
AlertJsonReference() passed that key to snprintf() with "%s%s" and
sized the destination from key_len. With key NULL and key_len 0, a rule
carrying reference:unknownkey,001-2010 logged "(null)001-" on glibc,
and handed a null pointer to a "%s" conversion, which the standard
leaves undefined.
Invert the lookup test so the add-and-retry runs when the lookup fails
and both paths reach one assignment. The comment on that assignment
named REFERENCE_SYSTEM_NAME_MAX; the string copied is a URL, bound by
REFERENCE_CONTENT_NAME_MAX.
DetectReferenceParseTest03() loaded a rule with an unknown key and
checked only that the signature parsed, which it did throughout. Check
the resolved key, its length, and the reference value.
Issue: 8849
Rust website has changed the location of the installer.
Additionally their installer now includes automatic append to
.bashrc to source the correct path to Rust binaries.
Cbindgen as an instruction is also removed as it is not needed
when building from the downloaded and packaged Suricata.
Ticket: 8344
The crash handler built its stack trace by advancing a pointer with
the snprintf return value. That value is the length the output would
have had, not what was written, so once the trace filled msg the
pointer moved past the end of the buffer. Every size argument after
that, SC_LOG_MAX_LOG_MSG_LEN - (temp - msg), was negative and
converted to a huge size_t, and snprintf went on writing into the
stack past msg. With a deep enough stack the handler faults before it
can log the crash it exists to report.
The frame name lookup also compared unw_get_proc_name() against
UNW_ENOMEM, but libunwind returns the negated code, so the test
never fired. A lookup that failed with UNW_ENOINFO fell through
to the success branch and printed the name buffer, which
libunwind had not written.
Track space used with offset and use that on each snprintf call. Advance
by MIN(cw, remaining - 1) to prevent overruns. The loop stops when
the buffer is full and closes the trace with "..." so a clipped message
is not read as a whole stack.
Failed symbol lookups now print "[unknown]:". -UNW_ENOMEM is not one of
them, since libunwind returns a truncated but usable name with it.
name starts as "?" so nothing is printed from it either way.
Ticket: 8846
nfs4_res_secinfo_no_name() used a local var "i2" to walk through
the security-flavor array entries, but returned the pre-walk var "i"
in the Ok at the end. Every byte of the flavor array was therefore left
unconsumed in the input stream and re-interpreted by count next iteration
as a subsequent compound operation.
Addressed by returning i2 instead of i so all flavor bytes are consumed.
Ticket: #8860.
SCACTileSearchLarge uses 32-bit next-state cells holding a 24-bit state
index. When there are more than 65536 states, CheckMatch was called with
(uint16_t)state, silently discarding the bits beyond 16.
Address this by widening CheckMatch's state parameter.
Ticket: #8841.
Prevent out-of-bounds read of protocol field when PPPoE session header is
exactly PPPOE_SESSION_HEADER_MIN_LEN bytes. Inspect first protocol octet
only to decide single vs double octet form, and read second octet only after
len >= 8 verification.
Add unit tests for the 7-byte boundary case for both the double-octet
(rejected as too small) and single-octet (decoded as PPP_IP) protocols.
Ticket: #8856.
When multithreaded output was enabled, all threads freed
shallow-copy of sensor and prefix names.
This commit adds a guard so that only the parent frees the
allocated buffers.
Ticket: 8861
Add support for ERF record types META and PAD, plus additional ETH types.
Add support for ERF extension headers.
These types and extension headers were already supported by erf-source-dag.
Made source-erf-file and source-erf-dag more consistent.
Ticket: 8962
When reading an ERF file do not trust the ERF header values.
Do not use wlen to set packet length.
Ticket: 8836
Read rlen bytes into MAX_PAYLOAD_SIZE buffer.
Use PacketCopyData() to safely update p.
Ticket: 8865
Record that << and >> yield 0 when rvalue is 64 or more, beside the
existing note about division by zero. A shift count can come from a
byte_extract variable, so the rule text alone does not say which
counts land in that case.
Issue: 8845
Fail rule load when byte_math pairs << or >> with a literal rvalue of
64 or more. rvalue was bounded only to u32::MAX, so a rule shifting by
100 loaded and then produced 0 for every packet it inspected, spending
detection work on a comparison whose outcome was settled before the
first packet arrived.
A variable rvalue still loads. Its value comes from a byte_extract on
the packet and is not known until the rule runs, so the guard in
DetectByteMathDoMatch() stays the only check covering that path.
DetectByteMathParse() reports every SCByteMathParse() failure as
"invalid bytemath values", so the rejected rule is named by the
"error parsing signature" line that follows rather than by the reason
the parser gave.
Issue: 8845
Zero the result when a byte_math right shift count reaches 64, the
width of the uint64_t being shifted, so the operation no longer
depends on behavior C11 6.5.7p3 leaves undefined. The left shift case
has done this since 473ca6dcf4; the right shift case was left
unguarded.
DetectByteMathDoMatch() shifted by whatever count it was handed. On
x86_64 the hardware masks the count to its low six bits, so a count of
64 became a shift of 0 and returned the extracted value unchanged
instead of 0. That value is stored in det_ctx->byte_values[] and feeds
any byte_test, isdataat, or content offset later in the signature, so
the signature's verdict follows from an arithmetic result the standard
does not define.
The count reaches the shift from the wire. When byte_math names a
variable for rvalue, DetectEngineContentInspectionInternal() reads it
out of det_ctx->byte_values[] at
detect-engine-content-inspection.c:614, where a preceding byte_extract
stored bytes taken from the payload, so one payload byte of 0x40 sets
the count to 64.
Issue: 8845
The fn to flush the ring is supposed to flush the entire ring buffer but
it only resets the buffer up to the "length" bytes. Fix this calculation
to correctly reflect the number of bytes that need to be reset.
Ticket: 8826
Setting a cpu-affinity path with --set, like
--set threading.cpu-affinity.worker-cpu-set.threads=28
creates an intermediate node under cpu-affinity whose val is NULL.
With a legacy list format cpu-affinity, AffinitySetupLoadFromConfig()
red the set name from the node val and passed it to
GetAffinitySetName(), which then dereferenced the NULL pointer.
Address it by simply checking for the NULL.
Ticket: #6735.
Jason Ish
Jeff Lucovsky
Philippe Antoine
Victor Julien
Urval Kheni
Lukas Sismis
Andreas Dolp
dependabot[bot]
Stephen Donnelly
Shivani Bhardwaj