Refactor ensure_pam_wheel_group_empty rule #11192
Conversation
The previous logic was collecting all groups without members and then comparing each group with a variable. In the report, when the rule failed it was not clear the reason. The logic is broken in two tests now. The first will search specifically for the desired group to make sure the group exists. The second test will use the already collected element and check the line against a regex to confirm the groups has no members. The report is more transparent now and without unnecessary information. The tests also reuse the same object.
The default option in this variable used by pam_wheel.so was an empty string. However, using an empty string in pam_wheel.so group option has no effect. The result in this case would be an invalid configuration to achienve the desired goal. It was included a generic group as default value.
Removed unnecessary commands, ensured same variables on all scripts and renamed the files in alignment to their content.
The group must exist and must be empty. Reduced the number of commands to achieve these goals.
The remediation was not efficient by always removing the group even if it was already compliant. Then it was again created. This is not efficient and unnecessary changes were made. Currently there is no option in user or group Ansible modules to remove members of a group. A logic to achieve this would make the Playbook more complex than necessary. Since this rule is only about local groups and users, we can safely simplify the Playbook using lineinfile module. Ansible and Bash are now aligned.
marcusburghardt
added
Ansible
|
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_ensure_pam_wheel_group_empty'.
--- xccdf_org.ssgproject.content_rule_ensure_pam_wheel_group_empty
+++ xccdf_org.ssgproject.content_rule_ensure_pam_wheel_group_empty
@@ -1,20 +1,26 @@
[title]:
-Ensure the Group Used by pam_wheel Module Exists on System and is Empty
+Ensure the Group Used by pam_wheel.so Module Exists on System and is Empty
[description]:
-Ensure that the group 'xccdf_org.ssgproject.content_value_var_pam_wheel_group_for_su'
-referenced by the pam_wheel group parameter exists and has no
-members. This ensures that no user can run commands with altered
-privileges through the su command.
+Ensure that the group 'xccdf_org.ssgproject.content_value_var_pam_wheel_group_for_su' referenced by
+var_pam_wheel_group_for_su variable and used as value for the pam_wheel.so
+group option exists and has no members. This empty group used by
+pam_wheel.so in /etc/pam.d/su ensures that no user can run commands with
+altered privileges through the su command.
+
+[warning]:
+Note that this rule just ensures the group exists and has no members. This rule does not
+configure pam_wheel.so module. The pam_wheel.so module configuration is
+accomplished by use_pam_wheel_group_for_su rule.
[reference]:
5.3.7
[rationale]:
-The su program allows to run commands with a substitute user and
-group ID. It is commonly used to run commands as the root user. Limiting
-access to such command is considered a good security practice.
+The su program allows to run commands with a substitute user and group ID.
+It is commonly used to run commands as the root user.
+Limiting access to such command is considered a good security practice.
[ident]:
CCE-86071-8
OVAL for rule 'xccdf_org.ssgproject.content_rule_ensure_pam_wheel_group_empty' differs.
--- oval:ssg-ensure_pam_wheel_group_empty:def:1
+++ oval:ssg-ensure_pam_wheel_group_empty:def:1
@@ -1,2 +1,3 @@
criteria AND
-criterion oval:ssg-test_ensure_pam_wheel_group_exist_and_has_no_member:tst:1
+criterion oval:ssg-test_ensure_pam_wheel_group_empty_group_exists:tst:1
+criterion oval:ssg-test_ensure_pam_wheel_group_empty_has_no_members:tst:1
OCIL for rule 'xccdf_org.ssgproject.content_rule_ensure_pam_wheel_group_empty' differs.
--- ocil:ssg-ensure_pam_wheel_group_empty_ocil:questionnaire:1
+++ ocil:ssg-ensure_pam_wheel_group_empty_ocil:questionnaire:1
@@ -1,5 +1,4 @@
-Run the following command to check if the group
-exists:
+Run the following command to check if the group exists:
grep /etc/group
The output should contain the following line:
:x:
bash remediation for rule 'xccdf_org.ssgproject.content_rule_ensure_pam_wheel_group_empty' differs.
--- xccdf_org.ssgproject.content_rule_ensure_pam_wheel_group_empty
+++ xccdf_org.ssgproject.content_rule_ensure_pam_wheel_group_empty
@@ -9,12 +9,7 @@
fi
# group must be empty
-grp_memb=$(groupmems -g ${var_pam_wheel_group_for_su} -l)
-if [ -n "${grp_memb}" ]; then
- for memb in ${grp_memb}; do
- deluser ${memb} ${var_pam_wheel_group_for_su}
- done
-fi
+groupmems -g ${var_pam_wheel_group_for_su} -p
else
>&2 echo 'Remediation is not applicable, nothing was done'
ansible remediation for rule 'xccdf_org.ssgproject.content_rule_ensure_pam_wheel_group_empty' differs.
--- xccdf_org.ssgproject.content_rule_ensure_pam_wheel_group_empty
+++ xccdf_org.ssgproject.content_rule_ensure_pam_wheel_group_empty
@@ -15,24 +15,9 @@
tags:
- always
-- name: Ensure the Group Used by pam_wheel Module Exists on System and is Empty -
- Ensure group {{ var_pam_wheel_group_for_su }} is removed
- group:
- name: '{{ var_pam_wheel_group_for_su }}'
- state: absent
- when: '"pam" in ansible_facts.packages'
- tags:
- - CCE-86071-8
- - ensure_pam_wheel_group_empty
- - low_complexity
- - low_disruption
- - medium_severity
- - no_reboot_needed
- - restrict_strategy
-
-- name: Ensure the Group Used by pam_wheel Module Exists on System and is Empty -
- Ensure group {{ var_pam_wheel_group_for_su }} exist
- group:
+- name: Ensure the Group Used by pam_wheel.so Module Exists on System and is Empty
+ - Ensure {{ var_pam_wheel_group_for_su }} Group Exists
+ ansible.builtin.group:
name: '{{ var_pam_wheel_group_for_su }}'
state: present
when: '"pam" in ansible_facts.packages'
@@ -44,3 +29,20 @@
- medium_severity
- no_reboot_needed
- restrict_strategy
+
+- name: Ensure the Group Used by pam_wheel.so Module Exists on System and is Empty
+ - Ensure {{ var_pam_wheel_group_for_su }} Group is Empty
+ ansible.builtin.lineinfile:
+ path: /etc/group
+ regexp: ^({{ var_pam_wheel_group_for_su }}:[^:]+:[0-9]+:).*$
+ line: \1
+ backrefs: true
+ when: '"pam" in ansible_facts.packages'
+ tags:
+ - CCE-86071-8
+ - ensure_pam_wheel_group_empty
+ - low_complexity
+ - low_disruption
+ - medium_severity
+ - no_reboot_needed
+ - restrict_strategy
New content has different text for rule 'xccdf_org.ssgproject.content_rule_use_pam_wheel_group_for_su'.
--- xccdf_org.ssgproject.content_rule_use_pam_wheel_group_for_su
+++ xccdf_org.ssgproject.content_rule_use_pam_wheel_group_for_su
@@ -3,20 +3,22 @@
Enforce Usage of pam_wheel with Group Parameter for su Authentication
[description]:
-To ensure that only users who are members of the group set in the
-group pam_wheel parameter can run commands with altered
-privileges through the su command, make sure that the
-following line exists in the file /etc/pam.d/su:
+To ensure that only users who are members of the group set in the group option of
+pam_wheel.so module can run commands with altered privileges through the su
+command, make sure that the following line exists in the file /etc/pam.d/su:
auth required pam_wheel.so use_uid group='xccdf_org.ssgproject.content_value_var_pam_wheel_group_for_su'
+
+[warning]:
+Note that ensure_pam_wheel_group_empty rule complements this requirement by
+ensuring the referenced group exists and has no members.
[reference]:
5.3.7
[rationale]:
-The su program allows to run commands with a substitute
-user and group ID. It is commonly used to run commands as the root
-user. Limiting access to such command is considered a good security
-practice.
+The su program allows to run commands with a substitute user and group ID.
+It is commonly used to run commands as the root user.
+Limiting access to such command is considered a good security practice.
[ident]:
CCE-86064-3 |
|
Hi @teacup-on-rockingchair , I see the |
...uide/system/accounts/accounts-restrictions/root_logins/ensure_pam_wheel_group_empty/rule.yml
Outdated
Show resolved
Hide resolved
Make it clear that this rule does not configure pam_wheel.so but only ensures the group expected to be used in pam_wheel.so exists and has no members. The warning is referencing the use_pam_wheel_group_for_su rule which in fact configure pam_wheel.so group option.
Included warning mentioning the ensure_pam_wheel_group_empty complements the rule by ensuring the group exists and has no members.
marcusburghardt
force-pushed
the
ensure_pam_wheel_group_empty
branch
from
160cc5b
to
29c39ec
Compare
|
Code Climate has analyzed commit 29c39ec 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 57.0%. View more on Code Climate. |
There was a problem hiding this comment.
I like this PR very much. I have reviewed the code. I have run test scenarios locally. I have viewed all HTML reports. I can say that the warning and the layout helps a lot to understand the fail in some situations.
Unfortunately, in the test scenario ensure_pam_wheel_group_empty-no_group.fail.sh we again hit OpenSCAP/openscap#1507.
|
@marcusburghardt A quick google search hints that they might intentionally remove |
Thanks for the reference @jan-cerny . I will include a Jinja2 conditional in this Bash remediation so we can move forward. |
Thanks @marcusburghardt and @jan-cerny for the heads up. I can confirm it never worked and we enabled the bash remediation for our platform by mistake. I will open an issue to handle that in a separate context. |
Thanks for the update and for opening the #11203 @teacup-on-rockingchair . @jan-cerny , I in this case the error is not a regression but something that was already there in the past and we already have an Upstream issue to fix it (in a separate PR). I believe we can waive the error and merge the PR. |
|
@marcusburghardt I agree with your proposal. |
Description:
Refactor
ensure_pam_wheel_group_emptyrule ir order to make it simpler and more efficient.This PR includes the following improvements:
More details about the changes in commit descriptions.
Rationale:
Review Hints:
I would recommend to check the commits and their respective descriptions one by one to see the context of the changes.
Technically, executing automatus should be enough to test the rule:
Bash
./tests/automatus.py rule --libvirt qemu:///session rhel8 --datastream build/ssg-rhel8-ds.xml --dontclean --remediate-using bash ensure_pam_wheel_group_emptyAnsible
./tests/automatus.py rule --libvirt qemu:///session rhel8 --datastream build/ssg-rhel8-ds.xml --dontclean --remediate-using ansible ensure_pam_wheel_group_emptyThis is an example of how is the report after this PR:
