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

bugfix: dir_perms_world_writable_root_owned: do not modify files under /proc #10563

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

maage
Copy link
Contributor

@maage maage commented May 14, 2023

Description:

At least under containerized env (podman) /proc can have files that are not modifiable even with root. This makes test or bash remediate fail.

Rationale:

This issue would be kernel regression and should be handled in another way.

Review Hints:

Probably should see how bash remediation survives.

@openshift-ci openshift-ci bot added the needs-ok-to-test Used by openshift-ci bot. label May 14, 2023
@openshift-ci
Copy link

openshift-ci bot commented May 14, 2023

Hi @maage. Thanks for your PR.

I'm waiting for a ComplianceAsCode member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@github-actions
Copy link

Start a new ephemeral environment with changes proposed in this pull request:

rhel8 (from CTF) Environment (using Fedora as testing environment)
Open in Gitpod

Fedora Testing Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@github-actions
Copy link

github-actions bot commented May 14, 2023

This datastream diff is auto generated by the check Compare DS/Generate Diff

Click here to see the full diff
bash remediation for rule 'xccdf_org.ssgproject.content_rule_dir_perms_world_writable_root_owned' differs.
--- xccdf_org.ssgproject.content_rule_dir_perms_world_writable_root_owned
+++ xccdf_org.ssgproject.content_rule_dir_perms_world_writable_root_owned
@@ -1,2 +1,4 @@
 
-find / -not -fstype afs -not -fstype ceph -not -fstype cifs -not -fstype smb3 -not -fstype smbfs -not -fstype sshfs -not -fstype ncpfs -not -fstype ncp -not -fstype nfs -not -fstype nfs4 -not -fstype gfs -not -fstype gfs2 -not -fstype glusterfs -not -fstype gpfs -not -fstype pvfs2 -not -fstype ocfs2 -not -fstype lustre -not -fstype davfs -not -fstype fuse.sshfs -type d -perm -0002 -uid +0 -exec chown root {} \;
+# At least under containerized env /proc can have files w/o possilibity to
+# modify even as root. And touching /proc is not good idea anyways.
+find / -path /proc -prune -o -not -fstype afs -not -fstype ceph -not -fstype cifs -not -fstype smb3 -not -fstype smbfs -not -fstype sshfs -not -fstype ncpfs -not -fstype ncp -not -fstype nfs -not -fstype nfs4 -not -fstype gfs -not -fstype gfs2 -not -fstype glusterfs -not -fstype gpfs -not -fstype pvfs2 -not -fstype ocfs2 -not -fstype lustre -not -fstype davfs -not -fstype fuse.sshfs -type d -perm -0002 -uid +0 -exec chown root {} \;

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_dir_perms_world_writable_root_owned' differs.
--- xccdf_org.ssgproject.content_rule_dir_perms_world_writable_root_owned
+++ xccdf_org.ssgproject.content_rule_dir_perms_world_writable_root_owned
@@ -32,7 +32,8 @@
 
 - name: Create empty list of excluded paths
 set_fact:
- excluded_paths: []
+ excluded_paths:
+ - /proc
 tags:
 - CCE-83375-6
 - DISA-STIG-RHEL-08-010700

@maage
Copy link
Contributor Author

maage commented May 14, 2023

This fail is because containers do not have capability to load kernel modules, here knfsd and start nfs-server. Unrelated issue, see #10387

ERROR - Rule 'dir_perms_world_writable_root_owned' test setup script 'world_writable_dir_on_nonlocal_fs.fail.sh' failed with exit code 32

@marcusburghardt marcusburghardt added the bugfix Fixes to reported bugs. label May 15, 2023
Copy link
Member

@marcusburghardt marcusburghardt left a comment

Choose a reason for hiding this comment

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

Could you also update the Ansible remediation to be aligned to the Bash, please?

…r /proc

At least under containerized env (podman) /proc can have files that are
not modifiable even with root. This makes test or bash remediate fail.
@maage
Copy link
Contributor Author

maage commented May 15, 2023

Could you also update the Ansible remediation to be aligned to the Bash, please?

Updated, but ansible remediation needs to be checked. My ansible remediation test in now broken due to unrelated changes.

Maybe I'm understanding something wrong, but in my view excludes: excluded_paths should be excludes: "{{{ excluded_paths }}}", these module fields are not like when and variants where format is jinja, here format is either str or list. And remediation checks should have failed, so I guess something is wrong with tests too.

Edit: added missing not

@codeclimate
Copy link

codeclimate bot commented May 15, 2023

Code Climate has analyzed commit d7e90dc and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 52.4% (0.0% change).

View more on Code Climate.

@Mab879 Mab879 self-assigned this May 15, 2023
@Mab879 Mab879 added this to the 0.1.68 milestone May 15, 2023
@Mab879 Mab879 removed their assignment May 26, 2023
@Mab879
Copy link
Member

Mab879 commented May 26, 2023

@maage I'm looking into Ansible remediation. There is also issue with it on master.

@jan-cerny jan-cerny modified the milestones: 0.1.68, 0.1.69 May 29, 2023
@marcusburghardt marcusburghardt self-assigned this Jun 2, 2023
@marcusburghardt
Copy link
Member

Could you also update the Ansible remediation to be aligned to the Bash, please?

Updated, but ansible remediation needs to be checked. My ansible remediation test in now broken due to unrelated changes.

Maybe I'm understanding something wrong, but in my view excludes: excluded_paths should be excludes: "{{{ excluded_paths }}}", these module fields are not like when and variants where format is jinja, here format is either str or list. And remediation checks should have failed, so I guess something is wrong with tests too.

Edit: added missing not

The issue is that excludes option in the find module considers the basename only so it won't match /proc. I also found opportunities to improve performance in this Ansible Playbook and I would be happy to work on it.
@maage , could you remove the Ansible changes in this PR, please? Then we can merge it and I send a PR for the Ansible remediation write after the merge. Thanks.

Copy link
Member

@marcusburghardt marcusburghardt left a comment

Choose a reason for hiding this comment

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

There are some issues with the Ansible remediation but they are not related to the changes in this PR. In fact, the changes in this PR helped us to find the issues. The Ansible remediation needs to be refactored and I will propose a PR for this right after merging this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes to reported bugs. needs-ok-to-test Used by openshift-ci bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants