The fix for issue 7447 introduced an error with threaded eve output.
The changes that were committed for that issue mishandled the return
value when a file is being opened for the 2nd or higher time.
Instead of returning the existing file context, null was returned.
Currently, the syntax includes direction as a part of the keyword which
is against how usually keywords are done. By making direction as a
mandatory argument, it is possible to make the syntax cleaner and the
implementation more compact and easily extendable.
Pros:
- Registration table sees lesser entries if newer options are added
- If the options have to be extended, it can be done trivially
- In accordance w existing keyword implementations
Note that this commit also retains the existing direction specific
keywords.
Since forever (1578ef1e3e) a valid RST
would update the internal `last_ack` representation to include all
unack'd data. This was originally done to make sure the unACK'd data was
inspected/processed at flow timeout.
It was observed however, that if GAPs existed in this unACK'd data, a
GAP could be reported in the stats and a GAP event would be raised. This
doesn't make sense, as missing segments in the unACK'd part of the
stream are completely normal. Segments simply do not all arrive in
order.
It turns out that the original behavior of updating `last_ack` to
include all unACK'd data is no longer needed.
For raw stream inspection, the detection engine will already include the
unACK'd data on flow end.
For app-layer updates the unACK'd data is often harmful, as the data
often has GAPs. Parser like the http parser would report these GAPs and
could also get confused about the post-GAP data being a new transaction
including a file. This lead to many reported errors and fantom txs and
files.
Since the GAP detection uses `last_ack` to determine GAPs, not moving
`last_ack` addresses the GAP false positives.
Ticket: #7422.
Some checks can be done w/o holding a lock:
- seeing if the flow matches the packet
- if the hash row needs a timeout check
This patch skips taking a lock in these conditions.
Since `Thread` objects are part of a big allocation, more than one
Thread could be on a single cache line, leading to false sharing. Atomic
updates to one `Thread` could then lead to poor performance accessing
another `Thread`. Align to CLS (cache line size) to avoid this.
Until now many accesses to the Thread structure required taking a global
lock, leading to performance issues. In practice this only happened in
offline mode.
This patch adds a finer grained locking scheme. It assumes that the
Thread object itself cannot disappear, and adds a spinlock to protect
updates to the structure.
Additionally, the `pktts` field is made an atomic, so that it can be
read w/o taking the spinlock. Updates to it are still done under lock.
Flow Manager skips rows based on a minimized tracker that tracks the
next second at which the first flow may time out.
If seconds match a flow can still be timing out.
If a thread doesn't receive packets for a while the packet timestamp
will no longer be used to determine a reasonable minimum timestamp for
flow timeout handling.
To avoid issues with the minimum timestamp to be set a bit too
aggressively, increase the time a thread can be inactive.
The flow worker needs to get the opportunity to run the flow update
before globally making it's current timestamp available. This is to
avoid another thread using the time to evict the flow that is about to
get a legitimate update.
Ticket: #7455.
Ticket: 7465
If a bug chunk of data is parsed in one go, we could create many
transactions even if marking them as complete, and have
quadratic complexity calling find_request.
Proposed solution is to fail on creating a new transaction if too
many already exist.
So we get:
1. request arrives - buffered due to not ackd
2. response arrives, acks request - request is now parsed, response isn't
3. ack for response, response parsed. Then detect runs for request,
generates alert. We now have 2 txs. txid will be 0 from AppLayerParserGetTransactionInspectId
But txid 1 is unidirectional in the other way, so we can use txid 0
metadata for logging
Ticket: 7449
Issue: 7466
This commit releases the memory for the flow variable "key" when
the flow variable is of type string. The key is allocated in the Lua
extension logic.
Issue: 7447
When the output file can't be opened, 2 error messages are displayed
for the same problem. The second message doesn't add value and lacks
context (error reason, e.g., "Permission denied").
Retaining the second message as a debug message.
Without this commit:
Error: logopenfile: Error opening file: "/home/jlucovsky/src/jal/suricata-verify/tests/bug-5198/output/noperms/eve.1.json": Permission denied [SCLogOpenFileFp:util-logopenfile.c:428]
Error: logopenfile: Unable to open slot 1 for file /home/jlucovsky/src/jal/suricata-verify/tests/bug-5198/output/noperms/eve.json [LogFileEnsureExists:util-logopenfile.c:737]
Error: runmodes: unable to initialize sub-module eve-log.stats [RunModeInitializeEveOutput:runmodes.c:692]
With commit:
Error: logopenfile: Error opening file: "/home/jlucovsky/src/jal/suricata-verify/tests/bug-5198/output/noperms/eve.1.json": Permission denied [SCLogOpenFileFp:util-logopenfile.c:428]
Error: runmodes: unable to initialize sub-module eve-log.stats [RunModeInitializeEveOutput:runmodes.c:692]
This commit improves error handling for cases when file(s) cannot be
opened.
- Return NULL if file object can't be opened
- checks whether the file object has been opened before
dereferencing the per-file context.
Issue: 7447
Victor Julien
Jeff Lucovsky
Shivani Bhardwaj
Philippe Antoine
dependabot[bot]