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

Add unknown_protocols.log for logging unsupported packet protocols #1254

Merged
merged 2 commits into from Nov 10, 2020

Conversation

timwoj
Copy link
Contributor

@timwoj timwoj commented Oct 26, 2020

This code is modeled after how we log weirds, and how rate-limiting happens for them. It's currently using the same values for the rate limiting, but I'm open to suggestions for adjustments to those. Also, I really don't like the first_bytes variable name but I was struggling to come up with anything better.

Fixes #1221

The external testing repos need merging from branches with the same name.

Copy link
Collaborator

@J-Gras J-Gras left a comment

Choose a reason for hiding this comment

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

I bet that feature will allow some interesting insights. I added some comments on minor naming details.

scripts/policy/misc/unknown-protocols.zeek Show resolved Hide resolved
src/packet_analysis/Manager.cc Outdated Show resolved Hide resolved
src/packet_analysis/Manager.cc Show resolved Hide resolved
@@ -113,11 +132,21 @@ class Manager : public plugin::ComponentManager<Tag, Component> {
*/
AnalyzerPtr InstantiateAnalyzer(const std::string& name);

bool PermitUnknownProtocol(const std::string& analyzer, uint32_t protocol);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather call that IsUnknownProtocolPermitted but I see that this aligns to the naming for Weird.

@timwoj timwoj force-pushed the topic/timw/1221-unknown-protocols branch 4 times, most recently from cb7aa69 to 8c8ae7a Compare November 2, 2020 16:03
Copy link
Contributor

@jsiwek jsiwek left a comment

Choose a reason for hiding this comment

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

General structure looked good, thanks. See the suggestions/comments though.

scripts/policy/misc/unknown-protocols.zeek Outdated Show resolved Hide resolved
src/event.bif Outdated Show resolved Hide resolved
src/packet_analysis/Manager.cc Outdated Show resolved Hide resolved
src/packet_analysis/Manager.cc Show resolved Hide resolved
scripts/policy/misc/unknown-protocols.zeek Outdated Show resolved Hide resolved

## How many reports for an analyzer/protocol pair will be allowed to
## raise events for logging being rate-limited.
option sampling_threshold : count = 25 &redef;
Copy link
Contributor

Choose a reason for hiding this comment

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

Think it may be be fine to have a much lower threshold by default. Maybe like 1-3 really seems enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yah the values of these was really up for review here. I picked the defaults used for weirds, but I agree that it can still be a ton of output.

## rate-limited pairs of a given type will be allowed to raise events
## for further script-layer handling. Setting the sampling rate to 0
## will disable all output of rate-limited pairs.
option sampling_rate : count = 1000 &redef;
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, thinking we may want pretty generous defaults since this stuff is all possibly checking per-packet. Maybe 100k ?

## How long an analyzer/protocol pair is allowed to keep state/counters in
## in memory. Once the threshold has been hit, this is the amount of time
## before the rate-limiting for a pair expires and is reset.
option sampling_duration = 10min &redef;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe in the realm of hour(s) instead ?

@timwoj
Copy link
Contributor Author

timwoj commented Nov 9, 2020

Fixed the few comments and updated the external testing baseline.

@timwoj timwoj force-pushed the topic/timw/1221-unknown-protocols branch from 2a9e1b8 to f3424e1 Compare November 10, 2020 00:16
@timwoj timwoj force-pushed the topic/timw/1221-unknown-protocols branch from f3424e1 to b8abdb6 Compare November 10, 2020 02:20
@timwoj timwoj force-pushed the topic/timw/1221-unknown-protocols branch from b8abdb6 to e7c2621 Compare November 10, 2020 03:18
@timwoj timwoj force-pushed the topic/timw/1221-unknown-protocols branch from e7c2621 to c3cf36e Compare November 10, 2020 03:37
@timwoj timwoj merged commit ad46a8b into master Nov 10, 2020
@timwoj timwoj deleted the topic/timw/1221-unknown-protocols branch April 7, 2021 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Packet analysis: rework weirds for missing analyzers
3 participants