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

Fix/address addl rules fps #1372

Merged
merged 13 commits into from
Sep 3, 2020
Merged

Fix/address addl rules fps #1372

merged 13 commits into from
Sep 3, 2020

Conversation

mstemm
Copy link
Contributor

@mstemm mstemm commented Aug 28, 2020

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

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

What this PR does / why we need it:

Address several sources of FPs, primarily from GKE environments.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new: Address several sources of FPs, primarily from GKE environments.

rules/falco_rules.yaml Show resolved Hide resolved
rules/falco_rules.yaml Show resolved Hide resolved
rules/k8s_audit_rules.yaml Outdated Show resolved Hide resolved
In some cases, dropped events around the time a new container is started
can result in missing the exec/clone for a process that does a setns to
enter the namespace of a container. Here's an example from an oss
capture:

```

282273 09:01:22.098095673 30 runc:[0:PARENT] (168555) < setns res=0
282283 09:01:22.098138869 30 runc:[0:PARENT] (168555) < setns res=0
282295 09:01:22.098179685 30 runc:[0:PARENT] (168555) < setns res=0
517284 09:01:30.128723777 13 <NA> (168909) < setns res=0
517337 09:01:30.129054963 13 <NA> (168909) < setns res=0
517451 09:01:30.129560037 2 <NA> (168890) < setns res=0
524597 09:01:30.162741004 19 <NA> (168890) < setns res=0
527433 09:01:30.179786170 18 runc:[0:PARENT] (168927) < setns res=0
527448 09:01:30.179852428 18 runc:[0:PARENT] (168927) < setns res=0
535566 09:01:30.232420372 25 nsenter (168938) < setns res=0
537412 09:01:30.246200357 0 nsenter (168941) < setns res=0
554163 09:01:30.347158783 17 nsenter (168950) < setns res=0
659908 09:01:31.064622960 12 runc:[0:PARENT] (169023) < setns res=0
659919 09:01:31.064665759 12 runc:[0:PARENT] (169023) < setns res=0
732062 09:01:31.608297074 4 nsenter (169055) < setns res=0
812985 09:01:32.217527319 6 runc:[0:PARENT] (169077) < setns res=0
812991 09:01:32.217579396 6 runc:[0:PARENT] (169077) < setns res=0
813000 09:01:32.217632211 6 runc:[0:PARENT] (169077) < setns res=0
```

When this happens, it can cause false positives for the "Change thread
namespace" rule as it allows certain process names like "runc",
"containerd", etc to perform setns calls.

Other rules already use the proc_name_exists macro to require that the
process name exists. This change adds proc_name_exists to the Change
Thread Namespace rule as well.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
Let programs spawned by linux-bench (CIS Linux Benchmark program) read
/etc/shadow. Tests in the benchmark check for permissions of the file
and accounts in the contents of the file.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
@mstemm
Copy link
Contributor Author

mstemm commented Sep 1, 2020

@fntlnz check out the last commit, it changes the trace files that get downloaded by the tests to have versioning now. I think this is a clean way to handle changes in trace files, as I needed for this PR.

We can also put the trace files in git somewhere but some of them are pretty big.

Kaizhe
Kaizhe previously approved these changes Sep 1, 2020
@poiana poiana added the lgtm label Sep 1, 2020
Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

Just found a dup entry (see the comment below), otherwise it looks very good to me!!!

Thank you!!!

rules/falco_rules.yaml Outdated Show resolved Hide resolved
@poiana poiana removed the lgtm label Sep 2, 2020
@leogr
Copy link
Member

leogr commented Sep 2, 2020

/milestone 0.26.0

@poiana poiana added this to the 0.26.0 milestone Sep 2, 2020
Previously any write to a file called sources.list would match the
access_repositories condition, even a file /usr/tmp/..../sources.list.

Change the macro so the files in repository_files must be somewhere
below any of repository_directories.

Also allow programs spawned by package management programs to change
these files, using package_mgmt_ancestor_procs.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
Add several calico images and command line programs that end up writing
below /etc/calico.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
Let mysqlsh write below /root/.mysqlsh.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
Related to https://github.com/GoogleCloudPlatform/guest-oslogin, full
cmdline is google_oslogin_control.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
Sort the items in the list falco_privileged_images alphabetically
and also separate them into individual lines. Make it easier to note
changes to the entries in the list using git blame.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
Most of these are seen in GKE and are uses for core routing/metrics
collection.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
Add a set of images known to run in the host network. Mostly related to
GKE, sometimes plus metrics collection.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
Seen when using K8s cluster autoscaling or addon manager.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
Add several images seen in GKE environments that can run in the
kube-system namespace.

Also change the names of the lists to be more specific. The old names
are retained but are kept around for backwards compatibility.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
Add system:managed-certificate-controller as a system role that can be
modified. Can be changed as a part of upgrades.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
Start versioning trace files with a unique date. Any time we need to
create new trace files, change TRACE_FILES_VERSION in this script and
copy to traces-{positive,negative,info}-<VERSION>.zip.

The zip file should unzip to traces-{positive,negative,info}, without
any version.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

LGTM now

Thank you!

@poiana poiana added the lgtm label Sep 2, 2020
@poiana
Copy link

poiana commented Sep 2, 2020

LGTM label has been added.

Git tree hash: e97492b11997ab0473c3a175601c2480c3802be7

@poiana poiana added the approved label Sep 2, 2020
@poiana
Copy link

poiana commented Sep 3, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Kaizhe, 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 f32bb84 into master Sep 3, 2020
@poiana poiana deleted the fix/address-addl-rules-fps branch September 3, 2020 16:56
@fntlnz
Copy link
Contributor

fntlnz commented Sep 21, 2020

@mstemm Thanks for the hard work here as alwayas! I have a question, how are the integration tests file generated? Now that they are versioned I would like to understand how we can get them to a shape where everyone can read a document on how to add new ones.

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