Falco: Validation Error when using a macro as definition of an overwritten macro.

[natalysheinin]

What happened / Replication Steps:

Given a macro that is defined in the default ruleset and for the purposes of being concrete let's use Clear Log Activities:

- macro: allowed_clear_log_files
  condition: (never_true)

1. Now, if you overwrite this macro in falco_rules.local.yaml, for example:

- macro: allowed_clear_log_files
  condition: >
    (container.name = "I am testing something")

When I run my validation script:

falco -c /etc/falco/falco.yml.erb               \
--validate /etc/falco/falco_rules.yml.erb        \                           
--validate /etc/falco/falco_macros.local.yml.erb  \                

Everything succeeds 🎉

1. However, if I overwrite this macro in falco_rules.local.yaml using another macro, for example:

- macro: allowed_clear_log_files
  condition: >
    consider_all_chmods

I get the following error message:
Runtime error: Error loading rules: /usr/share/falco/lua/compiler.lua:65: Undefined macro 'consider_all_chmods' used in filter.. Exiting.

(It is important to note that consider_all_chmods is in defined in the default ruleset, so I am not sure why it is failing here.

Anything else we need to know?:

Environment:

• Falco version (use falco --version): falco 0.14.0
• System info
• Cloud provider or hardware configuration:
• OS (e.g: cat /etc/os-release):
• Kernel (e.g. uname -a):
• Install tools (e.g. in kubernetes, rpm, deb, from source):
• Others:

[fntlnz]

Me and @leodido debugged this for a while this morning.

We've been able to reproduce this thanks for reporting this @natalysheinin !

The reason why this does not work as expected is because in the default falco_rules.yaml the consider_all_chmods is defined after the macro that uses it allowed_clear_log_files.

We crafted a minimum reproducible example to test it:

falco_rules.yaml

- macro: never_true
  condition: (evt.num=0)
- macro: open_write
  condition: (evt.type=open or evt.type=openat) and evt.is_open_write=true and fd.typechar='f' and fd.num>=0

- macro: allowed_clear_log_files
  condition: (never_true)

- macro: consider_all_chmods
  condition: (never_true)

- rule: Clear Log Activities
  desc: Detect clearing of critical log files
  condition: >
    open_write and
    not allowed_clear_log_files
  output: >
    Log files were tampered (user=%user.name command=%proc.cmdline file=%fd.name container_id=%container.id image=%container.image.repository)
  priority:
    WARNING
  tags: [file, mitre_defense_evasion]

falco_rules.local.yaml

- macro: allowed_clear_log_files
  condition: (consider_all_chmods)

That set of rules with that order will output the same error you are reporting:

./build/userspace/falco/falco -c falco.yaml --validate rules/falco_rules.yaml --validate rules/falco_rules.local.yaml
Fri Jul  5 12:22:39 2019: Validating rules file(s):
Fri Jul  5 12:22:39 2019:    rules/falco_rules.yaml
Fri Jul  5 12:22:39 2019:    rules/falco_rules.local.yaml
Fri Jul  5 12:22:39 2019: Runtime error: Error loading rules: ...ts/falcosecurity/falco/userspace/engine/lua/compiler.lua:74: attempt to index a nil value. Exiting.

After that, if we swap the order of allowed_clear_log_files and consider_all_chmods in falco_rules.yaml we have it working just fine.

That's what we observed with this reproducible example but it's actually the same if you just swap the order in the upstream falco_rules.yaml.

Here's the falco_rules.yaml with the macros swapped

- macro: never_true
  condition: (evt.num=0)
- macro: open_write
  condition: (evt.type=open or evt.type=openat) and evt.is_open_write=true and fd.typechar='f' and fd.num>=0

- macro: consider_all_chmods
  condition: (never_true)

- macro: allowed_clear_log_files
  condition: (never_true)

- rule: Clear Log Activities
  desc: Detect clearing of critical log files
  condition: >
    open_write and
    not allowed_clear_log_files
  output: >
    Log files were tampered (user=%user.name command=%proc.cmdline file=%fd.name container_id=%container.id image=%container.image.repository)
  priority:
    WARNING
  tags: [file, mitre_defense_evasion]

Here's the result:

./build/userspace/falco/falco -c falco.yaml --validate rules/falco_rules.yaml --validate rules/falco_rules.local.yaml
Fri Jul  5 12:25:22 2019: Validating rules file(s):
Fri Jul  5 12:25:22 2019:    rules/falco_rules.yaml
Fri Jul  5 12:25:22 2019:    rules/falco_rules.local.yaml
Fri Jul  5 12:25:22 2019: Ok

[fntlnz]

/assign @leodido
/assign @fntlnz

[fntlnz]

/area rules

[leodido]

While swapping the macro definitions in the rules file works, it cannot be considered a fix for this bug, for 2 reasons:

1. changing this in the upstream rules will fix this issue only for half of the users because the other half of the users will still have the reverse problem
2. we cannot rely on ordering based of the original position of the overridden macro definition.

So we think that, in order to truly fix this bug, the way the rule_loader.lua processes the macro definitions needs to be changed to be agnostic with respect to the position of the overridden macro definition.

[natalysheinin]

This is great! I'm glad you were able to easily replicate this scenario. So now I have another (similar) scenario.

Take for instance the exact same default rule file as you mentioned:

_falco_rules.yaml_

- macro: never_true
  condition: (evt.num=0)
- macro: open_write
  condition: (evt.type=open or evt.type=openat) and evt.is_open_write=true and fd.typechar='f' and fd.num>=0

- macro: allowed_clear_log_files
  condition: (never_true)

- macro: consider_all_chmods
  condition: (never_true)

- rule: Clear Log Activities
  desc: Detect clearing of critical log files
  condition: >
    open_write and
    not allowed_clear_log_files
  output: >
    Log files were tampered (user=%user.name command=%proc.cmdline file=%fd.name container_id=%container.id image=%container.image.repository)
  priority:
    WARNING
  tags: [file, mitre_defense_evasion]

However, now allowed_clear_log_file references a macro that is only defined in the custom rules file:

_falco_rules.local.yaml_

- macro: test_macro
  condition: >
    (container.name = "I am testing something")

- macro: allowed_clear_log_files
  condition: >
    (test_macro)

When I run my validation, once again, a configuration error (similar to above):

falco -c /etc/falco/falco.yml.erb               \
--validate /etc/falco/falco_rules.yml.erb        \                           
--validate /etc/falco/falco_macros.local.yml.erb  \    
Runtime error: Error loading rules: /usr/share/falco/lua/compiler.lua:65: Undefined macro 'test_macro' used in filter.. Exiting.            

[leodido]

Yes @natalysheinin, unfortunately this is due to the fact that test_macro macro does not exist in the default rules file (ie., falco_rules.yaml).

And this is very correlated to the original problem you reported. Also this is happening because all the processing (particularly the creation of the macros tree) that rule_loader.lua does are based solely on the default rules file.

[natalysheinin]

@leodido Thanks for clarifying that, it is extremely useful to know when the macro tree processing occurs.

Do you have any recommendations for how to approach solving this from the user-end for now?
And, do you believe that your earlier comment will address solving this?

the way the rule_loader.lua processes the macro definitions needs to be changed to be agnostic with respect to the position of the overridden macro definition.

[leodido]

Do you have any recommendations for how to approach solving this from the user-end for now?

The workaround I'd use right now would be putting everything in one file (making sure that test_macro is above allowed_clear_log_files macro).

Clearly the edits to the creation and the processing of the macros tree needs to fix also this case.

[fntlnz]

For the sake of completeness, this is the generated AST for the last rules that @natalysheinin sent in this comment

click me to see the ast dump

{
  allowed_clear_log_files = {
    ast = {
      type = "Macro",
      value = "never_true"
    },
    used = false
  },
  consider_all_chmods = {
    ast = {
      type = "Macro",
      value = "never_true"
    },
    used = false
  },
  never_true = {
    ast = {
      left = {
        type = "FieldName",
        value = "evt.num"
      },
      operator = "=",
      right = {
        type = "Number",
        value = 0
      },
      type = "BinaryRelOp"
    },
    used = true
  },
  open_write = {
    ast = {
      left = {
        left = {
          left = {
            left = {
              left = {
                type = "FieldName",
                value = "evt.type"
              },
              operator = "=",
              right = {
                type = "BareString",
                value = "open"
              },
              type = "BinaryRelOp"
            },
            operator = "or",
            right = {
              left = {
                type = "FieldName",
                value = "evt.type"
              },
              operator = "=",
              right = {
                type = "BareString",
                value = "openat"
              },
              type = "BinaryRelOp"
            },
            type = "BinaryBoolOp"
          },
          operator = "and",
          right = {
            left = {
              type = "FieldName",
              value = "evt.is_open_write"
            },
            operator = "=",
            right = {
              type = "BareString",
              value = "true"
            },
            type = "BinaryRelOp"
          },
          type = "BinaryBoolOp"
        },
        operator = "and",
        right = {
          left = {
            type = "FieldName",
            value = "fd.typechar"
          },
          operator = "=",
          right = {
            type = "String",
            value = "f"
          },
          type = "BinaryRelOp"
        },
        type = "BinaryBoolOp"
      },
      operator = "and",
      right = {
        left = {
          type = "FieldName",
          value = "fd.num"
        },
        operator = ">=",
        right = {
          type = "Number",
          value = 0
        },
        type = "BinaryRelOp"
      },
      type = "BinaryBoolOp"
    },
    used = false
  }
}

The test_macro is clearly missing, proving that it was not considered.

[mstemm]

Here's some background about how the objects are maintained and how it can cause this bug. When the objects are read they're maintained in an ordered array. The ordered array is necessary to assure that appends are applied in the right order.

I think the problem is that the macro allowed_clear_log_files replaces the instance of allowed_clear_log_files in that array (in this case, third). Since it's third, the macro reference test_macro doesn't exist yet.

I think the fix is to be more liberal about using macros defined later in the file when expanding macro references. We'll have to make sure that semantics about ordering still apply wrt appends and overrides.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[mstemm]

We still want to fix this.

[mfdii]

@kris-nova is going to start looking at this. It would be good to discuss on the upcoming repo planning call to get an idea of how long it might take to fix.

[fntlnz]

/assign @leodido
/assign @fntlnz

[leodido]

/priority high

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[leodido]

/milestone 1.0.0

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[leodido]

/open

On Wed, 26 Feb 2020 at 20:29, stale[bot] [e-mail] wrote:

Closed #706 https://github.com/falcosecurity/falco/issues/706.

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/falcosecurity/falco/issues/706?email_source=notifications&email_token=AAA5J44HY2LHSQ7L7JYOBQ3RE27H3A5CNFSM4H6AIRSKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOW5FHDEY#event-3075109267,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAA5J47FEWZOPMBAODW53ADRE27H3ANCNFSM4H6AIRSA
.

>

L.

[ykcilborw]

has there been any update on this?

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[JPLachance]

Hello!

I still experience this issue.

In my falco_rules.local.yaml file, if I define my own macro then try to use it somewhere in the falco_rules.local.yaml file, it does not work. This is a little bit annoying because without that feature, I have to repeat the exact same condition over and over again. Being able to define my own macros would allow me to create cleaner rules. See #504 for an example.

Can someone add a tag that will prevent that bot from closing relevant issues? :D

Thank you :)

