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

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

Merged
merged 2 commits into from
Jun 19, 2023

Conversation

jasondellaluce
Copy link
Contributor

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.

… new binary in container`

Rule was contributed by Lorenzo Susini in #20.

Co-authored-by: Lorenzo Susini <susinilorenzo1@gmail.com>

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
@github-actions
Copy link

rules/falco_rules.yaml

Comparing deff35ed9c8b33d719bda3969dc4f395b86ffd25 with latest tag falco-rules-1.0.0

No changes detected

@jasondellaluce
Copy link
Contributor Author

jasondellaluce commented Jun 12, 2023

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
Copy link
Contributor

incertum commented Jun 12, 2023

@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
Copy link
Contributor

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
Copy link
Contributor Author

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

@loresuso
Copy link
Member

loresuso commented Jun 13, 2023

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!

…rocess

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
@poiana poiana added size/L and removed size/M labels Jun 13, 2023
@github-actions
Copy link

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
Copy link
Contributor Author

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
Copy link
Contributor

Yes happy to approve once I can ...

Copy link
Contributor

@incertum incertum left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link

poiana commented Jun 19, 2023

LGTM label has been added.

Git tree hash: 697a70883fe14556c02690f78f5a3e4b939df927

@poiana
Copy link

poiana commented Jun 19, 2023

[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 [incertum,jasondellaluce]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana merged commit f5ef8d9 into main Jun 19, 2023
@poiana poiana deleted the jasondellaluce-patch-1 branch June 19, 2023 16:48
@pmusa
Copy link

pmusa commented Jun 19, 2023

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.

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

Successfully merging this pull request may close these issues.

5 participants