fix(rules/falco_rules): solve shadowing issues with "Drop and execute new binary in container"

jasondellaluce commented 1 year ago

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area rules

What this PR does / why we need it:

The rule Drop and execute new binary in container has been introduced in #20 (part of the falco-rules-1.0.0 release) and has a condition that is very similar to other rules, but that however is more narrow. Being defined before the rules below, the probability of "shadowing" them is quite high (a.k.a. case in Falco only reports the first rule matching, thus not triggering rules defined after in the YAML order):

Terminal shell in container
Launch Package Management Process in Container
Netcat Remote Code Execution in Container
Launch Suspicious Network Tool in Container
Launch Remote File Copy Tools in Container
The docker client is executed in a container
Container Run as Root User
Debugfs Launched in Privileged Container
Mount Launched in Privileged Container
Launch Ingress Remote File Copy Tools in Container

My proposal is to move Drop and execute new binary in container down in the declaration order. The rule has lots of value, but can't shadow other important rules.

cc @incertum @loresuso

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

On the long term, we could consider making Falco trigger all rules instead of the first one, but that would be a new feature for a future Falco release, and not a patch for the existing rulesets.

github-actions[bot] commented 1 year ago

rules/falco_rules.yaml

Comparing deff35ed9c8b33d719bda3969dc4f395b86ffd25 with latest tag falco-rules-1.0.0

No changes detected

jasondellaluce commented 1 year ago

rules/falco_rules.yaml

Comparing deff35ed9c8b33d719bda3969dc4f395b86ffd25 with latest tag falco-rules-1.0.0

No changes detected

For the future, rule/macro/list reordering is something we should catch in automatic change checks. If we can't predict the major/minor/nature of the reordering, my opinion would be to add an "other/extra changes" section for things like this.

cc @loresuso

incertum commented 1 year ago

@jasondellaluce ty! I think the "shadowing" is something we should discuss more in detail in the new rules maturity framework as we build it out. Perhaps would you have ideas on how to expose something easy to use for end users to dictate the ordering? For example, I have a custom Go patching script that re-orders based on a predefined priority list which includes custom rules as well.

But this is beyond this PR, LGTM!

On the long term, we could consider making Falco trigger all rules instead of the first one, but that would be a new feature for a future Falco release, and not a patch for the existing rulesets.

Agreed.

incertum commented 1 year ago

One more thought, should we add more output fields to the other rules that could be a new executable?

evt.arg.flags
proc.exe_ino.ctime_duration_proc_start

jasondellaluce commented 1 year ago

That sounds like a good idea to me. @loresuso word to you on this.

loresuso commented 1 year ago

I agree that in the long term, Falco should trigger each rule in which the condition is satisfied. I think we should start prioritizing this, since the shadowing problem should be solved from its root cause.

I am ok with moving the rule down in the file and as Melissa was saying, I think we can print out the evt.arg.flags in all the other rules so that we can understand if exe_upper_layer was also set from the output.

For the future, rule/macro/list reordering is something we should catch in automatic change checks. If we can't predict the major/minor/nature of the reordering, my opinion would be to add an "other/extra changes" section for things like this.

agreed!

github-actions[bot] commented 1 year ago

rules/falco_rules.yaml

Comparing f01320f2e27328efe38c79f0f66b04e67b3947f1 with latest tag falco-rules-1.0.0

Patch changes:

Rule DB program spawned process changed its output fields
Rule Run shell untrusted changed its output fields
Rule System user interactive changed its output fields
Rule Terminal shell in container changed its output fields
Rule Program run with disallowed http proxy env changed its output fields
Rule User mgmt binaries changed its output fields
Rule Launch Package Management Process in Container changed its output fields
Rule Netcat Remote Code Execution in Container changed its output fields
Rule Launch Suspicious Network Tool in Container changed its output fields
Rule Launch Suspicious Network Tool on Host changed its output fields
Rule Search Private Keys or Passwords changed its output fields
Rule Remove Bulk Data from Disk changed its output fields
Rule Delete Bash History changed its output fields
Rule Launch Remote File Copy Tools in Container changed its output fields
Rule Detect crypto miners using the Stratum protocol changed its output fields
Rule Container Run as Root User changed its output fields
Rule Sudo Potential Privilege Escalation changed its output fields
Rule Debugfs Launched in Privileged Container changed its output fields
Rule Mount Launched in Privileged Container changed its output fields
Rule Launch Ingress Remote File Copy Tools in Container changed its output fields
Rule Polkit Local Privilege Escalation Vulnerability (CVE-2021-4034) changed its output fields
Rule Find AWS Credentials changed its output fields
Rule Execution from /dev/shm changed its output fields

jasondellaluce commented 1 year ago

Is everyone onboard with the current change? I would like to issue a v1.0.1 patch release of the Falco rules including this.

incertum commented 1 year ago

Yes happy to approve once I can ...

poiana commented 1 year ago

LGTM label has been added.

Git tree hash: 697a70883fe14556c02690f78f5a3e4b939df927

poiana commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: incertum, jasondellaluce

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: - ~~[OWNERS](https://github.com/falcosecurity/rules/blob/main/OWNERS)~~ [incertum,jasondellaluce] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

pmusa commented 1 year ago

Hey folks, thank you for the PR. How can I know when is this going to be GA? This will break the labs for the Falco 101 workshop, and we have 10+ deliveries in the next 2 weeks.

falcosecurity / rules