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

Improve Performance on rules probing the whole file system #11319

Merged

Conversation

marcusburghardt
Copy link
Member

@marcusburghardt marcusburghardt commented Nov 30, 2023

Description:

There are rules in the project used to check some files properties in the system.
These rules need to probe all files in a system in order to ensure compliance.
However, there are particular systems with a huge number of files and in these systems the scanner (openscap) can hit some system resource limits during the assessment, specially related to memory usage.

The intention of this PR is to review and optimize the performance of all mapped rules that may consume high system resources while probing file systems:

The improvements consist of:

  • Creating and using a macro to select only relevant mount points to be searched
    • The macro skip all mount points related to remote file systems
    • The macro skip pseudo file systems, like /proc and /sys.
  • Adopt the macro in all rules so the behavior for aligned among all rules and is easier to maintain
  • Refactoring the OVAL in some rules to improve their efficiency by removing unnecessary probes or objects duplication.
  • Improve readability of reviewed rules.

Rationale:

It is expected a considerable performance improvement after reviewing all these rules, specially when multiple of these rules are used in the same profile.
In a small test, it was created a VM and included about 500.000 files in a local mount point. The automatus tests were executed using the file_permissions_unauthorized_suid before and after the improvements. Here is the result:

Before the improvements:

time ./tests/automatus.py rule --libvirt qemu:///session rhel9-nightly --datastream build/ssg-rhel9-ds.xml --dontclean --remediate-using bash file_permissions_unauthorized_suid
INFO - xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_suid
INFO - Script no_unpackaged_suid.pass.sh using profile (all) OK
INFO - Script unpackaged_suid.fail.sh using profile (all) OK

real	2m25.382s
user	0m7.554s
sys	0m0.789s

After the improvements:

time ./tests/automatus.py rule --libvirt qemu:///session rhel9-nightly --datastream build/ssg-rhel9-ds.xml --dontclean --remediate-using bash file_permissions_unauthorized_suid
Setting console output to log level INFO
INFO - xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_suid
INFO - Script no_unpackaged_suid.pass.sh using profile (all) OK
INFO - Script unpackaged_suid.fail.sh using profile (all) OK

real	1m45.165s
user	0m7.696s
sys	0m0.750s

Review Hints:

Automatus tests should be enough to testing the rules technically.
For context about the changes, check the commit messages.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Nov 30, 2023
Copy link

openshift-ci bot commented Nov 30, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@marcusburghardt marcusburghardt added refactoring Improvement which, once completed, will enable the project to progress faster. Update Rule Issues or pull requests related to Rules updates. and removed do-not-merge/work-in-progress Used by openshift-ci bot. labels Nov 30, 2023
@marcusburghardt marcusburghardt added this to the 0.1.72 milestone Nov 30, 2023
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Nov 30, 2023
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

Copy link

github-actions bot commented Nov 30, 2023

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

Click here to see the full diff
New content has different text for rule 'xccdf_org.ssgproject.content_rule_dir_perms_world_writable_sticky_bits'.
--- xccdf_org.ssgproject.content_rule_dir_perms_world_writable_sticky_bits
+++ xccdf_org.ssgproject.content_rule_dir_perms_world_writable_sticky_bits
@@ -3,21 +3,23 @@
 Verify that All World-Writable Directories Have Sticky Bits Set
 
 [description]:
-When the so-called 'sticky bit' is set on a directory,
-only the owner of a given file may remove that file from the
-directory. Without the sticky bit, any user with write access to a
-directory may remove any file in the directory. Setting the sticky
-bit prevents users from removing each other's files. In cases where
-there is no reason for a directory to be world-writable, a better
-solution is to remove that permission rather than to set the sticky
-bit. However, if a directory is used by a particular application,
-consult that application's documentation instead of blindly
-changing modes.
-
-To set the sticky bit on a world-writable directory DIR, run the
-following command:
+When the so-called 'sticky bit' is set on a directory, only the owner of a given file may
+remove that file from the directory. Without the sticky bit, any user with write access to a
+directory may remove any file in the directory. Setting the sticky bit prevents users from
+removing each other's files. In cases where there is no reason for a directory to be
+world-writable, a better solution is to remove that permission rather than to set the sticky
+bit. However, if a directory is used by a particular application, consult that application's
+documentation instead of blindly changing modes.
+
+To set the sticky bit on a world-writable directory DIR, run the following command:
 $ sudo chmod +t DIR
 
