A direction that hit stream.reassembly.depth stops collecting data, so
StreamTcpPruneSession() stops sliding its reassembly buffer and base_seq
freezes where the data stopped while the sequence numbers keep moving. The
base_seq bound on last_ack updates compares an ACK against that anchor with
SEQ_GT(), which is only meaningful within half the sequence space: 2^31 bytes
past the depth it reads a regular ACK as being behind base_seq and last_ack
stops moving, next_win with it, and every packet from then on is flagged
out-of-window or with an invalid ack. 2^32 bytes later both comparisons line up
again at once, last_ack takes one step of 4 GB and the reassembly engine
reports a gap over data the peer already acked - for TLS a decoder error, and
in IPS mode the exception policy drops the flow without RST or FIN.
Take the bound only while the direction is still reassembling, as that is when
base_seq tracks the stream; nothing is inserted once it stopped, so there is
nothing left for the bound to protect. Where base_seq is ahead of the ACK it
still refuses the update, so the check Redmine #6865 added stays in place for
streams that are still collected.
Ticket: #9141.
An empty SETTINGS frame (an ACK, or a SETTINGS without entries) opened
the settings array and closed it without items, producing a record the
eve schema rejects (settings has minItems 1). Skip the frame instead; a
tx whose only content is an empty SETTINGS frame is then not logged at
all, matching the empty headers handling.
Exposed by the flow-end flush of IDS-dropped flows: the dropped flow in
the drop-protocol-change test carries such a frame.
The '<' lower bound hook form is a firewall policy construct: the rule
covers its own hook and every prior one. For a threat detection rule the
form has no such meaning, and the flag it sets leaked into the firewall
policy walk, where the rule could influence the per-state decisions.
Reject the form at load for rules without SIG_FLAG_FIREWALL.
Ticket: #8944.
tls.version requires the connp's hello to have decoded (hello_decoded),
which only TLSDecodeHandshakeHello() set. The SSLv2 hello path sets the
client version without the flag, so an SSLv2-compatible ClientHello no
longer matched tls.version: on main it matched, so a firewall accept
rule using the keyword never accepted.
Set the flag in the SSLv2 client and server hello cases.
Ticket: #9047.
AppLayerParserGetTxEndState() replaces the remaining copies of the
transaction end state computation in the firewall tx setup, the
app-layer-state and app-layer-event keywords and the completion check.
One tolerated a missing tx data pointer, the others asserted or
dereferenced it, so they could disagree.
Move DetectTxCompleted() above its first use while there, so it no
longer needs a forward declaration, and shorten its comment.
Ticket: #8944.
Alerts and drops for firewall rules hooked at a sub-state (http2
stream/global) had no firewall.hook field: the logger resolved the state
name without the sub-state, so the lookup found nothing.
Keep the rule's sub-state on the Signature next to app_progress_hook,
set it from the hook and for the default policy signatures, resolve the
name in the sub-state table, and fall back to the plain state name when
that lookup fails.
Ticket: #8944.
An app-layer-state comparison that can never hold - mode '>' at or beyond
the completion state, or mode '<' at the starting state - can never match.
Both are fatal at rule load, so reject them at parsing.
The LTE (<hook) form at the first state (progress 0) is accepted and is
equivalent to the plain hook form: there are no prior states to auto-accept.
Derive the completion state from the direction the keyword resolved in
instead of the rule's flags, and document the new load time rejections.
Ticket: #8944.
eof is C > P, so an engine registered at a parser's completion state
never becomes complete: there is no next state to enter. A firewall
rule hooking such a state (e.g. tls:client_finished) stayed pending
forever and was optimistically accepted.
Treat progress == engine progress == tx end state as complete. This
also finalizes the buffer engines' eof on completion-state buffers, so
absent and bsize decisions now happen there, and it covers every parser
whose engine progress equals its completion state (dns, ntp, quic,
rdp-style two-state protocols), not only the firewall path that
motivated the change.
Ticket: #8944.
DetectEngineInspectGenericList returned NO_MATCH for every keyword miss,
also when the tx had moved past the phase the engine is registered at.
Unlike the buffer engines it never produced a terminal no match, so a
firewall LTE rule using such a keyword (tls.version on a hook) stayed
provisional and was accepted for the rest of the session.
Make a miss terminal at P + 1. This engine also backs the lua
on-request/on-response hooks and the tls_cert_*, dnp3/dnp3_ind and
ftpdata/ftp mode keywords, so a plain IDS rule using them keeps its
verdict once the tx has moved past the registered phase.
Export the parser's end state computation as AppLayerParserGetTxEndState()
here, so the version keyword can use it; the remaining call sites are
converted later.
Ticket: #8944.
A non-matching rule followed by an LTE rule (<hook) with an overlapping
hook can be decided by the wrong default policy. E.g.:
drop:flow tls:client_hello .. tls.sni; content:"match" \
content:"no-match";
accept:flow tls:<client_hello .. tls.sni; content:"match"
Here the `tls:client_started` state needs to be accepted by the second
rule, which acts as `accept:hook` on that state.
A failing `<` rule defers the per-state defaults of the states it
promised to cover; the last pending covering rule decides them, applying
its own hook default and sweeping forward from its own hook. There is no
backward sweep, so the states below that hook get no default of their
own.
The pending LTE coverage accounting was the problem: it counted
candidates before their header predicates were checked, compared against
a max that included the rule being evaluated, and kept rules that
resolved during the pass. Count only rules that pass the header checks,
keep per-hook coverage counts in per-thread scratch, drop a rule from
the counts once it resolves at or past its hook, and let the hook checks
consult the counts instead of the raw LTE flag.
Also fixes two latent issues in the same walk: the candidate list is now
sorted whenever the tx state store adds entries - the old condition
skipped the sort when the prefilter contributed nothing, so the
last-for-progress peek could see an unsorted list; and rule profiling
now receives `r == 1` instead of the raw return code, which used to
decrement the match counter on a no-match.
Ticket: #8944.
State table and failure/completion semantics for the
per-direction scale (banner/kex/session, done never reported),
the removed banner_wait_eol state and legacy hook names, the new
eve ssh error/state fields, and the transactions/lua/upgrade
examples - all updated to match.
Ticket: #9046.
banner_wait_eol only recorded that a banner line of 256 bytes or
more had arrived without EOL; it was not an active phase. The
direction kept reparsing the open line in place, so a rule or
firewall policy pointing at the state acted on a parked, not an
active, direction.
Move the condition to the event channel it already had (the
one-shot ssh.long_banner event) and key the continuation off an
explicit banner_published latch (set at the banner publish
sites): a direction with an open line stays in the banner state,
the line is consumed greedily until the EOL arrives, and the line
completion takes the normal banner-to-kex transition.
The state scale becomes dense: banner(0) kex(1) session(2),
completion done(3). request_banner_wait_eol /
response_banner_wait_eol no longer load: aliasing them to banner
would silently turn a long-banner rule into an any-banner rule.
Firewall rules that acted at the removed state must move to the
ssh.long_banner event or the banner state; and since the progress
gap between banner and kex is gone, the default-policy sweep no
longer drops a clean flow at the server banner.
Ticket: #9046.
A flow that hit an unrecoverable ssh parse error (invalid banner
or invalid record) was never visible in the eve ssh output: the
log condition only admitted a successful handshake (both
directions at kex or session), and the tx logger is one-shot, so
the successful-handshake log fired before a later failure could
be reported and the failure was then silent - the reason was
only visible through the app-layer events.
The log condition is now failure-only: the failure latches, so
the object is emitted exactly once - at the failure, carrying
the error field (invalid_banner / invalid_record) and the state
field (the state the failure occurred in - banner, kex, or
session if a later record fails after NewKeys) - and successful
flows are logged at the flow-end flush, where the engine logs
unconditionally. Logging on the success condition mid-flow would
consume the one-shot tx log and drop a later failure, so it is
not done. The state field is emitted only alongside the error.
The object may be error + state only, when no banner was
parsed. A unit test pins the condition for a failed, a
banner-only and a both-at-kex flow (the failure admits the log
on the failing delivery); the user guide and eve reference doc
commits that follow carry the field docs.
That flow-end flush is skipped for flows that end without a
further app-layer update, though: in IPS/firewall mode a drop
disables the app layer (StreamTcpCheckFlowDrops ->
StreamTcpDisableAppLayer), after which no app-layer update
reaches the tx loggers, and a bypassed flow gets no flow-end
packet either. A successful ssh record - e.g. one that only saw
the banners - was therefore never emitted for those flows. The
hook is the app-layer end: on the transition to app-layer EOF,
StreamTcpDisableAppLayer raises FLOW_APP_LAYER_FLUSH_PENDING,
and FlowUpdateState raises it on the transition to a bypassed
state; OutputTxLog runs the tx loggers once for the next packet,
also when that packet itself has no app-layer update and is not
marked established (the disabling packet returns from the
stream engine before that), and flow-worker clears the flag when
it frees the app layer of a bypassed flow. The flush is
deliberately not a flow-end flush: when the flush is the only
reason to run the loggers, eof is not forced, so each logger's
condition decides and no other parser starts logging records it
suppressed; a disabling packet that does carry an app-layer
update keeps the existing EOF behaviour. SSHTxLogCondition
admits an IPS drop or a bypass, so the pending record is emitted
there; an IDS drop does not end the app layer, so the condition
stays failure-only - a later failure still wins, and the success
record is emitted by the flow-end flush, which the flow timeout
path now also runs for IDS-dropped flows. The unrelated
schema-sort ordering of the new state keys is fixed up as well.
Ticket: #9046.
Test-only change. The parse-failure freeze was unit-tested for the
invalid first banner and the invalid record (Test28); the
banner_wait_eol failure and the oversized-no-EOL invalid banner were
only pinned by SV (ssh-events). Add a unit test driving both: a
direction parked in banner_wait_eol whose completed line is followed
by an invalid record freezes the direction at kex, and a >=256B
banner without EOL that fails the banner grammar freezes the
direction at banner at once. Both phases assert the toclient
direction is unaffected.
Ticket: #9046.
A banner line that completes while parked in banner_wait_eol now
takes the direction to kex: the banner data was already parsed at
entry, the completion only advances the state. Before, the
direction skipped the kex state entirely and jumped to session at
the newkeys record.
The tx is marked updated at parse entry, as the base parser did,
before the dispatch: every delivery - including the stash and
header-fragment cases that parse_record handles and publish
nothing - is decided by the state rules and the firewall
default-policy sweep (default: drop) rather than left to the
firewall default-accept, as an un-updated tx is not evaluated
mid-flow. The parse entry does not pre-check those cases:
parse_record returns the same AppLayerResult for them, and the
record_left arithmetic must stay in sync in one place only.
Ticket: #9046.
An SSH direction that fails to parse (invalid banner or invalid
record) now keeps the state it was in when the failure occurred:
that state is the last progress the direction reports, and the
failure is exposed through the app-layer events
(ssh.invalid_banner / ssh.invalid_record) and the per-direction
error field tracked here.
The failing direction is frozen at its state: the failure is
unrecoverable, so the parse entry points short-circuit on the
error flag - no further parsing, no state change - and
post-failure garbage can neither advance nor regress it. The
failure site returns a parse error, so the engine ends app-layer
parsing for the whole flow.
The no-inspection/bypass flag arming now runs on the session
transition itself, shared by the complete-record and
incomplete-header arms: a NewKeys record whose body arrives in a
later parser call (segment boundary after the 6-byte record
header) reached the session state without arming the flags, so
the encrypted stream was parsed as records. The arming keeps the
peer check (a merely-failed peer does not arm them). A unit test
pins the split-record arming, including BYPASS_READY in bypass
mode.
Ticket: #9046.
The old ssh state scale was coupled across the two directions: the
terminal finished state was only reached after both sides sent NewKeys,
and the progress accessor clamped every per-direction phase to a
shared value. A rule hooking the old top state therefore meant "the
flow completed", not "this direction completed the handshake", and
there was no room above the terminal state for a true completion
marker.
Replace the scale with per-direction phases:
banner(0) -> banner_wait_eol(1) -> kex(2) -> session(3)
NewKeys now sets session on the packet's own direction only, banner
completion sets kex, and the progress accessor is identity: each
direction reports its own raw phase. Add the registered completion
state done(4), never assigned during stream parsing: it stays out
of both name tables, so a rule referencing it fails to load, the
hook listing omits it and the firewall policy loader does not
consult it; the progress accessor reports it only on flows
disrupted by stream depth or async reassembly.
The legacy state names (in_progress, banner_done, finished) no
longer resolve through the name table (pre-production - breaking
changes allowed); the name-table unit test pins the rejection.
The old firewall policy state keys are added to the rename hint
table (like the TLS migration), so a config using them fails
loudly with the replacement named instead of degrading to a bare
warning. The non-prefilter engine-name registration now guards
the null id-to-name result (a hook-only rule at the completion
progress previously formatted a null name into the engine
name).
Add unit tests for the state name table (new names, rejected legacy
names, the unhookable completion state, id-to-name) and for the
per-direction session state.
The eve log condition compares the per-direction states with
>= rather than ==: a direction at session has necessarily passed
kex, and a single delivery can take a direction
banner -> kex -> session, so a final {session, kex}
pair must still admit the log (done is never assigned, so >=
changes only the kex branch).
Ticket: #9046.
When the tx progress crosses more than one app state and the last rule
for a direction sits below the current progress, the no-match path set
fw_next_progress_missing but never applied the default policies of the
states in between (only the match path did). With the hello phase now
entered at the start of the message, the record that completes a hello
walks over hello -> cert: without the missing-state policies a packet
whose last rule was at a lower hook was decided by the packet default
policy even though the app policy for the current state says accept.
Apply the missing-state policies up to the final state on the no-match
path as well, mirroring the match path - but only when the policy of
the current hook does not already carry the verdict: an accept with tx
or flow scope has decided the flow, and sweeping the config-less states
above it would hit the implicit drop:flow default and kill a flow the
rule just accepted.
Bound the sweep at the state the packet actually reached (tx_progress),
at both call sites: a state beyond the current progress was not walked
over, so its configured default must not decide this packet. Also make
a drop from the no-match sweep end the walk, like a policy drop at the
current hook does.
Ticket: #9047.
The firewall userguide pages still described the old TLS completion
milestones, and an upgrade from the old state names had no guidance.
Update the firewall design and example pages to the phase names,
list the migration page in the firewall index, and add the migration
page itself: the old/new state name mapping, the load-time
validation of rules and config keys, monitoring of ts_progress, and
the semantic changes - the fragmented hello decision, the removed
flight-completion state, one decision per state, the new ja3/ja3s
event timing, and the SSLv2 track behaviour. The 9.0.0 upgrade
guide gains Major Changes (a rule with an old state name fails to
load and is not enforced; config keys renamed) and Logging Changes
(ts_progress/tc_progress values) entries, both linking the
migration page.
Ticket: #9047.
The TLS state rename changed the name tables the rule parser and the
firewall policy config resolve against, and the state ids double as
the ts_progress values in alert events, but no unit test pinned the
tables themselves. Only the downstream sv behavior did.
Add unit tests asserting the current phase names resolve to their
expected ids in both directions (and back), that the pre-phase
milestone names no longer resolve at all, and that the completion
values keep the 5-value tracks.
Also pin the decode-failure suppression at the parser level: a
failed hello does not advance, a clean hello does (the suppression
is per message), and a failed certificate keeps the track out of
the cert phase. The tests allocate and free the full state through
the parser lifecycle functions instead of manual partial cleanup.
A certificate message whose first certificate is undecodable but
whose rest is valid is pinned as well: the bad leaf is skipped
(the valid certificate is kept in the chain and the certificate
data comes from it, compared against a direct extraction from the
fixture) and the track stays out of the cert phase. The
incomplete-data condition is pinned as sticky across a following
handshake message: the per-message suppression resets, the
condition does not, and the track stays out of the cert and data
phases.
Ticket: #9047.
The app-layer firewall policy config resolves per-state keys
against the current state name table only, so keys renamed or
removed in this release are silently ignored: the state falls back
to the implicit default policy (drop in firewall mode) with
nothing logged. A suricata upgrade without updating
firewall.policies.app.tls would then drop traffic with no
diagnostic.
Walk the policy config at load time and compare every per-state
key against the state names of the protocol section it sits in,
mirroring the enumeration the policy loader consults (the plain
state tables and the sub-state sections). An unknown key logs a
warning with the config path. A key matching a state name renamed
in this release (a deliberately name-based, per-protocol hint
list) escalates to an error with the new name suggested, and the
load aborts under init-failure-fatal.
Ticket: #9047.
The server states were completion milestones: server_hello landed
when the server hello record was fully parsed and server_cert_done
when the certificate message was done, so the states named moments
at which the phase data was complete instead of the phase being
processed. The flight-completion state (server_hello_done) named
a moment at which no data is published and no decision can act.
The states now denote the record phase being processed:
server_hello is entered when the server hello message begins
(its header is read once, the message may span several
records) and server_cert when the certificate message begins,
so the state names the message being parsed and a message that
fails to decode leaves the track in the state that names it.
The record that completes a hello hands the track over to the
certificate phase before a single certificate byte arrives. The
enter/leave is the shared handshake phase mechanism, so the client
track follows: client_hello is entered at the start of the client
hello as well, and the completing client hello is decided at
client_cert.
The flight-done state is removed and renumbers the tail
(server_data, server_finished), leaving both tracks with the
same five-phase shape and an absorbing finished state.
Hello and certificate decode failures suppress the phase advance
of their own message only: the decoders keep their pre-existing
return contract (the message bytes are consumed and the error
events are raised as before) and set a per-connp suppression flag
that is reset at the start of the next handshake message and
checked at the phase advance. A decode failure is not an app-layer
parse error: the flow is neither dropped nor blinded, and the next
message that decodes cleanly still advances.
An undecodable certificate in a chain is skipped, not fatal: one
decoder event is raised for the bad certificate and the remaining
certificates are still decoded - the rest of the chain is kept and
the certificate data is filled from the first fully decodable
certificate. The certificate phase is suppressed for such a
message (the phase data is incomplete). The incomplete-data
condition is sticky per connp: it is set while the certificate
message is being decoded, cleared by a fully decoded certificate
message only, and consulted by the handshake-done heuristic that
would otherwise establish the data phase from the certificate
data - a later handshake message does not complete the data, so
the tracks do not cross the cert phase on partial data.
The certificate field extraction is commit-on-success: the fields
are extracted into stashes and published to the connp only when
the whole certificate yields every field, so a certificate that
fails a late extraction leaves no partial data behind and the next
certificate can fill the fields. An extraction failure raises its decoder event once per message: the message no longer aborts at the first failing certificate, and the event is not repeated for the later failing entries.
The tls.cert_chain_len match is gated on the chain being final (the
per-connp flag set by a fully decoded certificate message), so it
does not match while the track sits in server_cert with a chain that
is still arriving.
TOCLIENT keyword registrations and the tc log threshold follow the
renamed constants (values 1/2 unchanged; the threshold is
triggered by the same writes as before, so the log fires on the
same packets). The ja3s lua registration moves to server_data: lua keeps a numeric
hook, so it must follow the renamed state the old hook value pointed
at (server hello done was 3, server data is 3). The ja3s content is
final once the ServerHello parses, which is why the C keywords
(ja3s.string, ja3s.hash) stay registered at server_hello. Two string-based
generic buffer lookups and the user guide follow the renamed
states.
Ticket: #9047.
The client states were completion milestones: client_hello_done
landed only when the hello record was fully parsed, so during the
in-hello window (possibly multiple packets) the flow sat in
client_in_progress and no firewall state named the phase being
processed.
The states now denote the record phase being processed:
client_hello is entered when the hello message is fully parsed
(it may span several records) and client_cert when the
certificate message is fully parsed, so the phase data (SNI,
certificates) is available when its state is entered; a message
that fails to parse does not advance the track. The milestone
writes are removed and the name table, TOSERVER keyword
registrations (values unchanged) and the firewall user guide
follow the new names.
Ticket: #9047.
In TLS 1.2 and 1.3 the Finished message precedes the application
data on both directions. The DATA milestone write is keyed on the
parsed certificate subjects and runs on every record, so the first
record after the handshake - for a session that ends with a fatal
alert, the alert itself - moved the tracks backwards (client 4->3,
server 5->4). The completion states were therefore not absorbing:
the finished hook windows were transient in decryption mode, and a
keyword registered at progress >= 3 would have its eof revoked and
be suppressed by the stored CANT_MATCH flag.
The setters now only move a state forward, so post-terminal writes
are no-ops and the finished states absorb.
Ticket: #9047.
Ticket: 6028
memmove_s is a C11 safer version adding a fourth argument which
is the total size of the destination buffer, so as to check there
is no risk of overflow
block_scrutinee now warns on a block-wrapped if let scrutinee, since
the block changes when temporaries drop. The file config reads in the
nfs, smb and http2 gettxfiles handlers were wrapped in a pointless
single-expression block; remove it.
PostRunDeinit stops the flow manager, then runs the final flow sweeps. TCP
flows that a foreign thread times out during those sweeps are parked on the
flow hash row's evicted list for the manager to pick up, but the manager is
gone.
FlowRemoveHash only walked the row's head list, so those flows were later
recycled without FlowFinish, skipping the final reassembly and pseudo
packet pass. Any app layer tx that only completes at flow end is then
lost: typically a response arriving after the peer's FIN and never ACKed,
as that data only becomes usable at eof in non-inline mode.
The result is an intermittent count mismatch in the suricata-verify
suite, always exactly one short, e.g. "expected 28 matches; got 27" for
http-sticky-server and "expected 7 matches; got 6" for
smb2-ntlmssp-negotiateflags. The flow event is still logged, with reason
"shutdown", so the flow is not lost, only its tail transactions. The
lost records can be spotted in a reference eve.json by "pkt_src":
"stream (flow timeout)" and no pcap_cnt.
Stranding a flow needs a hash row collision plus a foreign thread sweep
inside the [last manager sweep, manager stopped] window, and the row
mapping is seeded with a per process random value (flow.c hash_rand).
That makes it a per run lottery at the default 65536 rows, and
deterministic with a small flow.hash-size such as 12 or 20. Unit tests
cannot cover it: FlowBelongsToUs returns true in unittests runmode, so
the evicted list is never populated there.
Also sweep the evicted list, with the same decision point as the head
walk: FlowNeedsReassembly sets ffr_ts/ffr_tc, which CheckWorkQueue
requires before running FlowFinish. CheckWorkQueue applies its own
bypass and drop guards, so a bypassed flow may now take a worker round
trip and have its flow event logged there rather than by the recycler.
The evicted list has never been swept on this path.
Fixes: 244dd11c34 ("flow/manager: fix flows not evicted & freed in time")
Ticket: #9146.
Philippe Antoine
Victor Julien
dependabot[bot]