Ticket: 8775
Avoids that every flow that is the same except for spi
ends up in the same bucket, and that this bucket grow too much
since FlowCompareESP makes them different flows
So far, the properties parsed across various different types of MQTT
messages was unbounded and could be exploited. Add a limit of 255 on the
maximum number of properties that any message type can store.
Additionally, raise an event for too many properties indicating an
anomaly in the network traffic.
Ticket: 8839
Introduce a maximum SAN limit of 4k instead of returning a u16 casted
count of TLS X.509 SubjectAltNames, saturate the count to make sure it
stays within the 4k bounds without truncating the existing count value.
Ticket: 8864
ENIP parser makes use of nom's complete macros which by design expect
full data to be passed along to them. In case there is incomplete data
sent to them, they result in an error. This disables the ENIP parser and
further detection and could happen with an incomplete/fragmented
request/response.
Use AppLayerResult API to ask for more data from the stream engine in
case the stream slice currently at hand does not match the length
parameters received from the header or is not long enough to parse the
header itself.
Ticket: 8809
The truncation flag lived in FtpState, where the line getter set it and
the two callers cleared it once FtpLineState.lf_found said the line had
ended. The callers reuse one FtpLineState for every line in a slice, and
only the LF branch ever set lf_found, so a complete line left it set for
whatever came after. An over-long line behind a complete one in the same
slice therefore had its truncation state cleared, its remainder was parsed
as ordinary traffic, and a "PASS s3cret" sitting in that remainder became
a transaction of its own.
The flag could just as easily be left set. The request loop reached its
clear only for a line that matched an FTP command, continuing to the next
line otherwise, so an over-long run of "Z" left the flag standing and the
next command to arrive was discarded as that line's remainder. The
response loop's clear sat three ifs deep, inside blocks entered only when
the reply parsed, so a line that SCFTPParseResponseLine rejects -- a run
of spaces is empty once trimmed -- left it standing too.
The getter also raised the flag for a line whose LF it had just consumed.
Nothing of that line remained to discard, so the caller had to clear it
straight back.
Truncation is a property of the line, so FtpLineState now carries it. The
getter sets line.truncated for the line it hands back, and the FtpState
flag stays set only where truncation genuinely spans slices: the over-long
line that arrives without an LF. The callers read line.truncated and no
longer touch the shared state, so lf_found goes away entirely.
Issue: 8859
When a command or reply runs past the configured maximum length, the
parser keeps what fits, reports it truncated, and discards the remainder
of that line as it arrives. Only the remainder should go, but the parser
was discarding the whole stream slice that the line's LF landed in.
A slice holding "<remainder>\r\nQUIT\r\n" carries a complete QUIT behind
the remainder, and a response slice carries whatever reply comes next. The
recovery branch zeroed the remaining length and returned an error, which
ended the caller's parse loop, so those lines were discarded along with
the remainder they followed. They never became transactions, so nothing
inspected them and nothing logged them, which means a command can be
hidden by placing it after an over-long one.
Consume through the first LF, clear the truncation state there, and pick
up the next line in the same slice. The scan becomes a loop so the
remainder goes through the ordinary path, including when it is over-long
itself; the run-out-of-input check becomes the loop condition.
Raised in review of the 8859 fix.
Issue: 8859
When a line exceeds the configured maximum length and contains no LF,
FTPGetLineForDirection emitted a window starting at the beginning of the
stream slice rather than at the unconsumed remainder. If a complete
command had already been taken from the same slice, that window was
shifted left by the consumed count: it began inside the previous command
and stopped short of the end of the long one.
The command MPM match, the per-transaction request copy and the port_line
capture all ran over those stale bytes, so a long command hidden behind a
short one in the same segment was classified and logged as the previous
command. The response direction had the same problem.
The LF-found branch already derives the line from input->buf + o_consumed;
do the same here, and advance consumed so the cursor still describes the
slice it was handed.
Issue: 8859
bytevar.map() read its first argument as a Signature pointer.
Dereferencing the full userdata stopped the engine at rule load.
The commit provides a more intuitive user message when the user
provides full user data.
The flow, packet and dataset methods fetched their object argument
with lua_touserdata(), which does not validate the userdata type. A
script could pass any userdata as the method's "self", and it would be
dereferenced, immediately crashing the engine from inside the Lua sandbox.
Ticket: 8882
The maximum IPv6 payload of 65535 bytes results in a packet of 65575
bytes when the fixed header is included. This was truncated to a u16
before decoding a reassembled packet, causing it to bypass further
inspection.
Use a u32 for the IPv6 decoder length so maximum sized reassembled
packets are fully inspected.
Ticket: #8818
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.
Philippe Antoine
Shivani Bhardwaj
Jason Ish
Jeff Lucovsky
Lukas Sismis
Victor Julien
Urval Kheni
Andreas Dolp
dependabot[bot]