[leogr]

I believe there're several issues related to the validation of rule files that we still want to fix (for example #1332 ), including this one.
Thus, I've re-opened this and added the roadmap label (so the stale bot should not close it again).

[poiana]

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

[poiana]

Stale issues rot after 30d of inactivity.

Mark the issue as fresh with /remove-lifecycle rotten.

Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle rotten

[poiana]

Rotten issues close after 30d of inactivity.

Reopen the issue with /reopen.

Mark the issue as fresh with /remove-lifecycle rotten.

Provide feedback via https://github.com/falcosecurity/community.
/close

[poiana]

@poiana: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue with /reopen.

Mark the issue as fresh with /remove-lifecycle rotten.

Provide feedback via https://github.com/falcosecurity/community.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[leodido]

/reopen
L.

On Thu, Jan 28, 2021 at 9:57 AM poiana [e-mail] wrote:

@poiana https://github.com/poiana: Closing this issue.

In response to this
https://github.com/falcosecurity/falco/issues/706#issuecomment-768902710
:

Rotten issues close after 30d of inactivity.

Reopen the issue with /reopen.

Mark the issue as fresh with /remove-lifecycle rotten.

Provide feedback via https://github.com/falcosecurity/community.
/close

Instructions for interacting with me using PR comments are available here
https://git.k8s.io/community/contributors/guide/pull-requests.md. If
you have questions or suggestions related to my behavior, please file an
issue against the kubernetes/test-infra
https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:
repository.

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/falcosecurity/falco/issues/706#issuecomment-768902749,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAA5J43FLDQYD6KRWWX5PC3S4ERHRANCNFSM4H6AIRSA
.

[poiana]

@leodido: Reopened this issue.

In response to this:

/reopen
L.

On Thu, Jan 28, 2021 at 9:57 AM poiana [e-mail] wrote:

@poiana https://github.com/poiana: Closing this issue.

In response to this
https://github.com/falcosecurity/falco/issues/706#issuecomment-768902710
:

Rotten issues close after 30d of inactivity.

Reopen the issue with /reopen.

Mark the issue as fresh with /remove-lifecycle rotten.

Provide feedback via https://github.com/falcosecurity/community.
/close

Instructions for interacting with me using PR comments are available here
https://git.k8s.io/community/contributors/guide/pull-requests.md. If
you have questions or suggestions related to my behavior, please file an
issue against the kubernetes/test-infra
https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:
repository.

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/falcosecurity/falco/issues/706#issuecomment-768902749,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAA5J43FLDQYD6KRWWX5PC3S4ERHRANCNFSM4H6AIRSA
.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[poiana]

Rotten issues close after 30d of inactivity.

Reopen the issue with /reopen.

Mark the issue as fresh with /remove-lifecycle rotten.

Provide feedback via https://github.com/falcosecurity/community.
/close

[poiana]

@poiana: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue with /reopen.

Mark the issue as fresh with /remove-lifecycle rotten.

Provide feedback via https://github.com/falcosecurity/community.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[leogr]

/reopen
/remove-lifecycle rotten

/cc @leodido

[poiana]

@leogr: Reopened this issue.

In response to this:

/reopen
/remove-lifecycle rotten

/cc @leodido

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.