LuaDatasetAdd() dereferenced s->set without ensuring :get had been
called first. The userdata metatable validation it was originally
part of is already in place (Ticket: #8882); what remained is the
missing init check.
Reject :add on an uninitialized dataset. The pointer itself is
zeroed in a follow-up commit.
Ticket: #8882.
LuaFlowvarSet() accepted a script-supplied integer for the copy length
without validating it against the actual Lua string length returned by
luaL_checkstring. This could lead to OOB memory access.
This patch checks the length of the string before using it.
Ticket: #8858.
Both parse loops stopped as soon as a command or reply reached the
configured maximum length: they advanced the input cursor past the end and
broke out. When that line ended with an LF in the same stream slice it was
already complete, and the bytes behind the LF were ordinary FTP lines that
the break discarded. A segment holding "PORT <long>\r\nQUIT\r\n" lost the
QUIT: it never became a transaction, so inspection and logging never saw
it, and a hidden RETR or PORT never reached AppLayerExpectationCreate,
leaving its data connection unrecognized as ftp-data.
Remove both breaks so the loop continues to the next line. The cursor
assignment goes with them, since nothing reads it once the loop ends, and a
line with no LF still finishes the slice on its own because the getter
zeroes the remaining length.
Removing the breaks is not enough for a line that never reaches the clear.
Truncation was cleared deep inside command and reply handling, which is
skipped for an unrecognized request command, a reply the parser rejects (a
run of spaces, empty once trimmed), or an invalid EPRT/PORT that jumps to
tx_complete. The connection flag then stayed set and the getter dropped the
line behind it as this line's remainder. Snapshot and clear the flag up
front in both loops, for every line whose LF was consumed, so the following
line survives.
Issue: 8884
When the PADDED flag is set on HTTP/2 HEADERS or PUSH_PROMISE frames,
the parser reads the 1-byte Pad Length field but never strips trailing
padding bytes before passing the buffer to the HPACK decoder.
Fix by stripping trailing padding before invoking the HPACK decoder.
Ticket: #8787.
SWF depth settings were previously limited only to values representable by
uint32_t. This is not sufficient for the buffer-size calculations performed
during SWF decompression.
The decompressed output requires an additional 8-byte FWS header. The
inspection buffer also grows by doubling a uint32_t size, so a required buffer
larger than 2 GiB can cause the growth calculation to wrap.
For LZMA input, a synthesized 13-byte header to the compressed
data is used. The configured compressed-data limit must leave room for this header.
This patch applies the following upper limits:
- decompress-depth: 2147483640 bytes
(2 GiB minus the 8-byte FWS header)
- compress-depth: 4294967282 bytes
(UINT32_MAX minus the 13-byte LZMA header
Ticket: #8914
SWF FileLength was used to determine the decompressed buffer size when
decompress-depth is set to zero, but this field can be manipulated and it can
lead to evasion.
This patch treat FileLength as metadata, the length bytes are just copied, and
when decompress-depth is set to 0, a maximum value of 50MB is considered for
the decompressed buffer.
Ticket: #8855
The on-demand buffer growth introduced for SWF decompression called
lzma_decompress again from byte zero each time the output buffer filled.
Because lzma-rs can buffer up to a dictionary before writing, a single
SWF could be decoded repeatedly as the buffer grew, amplifying CPU use.
This adds a depth-bounded Rust writer that asks C to grow the inspection
buffer as decompressed bytes are emitted.
This keeps a single lzma-rs stream alive, so the input is decoded only once.
Ticket: #8829
Previously, SWF decompression guessed the final output size from FileLength,
allocated the whole buffer, and then decompressed once. A malicious SWF could
claim a 50MB output and force the per-thread inspection buffer to allocate and
retain 64 MiB before the compressed data was even checked.
Change the logic:
allocate 4 KiB, decompress, grow only when the buffer is full, and continue.
The buffer grows geometrically up to the configured output limit,
and the final inspection length is set to the number of bytes actually produced.
Also, for zlib, Z_OK does not by itself mean that decompression is complete.
It should be checked together with avail_out and avail_in,
grow the buffer when output space is exhausted, and continue the same inflate stream.
Ticket: #8829
When running with --reject-dev in autofp mode, the libnet context is
cached thread-locally and t_inject_mode is latched to LIBNET_LINK on the
first reject for an Ethernet packet. Subsequent rejects from different
interfaces that hit the fast path reuse the cached ctx but incorrectly
still call SetupEthernet(), which dereferences PacketGetEthernet() causing
a crash.
To address this check for if a packet is ethernet in all entry points.
Ticket: #8832.
The check was too weak: it allowed fh_handles far beyond what fits
in the remaining input, letting a crafted packet drive a large
Vec::with_capacity preallocation in nom7 count().
Bound fh_handles by the remaining input size and by 4096, the
maximum file layout stripe count used by the Linux kernel
(NFS4_PNFS_MAX_STRIPE_CNT).
Ticket: #8816.
On Windows the traversal logic only matched the literal "..\", but
Windows accepts "/" as well.
Fix SCPathContainsTraversal() on Windows to check both "..\\" and "../".
Ticket: #8830.
AddAndRotate() assumes the SYN queue has at least two elements when
evicting the oldest to make room for a new one. With stream.max-syn-queued
set to 1 this would not be true leading to crash.
Ticket: #8833.
Replace the BUG_ON with proper error handling so that any fopen() failure
logs a warning and skips the chunk instead of causing a crash.
Adds a few more warnings for error conditions.
Ticket: #8823.
Segment logging could create larger buffers than the thread buffer
available, which would be silently truncated. However the length passed
to libpcap was still of the non-truncated size, so this would lead
libpcap to read past the end of the buffer.
Fix by trying to expand the buffer as needed.
Ticket: #8835.
Negotiate protocol parser used an unbounded many1(complete(...))
combinator over the entire remaining packet input, discarding both
wct and bcc fields. Each NUL byte yielded one empty "dialect" entry,
causing excessive memory use.
Use bcc to limit the parsing; the dialect count is then bounded by
bcc (u16).
Ticket: #8852.
AppLayerExpectationCreate gets a locked and ref counted
IPPair from IPPairGetIPPairFromHash. On the success path it correctly
drops both with IPPairUnlock (keeping the ref for lifecycle). The else-
branch that creates per-IPPair ExpectationLists calls SCCalloc. If
that allocation fails, neither Unlock nor Release is called, leaving
the IPPair mutex permanently held and its refcount elevated.
Any subsequent worker thread that looks up the same IPPair blocks
forever in SCCMutexLock.
This patch addresses this by properly releasing the IPPair in the
allocation error path.
Ticket: #8851.
pthread_getattr_np re-initializes the attributes on use causing the
original pthread_attr_init to leak. Destroy after use to prevent the
memory leak.
Ticket: 9032
This patch distinguishes malformed DHCP options from truncated ones.
Previously, the malformed_options field was never set and parsing errors in
overloaded file and sname fields were silently discarded.
This caused malformed options to be reported as truncated or not reported at all.
Classify errors in the primary option stream and overloaded fields.
Treat failures within fixed-size overloaded fields as malformed because those
fields are already fully present.
Ticket #8874
DHCP lease, renewal, and rebinding time options contain a four-byte value,
but the parser previously ignored their declared length and always consumed
four bytes.
Reject these options unless their length is exactly four to
prevent parser desynchronization and incorrect keyword matches.
Ticket #8873
DHCP option 0 is a one-byte padding field, but it was parsed as a generic TLV.
This consumed the following option code as its length and could prevent later
options from being inspected.
Handle PAD explicitly so parsing resumes at the next option.
Ticket #8819
Since the PostgreSQL protocol doesn't impose any clear limits to how
many responses a backend could send, add one to the app-proto parser.
Using 21-64 as the accepted values, and 64 as the default:
The Start-up phase may have up to 17-18 ParameterStatus messages, plus
AuthOk, BackendKeyData and ReadyForQuery: 20, with extra room to be on
the safer side: 21
And 64 as upper limit, because as we currently summarize query
responses, we already don't get flooded by DataRow and Copy responses
messages, so we don't expect to see that many responses in a single
transaction for the pgsql, with the current implementation.
Ticket: #7263
Pgsql logger would only open a ParameterStatus message array and only
close it when a different type of message was seen (following an
expected pgsql transaction style). However, if no different type of
message followed the ParameterStatus message, the array remained open,
leading to an unbalanced `}` count, and poisoning the whole JSON log,
from that point on.
Now, if we finish the responses logging loop without reaching another
type of response, close the array, regardless.
Ticket: #9012
parse_row_description and parse_consolidated_data_row read a field_count
from the message body and use that to iterate a slice, without checking
whether the count actually fits the body.
A backend-controlled attack could lead to fewer fields logged, or
stalling of the parser to-client direction for the duration of the flow.
A malformed_response event is set for pgsql, if either of those
message's inner parsers has a field_count that can't be reconciled with
the body slice.
A new pgsql.response log field is added to indicate this, too:
pgsql.response.row_description_malformed, a boolean. This wasn't added
to the consolidated data row message, per its summarization nature.
The row_description unit test buffer declared a length one byte longer
than its three fields occupy. The old code silently discarded that
surplus byte; the new one reports it, so the buffer was corrected.
Ticket: #8885
Treat bad link-header CRCs and impossible link lengths as recoverable
errors and attempt to resynchronize.
Also consume a response frame whose trusted length is too short for the
required response headers, so later responses remain available.
Ticket: 8980
Ticket: 8916
Avoid type confusion and possible overread when calling
AppLayerParserGetStateProgress
DetectEngineInspectMultiBufferGeneric callback may be called with
the inner DNS tx of a HTTP2/DOH2 tx, so we should use dns as its
protocol
Avoids pushing multiple log messages, when the consumer will
only set one app-layer event for the tx
Also, this is a warning, not an error, as we continue parsing.
Ticket: 8867
Ticket: 8800
Thus avoiding unbounded recursion
Before that, parse_payload could call
parse_security_association_payload which could call back
parse_payload if a type in the list was SecurityAssociation
But a Security Association only accepts the type Proposal in its
payload, see RFC 2408: section 3.5
There was another recursive path with parse_payload calling
parse_proposal_payload which called back parse_payload
And a Proposal only accepts the type Transform in its payload, see
RFC 2408: section 3.6
Fixes: 9aeeac53 ("pgsql: remove probe_ts function")
When the probe_ts function was removed in 9aeeac532, the request gap
handling got inverted, preventing re-sync of the app-proto from the
frontend side.
This could lead to detection evasion after a gap.
Ticket: #8822
Ticket: 8755
The config parameter was only taken into account from the
settings frame HEADER_TABLE_SIZE
Emits an event http2.header_table_overflow when we have the first
header that we cannot keep in Suricata's memory due to the
configured limit.
The per-thread threshold decision cache (limit/both thresholds tracked
by_src/by_dst, IPv4) grew without bound: every distinct tracked address
allocated an item plus a hash-table bucket, while housekeeping reclaims
at most two entries per whole second.
Address this by:
Cap the cache at detect.thresholds.cache.max-entries entries per
thread (default 256; valid range 256-1048576, out-of-range values
fail startup); on overflow evict the entry with the earliest expiry
before inserting
Account cache memory in the new detect.thresholds.cache.memuse
counter: each entry (item plus bucket node) is charged on insert and
released on eviction, expiry and on thread teardown, and the fixed
per-thread charge (thread context, hash table struct and the eagerly
allocated bucket pointer array) is charged at cache init and released
symmetrically on teardown, so the gauge reflects the cache memory in
use and does not accumulate stale charges when threads are recreated,
e.g. unix-socket PCAP file jobs
Size the cache hash table to the entry cap (next power of two, minimum 256)
at thread init so lookups stay O(1) up to the cap.
Fail startup when the threshold table cannot be initialized, instead
of continuing with a NULL table.
Ticket: #8956.
DetectBytejumpValidateNbytesOnly() only performed signed upper-bound
checks but had no lower-bound guard. When byte_jump used a
variable-supplied nbytes value derived from byte_extract/byte_math, an
extracted field with bit 31 set arrived as a negative int32_t that
passed validation and the ptr+len boundary check.
The downstream (uint16_t)nbytes truncation in the binary extract branch
then produced a small positive count and ByteExtractUint64() over-read
up to 7 bytes past the end of the inspection buffer.
Address this by adding a nbytes >= 0 check.
Ticket: #8831.
Fix an infinite loop in SCInspectionBufferCheckAndExpand where doubling
a uint32_t new_size can overflow if min_size is too large. This causes
detection worker threads to spin at 100% CPU. This is a very unlikely
scenario, but this patch hardens the logic to be defensive.
Ticket: #8834.
DetectFileInspect() walks the per-signature SigMatchData array with
a pointer (smd) that is incremented inside the inner keyword-matching
loop. Because smd is the function parameter, this mutation persists
across iterations of the outer for file loop and there is no reset
of the cursor between files.
With a single file this works correctly. In any transaction that carries
two or more File objects every subsequent file is evaluated starting at
the already-advanced smd instead of the beginning of the keyword chain,
leading to skipping of keywords.
This patch addresses the issue by using a local iterator instead of
using the function parameter directly, so every file sees the full keyword
list.
Ticket: #8815.
Victor Julien
Jeff Lucovsky
Philippe Antoine
Giuseppe Longo
Jason Ish
Juliana Fajardini