Refactor Ansible remediation for dir_perms_world_writable_root_owned #10839
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Line size limit is 99
The find tasks were not working as expected due to a misinterpretation of the "excludes" parameter. As a consequence some special dirs were not ignored, hitting performance. Also, it was noticed a corner case revelead by the test scenario where a NFS file system was targeting a local folder. The corresponding local folder was not fixed due to the way it was represented in Ansible Facts. All issues were solved in this refactoring.
marcusburghardt
added
bugfix
marcusburghardt
commented
Jul 13, 2023
|
|
||
| - name: "{{{ rule_title }}} - Find All Uncompliant Directories in Local File Systems" | ||
| ansible.builtin.command: | ||
| cmd: find {{ item }} -xdev -type d -perm -0002 -uid +0 |
There was a problem hiding this comment.
We don't have something similar to -xdev (or -mount) in the find module and this was the simplest option for this specific case.
|
Start a new ephemeral environment with changes proposed in this pull request: rhel8 (from CTF) Environment (using Fedora as testing environment) Fedora Testing Environment Oracle Linux 8 Environment |
|
This datastream diff is auto generated by the check Click here to see the full diffNew content has different text for rule 'xccdf_org.ssgproject.content_rule_dir_perms_world_writable_root_owned'.
--- xccdf_org.ssgproject.content_rule_dir_perms_world_writable_root_owned
+++ xccdf_org.ssgproject.content_rule_dir_perms_world_writable_root_owned
@@ -1,12 +1,11 @@
[title]:
-Ensure All World-Writable Directories Are Owned by root user
+Ensure All World-Writable Directories Are Owned by root User
[description]:
-All directories in local partitions which are world-writable should be owned
-by root. If any world-writable directories are not owned by root, this
-should be investigated. Following this, the files should be deleted or
-assigned to root user.
+All directories in local partitions which are world-writable should be owned by root.
+If any world-writable directories are not owned by root, this should be investigated.
+Following this, the files should be deleted or assigned to root user.
[reference]:
BP28(R40)
@@ -27,10 +26,9 @@
SV-230318r743960_rule
[rationale]:
-Allowing a user account to own a world-writable directory is
-undesirable because it allows the owner of that directory to remove
-or replace any files that may be placed in the directory by other
-users.
+Allowing a user account to own a world-writable directory is undesirable because it allows the
+owner of that directory to remove or replace any files that may be placed in the directory by
+other users.
[ident]:
CCE-83375-6
OCIL for rule 'xccdf_org.ssgproject.content_rule_dir_perms_world_writable_root_owned' differs.
--- ocil:ssg-dir_perms_world_writable_root_owned_ocil:questionnaire:1
+++ ocil:ssg-dir_perms_world_writable_root_owned_ocil:questionnaire:1
@@ -1,5 +1,5 @@
The following command will discover and print world-writable directories that
-are not owned by root. Run it once for each local partition PART:
+are not owned by root. Run it once for each local partition PART:
$ sudo find PART -xdev -type d -perm -0002 -uid +0 -print
- Is it the case that there is output?
+ Is it the case that there are world-writable directories not owned by root?
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,4 +1,9 @@
# 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 {} \;
+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
@@ -1,5 +1,6 @@
-- name: Configure excluded (non local) file systems
- set_fact:
+- name: Ensure All World-Writable Directories Are Owned by root User - Define Excluded
+ (Non-Local) File Systems and Paths
+ ansible.builtin.set_fact:
excluded_fstypes:
- afs
- ceph
@@ -20,6 +21,13 @@
- lustre
- davfs
- fuse.sshfs
+ excluded_paths:
+ - dev
+ - proc
+ - run
+ - sys
+ search_paths: []
+ world_writable_dirs: []
tags:
- CCE-83375-6
- DISA-STIG-RHEL-08-010700
@@ -30,10 +38,15 @@
- no_reboot_needed
- restrict_strategy
-- name: Create empty list of excluded paths
- set_fact:
- excluded_paths:
- - /proc
+- name: Ensure All World-Writable Directories Are Owned by root User - Find Relevant
+ Root Directories Ignoring Pre-Defined Excluded Paths
+ ansible.builtin.find:
+ paths: /
+ file_type: directory
+ excludes: '{{ excluded_paths }}'
+ hidden: true
+ recurse: false
+ register: result_relevant_root_dirs
tags:
- CCE-83375-6
- DISA-STIG-RHEL-08-010700
@@ -44,11 +57,11 @@
- no_reboot_needed
- restrict_strategy
-- name: Detect nonlocal file systems and add them to excluded paths
- set_fact:
- excluded_paths: '{{ excluded_paths | union([item.mount]) }}'
- loop: '{{ ansible_mounts }}'
- when: item.fstype in excluded_fstypes
+- name: Ensure All World-Writable Directories Are Owned by root User - Include Relevant
+ Root Directories in a List of Paths to be Searched
+ ansible.builtin.set_fact:
+ search_paths: '{{ search_paths | union([item.path]) }}'
+ loop: '{{ result_relevant_root_dirs.files }}'
tags:
- CCE-83375-6
- DISA-STIG-RHEL-08-010700
@@ -59,14 +72,14 @@
- no_reboot_needed
- restrict_strategy
-- name: Find all directories excluding non-local partitions
- find:
- paths: /
- excludes: excluded_paths
- file_type: directory
- hidden: true
- recurse: true
- register: found_dirs
+- name: Ensure All World-Writable Directories Are Owned by root User - Increment Search
+ Paths List with Local Partitions Mount Points
+ ansible.builtin.set_fact:
+ search_paths: '{{ search_paths | union([item.mount]) }}'
+ loop: '{{ ansible_mounts }}'
+ when:
+ - item.fstype not in excluded_fstypes
+ - item.mount != '/'
tags:
- CCE-83375-6
- DISA-STIG-RHEL-08-010700
@@ -77,9 +90,12 @@
- no_reboot_needed
- restrict_strategy
-- name: Create list of world writable directories
- set_fact:
- world_writable_dirs: '{{ found_dirs.files | selectattr(''woth'') | list }}'
+- name: Ensure All World-Writable Directories Are Owned by root User - Increment Search
+ Paths List with Local NFS File System Targets
+ ansible.builtin.set_fact:
+ search_paths: '{{ search_paths | union([item.device.split('':'')[1]]) }}'
+ loop: '{{ ansible_mounts }}'
+ when: item.device is search("localhost:")
tags:
- CCE-83375-6
- DISA-STIG-RHEL-08-010700
@@ -90,12 +106,13 @@
- no_reboot_needed
- restrict_strategy
-- name: Change owner to root on directories which are world writable
- file:
- path: '{{ item.path }}'
- owner: root
- loop: '{{ world_writable_dirs }}'
- ignore_errors: true
+- name: Ensure All World-Writable Directories Are Owned by root User - Find All Uncompliant
+ Directories in Local File Systems
+ ansible.builtin.command:
+ cmd: find {{ item }} -xdev -type d -perm -0002 -uid +0
+ loop: '{{ search_paths }}'
+ changed_when: false
+ register: result_found_dirs
tags:
- CCE-83375-6
- DISA-STIG-RHEL-08-010700
@@ -105,3 +122,35 @@
- medium_severity
- no_reboot_needed
- restrict_strategy
+
+- name: Ensure All World-Writable Directories Are Owned by root User - Create List
+ of World Writable Directories Not Owned by root
+ ansible.builtin.set_fact:
+ world_writable_dirs: '{{ world_writable_dirs | union(item.stdout_lines) | list
+ }}'
+ loop: '{{ result_found_dirs.results }}'
+ tags:
+ - CCE-83375-6
+ - DISA-STIG-RHEL-08-010700
+ - dir_perms_world_writable_root_owned
+ - low_complexity
+ - medium_disruption
+ - medium_severity
+ - no_reboot_needed
+ - restrict_strategy
+
+- name: Ensure All World-Writable Directories Are Owned by root User - Ensure root
+ Ownership on Local World Writable Directories
+ ansible.builtin.file:
+ path: '{{ item }}'
+ owner: root
+ loop: '{{ world_writable_dirs }}'
+ tags:
+ - CCE-83375-6
+ - DISA-STIG-RHEL-08-010700
+ - dir_perms_world_writable_root_owned
+ - low_complexity
+ - medium_disruption
+ - medium_severity
+ - no_reboot_needed
+ - restrict_strategy |
|
Code Climate has analyzed commit 1ca5578 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 53.4% (0.0% change). View more on Code Climate. |
Mab879
approved these changes
Jul 13, 2023
|
Waving the Automatus tests as they pass locally in a VM. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
During the review of #10563 it was noticed some issues in the Ansible remediation due to a misinterpretation of the
excludeparameter used by thefindmodule, as more detailed in the #10563 (comment) comment.In a deeper analysis of the remediation it was also noticed some performance issues and a corner case.
This PR complements the #10563 by refactoring the Ansible remediation so it is faster, accurate and more robust.
Rationale:
Review Hints:
Automatus tests using Bash and Ansible should be enough to confirm it is working as expected.