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.

Sign up for GitHub

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

Jump to bottom

Fix falco-driver-loader SELinux insmod denials #1756

Merged
merged 3 commits into from
Nov 2, 2021
Merged

Fix falco-driver-loader SELinux insmod denials #1756

merged 3 commits into from
Nov 2, 2021

Conversation

dwindsor
Copy link
Contributor

@dwindsor dwindsor commented Oct 13, 2021 •
edited by leogr
Loading

Signed-off-by: David Windsor [email protected]

What type of PR is this?

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

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

What this PR does / why we need it:
Currently, SELinux prevents falco-driver-loader from calling insmod on falco .ko files. This is because the .ko files are mislabeled - they need to be relabeled modules_object_t:

type=AVC msg=audit(1634143507.210:1101): avc:  denied  { module_load } for  pid=15732 comm="insmod" path="/root/.falco/falco_centos_4.18.0-305.3.1.el8.x86_64_1.ko" dev="dm-0" ino=3427522 scontext=unconfined_u:unconfined_r:kmod_t:s0-s0:c0.c1023 tcontext=unconfined_u:object_r:admin_home_t:s0 tclass=system permissive=0

Which issue(s) this PR fixes:

Fixes #1755

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix(scripts/falco-driver-loader): fix for SELinux insmod denials

@poiana
Copy link
Contributor

poiana commented Oct 13, 2021

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

@poiana poiana added the size/XS label Oct 13, 2021
leodido
leodido previously approved these changes Oct 13, 2021
View reviewed changes
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.

Really good catch!

/lgtm

@poiana poiana added the lgtm label Oct 13, 2021
@poiana
Copy link
Contributor

poiana commented Oct 13, 2021

LGTM label has been added.

Git tree hash: 1a34b1678c3bcab594dd6ba14825a9c67bdd63a9

@LucaGuerra
Copy link
Contributor

Do you think there could be issues if the OS doesn't have SELinux installed or enabled? I was also thinking about the availability of the chcon command itself but it looks much more ubiquitous than I thought!

leodido
leodido reviewed Oct 15, 2021
View reviewed changes
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.

Left suggestions which I believe is worth considering

scripts/falco-driver-loader Outdated Show resolved Hide resolved
scripts/falco-driver-loader Outdated Show resolved Hide resolved
scripts/falco-driver-loader Outdated Show resolved Hide resolved
ghost
ghost previously approved these changes Oct 15, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Good catch. We were only considering CentOS 7 and 8, both of which will have chcon. LGTM.

@poiana
Copy link
Contributor

poiana commented Oct 15, 2021

@dwindsor-scwx: changing LGTM is restricted to collaborators

In response to this:

Good catch. We were only considering CentOS 7 and 8, both of which will have chcon. LGTM.

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.

@dwindsor dwindsor dismissed stale reviews from ghost and leodido via a0f4dd9 October 19, 2021 11:55
@dwindsor @leodido
Apply suggestions from code review
4b37ac8 
Don't fail if chcon is not present

Co-authored-by: Leo Di Donato <[email protected]>
Signed-off-by: David Windsor <[email protected]>
@leogr leogr requested a review from leodido October 29, 2021 08:43
leogr
leogr approved these changes Oct 29, 2021
View reviewed changes
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.

Thank you for submitting this PR :)

LGTM
/approve
/milestone 0.31.0

@poiana poiana added the lgtm label Oct 29, 2021
@poiana
Copy link
Contributor

poiana commented Oct 29, 2021

LGTM label has been added.

Git tree hash: d45258ea20172fe7e49136eafb4faed3741c7bc2

@poiana poiana added this to the 0.31.0 milestone Oct 29, 2021
@poiana
Copy link
Contributor

poiana commented Oct 29, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dwindsor, leodido, 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

@leogr
Copy link
Member

leogr commented Nov 2, 2021

/cc @leodido

@poiana poiana merged commit 8448d02 into falcosecurity:master Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leodido leodido leodido approved these changes

@leogr leogr leogr approved these changes

@krisnova krisnova Awaiting requested review from krisnova

Assignees

@leodido leodido

@leogr leogr

Labels
approved area/build dco-signoff: yes kind/bug lgtm release-note size/XS
Projects
None yet
Milestone
0.31.0
Development

Successfully merging this pull request may close these issues.

SELinux is preventing falco-driver-loader from loading falco kernel drivers on CentOS 8
5 participants
@dwindsor @poiana @LucaGuerra @leogr @leodido