-
Notifications
You must be signed in to change notification settings - Fork 685
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
Create OVAL macro to consistently identify Interactive Users #10215
Create OVAL macro to consistently identify Interactive Users #10215
Conversation
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_accounts_user_dot_group_ownership'.
--- xccdf_org.ssgproject.content_rule_accounts_user_dot_group_ownership
+++ xccdf_org.ssgproject.content_rule_accounts_user_dot_group_ownership
@@ -1,6 +1,6 @@
[title]:
-User Initialization Files Must Be Group-Owned By The Primary User
+User Initialization Files Must Be Group-Owned By The Primary Group
[description]:
Change the group owner of interactive users files to the group found
bash remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_user_dot_group_ownership' differs.
--- xccdf_org.ssgproject.content_rule_accounts_user_dot_group_ownership
+++ xccdf_org.ssgproject.content_rule_accounts_user_dot_group_ownership
@@ -1,2 +1,2 @@
-awk -F':' '{ if ($3 >= 1000 && $3 != 65534) system("chgrp -f " $3" "$6"/.[^\.]?*") }' /etc/passwd
+awk -F':' '{ if ($3 >= 1000 && $3 != 65534) system("chgrp -f " $4" "$6"/.[^\.]?*") }' /etc/passwd
ansible remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_user_dot_group_ownership' differs.
--- xccdf_org.ssgproject.content_rule_accounts_user_dot_group_ownership
+++ xccdf_org.ssgproject.content_rule_accounts_user_dot_group_ownership
@@ -1,7 +1,7 @@
- name: Ensure interactive local users are the group-owners of their respective initialization
files
ansible.builtin.command:
- cmd: awk -F':' '{ if ($3 >= 1000 && $3 != 65534) system("chgrp -f " $3" "$6"/.[^\.]?*")
+ cmd: awk -F':' '{ if ($3 >= 1000 && $3 != 65534) system("chgrp -f " $4" "$6"/.[^\.]?*")
}' /etc/passwd
tags:
- accounts_user_dot_group_ownership
New content has different text for rule 'xccdf_org.ssgproject.content_rule_accounts_users_home_files_groupownership'.
--- xccdf_org.ssgproject.content_rule_accounts_users_home_files_groupownership
+++ xccdf_org.ssgproject.content_rule_accounts_users_home_files_groupownership
@@ -1,6 +1,6 @@
[title]:
-All User Files and Directories In The Home Directory Must Be Group-Owned By The Primary User
+All User Files and Directories In The Home Directory Must Be Group-Owned By The Primary Group
[description]:
Change the group of a local interactive users files and directories to a
bash remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_users_home_files_groupownership' differs.
--- xccdf_org.ssgproject.content_rule_accounts_users_home_files_groupownership
+++ xccdf_org.ssgproject.content_rule_accounts_users_home_files_groupownership
@@ -1,5 +1,5 @@
-for user in $(awk -F':' '{ if ($4 >= 1000 && $4 != 65534) print $1 }' /etc/passwd); do
+for user in $(awk -F':' '{ if ($3 >= 1000 && $3 != 65534) print $1 }' /etc/passwd); do
home_dir=$(getent passwd $user | cut -d: -f6)
group=$(getent passwd $user | cut -d: -f4)
# Only update the group-ownership when necessary. This will avoid changing the inode timestamp
ansible remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_users_home_files_groupownership' differs.
--- xccdf_org.ssgproject.content_rule_accounts_users_home_files_groupownership
+++ xccdf_org.ssgproject.content_rule_accounts_users_home_files_groupownership
@@ -25,15 +25,15 @@
- no_reboot_needed
- restrict_strategy
-- name: Test for existence home directories to avoid creating them, but only fixing
+- name: Test for existence of home directories to avoid creating them, but only fixing
ownership
ansible.builtin.stat:
path: '{{ item.value[4] }}'
register: path_exists
loop: '{{ local_users }}'
when:
- - item.value[2]|int >= 1000
- - item.value[2]|int != 65534
+ - item.value[1]|int >= 1000
+ - item.value[1]|int != 65534
tags:
- CCE-86534-5
- DISA-STIG-RHEL-08-010741
ansible remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_users_home_files_ownership' differs.
--- xccdf_org.ssgproject.content_rule_accounts_users_home_files_ownership
+++ xccdf_org.ssgproject.content_rule_accounts_users_home_files_ownership
@@ -21,7 +21,7 @@
- no_reboot_needed
- restrict_strategy
-- name: Test for existence home directories to avoid creating them, but only fixing
+- name: Test for existence of home directories to avoid creating them, but only fixing
ownership
ansible.builtin.stat:
path: '{{ item.value[4] }}'
New content has different text for rule 'xccdf_org.ssgproject.content_rule_file_groupownership_home_directories'.
--- xccdf_org.ssgproject.content_rule_file_groupownership_home_directories
+++ xccdf_org.ssgproject.content_rule_file_groupownership_home_directories
@@ -1,6 +1,6 @@
[title]:
-All Interactive User Home Directories Must Be Group-Owned By The Primary User
+All Interactive User Home Directories Must Be Group-Owned By The Primary Group
[description]:
Change the group owner of interactive users home directory to the
bash remediation for rule 'xccdf_org.ssgproject.content_rule_file_groupownership_home_directories' differs.
--- xccdf_org.ssgproject.content_rule_file_groupownership_home_directories
+++ xccdf_org.ssgproject.content_rule_file_groupownership_home_directories
@@ -1,2 +1,2 @@
-awk -F':' '{ if ($4 >= 1000 && $4 != 65534) system("chgrp -f " $4" "$6) }' /etc/passwd
+awk -F':' '{ if ($3 >= 1000 && $3 != 65534) system("chgrp -f " $4" "$6) }' /etc/passwd
ansible remediation for rule 'xccdf_org.ssgproject.content_rule_file_groupownership_home_directories' differs.
--- xccdf_org.ssgproject.content_rule_file_groupownership_home_directories
+++ xccdf_org.ssgproject.content_rule_file_groupownership_home_directories
@@ -32,8 +32,8 @@
register: path_exists
loop: '{{ local_users }}'
when:
- - item.value[2]|int >= 1000
- - item.value[2]|int != 65534
+ - item.value[1]|int >= 1000
+ - item.value[1]|int != 65534
tags:
- CCE-83434-1
- DISA-STIG-RHEL-08-010740
ansible remediation for rule 'xccdf_org.ssgproject.content_rule_file_ownership_home_directories' differs.
--- xccdf_org.ssgproject.content_rule_file_ownership_home_directories
+++ xccdf_org.ssgproject.content_rule_file_ownership_home_directories
@@ -23,7 +23,7 @@
- no_reboot_needed
- restrict_strategy
-- name: Test for existence home directories to avoid creating them, but only fixing
+- name: Test for existence of home directories to avoid creating them, but only fixing
ownership
ansible.builtin.stat:
path: '{{ item.value[4] }}' |
Automatus CS8 is failing with this message: The |
cc4dc78
to
b697b72
Compare
Many rules deal with interactive users in a pretty similar way. It was created a generic macro to be used in these rules in order to keep the approach consistent among the rules and the maintainability easier.
There is a new OVAL macro to filter interactive users in a consistent way among the relevant rules. It was also included a new test scenario to test the exclusion of users using /sbin/nologin shell.
As a consequence of this improvement, a minor fix was also included in this commit. The filter for interactive users was checking the "unix:user_id" field but using "{{{ gid_min }}}" as pattern. The intention of the object was to collect interactive users and not interactive groups.
Also improved the existing test scenarios to ensure the expected permissions are set in the test environment. Comparing to chmod alone find command can better manage hidden files.
Like the accounts_users_home_files_groupownership rule, this was also improved with the macro. Before, the interactive users were filtered by gids while uids is more appropriated.
This commit also created some minor updates in the test scenarios in order to improve the readability.
This rule is deprecated in favor of file_permissions_home_directories. However, its OVAL was also updated to keep the approach of collecting interactive users aligned in all related rules.
b697b72
to
dec8988
Compare
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea very much
- item.value[1]|int >= {{{ uid_min }}} | ||
- item.value[1]|int != {{{ nobody_uid }}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit pick: it might be useful for future readers to add a comment about type of data at index 1 (and at index 4 above) in the array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point. I wrote this some time ago and when working on this PR, honestly I had to confirm the position of the fields myself. : )) I also intend to open another PR to improve these remediations. But for now, these comments will already come in handy. I will include.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -2,7 +2,7 @@ documentation_complete: true | |||
|
|||
prodtype: ol7,ol8,rhel7,rhel8,rhv4,sle12,sle15,ubuntu2004,ubuntu2204 | |||
|
|||
title: 'User Initialization Files Must Be Group-Owned By The Primary User' | |||
title: 'User Initialization Files Must Be Group-Owned By The Primary Group' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
item.value[1]|int is used in some Ansible tasks when looping through a dictionary created from /etc/passwd file. It could not be intuitive the index and values of the resulting dictionary. It is basically formed by the fields in /etc/passwd, where the first field is the key and the next 6 fields are in the list of values. Each file line is a item in the dictionary.
Code Climate has analyzed commit 97d023a 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 51.7% (0.0% change). View more on Code Climate. |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed the code and I have executed some of the test scenarios
[jcerny@thinkpad scap-security-guide{pr/10215}]$ python3 tests/automatus.py rule --libvirt qemu:///system ssgts_rhel9 accounts_umask_interactive_users
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-02-20-1817/test_suite.log
INFO - xccdf_org.ssgproject.content_rule_accounts_umask_interactive_users
INFO - Script bash_history_ignored.pass.sh using profile (all) OK
INFO - Script hidden_folder_ignored.pass.sh using profile (all) OK
INFO - Script home_dirs_all_absent.pass.sh using profile (all) OK
INFO - Script home_dirs_one_absent.pass.sh using profile (all) OK
INFO - Script interactive_users_absent.pass.sh using profile (all) OK
INFO - Script no_dot_file_ignored.pass.sh using profile (all) OK
INFO - Script umask_defined.fail.sh using profile (all) OK
INFO - Script interactive_user_nologin_ignored.pass.sh using profile (all) OK
[jcerny@thinkpad scap-security-guide{pr/10215}]$ python3 tests/automatus.py rule --libvirt qemu:///system ssgts_rhel9 --remediate-using ansible accounts_umask_interactive_users
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-02-20-1821/test_suite.log
INFO - xccdf_org.ssgproject.content_rule_accounts_umask_interactive_users
INFO - Script bash_history_ignored.pass.sh using profile (all) OK
INFO - Script hidden_folder_ignored.pass.sh using profile (all) OK
INFO - Script home_dirs_all_absent.pass.sh using profile (all) OK
INFO - Script home_dirs_one_absent.pass.sh using profile (all) OK
INFO - Script interactive_users_absent.pass.sh using profile (all) OK
INFO - Script no_dot_file_ignored.pass.sh using profile (all) OK
INFO - Script umask_defined.fail.sh using profile (all) OK
INFO - Script interactive_user_nologin_ignored.pass.sh using profile (all) OK
[jcerny@thinkpad scap-security-guide{pr/10215}]$ python3 tests/automatus.py rule --libvirt qemu:///system ssgts_rhel9 file_permissions_home_directories
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-02-20-1825/test_suite.log
INFO - xccdf_org.ssgproject.content_rule_file_permissions_home_directories
INFO - Script acceptable_permission.pass.sh using profile (all) OK
INFO - Script expected_permissions.pass.sh using profile (all) OK
INFO - Script home_dirs_absent.pass.sh using profile (all) OK
INFO - Script interactive_users_absent.pass.sh using profile (all) OK
INFO - Script lenient_permission.fail.sh using profile (all) OK
INFO - Script special_permissions.fail.sh using profile (all) OK
INFO - Script interactive_user_nologin_ignored.pass.sh using profile (all) OK
[jcerny@thinkpad scap-security-guide{pr/10215}]$ python3 tests/automatus.py rule --libvirt qemu:///system ssgts_rhel9 --remediate-using ansible file_permissions_home_directories
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-02-20-1828/test_suite.log
INFO - xccdf_org.ssgproject.content_rule_file_permissions_home_directories
INFO - Script acceptable_permission.pass.sh using profile (all) OK
INFO - Script expected_permissions.pass.sh using profile (all) OK
INFO - Script home_dirs_absent.pass.sh using profile (all) OK
INFO - Script interactive_users_absent.pass.sh using profile (all) OK
INFO - Script lenient_permission.fail.sh using profile (all) OK
INFO - Script special_permissions.fail.sh using profile (all) OK
INFO - Script interactive_user_nologin_ignored.pass.sh using profile (all) OK
The error of automatus is expected because some of the rules aren't present in the RHEL 9 data stream |
Description:
Currently, there are 14 rules checking for requirements related to "interactive users":
For all these rules, the approach to identify interactive users is exactly the same.
However, each rule had its own OVAL check.
Besides the code duplication, during the review of these rules it was also noticed other minor issues:
nobody
user while others not.These issues prove how difficult is to keep consistency among these rules.
The maintenance is also expensive considering 14 rules.
Recently, after some discussions in CIS Community, after checking related requirements in existing benchmarks, like STIG and CIS, and also after offline conversations with experts, it was agreed that users configured with
/sbin/nologin
shell should also be ignored in the "Interactive Users" list.This change inevitably would demand OVAL update in the 14 mentioned rules.
It was a great opportunity to improve the consistency of these rules, reduce maintenance cost and make it easier to introduce new similar rules.
So, in this PR I have created a OVAL macro to properly and consistently identify "Interactive Users" and provide a OVAL object to be used in the rules. Once the macro was created, the OVAL for these all 14 rules was updated to use the new macro and a new test scenario was included to cover the case of non-system accounts that has
/sbin/nologin
shell.The minor issues mentioned before were also fixed.
Rationale:
Review Hints:
Although I identified improvement opportunities also in remediation, I decided to limit the scope of this PR to OVAL updates and minor fixes in remediation. It makes the PR easier to review and test since the changes are more controlled.
For review, check commit by commit in the order they are presented.
For test, I would recommend to use a loop for testing these 14 rules using
automatus
. Here is a simple example that can be used as reference:As a follow-up, something similar should be done with the remediation.