Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pcap conditional v2.2.7 #6408

Closed

Conversation

scottfgjordan
Copy link
Contributor

Make sure these boxes are signed before submitting your Pull Request -- thank you.

Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/120

Describe changes:

suricata-verify-pr: 542

regit and others added 23 commits September 23, 2021 17:19
Add an option to only write to pcap packets with alerts and flow
that have alerted.
It will be used later when multiple writing operations will be
necessary.
We add a flag to packet to be able to know if this packet was the
first one to get alerts on the flow.
This patch optionally adds packet header to the TCP segment
and update the for each segment function by changing the
callback.

This patch is based on the work by Scott Jordan <scottfgjordan@gmail.com>
This patch update the alert mode of pcap logging.

It uses the packet header data added to the TCP segments
to build packets corresponding to the acked data that did trigger
the alert. It then write it to the pcap file before starting to
dump all packet for the flow that did alert.
In alert mode, we need to write the root packet to the pcap
file instead of the packet that did trigger the alert.
This patch adds the tag as logging condition. If this option is
used all tagged packets are written to the pcap.
TCP memory cap was not taking into account the memory that can
be used by realloc of Packet headers in TCP segments.
This patch adds a function to get the current pcap file name that
will be used to current packet.
This patch updates EVE alerts to add pcap output filename when pcap
capture is done in multi mode. In other mode, the pcap file can be
determined by checking the timestamps.
This patch updates tcp segment dumping to dump segments
from both sides of the session in order when capturing
alerts and tags.
We may need to know that a packet has been tagged but is the
first one (and thus is not tagged).
We were missing the first packet when using condition pcap logging
in tag mode as it was not tagged. As a result we were not getting
the stream data triggering the alert in the pcap file.
As Suricata is not supporting pcap-ng we have to stick with one single
datalink type for the capture if ever we want to do pcap logging.
Assuming this, this patch introduces a function to set the link
type globally. This will be used with pcap conditional logging
to get the logging of TCP segments with the correct link type.
Set pseudo packet datalink to the global one. This fixes the case
where the pcap handle is open with information coming from a
pseudo packet. Without this, we did end up in most cases with
an Ethernet packet being written in a Raw pcap.
Addressed some small code cleanup comments from PR review.
@scottfgjordan scottfgjordan requested review from norg and a team as code owners September 24, 2021 12:58
@scottfgjordan scottfgjordan mentioned this pull request Sep 24, 2021
3 tasks
@codecov
Copy link

codecov bot commented Sep 25, 2021

Codecov Report

Merging #6408 (b4c9386) into master (a480ec2) will decrease coverage by 0.05%.
The diff coverage is 61.46%.

@@            Coverage Diff             @@
##           master    #6408      +/-   ##
==========================================
- Coverage   77.02%   76.97%   -0.06%     
==========================================
  Files         612      613       +1     
  Lines      186105   186367     +262     
==========================================
+ Hits       143353   143447      +94     
- Misses      42752    42920     +168     
Flag Coverage Δ
fuzzcorpus 52.69% <10.50%> (-0.15%) ⬇️
suricata-verify 51.79% <68.44%> (+0.11%) ⬆️
unittests 63.03% <6.18%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@@ -577,6 +632,9 @@ int StreamTcpReassembleInsertSegment(ThreadVars *tv, TcpReassemblyThreadCtx *ra_
StreamTcpSegmentReturntoPool(seg);
SCReturnInt(-1);
}
if (IsTcpSessionDumpingEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest making IsTcpSessionDumpingEnabled an inline function and changing it's return type from int to bool given the frequency with which it'll be invoked.

SCFree(seg);
return NULL;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing call to StreamTcpReassembleIncrMemuse to adjust the memory used that's tracked by the memcap value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Thanks.

return g_datalink_value;
}

int DatalinkHasMultipleValues(void)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strongly suggest type bool for this

@scottfgjordan scottfgjordan mentioned this pull request Sep 30, 2021
3 tasks
@scottfgjordan
Copy link
Contributor Author

Replaced by: #6431

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants