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

feat: Support for detecting outbound connection to c2 servers with FQ… #2241

Merged
merged 1 commit into from
Dec 15, 2022

Conversation

Nicolas-Peiffer
Copy link
Contributor

@Nicolas-Peiffer Nicolas-Peiffer commented Oct 10, 2022

…DN domains and IP addresses.

What type of PR is this?

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 rules

What this PR does / why we need it:

This is a modification and enhancement of the Falco rule outbound connection to c2 servers. The rule was only considering IP addresses as a list of C2 servers. The rule was not considering Domain names (FQDN).

Now the rule both considers FQDN and IP address. This is espacially convenient for public list of C2 servers with both FQDN and IP addresses and that can be found on the internet, like this one:
https://feodotracker.abuse.ch/downloads/ipblocklist_recommended.json

The new outbound connection to c2 servers rules is inspired by other outbound rules like:
https://github.com/falcosecurity/falco/blob/master/rules/falco_rules.yaml#L392

Which issue(s) this PR fixes:

No issue.

Special notes for your reviewer:

I am not sure that there is a user-facing change. Else I would say:
Rule: Adding support for detecting outbound connection to c2 servers with both FQDN domains and IP addresses.

Does this PR introduce a user-facing change?:

rule(Outbound Connection to C2 Server): Update the "Outbound connection to C2 server" rule to match both FQDN and IP addresses. Prior to this change, the rule only matched IP addresses and not FQDN.

@poiana
Copy link
Contributor

poiana commented Oct 10, 2022

Welcome @Nicolas-Peiffer! It looks like this is your first PR to falcosecurity/falco 🎉

@jasondellaluce
Copy link
Contributor

/milestone 0.34.0

@poiana poiana added this to the 0.34.0 milestone Oct 10, 2022
Copy link
Contributor

@darryk10 darryk10 left a comment

Choose a reason for hiding this comment

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

Hi,
thanks for contributing :) I agree with your idea and it can be helpful to use public lists.
I added some comments to simplify the change without rewriting the overall rule.

# - "57.ip-142-44-247.net"
# ```

- rule: Outbound Connection to C2 Servers IPs and FQDNs
Copy link
Contributor

Choose a reason for hiding this comment

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

I honestly don't think we need to change the rule name since it's accurate as it is.
we can just add in the existent description that the rules match IPs and FQDN.
I would just update the rule condition as done to support fqdn as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I agree with your suggestions. The name of the rule can stay the same (Outbound Connection to C2 Servers), as well as the output: field.

If you prefer, I can revert to the previous rule name.


Question: I understand you prefer this

  desc: Detect outbound connection to command & control servers thanks to a list of IP addresses & a list of FQDN.

over this?

  desc: >
    Detect outbound connection to command & control servers. For example, fetch
    a list of IP addresses and FQDN on this website:
    https://feodotracker.abuse.ch/downloads/ipblocklist_recommended.json.

I have no opinion on this one. I understand it is maybe better not to include a link to feodotracker.abuse.ch.

Copy link
Contributor

@darryk10 darryk10 Nov 4, 2022

Choose a reason for hiding this comment

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

@Nicolas-Peiffer I'll go with this one

desc: Detect outbound connection to command & control servers thanks to a list of IP addresses & a list of FQDN.

I feel the desc you added is more a comment for the rule so since it is actually a useful comment I think you can add it on top fo the rule as a proper comment as done in other rules. What do you think?

@darryk10
Copy link
Contributor

darryk10 commented Nov 4, 2022

@Nicolas-Peiffer can you also pls add something in the PR section for the release note?
Does this PR introduce a user-facing change?

@Nicolas-Peiffer
Copy link
Contributor Author

@darryk10 I updated the release note.

And I cleaned my commits and modifed the rule taking into account your suggestions.

outbound and
((fd.sip in (c2_server_ip_list)) or
(fd.sip.name in (c2_server_fqdn_list)))
output: Outbound connection to C2 server (command=%proc.cmdline connection=%fd.name user=%user.name user_loginuid=%user.loginuid container_id=%container.id image=%container.image.repository)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be also useful to print out fd.sip and fd.sip.name for troubleshooting and investigation.
WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not think of it but you are right about that enhancement of the output:.
However, I am not sure what is the behaviour if there is only an IP addresse or only an FQDN: would either %fd.sip.name or %fd.sip be blank in that case?

But indeed, we can consider something like this:

output: Outbound connection to C2 server domain=%fd.sip.name addr=%fd.sip (command=%proc.cmdline connection=%fd.name user=%user.name user_loginuid=%user.loginuid container_id=%container.id image=%container.image.repository)

or this (moving the parenthesis):

output: Outbound connection to C2 server (domain=%fd.sip.name addr=%fd.sip command=%proc.cmdline connection=%fd.name user=%user.name user_loginuid=%user.loginuid container_id=%container.id image=%container.image.repository)

We could also be more specific by adding a prefix c2_ like this: c2_domain=%fd.sip.name c2_addr=%fd.sip .
I have no particular preference.

Copy link
Contributor

@darryk10 darryk10 Nov 8, 2022

Choose a reason for hiding this comment

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

I agree the option with c2_domain and c2_addr would be specific and clear so I would go in this direction.
I would also go for this output you mentioned

output: Outbound connection to C2 server (domain=%fd.sip.name addr=%fd.sip command=%proc.cmdline connection=%fd.name user=%user.name user_loginuid=%user.loginuid container_id=%container.id image=%container.image.repository)

As far as the empty fields goes I don't think it would be a problem if left empty but if you could do a quick test to see if all makes sense from your side would be awesome.
Thanks again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay no big deal with blank IP addresses or blank FQDN : falco will output <NA> if there are no values.

In the following, we can see c2_domain=<NA> c2_addr=46.101.90.205 because the rule was triggered only with the IP adress.

I am pushing the modification.

falco 18:25:57.134661204: Warning Outbound connection to C2 server (c2_domain=<NA> c2_addr=46.101.90.205 command
=curl 46.101.90.205 connection=10.42.0.7:59170->46.101.90.205:80 user=<NA> user_loginuid=-1 container_id=62513c2
df2f2 image=<NA>) k8s.ns=default k8s.pod=tmp-shell container=62513c2df2f2

…DN domains and IP addresses.

Signed-off-by: Nicolas-Peiffer <[email protected]>

feat: Support for detecting outbound connection to c2 servers with FQDN domains and IP addresses.

doc: add comment

Fixing DCO append amend

Signed-off-by: Nicolas-Peiffer <[email protected]>

Revert to original C2 rule name

Signed-off-by: Nicolas-Peiffer <[email protected]>

modify comments on C2 rule

Signed-off-by: Nicolas-Peiffer <[email protected]>

comment

Signed-off-by: Nicolas-Peiffer <[email protected]>

clean comments

Signed-off-by: Nicolas-Peiffer <[email protected]>

clean comments

Signed-off-by: Nicolas-Peiffer <[email protected]>

modify stdout

Signed-off-by: thedetective <[email protected]>
Copy link
Contributor

@darryk10 darryk10 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 @Nicolas-Peiffer for the changes :)

@poiana poiana added the lgtm label Nov 10, 2022
@poiana
Copy link
Contributor

poiana commented Nov 10, 2022

LGTM label has been added.

Git tree hash: 5f7c82748c8021b8b6c1b7522d5284376eda23fd

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

@leogr
Copy link
Member

leogr commented Dec 15, 2022

Closing and reopening to trigger the CI
/close

@poiana poiana closed this Dec 15, 2022
@poiana
Copy link
Contributor

poiana commented Dec 15, 2022

@leogr: Closed this PR.

In response to this:

Closing and reopening to trigger the CI
/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
Copy link
Member

leogr commented Dec 15, 2022

/reopen

@poiana poiana reopened this Dec 15, 2022
@poiana
Copy link
Contributor

poiana commented Dec 15, 2022

@leogr: Reopened this PR.

In response to this:

/reopen

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

leogr commented Dec 15, 2022

/approve

@poiana
Copy link
Contributor

poiana commented Dec 15, 2022

[APPROVALNOTIFIER] This PR is APPROVED

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

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 1f15af1 into falcosecurity:master Dec 15, 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.

5 participants