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

Rule: PTRACE attached to process #2226

Merged
merged 3 commits into from
Nov 30, 2022
Merged

Conversation

Brucedh
Copy link
Contributor

@Brucedh Brucedh commented Sep 28, 2022

Signed-off-by: Alessandro Brucato alessandro.brucato@sysdig.com

What type of PR is this?

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

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind release

If contributing rules or changes to rules, please make sure to also uncomment one of the following line:

/kind rule-update

/kind rule-create

Any specific area of the project related to this PR?

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

/area build

/area engine

/area rules

/area tests

/area proposals

/area CI

What this PR does / why we need it:
The ptrace system call can be used to alter a target's memory and execution which make it popular for injecting code. It is often used by malware for this purpose in order to be more stealthy, or steal information from the process. Legitimate tools, such as GDB or security software, may also leverage this method.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(rules): New rule to detect attempts to inject code into a process using PTRACE

Signed-off-by: Alessandro Brucato <alessandro.brucato@sysdig.com>

- rule: PTRACE attached to process
desc: "This rule detects an attempt to inject code into a process using PTRACE."
condition: evt.type=ptrace and evt.dir=> and evt.arg.request=11 and proc_name_exists
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a really initiative!! Thanks!

Here are my two cents/questions:

  1. Should we include PTRACE_SEIZE as well?
  2. Why do we care proc_name_exists?
  3. Security tooling might use ptrace capability (e.g. Falco in certain scenario), maybe we should add macro to exclude the known processes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @Kaizhe , thank you for your feedback!

  1. Sure, PTRACE_SEIZE makes sense to me, I added it in the condition
  2. We saw some false positives without that macro
  3. Sure, I added the macro (as an empty list)

@jasondellaluce
Copy link
Contributor

/milestone 0.34.0

@poiana poiana added this to the 0.34.0 milestone Sep 28, 2022

- rule: PTRACE attached to process
desc: "This rule detects an attempt to inject code into a process using PTRACE."
condition: evt.type=ptrace and evt.dir=> and evt.arg.request=11 and proc_name_exists
Copy link
Member

@loresuso loresuso Sep 30, 2022

Choose a reason for hiding this comment

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

Nice idea @Brucedh!

Just wanted to add something to this. Code injection is usually performed by issuing the ptrace syscall and using the following three requests in my opinion: PTRACE_ATTACH, PTRACE_POKETEXT and PTRACE_SETREGS. The poke text requests usually copy a shell code to the memory of the victim process and the set regs request usually modifies the instruction pointer to execute from that shell code. As you were saying, the attach requests could possibly lead to false positives, so I think we can also use the two other requests to improve this rule.

WDYT?

Choose a reason for hiding this comment

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

Hey, thanks for the feedback. Do you know why Falco thinks that "POKETEXT" is evt.arg.request=5 and not 4, same with SETREGS being 27 from Falco's point of view but 13 from a C programmer's perspective? https://elixir.bootlin.com/linux/v4.8/source/arch/arm/include/uapi/asm/ptrace.h#L16

Copy link
Member

@loresuso loresuso Oct 14, 2022

Choose a reason for hiding this comment

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

Hello @nicklangsysdig, the reason is that Falco maps those flags to its internal representations. This is done for portability so that regardless of the system Falco is running, the flags will all have a unique representation. This is useful if flags can change from one OS to another. If you want to dig deeper and know about how Falco redefines these flags, please check it here: https://github.com/falcosecurity/libs/blob/master/driver/ppm_events_public.h#L416-L454
https://github.com/falcosecurity/libs/blob/master/driver/ppm_flag_helpers.h#L1435
keep in mind that the same reasoning applies to all flags, so this is an advice you can re-use in the future 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback @loresuso !
I agree it’s a good idea to cover also PTRACE_POKETEXT and PTRACE_SETREGS. I added them along with PTRACE_POKEDATA, which is equivalent to PTRACE_POKETEXT

Copy link
Member

Choose a reason for hiding this comment

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

It makes sense, thank you!

…and whitelist macro

Signed-off-by: Alessandro Brucato <alessandro.brucato@sysdig.com>
@poiana poiana added size/S and removed size/XS labels Oct 18, 2022
Signed-off-by: Alessandro Brucato <alessandro.brucato@sysdig.com>
@nicklangsysdig
Copy link

@Kaizhe @loresuso Looking good so far?

@darryk10
Copy link
Contributor

darryk10 commented Nov 4, 2022

LGTM

@poiana
Copy link
Contributor

poiana commented Nov 4, 2022

LGTM label has been added.

Git tree hash: 608066d4bbdff5aae572605fa6d5186876f64f5e

Copy link
Contributor

@jasondellaluce jasondellaluce 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
Contributor

poiana commented Nov 30, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Brucedh, darryk10, jasondellaluce, leogr

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 3697d1f into falcosecurity:master Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants