-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
eve/alert: include rule text in alert output #2881
Conversation
For SIEM analysis it is often useful to refer to the actual rules to find out why a specific alert has been triggered when the signature message does not convey enough information. Turn on the new rule flag to include the rule text in eve alert output. The feature is turned off by default. With a rule like this: alert dns $HOME_NET any -> 8.8.8.8 any (msg:"Google DNS server contacted"; sid:42;) The eve alert output might look something like this (pretty-printed for readability): { "timestamp": "2017-08-14T12:35:05.830812+0200", "flow_id": 1919856770919772, "in_iface": "eth0", "event_type": "alert", "src_ip": "10.20.30.40", "src_port": 50968, "dest_ip": "8.8.8.8", "dest_port": 53, "proto": "UDP", "alert": { "action": "allowed", "gid": 1, "signature_id": 42, "rev": 0, "signature": "Google DNS server contacted", "category": "", "severity": 3, "rule": "alert dns $HOME_NET any -> 8.8.8.8 any (msg:\"Google DNS server contacted\"; sid:43;)" }, "app_proto": "dns", "flow": { "pkts_toserver": 1, "pkts_toclient": 0, "bytes_toserver": 81, "bytes_toclient": 0, "start": "2017-08-14T12:35:05.830812+0200" } } Feature OISF#2020
@@ -1071,7 +1071,10 @@ int SigParse(DetectEngineCtx *de_ctx, Signature *s, const char *sigstr, uint8_t | |||
SignatureParser parser; | |||
memset(&parser, 0x00, sizeof(parser)); | |||
|
|||
s->sig_str = sigstr; | |||
s->sig_str = SCStrdup(sigstr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will leak if SigParseBasics fails to parse the rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. The only caller of SigParse() calls SigFree() in case the function returned an error. See https://github.com/rs-natano/suricata/blob/9af2e9a50cfdb9d75d206614a8e3b03cb8486590/src/detect-parse.c#L1644 . SigFree() also frees sig_str when not NULL. Note that all other dynamically allocated members of the signature struct are handled in the same way (class_msg, sp, dp, ...); otherwise the cleanup code would be much more complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does. Took too much of a micro-look at the patch.
@@ -1100,8 +1103,6 @@ int SigParse(DetectEngineCtx *de_ctx, Signature *s, const char *sigstr, uint8_t | |||
} while (ret == 1); | |||
} | |||
|
|||
s->sig_str = NULL; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So maybe dup the string based on the value of r.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment. Looks good otherwise. Can you resubmit an updated branch?
/* signature text */ | ||
if (json_output_ctx->flags & LOG_JSON_RULE) { | ||
hjs = json_object_get(js, "alert"); | ||
BUG_ON(!json_is_object(hjs)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will need a proper check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's wrong about the check? When the "alert" key is not there it's a bug in the code before, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see now, the "alert" key might not be there in out of memory conditions. I will create an updated PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah we generally try to allow for alloc issues. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated PR: #2897
as they are in the protocol cf bug OISF#2881
as they are in the protocol cf bug OISF#2881
as they are in the protocol cf bug OISF#2881
Make sure these boxes are signed before submitting your Pull Request -- thank you.
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/2020
Describe changes:
For SIEM analysis it is often useful to refer to the actual rules to find out why a specific alert has been triggered when the signature message does not convey enough information.
Turn on the new rule flag to include the rule text in eve alert output. The feature is turned off by default.
With a rule like this:
The eve alert output might look something like this (pretty-printed for readability):