SMTPGetLine assigned response frames to the current transaction even when a
queued reply belonged to an older transaction.
Use the queued command owner so frame EVE output and frame-based detection
receive the correct transaction id.
Includes some re-org to avoid prototypes for static functions.
Ticket: #8741
A BDAT command that failed to parse, such as "BDAT 5 X", returned -1,
disabling SMTP parsing for the rest of the flow. A server may reject the
command and continue the session, leaving following messages
uninspected.
Instead raise a decoder event and queue the command as an ordinary
command.
Ticket: #8741
Track the BDAT LAST marker so the final chunk and its reply complete the
transaction in each direction, preventing a following MAIL FROM from
being merged into the previous transaction.
Ticket: #8741
Ticket: 8797
Add check to rlen value read from ERF record to avoid potential infinite loop, must be >= dag_record_length (16).
Similar check exists in source_erf_file already.
The TLV loop for the IPv6 hop-by-hop and destination option headers
gates each option on (offset + 1 + ip6_optlen) > optslen. An option is
two header bytes (type, length) plus ip6_optlen data bytes, so its data
ends at offset + 1 + ip6_optlen and the bound has to be
offset + 2 + ip6_optlen.
Before: an option that declares its data running one byte past the
option area is accepted, and the Router Alert / Jumbo / Home Address
memcpy then copies sizeof(value) bytes ending one byte past the area;
when the header sits at the tail of the IPv6 payload that read goes
past the payload.
After: the option is rejected with IPV6_EXTHDR_INVALID_OPTLEN, matching
how the IPv4 option loop already bounds a full option against the
remaining length. The check stays next to the read so each option type
does not need its own guard.
Ticket: #8817.
Make sure the transaction still exists before completing it in the
to-client direction. A pipelined RSET reply may already have completed and
freed it while a later end-of-DATA marker still refers to it.
Found by OSS-Fuzz testcase 5498180758994944.
Bug #8739.
Make sure that every buffer in a signature is actually used in the
setup of the engines. Reject sigs that are locked to a certain substate
and use buffers that require another.
In firewall mode the alproto logic when building prefilter and rule
engines is strict, but was out of sync between prefilter and rule app
inspect engines.
For SIGNATURE_HOOK_TYPE_APP rules the logic is strict, with an exception
for HTTP/2 and DOH2. DOH2 is not a full protocol implementation, but
rather HTTP/2 with a different alproto.
Fixes: d64954a873 ("detect: don't register unrelated inspect engines")
With substate support the fixed table approach for policies was no
longer a good fit, so convert to a hash table. The policies are stored
per alproto, sub_state, progress and direction.
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.
Support for per transaction sub states: different state machines per
transaction type.
Skip engines not belonging to our substate.
Store tx_type in DetectTransaction.
Each protocol supporting sub states will register states from 1 and up.
To support:
Ticket: #8386.
Allow registration of per substate progress name mappings.
Implement logic for getting sub-state names, id's.
Add transaction type and end of progress values to AppLayerTxData.
Introduce helpers to keep logic clean.
Track the transaction id for each queued SMTP command so replies can update the
transaction that created the command instead of always using the current
transaction.
Ticket: #8393
Add directionality to completion states, and replace tx->done by checking for
both directions being complete.
This means that the transaction is now not complete until the server responds
to the clients of data marker, previously the tx was completed when the client
send end of data without waiting for the server response.
This keeps smtp:response_complete from being exposed before the server response
is parsed.
Ticket: #8393
Add minimal SMTP progress states to support envelope validation before
moving to data.
Update SMTP, file and email keywords to hook into the appropriate
states.
Purposefully kept minimal for now as to not break the current idea of an
SMTP transaction, which is probably not ideal for firewall mode.
Ticket: #8393
Extend the app-layer-protocol keyword to accept a pipe-separated list of
protocol values, so a single rule can match any of several protocols:
app-layer-protocol:[!]<proto1>|<proto2>[|...][,<qualifier>]...;
A non-negated list matches when the flow's protocol equals any listed value
(OR); a negated list matches when it equals none of them (NOR). The
single-value form and the trailing mode qualifier are unchanged.
Matching keeps the historical AppProtoEquals() equivalences by default
(dns/doh2, http/http1/http2, dcerpc/smb, ...). An `exact` qualifier selects
strict identity matching with no equivalences and no http umbrella; it
combines with a direction mode in any order. Because a flow is never the
generic ALPROTO_HTTP, `http,exact` is rejected at load.
Values are expanded once at rule load into an effective match-set bitmask, so
the per-packet match is a single bitmask test. Single-value rules remain
prefilterable; multi-value rules are excluded from prefiltering and an
explicit prefilter on them is rejected. Conflicting keyword combinations
(duplicate or overlapping negations, mixed positive/negated) are rejected at
load. Engine-analysis reports the effective match set.
Ticket: 7705
Ticket: 8629
When we are in async-oneside mode, we see only one direction
of the traffic, and should not wait for the other direction
before cleaning up a transaction.
Ticket: 8694
Otherwise, a flow full of small compression bombs is too slow
to process.
When the threshold is reached, decompression is skipped for the
rest of the flow.
Ticket: 8592
Fixes: 5ddd808e9b ("ftp: don't halt the flow when raising
too_many_transactions")
In the case we receive a big chunk of TCP data, we end up
creating much more than max-tx transactions, and have
quadratic complexity on this packet, even if all
these transactions get cleaned up at the end of the processing
of this packet.
EBPFLoadFile() unconditionally raised RLIMIT_MEMLOCK to infinity and
aborted the eBPF/XDP load if the call failed.
Since Linux 5.11 BPF map memory is memcg-accounted and is no longer
charged against RLIMIT_MEMLOCK (https://lwn.net/Articles/829307/), so
raising the limit is unnecessary on those kernels.
Ticket: 8719
Fix Flow Manager error when flows were not being timed out
in case flow.hash-size < 10.
This happened because calculation of variable rows_per_sec in
flow-manager.c rounded the value to zero.
This commit ensures that variable rows_per_sec is at least 1.
Ticket: 8710
In firewall mode, a accept:hook or accept:tx needs to lead to a accept
packet when the action is applied to the last TX. For this the code
relied of the `DetectTransaction::is_last` field, where the assumption
was that there would always be an inspection on the last transaction.
This assumption was wrong however, as transactions can be skipped for a
few reasons: not updated, fully inspected, unidirectional for the other
direction. This would cause the accept not be applied to the packet,
leading to a default drop.
The reason this wasn't noticed before is that until now the work had
focused on protocols that used sequential transactions (http/tls),
and/or short lived sequential unidir transactions (dns)
This patch addresses the issue by making a simple assumption: if the
last available transaction in the main detection loop is skipped, it
means it has been accepted before. Therefore we can apply the "accept
packet" logic in this case.
Bug: #8698.
Fix how error messages are printed in a case error occurs in the ebpf
flow table code. The log flag g_flowv4_ok was inverted,
so the error messages were never printed.
The fix follows g_flowv6_ok flag code structure.
Ticket: 8704
buffer_type_id is a u32
We have a list per buffer of a list of AppProto
But the list of AppProto can grow maximum for the case of file.data
to all the alprotos supporting files.
Ticket: 8001
In almost every case, if there is a alstate there is also a pstate. So
remove the conditional pstate handling, and replace it by
unconditionally using the pointer. Add debug validation to make sure the
assumption is and stays correct.
Explicitly handle the one exception in AppLayerParserParse, which
follows an error before the pstate is allocated, or when pstate
allocation itself fails.
Completes commit 7e725c650d
autofp-scheduler with value ftp-hash ends up using
FlowGetIpPairProtoHash which ignores the ports for ftp-looking
flows so that the ftp and ftp-data flow get processed by the
same thread.
As for the other cases, we want to use every other parameter
to compute the flow hash, inclusing the live device
Support STOU on the FTP data channel.
With STOU a filename is optional. If no filname is provided, use
"<stou>". The server does respond with the filename to use, but there
can be a race condition in getting this filename and setting up file
storage before data starts flowing.
Ticket: #8664
Register ftp.dynamic_port at the FTP transaction completion progress
instead of progress 0. This allows ftp.dynamic_port to be used in
ftp:response_complete.
Ticket: #8392
Revert issue 8619 fix in favor of a more general change that
sets INSPECTED_T{S,C} correctly on passed flows. This makes clean no
longer need the FLOW_ACTION_PASS special case to free those
transactions.
Issue: 8621
Issue: 8628
AppLayerParserSetTransactionInspectId() had an inverted guard when
tagging completed transactions as inspected: it set the INSPECTED flag
only on txs that already had it, so a finished tx was never newly
marked.
On a normal flow DetectRunTx() sets the same flag and masks the
mistake. On a passed flow -- a `pass` rule or a pass-the-flow exception
policy -- detection is skipped and this function is the only thing that
marks txs inspected. With the inverted guard the transactions are never
flagged, AppLayerParserTransactionsCleanup() never frees them, and the
per-flow transaction list grows without bound while each packet
re-scans it, giving O(n^2) cleanup cost.
This is an unintended regression from 834378ff88.
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
This reverts commit 923ad6af77.
Several issues have been reported:
1. non-deterministic thread to queue mapping (privately reported)
2. race condition causing IPS mode to loose packets, or get stuck
completely, ticket 8667.
Ticket: #8667.
Ticket: 8600
CreateServiceA doc states for example :
> If the path contains a space, it must be quoted so that it is
correctly interpreted.
Also fixes strlcat usage and check return value to error out on
truncated path
The ftpdata_command would never match a RETR in the to_server direction,
or a STOR in the to_client direction, this effectively makes the
ftpdata_command applicable to passive mode only.
Instead, remove the directionality on the ftpdata_command. Its "meta"
information passed from the control channel and isn't really
directional. I think the idea was to abstract away active/passive from
the rule writer, but unfortunately doesn't allow the ftpdata_command
keyword to work in the more strict firewll mode.
Ticket: #8663
In FTP, the first transaction is a banner from the server. Skip TS
inspection on this transaction.
Allows firewall mode to get past this transaction and onto the next.
Ticket: #8662
Reopen file descriptor for lz4 with the init function. This helps
code analyzers understand the handle it's leaked.
Improve flow of profiling dumps to avoid analyzer confusion around the
file descriptor.
Suppress TAILQ related warnings.
To assist code analyzers. Gcc -fanalyzer got confused about it.
Also test data pointer and length before calling fwrite and check the
result better.
Use a single atomic for the max open files check.
Reading a pcap from /dev/stdin or a named pipe currently fails with "failed to get first packet timestamp. pcap_next_ex(): -1" because InitPcapFile calls setvbuf on the FILE* underlying the pcap handle after libpcap has already consumed the pcap header. On a non-seekable fd glibc cannot recover from that and the next read returns -1.
Detect non-regular files via fstat and skip setvbuf in that case so the read keeps working on pipes, fifos and stdin.
Accept pcap-file.buffer-size values of 0, which disables setvbuf buffering as an explicit opt-out, or PCAP_FILE_BUFFER_SIZE_MIN (4 KiB) to PCAP_FILE_BUFFER_SIZE_MAX (64 MiB). Treat any non-zero setvbuf return value as an error, not just negative values.
When pcap-file.buffer-size fails to parse, retain the default buffer size instead of falling through and setting it to 0. The branches are now mutually exclusive so only one of the parse-error, accepted, or out-of-range messages is logged.
Update the user guide: --pcap-file-buffer-size now documents valid values of 0 (disables setvbuf buffering) or 4 KiB to 64 MiB, and pcap-file.rst notes that 0 is the opt-out for non-seekable sources such as stdin and named pipes.
Bug: #8464.
DetectEngineThreadCtxInitKeywords returns TM_ECODE_FAILED when a per-thread keyword init fails (for example DetectFilemagicThreadInit), but ThreadCtxDoInit discarded that result and still returned OK. The detect thread then ran with a partially initialized keyword context array, producing indeterminate results. Propagate the failure so the callers abort thread init and clean up.
Add a unit test that registers a keyword whose thread init fails and verifies that DetectEngineThreadCtxInit reports the failure.
Ticket: #8237
For rules that specify an explicit app-layer hook,
e.g. http1:request_headers, don't register inspect engines for
other protocols like HTTP/2. These have their own progress tracking,
so should be excluded from these rules.
EveEmailLogJsonData() freed the provided SCJsonBuilder when
SMTP state was unavailable, even though ownership remains with
the caller. The caller may continue using the builder for
cleanup after the function returns failure.
Return failure without freeing the builder and add defensive
NULL checks in EveSmtpDataLogger().
Signed-off-by: Urval Kheni <urvalkheni777@gmail.com>
SCDetectEngineRegisterRateFilterCallback() dereferences the result of
DetectEngineGetCurrent() unconditionally. Add a NULL guard with
SCLogError and return false so callers can detect registration failure.
Flagged by Svace and confirmed by gcc -fanalyzer.
Ticket: 8560
Add DEBUG_VALIDATE_BUG_ON() at four sites in DetectEngineSignatureIsDuplicate()
where HashListTableLookup() is assumed to return non-NULL. Documents the
invariant that every Signature in sig_list has a dup_sig_hash_table entry
and catches violations in debug builds.
Flagged by Svace static analyzer.
Ticket: 8635
PacketAlertCreate is called from PacketInit on the packet allocation
path. Make PacketInit return bool and propagate the NULL result from
PacketAlertCreate up through PacketGetFromAlloc, which already returns
NULL to signal allocation failure to its callers.
Update the UNITTESTS-only helpers in defrag.c accordingly: helpers
returning Packet * use an explicit NULL check; the one returning int
keeps the existing FAIL_IF style.
Two fixes:
- Remove extra parentheses in existing NULL check: (*ext) -> *ext,
which was causing the cocci script to miss the check as a false negative.
- Simplify SCHSConfigInit to return SCCalloc() directly; the caller
in detect-engine.c already checks the return value for NULL.
The original pointer was overwritten with the SCRealloc result before
checking for NULL, causing a memory leak if reallocation fails.
Check the temporary pointer first before assigning.
When Redis output is configured in stream/xadd mode with a positive
stream-maxlen, SCConfLogOpenRedis() allocates redis_setup.stream_format
and immediately passes it to snprintf().
If SCCalloc() fails, snprintf() receives a NULL destination pointer and
the process can crash during Redis output initialization. Handle this
unrecoverable setup failure with FatalError(), matching the surrounding
Redis initialization error handling.
Ticket: 8588
Track the first DATA chunk's data offset and length during chunk iteration,
then reassign p->payload to point at the user data.
When no DATA chunk is present (INIT, SACK, HEARTBEAT, etc.),
payload_len is set to 0 since there is no application data.
Ticket #4251
Extend the SCTP decoder to parse chunk headers after the 12-byte common
header. Each chunk is validated for minimum header size and length
consistency per RFC 4960 sec 3.2.
Add SCTPChunkHdr and SCTPVars structs to track per-packet chunk metadata
Add five new decoder events for protocol violations:
- SCTP_CHUNK_TOO_SMALL: insufficient data for a chunk header
- SCTP_CHUNK_LEN_INVALID: chunk length < 4 or exceeds packet
- SCTP_INIT_CHUNK_NOT_ALONE: INIT/INIT_ACK bundled (RFC 4960 sec 6.10)
- SCTP_INIT_WITH_NON_ZERO_VTAG: INIT with vtag != 0 (RFC 4960 sec 8.5.1)
- SCTP_DATA_WITH_ZERO_VTAG: DATA chunk with vtag == 0
Ticket #4251
Call a SPARC-specific helper during pre-init so the kernel emulates unaligned accesses for this process instead of terminating on alignment faults. This matches the Solaris/SPARC portability fix that motivated the original patch.
Replace uses of the non-standard sun macro with __sun and use Solaris byte swap helpers from <sys/byteorder.h>. This keeps the Solaris-specific code paths reachable when building with standard-conforming toolchains.
toggle command is not used by any major rulesets and increases the state
complexity of flowbits management. Also, all operations can be carried
out with the combination of other available commands. So, remove it.
Task 8595
At some point, bindgen include ordering changed such that
AppLayerGetFileState was being bindgen'd as opaque, as the definition of
StreamBufferingConfig was not available when bindgen hit
AppLayerGetFileState, and bindgen processes in order.
Move the util includes before the app-layer includes to fix the ordering
problem, but still keep util includes grouped.
The sys diff is large as many things have been re-ordered.
Ticket: 8619
Do not wait to run inspection on the other side of transactions
as we do not run any tx detection on passing flows.
Avoids accumulating txs
Ticket: 8518
Keywords that work for HTTP2 headers match now as soon as possible
A push promise is now considered like a headers frame with regards
to the progress (no dedicated "reserved" progress/state)
http.protocol and http.stat_msg keywords are now registered at
earliest progress, since these are synthetic like "HTTP/2" and
not really seen on the wire.
http.request_line and http.response_line match only on data,
and not on headers, since we must wait the end of headers
to be sure to have the full line
http2.size_update now matches at headers progress as it should
http2.frametype, http2.errorcode, http2.priority now match like
http2.window, when the tx is complete from both sides, as a
half-closed client may still send priority, rst_stream
or window_update frames
Add detection keywords for LLMNR protocol inspection:
- llmnr.queries.rrname
- llmnr.answers.rrname
- llmnr.authorities.rrname
- llmnr.additionals.rrname
- llmnr.response.rrname
LLMNR reuses the existing DNS functions since it follows the same
protocol structure. The implementation registers LLMNR-specific keywords
that leverage the DNS data extraction functions.
Both to_server and to_client directions are supported.
Ticket #8366
This adds an LLMNR protocol logger that reuses existing DNS functions,
following the same approach as the mDNS logger:
- No grouped logging
- Rdata is logged in a field that is named after the rdata type
- Types are logged in lower case
- Flags are logged as an array
Ticket #8366
This adds a parser for LLMNR protocol over both UDP and TCP.
The parser reuses the existing DNS functions since LLMNR shares
the same wire format as DNS, but implements its own protocol-specific
validation:
- LLMNR-specific flag checks (C, TC, T bits)
- Opcode validation (only standard query opcode 0 is valid)
- An Event is set if Z-flag is set, since it's must be zero per RFC4795
LLMNR transactions inherit DNS behavior where each packet creates a new state
with one transaction.
Ticket #8366
Jason Ish
Stephen Donnelly
Dmitry Uryvchikov
Uwez Khan
Victor Julien
Philippe Antoine
Jeff Lucovsky
Shivani Bhardwaj
Yash Datre
Amir Boussejra
Adam Kiripolsky
Philippe Antoine
Jeff Lucovsky
Abhijeet Singh
Samaresh Kumar Singh
Urval
Denis Balashov
Giuseppe Longo
Martin Rehak
Antoine Abou Faysal
William Ling
Giuseppe Longo