+[warning]:
+This rule can take a long time to perform the check and might consume a considerable
+amount of resources depending on the number of directories present on the system. It is
+not a problem in most cases, but especially systems with a large number of directories can
+be affected. See https://access.redhat.com/articles/6999111.
+
 [reference]:
 BP28(R40)
 
@@ -193,14 +195,13 @@
 SV-230243r792857_rule
 
 [rationale]:
-Failing to set the sticky bit on public directories allows unauthorized
-users to delete files in the directory structure.
-
-The only authorized public directories are those temporary directories
-supplied with the system, or those designed to be temporary file
-repositories. The setting is normally reserved for directories used by the
-system, by users for temporary file storage (such as /tmp), and
-for directories requiring global read/write access.
+Failing to set the sticky bit on public directories allows unauthorized users to delete files
+in the directory structure.
+
+The only authorized public directories are those temporary directories supplied with the
+system, or those designed to be temporary file repositories. The setting is normally reserved
+for directories used by the system, by users for temporary file storage (such as /tmp),
+and for directories requiring global read/write access.
 
 [ident]:
 CCE-80783-4

New content has different text for rule 'xccdf_org.ssgproject.content_rule_dir_perms_world_writable_system_owned'.
--- xccdf_org.ssgproject.content_rule_dir_perms_world_writable_system_owned
+++ xccdf_org.ssgproject.content_rule_dir_perms_world_writable_system_owned
@@ -3,12 +3,16 @@
 Ensure All World-Writable Directories Are Owned by a System Account
 
 [description]:
-All directories in local partitions which are
-world-writable should be owned by root or another
-system account. If any world-writable directories are not
-owned by a system account, this should be investigated.
-Following this, the files should be deleted or assigned to an
+All directories in local partitions which are world-writable should be owned by root or
+another system account. If any world-writable directories are not owned by a system account,
+this should be investigated. Following this, the files should be deleted or assigned to an
 appropriate owner.
+
+[warning]:
+This rule can take a long time to perform the check and might consume a considerable
+amount of resources depending on the number of directories present on the system. It is
+not a problem in most cases, but especially systems with a large number of directories can
+be affected. See https://access.redhat.com/articles/6999111.
 
 [reference]:
 12
