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.
PrefilterPostRuleMatch() repopulates det_ctx->pmq with flowbits:isset
candidates after a matching flowbits:set rule is found. The subsequent
append loop writes directly into det_ctx->tx_candidates[] without first
checking that the array has room.
The buffer was initially sized for prefilter results only not accounting
for post-rule additions. When array_idx already equals tx_candidates_size,
the writes go past the end of the heap-allocated block - a heap buffer
overflow followed by qsort() over OOB memory, causing a crash (DoS).
Addressed by adding a capacity check + expand before the append loop,
matching the existing pattern at all other candidate append logic.
Ticket: #8866.
Ticket: 8973
Before that, the update to DoH2 happened before http1 state was
freed, leading to a type confusion.
In case of a HTTP1->HTTP2 upgrade, if the client sends a DOH2
request, Suricata went to change again the flow's alproto
with SCAppLayerForceProtocolChange
But SCAppLayerForceProtocolChange did not take into account the
fact that we were doing a HTTP1->HTTP2 upgrade, and the previous
HTTP1 state was not freed yet.
Now SCAppLayerForceProtocolChange checks FlowChangeProto
and update alproto_orig only if we were not doing a proto change
already, as alproto_orig is used to know which type of state
to free.
parse_t123_tpkt runs the X.224 and X.223/MCS sub-parsers over the
length-bounded tpkt payload, but they use streaming nom parsers.
An inner length field overrunning the payload, such as an oversized
CS_CORE block, escaped as Err::Incomplete although the tpkt was fully
present.
This patch wraps the child parsers in complete() so Incomplete from within the
bounded payload becomes an Error.
The tpkt is then returned as an unrecognized Raw child, which the caller skips,
and parsing resumes at the next tpkt.
Ticket #8838
When an incomplete SSH_MSG_KEXINIT record has a body of at least
SSH_MAX_REASSEMBLED_RECORD_LEN bytes and hassh is enabled, record_left
was overwritten with the full body length even though the function
returns ok() and thus consumes the body bytes already present in the
input. The next call then over-skipped by that many bytes into the
following record, desynchronising the parser for that direction so
hassh was never computed and ssh.hassh keywords could not match.
This patch keeps the record_left computed from the bytes actually consumed in
the oversized path, and only use the full body length in the path that
returns incomplete, where the body bytes are delivered again.
Ticket #8837
MQTT5 logs user properties wherein a user can send any metadata
over the wire in the form of key-value pairs.
MQTT logger used these key-value pairs directly to populate the common
"properties" field. But we do not want to use arbitrary json keys
(which may contain arbitrary characters)
1. A fixed new child object to the common "properties" called
"user_properties" which carries a list of dicts.
2. Each key-value pair received as a part of the user properties is now
saved as:
"properties": {
...
"user_properties": [{"key": "mykey", "value": "myvalue"}]
...
}
instead of:
"properties": {
...
"key": "value"
...
}
Ticket: 8785
Keys were not encoded as json UTF-8 strings.
This allowed json injection/corruption when an attacker
could control the key and put special JSON chars in it
such as double quote. Encode the keys when needed.
Ticket: 8999
Ticket: 9003
When a single tx is getting inspected by rules having
http.response_header keyword both with a transform and without
a transform, the code went like this :
Http2ThreadMultiBuf structure contains a list (Vec) of buffers
(Vec<u8>) and owns these Vec<u8> (they are not owned by the tx).
For the first list_id, we computed each buffer for a http2 header
and put them in these Http2ThreadMultiBuf structure.
But for the second list_id, the code began by freeing the memory
of these http2 headers, and reallocated new buffers (Vec<u8>)
and recomputed each buffer.
After these rounds, which may happen in DetectRunPrefilterTx,
the buffers might need further checks, hence the use-after-free.
Now, the code does not free and allocate a new buffer, it just rewrites
the old one, as is done for HTTP1.
Due to a lack of tests with varied ErrorResponse messages, it wasn't
noticed before that these were missing.
Partially covers
Ticket: #8903
Related to
Bug: #8886
CopyIn and CopyOut message parsers read the length field, but did not
use that information to parse the rest of the message, instead, parsing
as many columns as indicated in the columns field, without ever
checking against the informed length. This opened the door for
attackers to fabricate messages that were never sent by the server,
as well as to obfuscate following valid backend messages, in case the
columns value was bigger than the actual message (up to ~128KiB).
Those parsers also had a message length parsing check off-by-one, fixed
in this patch.
When a malformed CopyIn or CopyOut response is seen, it is logged as
previously, with a new field added to indicate whether it is malformed
or not.
Ticket: #8863
Victor Julien
Jason Ish
Giuseppe Longo
Juliana Fajardini
Philippe Antoine
Shivani Bhardwaj