-
Notifications
You must be signed in to change notification settings - Fork 70
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
cleanup: remove evt.arg.*
fields when always return <NA>
#215
Conversation
Rules files suggestionsfalco_rules.yamlComparing Minor changes:
Patch changes:
falco-sandbox_rules.yamlComparing Patch changes:
falco-incubating_rules.yamlComparing Minor changes:
Patch changes:
|
Signed-off-by: Andrea Terzolo <[email protected]>
b68614d
to
9df6092
Compare
@@ -87,7 +87,7 @@ | |||
and ssh_port | |||
and not allowed_ssh_hosts | |||
enabled: false | |||
output: Disallowed SSH Connection (connection=%fd.name lport=%fd.lport rport=%fd.rport fd_type=%fd.type fd_proto=fd.l4proto evt_type=%evt.type user=%user.name user_uid=%user.uid user_loginuid=%user.loginuid process=%proc.name proc_exepath=%proc.exepath parent=%proc.pname command=%proc.cmdline terminal=%proc.tty exe_flags=%evt.arg.flags %container.info) | |||
output: Disallowed SSH Connection (connection=%fd.name lport=%fd.lport rport=%fd.rport fd_type=%fd.type fd_proto=fd.l4proto evt_type=%evt.type user=%user.name user_uid=%user.uid user_loginuid=%user.loginuid process=%proc.name proc_exepath=%proc.exepath parent=%proc.pname command=%proc.cmdline terminal=%proc.tty %container.info) |
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.
events like accept,accept4,listen,connect
don't have a flag
param so exe_flags=%evt.arg.flags
will be always evaluated to <NA>
. Same thing in many of the other cases in this PR
@@ -1040,6 +1044,11 @@ | |||
- macro: user_known_set_setuid_or_setgid_bit_conditions | |||
condition: (never_true) | |||
|
|||
# todo!: the usage of `evt.arg*` filter check in the output should be avoided |
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 #214
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.
Just picking this comment as random comment to provide feedback re the open item #176 (comment), can we also consistently rename it to flags=
.
It's a real real bummer Falco cannot handle this gracefully, a bit disappointed and surprised. Given we won't schedule the fixes for Falco 0.37 and we probably need more discussions, yes we need to remove them in such cases.
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.
uhm here I would prefer to remove the flags=%...
output field unless it is not really necessary in the rule. I would reduce the output fields in our rules as much as possible, so if one field is not strictly necessary for the scope of the rule, I prefer to remove it. I have no strong opinions here, this was just my thought
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.
BTW the comment here is related to another possible issue #214, that is not related to the exe_flags=%evt.arg.flags
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.
ACK, yes I have the website PR already up to make this clear in the Style Guide.
I'll open a follow up PR to rename exe_flags to flags (unrelated to this PR) https://github.com/falcosecurity/rules/pull/215/files#r1443163428
ACK re https://github.com/falcosecurity/rules/pull/215/files#r1444885132
rules/falco_rules.yaml
Outdated
and (evt.arg.oldpath in (sensitive_file_names)) | ||
output: Hardlinks created over sensitive files (target=%evt.arg.target linkpath=%evt.arg.linkpath evt_type=%evt.type user=%user.name user_uid=%user.uid user_loginuid=%user.loginuid process=%proc.name proc_exepath=%proc.exepath parent=%proc.pname command=%proc.cmdline terminal=%proc.tty exe_flags=%evt.arg.flags %container.info) | ||
and (fs.path.source in (sensitive_file_names)) | ||
output: Hardlinks created over sensitive files (target=%fs.path.target linkpath=%fs.path.source evt_type=%evt.type user=%user.name user_uid=%user.uid user_loginuid=%user.loginuid process=%proc.name proc_exepath=%proc.exepath parent=%proc.pname command=%proc.cmdline terminal=%proc.tty %container.info) |
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.
evt.arg.target
and evt.arg.linkpath
don't exist for link
and linkat
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.
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.
Actually this was a refactor mistake (by me since I had to do it all), I would instead suggest to revert to the old correct outputs: target=%evt.arg.oldpath linkpath=%evt.arg.newpath
, see in the git history https://github.com/falcosecurity/rules/blob/dc90ca5e179fcf11e6a9b8486e4f23dacb613087/rules/falco_rules.yaml#L2808C126-L2808C175
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.
Because using the new fs.path*
fields would break things and it would be inconsistent with the symlink 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.
done, thank you for the feedback!
@@ -998,11 +998,11 @@ | |||
and privilege escalation (CVE-2020-14386) by an attacker. Noise can be reduced by using the user_known_packet_socket_binaries | |||
template list. | |||
condition: > | |||
evt.type=socket | |||
evt.type=socket and evt.dir=> |
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.
evt.arg.domain
is only defined for the enter event
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.
great catch
and container | ||
and evt.arg[0] contains AF_PACKET | ||
and evt.arg.domain contains AF_PACKET |
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.
just for clarity
Rules files suggestionsfalco_rules.yamlComparing Minor changes:
Patch changes:
falco-sandbox_rules.yamlComparing Patch changes:
falco-incubating_rules.yamlComparing Minor changes:
Patch changes:
|
this commit restores `target=%evt.arg.oldpath linkpath=%evt.arg.newpath` fields for the `Create Hardlink Over Sensitive Files` rule. Signed-off-by: Andrea Terzolo <[email protected]> Co-authored-by: Melissa Kilby <[email protected]>
Rules files suggestionsfalco_rules.yamlComparing Minor changes:
Patch changes:
falco-sandbox_rules.yamlComparing Patch changes:
falco-incubating_rules.yamlComparing Minor changes:
Patch changes:
|
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.
/approve
LGTM label has been added. Git tree hash: 3a4203d579fd86e146560c316b9d1afa5993e130
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, incertum The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
Any specific area of the project related to this PR?
/area rules
What this PR does / why we need it:
This PR removes all
evt.arg.*
fields that will be always resolved as<NA>
.Related to this: #214
Which issue(s) this PR fixes:
Special notes for your reviewer: