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
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.
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.
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
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
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
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
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.
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
Disable the clippy warning for not using modern C string
literals. Ubuntu 24.04 still ships cbindgen 0.26.0 that doesn't
support these, so migrating to modern C string literals would require
Ubuntu 24.04 users wishing to build from git to install cbindgen with
cargo.
With this patch, the logged uuids will also have corresponding
stringified service names. The requests that will have an opnum will
have a corresponding stringified procedure name.
Feature 2727
Feature 8523
This data directly comes from Zeek @d11a8750. It was scriptually
converted to NDJSON ingestible to Suricata using serde and serde_json.
The script used for this conversion lives in
scripts/extract_dce_rpc_interfaces.py.
Part of this data within Zeek codebase comes from the MITRE BZAR
project. Hence, the LICENSE file alongwith this data consists of both
the licenses: Zeek and BZAR.
Feature 2727
Feature 8523
DOH2 depends on HTTP/2, so it makes no sense to enable it separately.
It would also put the sub state handling in a weird state, as the DOH2
side reuses the registered HTTP/2 callbacks.
Split into 2 sub-states:
- stream, which has the "HTTP" requests and responses, including DOH2
- global, which has the settings and other global or control handling
Introduce a simpler progress tracking for the global sub state:
- HTTP2ProgGlobalStart and HTTP2ProgGlobalComplete.
The stream sub state uses the same state machine as before.
Ticket: #8386.
Ticket: 8725
So that multiple HTTP2 DATA frames with EndOfStream flag set,
do not make the buffer grow, while processing it each time,
resulting in quadratic complexity
new_tx() now refuses to create a transaction when the list is already at
SMB_MAX_TX, returning None instead of a transaction. Every creation path --
the new_*_tx helpers and their callers across smb1/smb2/dcerpc/session/
files/ioctl -- propagates that, so no single input can create more than the
limit, including a compound SMB2 request that chains many PDUs in one
record. When the list is full the parser puts the flow into an error state
and stops processing it.
This replaces the previous force-completion of old transactions, which did
not reliably bound the list and could leave transactions unreclaimable on
asymmetric flows. The now-unused tx_index_completed bookkeeping is removed.
Issue: 8629
Ticket: 8649
Fully resets all the fields before tackling an ecapsulated message
to avoid evasion, due to the encoding of the upper file
leaking into the next one...
Body lengths were stored as u16, so a SIP body of 65536 bytes truncated
body_len to 0 and the RequestBody/ResponseBody frame was never created,
letting body content evade inspection. Widen the framing fields to u32.
Ticket #8582
dcerpc parser creates a new tx with id 0 and compensates for the 1 based
index handling throughout the code by overriding that value in a trait
implementation. Make this consistent with other applayer parsers.
Task 8720
Tx ID handling did not take the required + 1 into account.
From a report:
RDP can skip cleanup because its id convention does not match the
generic Rust iterator. The generic iterator in applayer.rs returns
tx.id() - 1, and cleanup trusts that id when calling StateTransactionFree
in app-layer-parser.c. RDP registers that iterator in rdp.rs, but
RdpTransaction::id() returns the stored id unchanged in rdp.rs, while
free_tx also compares against the raw stored id in rdp.rs. For a single
freeable RDP tx with stored id 1, the iterator returns C id 0; cleanup
calls free_tx(0), nothing is removed, then has_next == false allows
min_id to advance to total_txs in app-layer-parser.c. That leaves the
tx live but now below min_id, so later cleanup will not revisit it.
This patch brings the handling in line with the other parsers.
Bug: #8717.
Mark the direction into RDP transactions at creation time,
so the tx carries SKIP_INSPECT for the direction it is
never seen in, matching DHCP and the other single-direction
parsers. This lets cleanup free completed transactions and
stops a tx from being inspected (and alerting) twice, once per
direction.
RDP bounds its transactions to connection setup and stops
parsing once bypass_parsing is set.
Issue: 8621
DHCP is a stateless parser where each datagram is its own standalone,
single-direction transaction. It was creating transactions with
AppLayerTxData::new(), which leaves both SKIP_INSPECT bits clear, so the
engine treats every tx as still needing inspection in both directions.
For a flow that only ever carries one direction (broadcast DHCP, or a
relay seeing one side), the never-observed direction's inspect bit can
never be set, so AppLayerParserTransactionsCleanup() never frees the tx.
The per-flow transaction Vec then grows without bound and every packet
re-scans the whole list, giving O(n^2) CPU and unbounded memory on a
busy DHCP aggregation point.
Use AppLayerTxData::for_direction() like every other stateless parser
(DNS, SNMP, NTP, IKE, KRB5, MQTT, QUIC, SIP, WebSocket, bittorrent-dht)
so the tx carries SKIP_INSPECT for the direction it will never be seen
in. This lets cleanup free completed transactions and also stops the tx
from being inspected (and alerting) twice, once per direction.
Issue: 8621
Ticket: 8659
If a client sends a SOTR/RETR command before doing a PASV/PORT
command, ther server may reply
425 Use PORT or PASV first.\r\n
and allow the client to continue sendinf other commands
So, Suricata should not put the whole flow into error, it just
sets an event, and continues to parse further
Extend the xor transform to accept a variable-position key using
the 'extract <nbytes> <offset>' syntax. The engine reads <nbytes> bytes
at buffer position <offset> at transform time.
The optional 'offset' parameter specifies where XOR decoding
starts in the buffer; bytes before that position are left unchanged.
Syntax:
xor:"<hex_key>"
xor:extract <nbytes> <offset>
xor:offset <N>,"<hex_key>"
xor:offset <N>,extract <nbytes> <offset>
Variable-key helpers (VariableKeyLocation, variable_key_bytes,
parse_key_location, strip_keyword_prefix) are imported from the new
varkey module.
The variable key bytes are copied out of the inspection buffer before
decoding, so an in-place transform cannot corrupt the key when the key
region overlaps the decoded range.
Issue: 8671
Introduce rust/src/detect/transforms/varkey.rs to centralise helpers
used by transforms that read key material directly from the inspection
buffer at transform time.
Exported items:
VariableKeyLocation { offset: u16, nbytes: u8 }
variable_key_bytes bounds-checked slice into an inspection buffer
strip_keyword_prefix strip 'keyword<ws>' from option strings
parse_key_location parse '<nbytes> <offset>' into VariableKeyLocation
All four items are pub so any transform can import them. Unit tests
are included in the module.
Issue: 8671
SIP and SSDP share method names like NOTIFY and SUBSCRIBE,
causing SSDP traffic to be misidentified as SIP.
Add a probing parser callback that checks for "SIP/" in the
payload before accepting a pattern match.
Example of a misidentified flow before the fix:
{"timestamp":"2014-02-27T19:44:43.164211+0100","flow_id":986757542077835,"event_type":"flow","src_ip":"192.168.1.1","src_port":9489,"dest_ip":"239.255.255.250
","dest_port":1900,"ip_v":4,"proto":"UDP","app_proto":"sip","flow":{"..."}}
After the fix:
{"timestamp":"2014-02-27T19:44:43.164211+0100","flow_id":986757542077835,"event_type":"flow","src_ip":"192.168.1.1","src_port":9489,"dest_ip":"239.255.255.250
","dest_port":1900,"ip_v":4,"proto":"UDP","app_proto":"failed","flow":{"..."}}
Ticket #8355
Per RFC 2132 the BOOTP sname and file fields can hold extra DHCP
options when option 52 is present, but the parser ignored them. After
parsing the main options we now look up option 52 and walk sname or
file as additional option streams, appending what we find to the same
options vector so the logger and detection keywords see the
overloaded values too.
Bug: #8538.
Giuseppe Longo
Juliana Fajardini
Philippe Antoine
Shivani Bhardwaj
Jason Ish
Victor Julien
Jeff Lucovsky
Jeff Lucovsky
William Ling
Samaresh Kumar Singh