@@ -143,7 +147,6 @@
 SRG-OS-000480-GPOS-00227
 
 [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.

OVAL for rule 'xccdf_org.ssgproject.content_rule_dir_perms_world_writable_system_owned' differs.
--- oval:ssg-dir_perms_world_writable_system_owned:def:1
+++ oval:ssg-dir_perms_world_writable_system_owned:def:1
@@ -1,2 +1,2 @@
 criteria AND
-criterion oval:ssg-test_dir_world_writable_uid_gt_value:tst:1
+criterion oval:ssg-test_dir_perms_world_writable_system_owned:tst:1

OCIL for rule 'xccdf_org.ssgproject.content_rule_dir_perms_world_writable_system_owned' differs.
--- ocil:ssg-dir_perms_world_writable_system_owned_ocil:questionnaire:1
+++ ocil:ssg-dir_perms_world_writable_system_owned_ocil:questionnaire:1
@@ -1,6 +1,6 @@
-The following command will discover and print world-writable directories that
-are not owned by a system account, given the assumption that only system
-accounts have a uid lower than 500.  Run it once for each local partition PART:
-$ sudo find PART -xdev -type d -perm -0002 -uid +499 -print
+The following command will discover and print world-writable directories that are not owned by
+a system account, given the assumption that only system accounts have a uid lower than 500.
+Run it once for each local partition PART:
+$ sudo find PART -xdev -type d -perm -0002 -uid +1000 -print
       Is it the case that there is output?
       
New content has different text for rule 'xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_sgid'.
--- xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_sgid
+++ xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_sgid
@@ -3,16 +3,20 @@
 Ensure All SGID Executables Are Authorized
 
 [description]:
-The SGID (set group id) bit should be set only on files that were
-installed via authorized means. A straightforward means of identifying
-unauthorized SGID files is determine if any were not installed as part of an
-RPM package, which is cryptographically verified. Investigate the origin
-of any unpackaged SGID files.
-This configuration check considers authorized SGID files which were installed via RPM.
-It is assumed that when an individual has sudo access to install an RPM
-and all packages are signed with an organizationally-recognized GPG key,
-the software should be considered an approved package on the system.
-Any SGID file not deployed through an RPM will be flagged for further review.
+The SGID (set group id) bit should be set only on files that were installed via authorized
+means. A straightforward means of identifying unauthorized SGID files is determine if any were
+not installed as part of an RPM package, which is cryptographically verified. Investigate the
+origin of any unpackaged SGID files. This configuration check considers authorized SGID files
+those which were installed via RPM. It is assumed that when an individual has sudo access to
+install an RPM and all packages are signed with an organizationally-recognized GPG key, the
+software should be considered an approved package on the system. Any SGID file not deployed
+through an RPM will be flagged for further review.
+
+[warning]:
+This rule can take a long time to perform the check and might consume a considerable
+amount of resources depending on the number of files present on the system. It is not a
+problem in most cases, but especially systems with a large number of files can be affected.
+See https://access.redhat.com/articles/6999111.
 
 [reference]:
 BP28(R37)
@@ -150,10 +154,9 @@
 6.1.15
 
 [rationale]:
-Executable files with the SGID permission run with the privileges of
-the owner of the file. SGID files of uncertain provenance could allow for
-unprivileged users to elevate privileges. The presence of these files should be
-strictly controlled on the system.
+Executable files with the SGID permission run with the privileges of the owner of the file.
+SGID files of uncertain provenance could allow for unprivileged users to elevate privileges.
+The presence of these files should be strictly controlled on the system.
 
 [ident]:
 CCE-80816-2

New content has different text for rule 'xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_suid'.
--- xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_suid
+++ xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_suid
@@ -3,16 +3,20 @@
 Ensure All SUID Executables Are Authorized
 
 [description]:
-The SUID (set user id) bit should be set only on files that were
-installed via authorized means. A straightforward means of identifying
-unauthorized SUID files is determine if any were not installed as part of an
-RPM package, which is cryptographically verified. Investigate the origin
-of any unpackaged SUID files.
-This configuration check considers authorized SUID files which were installed via RPM.
-It is assumed that when an individual has sudo access to install an RPM
-and all packages are signed with an organizationally-recognized GPG key,
-the software should be considered an approved package on the system.
-Any SUID file not deployed through an RPM will be flagged for further review.
+The SUID (set user id) bit should be set only on files that were installed via authorized
+means. A straightforward means of identifying unauthorized SUID files is determine if any were
+not installed as part of an RPM package, which is cryptographically verified. Investigate the
+origin of any unpackaged SUID files. This configuration check considers authorized SUID files
+those which were installed via RPM. It is assumed that when an individual has sudo access to
+install an RPM and all packages are signed with an organizationally-recognized GPG key, the
+software should be considered an approved package on the system. Any SUID file not deployed
+through an RPM will be flagged for further review.
+
+[warning]:
+This rule can take a long time to perform the check and might consume a considerable
+amount of resources depending on the number of files present on the system. It is not a
+problem in most cases, but especially systems with a large number of files can be affected.
+See https://access.redhat.com/articles/6999111.
 
 [reference]:
 BP28(R37)
@@ -150,10 +154,9 @@
 6.1.14
 
 [rationale]:
-Executable files with the SUID permission run with the privileges of
-the owner of the file. SUID files of uncertain provenance could allow for
-unprivileged users to elevate privileges. The presence of these files should be
-strictly controlled on the system.
+Executable files with the SUID permission run with the privileges of the owner of the file.
+SUID files of uncertain provenance could allow for unprivileged users to elevate privileges.
+The presence of these files should be strictly controlled on the system.
 
 [ident]:
 CCE-80817-0

New content has different text for rule 'xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_world_writable'.
--- xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_world_writable
+++ xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_world_writable
@@ -3,13 +3,17 @@
 Ensure No World-Writable Files Exist
 
 [description]:
-It is generally a good idea to remove global (other) write
-access to a file when it is discovered. However, check with
-documentation for specific applications before making changes.
-Also, monitor for recurring world-writable files, as these may be
-symptoms of a misconfigured application or user account. Finally,
-this applies to real files and not virtual files that are a part of
-pseudo file systems such as sysfs or procfs.
+It is generally a good idea to remove global (other) write access to a file when it is
+discovered. However, check with documentation for specific applications before making changes.
+Also, monitor for recurring world-writable files, as these may be symptoms of a misconfigured
+application or user account. Finally, this applies to real files and not virtual files that
+are a part of pseudo file systems such as sysfs or procfs.
+
+[warning]:
+This rule can take a long time to perform the check and might consume a considerable
+amount of resources depending on the number of files present on the system. It is not a
+problem in most cases, but especially systems with a large number of files can be affected.
+See https://access.redhat.com/articles/6999111.
 
 [reference]:
 BP28(R40)
@@ -174,11 +178,9 @@
 6.1.11
 
 [rationale]:
-Data in world-writable files can be modified by any
-user on the system. In almost all circumstances, files can be
-configured using a combination of user and group permissions to
-support whatever legitimate access is needed without the risk
-caused by world-writable files.
+Data in world-writable files can be modified by any user on the system. In almost all
+circumstances, files can be configured using a combination of user and group permissions to
+support whatever legitimate access is needed without the risk caused by world-writable files.
 
 [ident]:
 CCE-80818-8

bash remediation for rule 'xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_world_writable' differs.
--- xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_world_writable
+++ xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_world_writable
@@ -1,2 +1,11 @@
 
-find / -xdev -type f -perm -002 -exec chmod o-w {} \;
+FILTER_NODEV=$(awk '/nodev/ { print $2 }' /proc/filesystems | paste -sd,)
+PARTITIONS=$(findmnt -n -l -k -it $FILTER_NODEV | awk '{ print $1 }')
+for PARTITION in $PARTITIONS; do
+  find "${PARTITION}" -xdev -type f -perm -002 -exec chmod o-w {} \; 2>/dev/null
+done
+
+# Ensure /tmp is also fixed whem tmpfs is used.
+if grep "^tmpfs /tmp" /proc/mounts; then
+  find /tmp -xdev -type f -perm -002 -exec chmod o-w {} \; 2>/dev/null
+fi

New content has different text for rule 'xccdf_org.ssgproject.content_rule_file_permissions_ungroupowned'.
--- xccdf_org.ssgproject.content_rule_file_permissions_ungroupowned
+++ xccdf_org.ssgproject.content_rule_file_permissions_ungroupowned
@@ -3,20 +3,27 @@
 Ensure All Files Are Owned by a Group
 
 [description]:
-If any files are not owned by a group, then the
-cause of their lack of group-ownership should be investigated.
-Following this, the files should be deleted or assigned to an
-appropriate group. The following command will discover and print
-any files on local partitions which do not belong to a valid group:
-$ df --local -P | awk '{if (NR!=1) print $6}' | sudo xargs -I '{}' find '{}' -xdev -nogroup
-To search all filesystems on a system including network mounted
-filesystems the following command can be run manually for each partition:
-$ sudo find PARTITION -xdev -nogroup
+If any file is not group-owned by a group present in /etc/group, the cause of the lack of
+group-ownership must be investigated. Following this, those files should be deleted or
+assigned to an appropriate group.
+
+Locate the mount points related to local devices by the following command:
+$ findmnt -n -l -k -it $(awk '/nodev/ { print $2 }' /proc/filesystems | paste -sd,)
+
+For all mount points listed by the previous command, it is necessary to search for files which
+do not belong to a valid group using the following command:
+$ sudo find MOUNTPOINT -xdev -nogroup 2>/dev/null
 
 [warning]:
-This rule only considers local groups.
+This rule only considers local groups as valid groups.
 If you have your groups defined outside /etc/group, the rule won't consider those.
 
+[warning]:
+This rule can take a long time to perform the check and might consume a considerable
+amount of resources depending on the number of files present on the system. It is not a
+problem in most cases, but especially systems with a large number of files can be affected.
+See https://access.redhat.com/articles/6999111.
+
 [reference]:
 BP28(R55)
 
@@ -348,13 +355,11 @@
 SV-230327r627750_rule
 
 [rationale]:
-Unowned files do not directly imply a security problem, but they are generally
-a sign that something is amiss. They may
-be caused by an intruder, by incorrect software installation or
-draft software removal, or by failure to remove all files belonging
-to a deleted account. The files should be repaired so they
-will not cause problems when accounts are created in the future,
-and the cause should be discovered and addressed.
+Unowned files do not directly imply a security problem, but they are generally a sign that
+something is amiss. They may be caused by an intruder, by incorrect software installation or
+draft software removal, or by failure to remove all files belonging to a deleted account, or
+other similar cases. The files should be repaired so they will not cause problems when
+accounts are created in the future, and the cause should be discovered and addressed.
 
 [ident]:
 CCE-83497-8

OCIL for rule 'xccdf_org.ssgproject.content_rule_file_permissions_ungroupowned' differs.
--- ocil:ssg-file_permissions_ungroupowned_ocil:questionnaire:1
+++ ocil:ssg-file_permissions_ungroupowned_ocil:questionnaire:1
@@ -1,9 +1,11 @@
-The following command will discover and print any
-files on local partitions which do not belong to a valid group.
-$ df --local -P | awk '{if (NR!=1) print $6}' | sudo xargs -I '{}' find '{}' -xdev -nogroup
+The following command will locate the mount points related to local devices:
+$ findmnt -n -l -k -it $(awk '/nodev/ { print $2 }' /proc/filesystems | paste -sd,)
 
-Either remove all files and directories from the system that do not have a valid group,
-or assign a valid group with the chgrp command:
-$ sudo chgrp group file
+The following command will show files which do not belong to a valid group:
+$ sudo find MOUNTPOINT -xdev -nogroup 2>/dev/null
+
+Replace MOUNTPOINT by the mount points listed by the fist command.
+
+No files without a valid group should be located.
       Is it the case that there is output?
       
New content has different text for rule 'xccdf_org.ssgproject.content_rule_no_files_unowned_by_user'.
--- xccdf_org.ssgproject.content_rule_no_files_unowned_by_user
+++ xccdf_org.ssgproject.content_rule_no_files_unowned_by_user
@@ -3,15 +3,15 @@
 Ensure All Files Are Owned by a User
 
 [description]:
-If any files are not owned by a user, then the
-cause of their lack of ownership should be investigated.
-Following this, the files should be deleted or assigned to an
-appropriate user. The following command will discover and print
-any files on local partitions which do not belong to a valid user:
-$ df --local -P | awk {'if (NR!=1) print $6'} | sudo xargs -I '{}' find '{}' -xdev -nouser
-To search all filesystems on a system including network mounted
-filesystems the following command can be run manually for each partition:
-$ sudo find PARTITION -xdev -nouser
+If any files are not owned by a user, then the cause of their lack of ownership should be
+investigated. Following this, the files should be deleted or assigned to an appropriate user.
+
+Locate the mount points related to local devices by the following command:
+$ findmnt -n -l -k -it $(awk '/nodev/ { print $2 }' /proc/filesystems | paste -sd,)
+
+For all mount points listed by the previous command, it is necessary to search for files which
+do not belong to a valid user using the following command:
+$ sudo find MOUNTPOINT -xdev -nouser 2>/dev/null
 
 [warning]:
 For this rule to evaluate centralized user accounts, getent must be working properly
@@ -20,8 +20,10 @@
 in your organization's domain to return a complete list of users
 
 [warning]:
-Enabling this rule will result in slower scan times depending on the size of your organization
-and number of centralized users.
+This rule can take a long time to perform the check and might consume a considerable
+amount of resources depending on the number of files present on the system. It is not a
+problem in most cases, but especially systems with a large number of files can be affected.
+See https://access.redhat.com/articles/6999111.
 
 [reference]:
 BP28(R55)
@@ -363,13 +365,11 @@
 SV-230326r627750_rule
 
 [rationale]:
-Unowned files do not directly imply a security problem, but they are generally
-a sign that something is amiss. They may
-be caused by an intruder, by incorrect software installation or
-draft software removal, or by failure to remove all files belonging
-to a deleted account. The files should be repaired so they
-will not cause problems when accounts are created in the future,
-and the cause should be discovered and addressed.
+Unowned files do not directly imply a security problem, but they are generally a sign that
+something is amiss. They may be caused by an intruder, by incorrect software installation or
+draft software removal, or by failure to remove all files belonging to a deleted account, or
+other similar cases. The files should be repaired so they will not cause problems when
+accounts are created in the future, and the cause should be discovered and addressed.
 
 [ident]:
 CCE-83499-4

OVAL for rule 'xccdf_org.ssgproject.content_rule_no_files_unowned_by_user' differs.
--- oval:ssg-no_files_unowned_by_user:def:1
+++ oval:ssg-no_files_unowned_by_user:def:1
@@ -1,2 +1,2 @@
 criteria AND
-criterion oval:ssg-no_files_unowned_by_user_test:tst:1
+criterion oval:ssg-test_no_files_unowned_by_user:tst:1

OCIL for rule 'xccdf_org.ssgproject.content_rule_no_files_unowned_by_user' differs.
--- ocil:ssg-no_files_unowned_by_user_ocil:questionnaire:1
+++ ocil:ssg-no_files_unowned_by_user_ocil:questionnaire:1
@@ -1,10 +1,11 @@
-The following command will discover and print any
-files on local partitions which do not belong to a valid user.
-$ df --local -P | awk {'if (NR!=1) print $6'} | sudo xargs -I '{}' find '{}' -xdev -nouser
+The following command will locate the mount points related to local devices:
+$ findmnt -n -l -k -it $(awk '/nodev/ { print $2 }' /proc/filesystems | paste -sd,)
 
-Either remove all files and directories from the system that do not have a
-valid user, or assign a valid user to all unowned files and directories on
-the system with the chown command:
-$ sudo chown user file
+The following command will show files which do not belong to a valid user:
+$ sudo find MOUNTPOINT -xdev -nouser 2>/dev/null
+
+Replace MOUNTPOINT by the mount points listed by the fist command.
+
+No files without a valid user should be located.
       Is it the case that files exist that are not owned by a valid user?
       

@jan-cerny jan-cerny self-assigned this Nov 30, 2023
@marcusburghardt marcusburghardt marked this pull request as ready for review November 30, 2023 15:04
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Nov 30, 2023
@jan-cerny
Copy link
Collaborator

@marcusburghardt This is getting too large. Please split the changes to multiple small PRs.

@marcusburghardt
Copy link
Member Author

@marcusburghardt This is getting too large. Please split the changes to multiple small PRs.

@jan-cerny I don't plan to introduce more changes in this PR. All the planned rules were already reviewed. I tried to be as much conservative as possible with Remediation and Tests, focusing basically on OVAL. I plan to introduce some small / non-critical improvements in remediation and tests in a separate PR, but this doesn't block this one.

@marcusburghardt
Copy link
Member Author

automatus tests are expected to fail because containers doesn't have file systems mounted from /dev devices.

@Mab879
Copy link
Member

Mab879 commented Dec 1, 2023

/packit retest-failed

@marcusburghardt
Copy link
Member Author

I am moving this PR to Draft. The intention is to split some commits in separate PRs to make it easier to be reviewed.

The changes were only about style. No logic was changed.
Also removed outdated comments.
Optimized the file probe by ignoring pseudo and remote file systems.
This will bring performance improvements and also likely reduce
the memory consumed by the scanner tool. The test was not changed but
only the number of objects to be tested.
This macro is expected to be used in many rules that need to check file
properties in the whole system. The nature of these rules is to consume
a lot of resources. This macro optimize the efficiency of these rules by
avoiding remote file systems as well as special file systems, like
/proc, /sys, etc.
Updated the OVAL to use the create_local_mount_points_list macro.
The rule description was reviewed and updated to be more aligned to the
OVAL check.
Warning about high consume of system resources in some scenarios.
Included warning about high consume of system resources in some scenarios.
Also aligned the rule description with the project Style Guide.
Good readability can reveal some improvement opportunities.
The OVAL was specifying the same file_object twice while the second
object had just an extra filter. The OVAL test was changed to use
variables which are constructed based on already collected objects.
Updated the OVAL to use the create_local_mount_points_list macro.
This commit completes the improvements in
file_permissions_unauthorized_suid rule. To have some idea of the gains,
in a testing VM was created about 500.000 files and executed the OVAL
check before and after the improvements. The assessment time was reduced
from 2:25 to 1:45.
Also include warning about high consume of system resources in some
scenarios.
Adopted the create_local_mount_points_list macro.
Also aligned the rule logic to the file_permissions_unauthorized_suid
rule.
Also include warning about high consume of system resources in some
scenarios.
Adopted the create_local_mount_points_list macro.
Removed unnecessary filters already considered by the macro.
Aligned the Bash remediation to the OVAL check.
Also include warning about high consume of system resources in some
scenarios.
Adopted the create_local_mount_points_list macro.
Simplified the test logic by removing the "negate" attribute.
Also include warning about high consume of system resources in some
scenarios.
Adopted the create_local_mount_points_list macro.
Simplified the test logic by removing the "negate" attribute.
Improved readability.
Also update warning about high consume of system resources in some
scenarios.
Adopted the create_local_mount_points_list macro.
Improved the OVAL readability.
@marcusburghardt marcusburghardt marked this pull request as ready for review December 4, 2023 10:27
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Dec 4, 2023
@marcusburghardt
Copy link
Member Author

Moved the last commits related to rpm_verify_... rules to separate PRs.
Rebased and moved this PR back to "ready for review".

Copy link

codeclimate bot commented Dec 4, 2023

Code Climate has analyzed commit 1269df0 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 58.5%.

View more on Code Climate.

@jan-cerny
Copy link
Collaborator

The Automatus's fails don't happen if we run the tests locally with a virtual machine back end. Here are results with a RHEL 9 VM:

jcerny@fedora ~/work/git/scap-security-guide (pr/11319) $ python3 tests/automatus.py rule --libvirt qemu:///system ssgts_rhel9  file_permissions_ungroupowned,dir_perms_world_writable_system_owned,no_files_unowned_by_user,file_permissions_unauthorized_world_writable,file_permissions_unauthorized_sgid,dir_perms_world_writable_sticky_bits,file_permissions_unauthorized_suid
Setting console output to log level INFO
INFO - The base image option has not been specified, choosing libvirt-based test environment.
INFO - Logging into /home/jcerny/work/git/scap-security-guide/logs/rule-custom-2023-12-04-1542/test_suite.log
WARNING - Rule 'xccdf_org.ssgproject.content_rule_dir_perms_world_writable_system_owned' isn't present in benchmark 'xccdf_org.ssgproject.content_benchmark_RHEL-9' in '/tmp/ssgts-ds-wut33qwl'
INFO - xccdf_org.ssgproject.content_rule_dir_perms_world_writable_sticky_bits
INFO - Script tmp_no_sticky.fail.sh using profile (all) OK
INFO - Script correct.pass.sh using profile (all) OK
INFO - xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_sgid
INFO - Script no_unpackaged_sgid.pass.sh using profile (all) OK
INFO - Script unpackaged_sgid.fail.sh using profile (all) OK
INFO - xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_suid
INFO - Script no_unpackaged_suid.pass.sh using profile (all) OK
INFO - Script unpackaged_suid.fail.sh using profile (all) OK
INFO - xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_world_writable
INFO - Script no_world_writable.pass.sh using profile (all) OK
INFO - Script world_writable.fail.sh using profile (all) OK
INFO - xccdf_org.ssgproject.content_rule_file_permissions_ungroupowned
INFO - Script all_owned.pass.sh using profile (all) OK
INFO - Script unowned_file.fail.sh using profile (all) OK
INFO - xccdf_org.ssgproject.content_rule_no_files_unowned_by_user
INFO - Script all_owned.pass.sh using profile (all) OK
INFO - Script unowned_file.fail.sh using profile (all) OK
jcerny@fedora ~/work/git/scap-security-guide (pr/11319) $ python3 tests/automatus.py rule --libvirt qemu:///system ssgts_rhel9  --remediate-using ansible file_permissions_ungroupowned,dir_perms_world_writable_system_owned,no_files_unowned_by_user,file_permissions_unauthorized_world_writable,file_permissions_unauthorized_sgid,dir_perms_world_writable_sticky_bits,file_permissions_unauthorized_suid
Setting console output to log level INFO
INFO - The base image option has not been specified, choosing libvirt-based test environment.
INFO - Logging into /home/jcerny/work/git/scap-security-guide/logs/rule-custom-2023-12-04-1546/test_suite.log
WARNING - Rule 'xccdf_org.ssgproject.content_rule_dir_perms_world_writable_system_owned' isn't present in benchmark 'xccdf_org.ssgproject.content_benchmark_RHEL-9' in '/tmp/ssgts-ds-sau3da6s'
INFO - xccdf_org.ssgproject.content_rule_dir_perms_world_writable_sticky_bits
INFO - Script tmp_no_sticky.fail.sh using profile (all) OK
INFO - Script correct.pass.sh using profile (all) OK
INFO - xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_sgid
INFO - Script no_unpackaged_sgid.pass.sh using profile (all) OK
INFO - Script unpackaged_sgid.fail.sh using profile (all) OK
INFO - xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_suid
INFO - Script no_unpackaged_suid.pass.sh using profile (all) OK
INFO - Script unpackaged_suid.fail.sh using profile (all) OK
INFO - xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_world_writable
INFO - Script no_world_writable.pass.sh using profile (all) OK
INFO - Script world_writable.fail.sh using profile (all) OK
WARNING - No remediation is available for rule 'xccdf_org.ssgproject.content_rule_file_permissions_unauthorized_world_writable'.
INFO - xccdf_org.ssgproject.content_rule_file_permissions_ungroupowned
INFO - Script all_owned.pass.sh using profile (all) OK
INFO - Script unowned_file.fail.sh using profile (all) OK
INFO - xccdf_org.ssgproject.content_rule_no_files_unowned_by_user
INFO - Script all_owned.pass.sh using profile (all) OK
INFO - Script unowned_file.fail.sh using profile (all) OK

@jan-cerny jan-cerny merged commit e389774 into ComplianceAsCode:master Dec 5, 2023
34 of 38 checks passed
@jan-cerny jan-cerny added the OVAL OVAL update. Related to the systems assessments. label Dec 5, 2023
@marcusburghardt marcusburghardt deleted the probe_file_system_rules branch December 5, 2023 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OVAL OVAL update. Related to the systems assessments. refactoring Improvement which, once completed, will enable the project to progress faster. Update Rule Issues or pull requests related to Rules updates.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants