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

correct rule conditions to support syscall variants #1250

Merged
merged 3 commits into from
Jun 10, 2020

Conversation

leogr
Copy link
Member

@leogr leogr commented Jun 4, 2020

What type of PR is this?

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

/kind bug

/kind rule-update

Any specific area of the project related to this PR?

/area rules

What this PR does / why we need it:

This PR fixes some rule/macro conditions to:

  • Make Mkdir binary dirs also work with mkdirat syscall
  • Make Modify binary dirs also work with rename, renameat, and unlinkat syscalls
  • Make Create files below dev also work with openat syscall

Additional context can be found in commits' messages.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

This is my first PR that fix the rules :)

Test cases

  • Mkdir binary dirs with mkdirat
docker run --rm -it falcosecurity/event-generator:0.3.0 run syscall.MkdirBinaryDirs
  • Modify binary dirs with rename
sudo touch /bin/test-falco-rules
sudo rename test-falco-rules test-falco-rules-renamed /bin/test-falco-rules
  • Modify binary dirs with renameat
docker run --rm -it falcosecurity/event-generator:0.3.0 run syscall.ModifyBinaryDirs
  • Modify binary dirs with unlinkat
sudo touch /bin/test-falco-rules
sudo rm /bin/test-falco-rules
  • Create files below dev with openat
docker run --rm -it falcosecurity/event-generator:0.3.0 run syscall.CreateFilesBelowDev

Additional notes
There're still some missing syscalls that should be added but are not currently supported by sinsp, for example:

I will fill a separate issue for those Reported on #676

Related issues

Does this PR introduce a user-facing change?:

rule(Mkdir binary dirs): correct condition in macro `bin_dir_mkdir` to catch `mkdirat` syscall
rule(Modify binary dirs): correct condition in macro `bin_dir_rename` to catch `rename`, `renameat`, and `unlinkat` syscalls
rule(Create files below dev): correct condition to catch `openat` syscall

leogr added 2 commits June 4, 2020 14:44
Since the dir's path is found:
-  in `evt.arg[1]` for `mkdir`
-  but in `evt.arg[2]` for `mkdirat`
switch to `evt.arg.path` to catch both.
That ensures `Mkdir binary dirs` works properly.

Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Since `evt.arg[1]` does not work for all syscalls, switch to:
 - `evt.arg.path` for `rmdir` and `unlink` (used by `remove` macro)
 - `evt.arg.name` for `unlinkat` (used by `remove` macro)
 - `evt.arg.oldpath/newpath` for `rename` and `renameat` (used by `rename` macro)

That ensures `Modify binary dirs` works properly.

Note that we cannot yet use `renameat2` (not supported by sinsp, see draios/sysdig#1603 )

Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
@leogr
Copy link
Member Author

leogr commented Jun 4, 2020

/cc @Kaizhe

@poiana poiana requested a review from Kaizhe June 4, 2020 14:01
@leogr leogr changed the title wip: correct rule conditions to support syscall variants correct rule conditions to support syscall variants Jun 5, 2020
@leogr
Copy link
Member Author

leogr commented Jun 5, 2020

/milestone 0.24.0

@poiana poiana added this to the 0.24.0 milestone Jun 5, 2020
@leogr leogr mentioned this pull request Jun 5, 2020
5 tasks
Copy link
Member

@leodido leodido left a comment

Choose a reason for hiding this comment

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

SGTM

@poiana
Copy link

poiana commented Jun 8, 2020

LGTM label has been added.

Git tree hash: b1c76d6eb22d7050be66f1e5fc8480ec88505098

@poiana poiana added the approved label Jun 8, 2020
Copy link
Contributor

@mstemm mstemm left a comment

Choose a reason for hiding this comment

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

Great improvement!

@poiana
Copy link

poiana commented Jun 9, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fntlnz, leodido, mstemm

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 578ef7f into master Jun 10, 2020
@poiana poiana deleted the fix/rule-evt-conditions branch June 10, 2020 10:22
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.

None yet

5 participants