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
parse_request and parse_response were using the parser state for
transaction bookkeeping.
Instead, use the message types themselves.
This change means that message types that ended up not being added to
the responses array, before (due to not advancing the parser state), now
are pushed and thus can be logged.
** Noticeable effect: AuthenticationSSPI, Notice and Notification
messages, previously missed due to falling into the None state progress
branch, could now get logged.
** To prevent affecting existing setups that don't expect these, since
they weren't logged before, we refrain from pushing them into the
responses queue, for now.
Leave state progress for state machine keeping.
Bug: #8886
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
Philippe Antoine
Juliana Fajardini
Victor Julien
Giuseppe Longo
Jason Ish
Shivani Bhardwaj
Jeff Lucovsky
Lukas Sismis