Skip to content

Conversation

ytsssun
Copy link
Contributor

@ytsssun ytsssun commented Jan 22, 2025

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind feature

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area rules

/area registry

/area build

/area documentation

Proposed rule maturity level

Uncomment one (or more) /area <> lines (only for PRs that add or modify rules):

/area maturity-stable

/area maturity-incubating

/area maturity-sandbox

/area maturity-deprecated

What this PR does / why we need it:
This PR is to extend the known_memfd_execution_binaries with runc since in 1.1.15 of runc, it introduced a behavior which would use the memfd approach to execute runc binary. This PR is to include the rule for it so that we don't falsely flag the runc binary.

Which issue(s) this PR fixes:

Fixes #3444

Special notes for your reviewer:
I can use come help confirming the change. I was able to verify the override works, see the proof.

But I would need help on verifying that via the first party rules for falco.

@poiana poiana added kind/bug Something isn't working dco-signoff: no area/rules area/maturity-sandbox See the Rules Maturity Framework labels Jan 22, 2025
@poiana poiana requested review from Kaizhe and loresuso January 22, 2025 22:10
@poiana
Copy link

poiana commented Jan 22, 2025

Welcome @ytsssun! It looks like this is your first PR to falcosecurity/rules 🎉

Copy link

Rules files suggestions

falco_rules.yaml

Comparing 1525e9551ca017da0d0271e816fe591a493e9c7b with latest tag falco-rules-3.2.0

Patch changes:

  • List known_memfd_execution_binaries has some item added or removed

@leogr
Copy link
Member

leogr commented Jan 29, 2025

/area maturity-stable

@poiana poiana added the area/maturity-stable See the Rules Maturity Framework label Jan 29, 2025
@leogr leogr removed the area/maturity-sandbox See the Rules Maturity Framework label Jan 29, 2025
@leogr leogr added this to the falco-0.41-rules milestone Jan 29, 2025
- macro: known_memfd_execution_processes
condition: (proc.name in (known_memfd_execution_binaries))
condition: >
(proc.name in (known_memfd_execution_binaries))
Copy link
Member

Choose a reason for hiding this comment

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

Hi there! thanks for opening the PR.
I am not sure though this will work since:

  • when we execute from memfd, usually we use the proc filesystem reference to the fd to do so. The process name will be a numeric value such as "3"
  • IIRC, Falco doesn't resolve exepath and exe that way
    Possibly, the best option would be to tune the rule by using ancestors information, such as proc.pname and proc.aname

@ytsssun
Copy link
Contributor Author

ytsssun commented Feb 7, 2025

Hi @loresuso , if you see my original repro and fix, this worked for me.

customRules:
  rules-allow-runc-memfd.yaml: |-
    - list: known_memfd_execution_binaries
      items: [runc]
      override:
        items: append

There indeed was a case where the proc.name is numerical value. And in that case this worked for them.

customRules:
  rules-allow-runc-memfd.yaml: |-
    # Add runc to known binaries
    - list: known_memfd_execution_binaries
      items: [runc]
      override:
        items: append

    # Add specific path conditions to existing macro
    - macro: known_memfd_execution_processes
      condition: or (proc.exepath = "memfd:runc_cloned:/proc/self/exe") or (proc.exe = "memfd:runc_cloned:/proc/self/exe")
      override:
        condition: append

The implementation covers both cases. Do you think I still need to update it to include "proc.pname" and "proc.aname"?

@leogr leogr requested a review from loresuso February 13, 2025 09:08
@loresuso
Copy link
Member

loresuso commented Feb 13, 2025

Okay, I took some time to verify how Falco now resolves the memfd binary path and you are correct! When we added the full resolution for exepath, we also got the full resolution of the memfd binaries, implicitly.

I would still introduce the exception considering the lineage of the process, so adding proc.pname or proc.aname to match this exact use case accordingly. This will make the rule better, since in theory nobody is preventing an attacker to name his own memfd "runc_cloned", bypassing the rule. If the information about the ancestors is there, the rule turns out to be stronger.

I'll also write this in case we need to remember it, it looks like runc recently dropped the memfd execution in more modern systems in favor of an overlayfs sealed execution for performance reasons opencontainers/runc#4448

@ytsssun ytsssun force-pushed the update-rule-for-runc-memfd-exec branch from ebccfef to 10a0931 Compare February 16, 2025 01:52
@ytsssun
Copy link
Contributor Author

ytsssun commented Feb 16, 2025

I would still introduce the exception considering the lineage of the process, so adding proc.pname or proc.aname to match this exact use case accordingly. This will make the rule better, since in theory nobody is preventing an attacker to name his own memfd "runc_cloned", bypassing the rule. If the information about the ancestors is there, the rule turns out to be stronger.

I appended the condition to also check for proc.pname in the match so that it is more comprehensive.

I'll also write this in case we need to remember it, it looks like runc recently dropped the memfd execution in more modern systems in favor of an overlayfs sealed execution for performance reasons opencontainers/runc#4448

Looking at the PR commit, it looks like this is indeed the optimization they included later on. This change is only included in the >1.2.0 versions and is not backported to the 1.1.x versions. For now the affected version would be 1.1.15 and any future 1.1.x versions.

Copy link
Member

@loresuso loresuso left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@poiana
Copy link

poiana commented Feb 17, 2025

LGTM label has been added.

Git tree hash: 1317e51be90ef00ebb0d2b63db6476bebeda3e86

@loresuso
Copy link
Member

/approve

@poiana
Copy link

poiana commented Feb 17, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leogr, loresuso, ytsssun

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana merged commit 4633f29 into falcosecurity:main Feb 17, 2025
2 checks passed
@jcchavezs
Copy link

@leogr can we get this in a release even if beta?

@leogr
Copy link
Member

leogr commented May 14, 2025

@leogr can we get this in a release even if beta?

This will be shipped with Falco 0.41 (due by May 26th).

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

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants