Fix account_password_selinux_faillock_dir rule #9381

marcusburghardt · 2022-08-22T07:35:50Z

Description:

Many issues were fixed in this rule:

The description was aligned with other similar rules when referring PAM modules
The OVAL was aligned to the style guide
Eliminated copy-and-paste issues from the OVAL and ensured proper comments
Fix bash remediation which could fail in many cases
Included test scenarios for multiple dirs
Fixed test scenario using invalid fcontext with semanage

Rationale:

Makes the rule more robust
Possibly fix an issue with packit test in CS9:
xccdf_org.ssgproject.content_rule_account_password_selinux_faillock_dir:error

Reference: https://artifacts.dev.testing-farm.io/de16abc5-0a45-4a80-8768-f8d6c85df4cc/

github-actions · 2022-08-22T07:36:53Z

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

github-actions · 2022-08-22T07:40:40Z

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

Click here to see the full diff

OVAL definition oval:ssg-account_password_selinux_faillock_dir:def:1 differs:
--- old datastream
+++ new datastream
- criterion oval:ssg-test_selinux_faillock_dir:tst:1
+ criterion oval:ssg-test_account_password_selinux_faillock_dir:tst:1
OCIL for rule 'xccdf_org.ssgproject.content_rule_account_password_selinux_faillock_dir' differs:
--- old datastream
+++ new datastream
@@ -1,4 +1,4 @@
-If the system does not have SELinux enabled and enforcing a targeted policy, or if the pam_faillock module is not configured for use, this requirement is not applicable.
+If the system does not have SELinux enabled and enforcing a targeted policy, or if the pam_faillock.so module is not configured for use, this requirement is not applicable.
 
 Verify the location of the non-default tally directory for the pam_faillock module with the following command:
 

bash remediation for rule 'xccdf_org.ssgproject.content_rule_account_password_selinux_faillock_dir' differs:
--- old datastream
+++ new datastream
@@ -3,17 +3,18 @@
 
 #!/bin/bash
 
-FAILLOCK_DIR=$(grep -oP "^\s*(?:auth.*pam_faillock.so.*)?dir\s*=\s*(\S+)" \
- /etc/security/faillock.conf \
- /etc/pam.d/system-auth \
- /etc/pam.d/password-auth \
- | sed -r 's/.*=\s*(\S+)/\1/')
+FAILLOCK_CONF_FILES="/etc/security/faillock.conf /etc/pam.d/system-auth /etc/pam.d/password-auth"
 
-mkdir -p $FAILLOCK_DIR
-
-semanage fcontext -a -t faillog_t "$FAILLOCK_DIR(/.*)?"
-
-restorecon -R -v "$FAILLOCK_DIR"
+for dir in $(grep -oP "^\s*(?:auth.*pam_faillock.so.*)?dir\s*=\s*(\S+)" $FAILLOCK_CONF_FILES \
+ | sed -r 's/.*=\s*(\S+)/\1/'); do
+ if ! semanage fcontext -a -t faillog_t "$dir(/.*)?"; then
+ semanage fcontext -m -t faillog_t "$dir(/.*)?"
+ fi
+ if [ ! -e $dir ]; then
+ mkdir -p $dir
+ fi
+ restorecon -R -v $dir
+done
 
 else
 >&2 echo 'Remediation is not applicable, nothing was done'

Keep the PAM module name aligned to other related rules and fixed some minor typos and issues in the text.

The bash remediation was not prepared for a situation where more than one directory is defined, for example, in faillock.conf and in PAM files. In situations like this, the remediation would fail. Also, the remediation was not considering an existing configuration for SELinux context. For example, if the directory was already defined via semanage, an error is returned if tried to insert it again. In this case, the SELinux context should be updated with appropriate command.

It is not acceptable to inform any value for fcontext in semanage command. It is necessry to inform a valid/existing fcontext to avoid errors. This was fixed by using the "tmp_t" context for the context of these test scenarios. Also, extra spaces were removed.

marcusburghardt · 2022-08-22T08:11:23Z

It is expected the remediation fails in CI tests because the SELinux may be disabled in these containers.

jan-cerny · 2022-08-22T11:51:49Z

...ocking_out_password_attempts/account_password_selinux_faillock_dir/tests/wrong_value.fail.sh

-echo "dir=/var/log/faillock" >  /etc/security/faillock.conf
+echo "dir=/var/log/faillock" > /etc/security/faillock.conf

 mkdir /var/log/faillock
-
-semanage fcontext -a -t dummy_t "/var/log/faillock(/.*)?"
-
+semanage fcontext -a -t tmp_t "/var/log/faillock(/.*)?"
 restorecon -R -v "/var/log/faillock"
-
-


We usually prefer to not check in changes that are only white space or formatting changes, it adds noise and the reviewer needs to check if the whitespace change is the only change that is happening.

While I agree not to exclusively remove white spaces in large or complex PRs, I advocate that we should remove even minor issues in files we are already editing. It is annoying, at least for me, to keep unnecessary spaces in a file that I am already modifying, especially if only short lines are affected. It's also very unlikely that a new PR will be created in the future just to remove unnecessary spaces, tending to keep those extra spaces forever if not removed when we have the opportunity. So, I use the good sense when I believe this will make the file quality better without significantly impact in the review. In this specific file it was changed the fcontext. And in the other two test scenario scripts the extra space was also removed to keep them aligned.

You don't have to remove them from this PR but keep it in mind for the future.

jan-cerny · 2022-08-22T12:19:19Z

...ounts-pam/locking_out_password_attempts/account_password_selinux_faillock_dir/bash/shared.sh

+
+for dir in $(grep -oP "^\s*(?:auth.*pam_faillock.so.*)?dir\s*=\s*(\S+)" $FAILLOCK_CONF_FILES \
+            | sed -r 's/.*=\s*(\S+)/\1/'); do
+    if ! `semanage fcontext -a -t faillog_t "$dir(/.*)?"`; then


Why is this command inside backticks?

Thanks for asking. Indeed, it is not necessary to use command substitution here as it is.

codeclimate · 2022-08-22T16:37:16Z

Code Climate has analyzed commit 2961247 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 42.6% (-0.1% change).

View more on Code Climate.

openshift-ci · 2022-08-22T17:19:45Z

@marcusburghardt: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-rhcos4-high	`2961247`	link	true	`/test e2e-aws-rhcos4-high`
ci/prow/e2e-aws-rhcos4-moderate	`2961247`	link	true	`/test e2e-aws-rhcos4-moderate`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

jan-cerny

[jcerny@thinkpad scap-security-guide{pr/9381}]$ python3 tests/automatus.py rule --libvirt qemu:///system ssgts_rhel9 account_password_selinux_faillock_dir
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-2022-08-23-0927/test_suite.log
INFO - xccdf_org.ssgproject.content_rule_account_password_selinux_faillock_dir
INFO - Script correct_value.pass.sh using profile (all) OK
INFO - Script correct_value_multiple_dirs.pass.sh using profile (all) OK
INFO - Script missing_dir.fail.sh using profile (all) OK
INFO - Script wrong_value.fail.sh using profile (all) OK
INFO - Script wrong_value_multiple_dirs.fail.sh using profile (all) OK
[jcerny@thinkpad scap-security-guide{pr/9381}]$  python3 tests/automatus.py rule --libvirt qemu:///system ssgts_rhel9 --remediate-using ansible account_password_selinux_faillock_dir
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-2022-08-23-0931/test_suite.log
INFO - xccdf_org.ssgproject.content_rule_account_password_selinux_faillock_dir
INFO - Script correct_value.pass.sh using profile (all) OK
INFO - Script correct_value_multiple_dirs.pass.sh using profile (all) OK
INFO - Script missing_dir.fail.sh using profile (all) OK
WARNING - No remediation is available for rule 'xccdf_org.ssgproject.content_rule_account_password_selinux_faillock_dir'.
INFO - Script wrong_value.fail.sh using profile (all) OK
WARNING - No remediation is available for rule 'xccdf_org.ssgproject.content_rule_account_password_selinux_faillock_dir'.
INFO - Script wrong_value_multiple_dirs.fail.sh using profile (all) OK
WARNING - No remediation is available for rule 'xccdf_org.ssgproject.content_rule_account_password_selinux_faillock_dir'.
[jcerny@thinkpad scap-security-guide{pr/9381}]$

marcusburghardt added bugfix Fixes to reported bugs. Test Suite Update in Test Suite. OVAL OVAL update. Related to the systems assessments. Bash Bash remediation update. labels Aug 22, 2022

marcusburghardt added this to the 0.1.64 milestone Aug 22, 2022

marcusburghardt added 5 commits August 22, 2022 10:06

Improve the rule description for selinux_faillock_dir

280458c

Keep the PAM module name aligned to other related rules and fixed some minor typos and issues in the text.

Fix many copy-and-paste issues and improved style

3210d72

Include test scenario for multiple dirs

932a990

marcusburghardt force-pushed the account_password_selinux_faillock_dir branch from eee2661 to 2f2b2c6 Compare August 22, 2022 08:08

jan-cerny requested changes Aug 22, 2022

View reviewed changes

jan-cerny self-assigned this Aug 22, 2022

Remove unnecessary command substitution

2961247

marcusburghardt requested a review from jan-cerny August 22, 2022 15:01

jan-cerny approved these changes Aug 23, 2022

View reviewed changes

jan-cerny merged commit cedbd31 into ComplianceAsCode:master Aug 23, 2022

marcusburghardt deleted the account_password_selinux_faillock_dir branch August 23, 2022 08:57

marcusburghardt mentioned this pull request Sep 6, 2022

Many rules in RHEL 9 STIG profile fail after remediation and reboot #9432

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix account_password_selinux_faillock_dir rule #9381

Fix account_password_selinux_faillock_dir rule #9381

marcusburghardt commented Aug 22, 2022

github-actions bot commented Aug 22, 2022

github-actions bot commented Aug 22, 2022 •

edited

marcusburghardt commented Aug 22, 2022

jan-cerny Aug 22, 2022

marcusburghardt Aug 22, 2022 •

edited

jan-cerny Aug 23, 2022

jan-cerny Aug 22, 2022

marcusburghardt Aug 22, 2022

marcusburghardt Aug 22, 2022

codeclimate bot commented Aug 22, 2022

openshift-ci bot commented Aug 22, 2022

jan-cerny left a comment

Fix account_password_selinux_faillock_dir rule #9381

Fix account_password_selinux_faillock_dir rule #9381

Conversation

marcusburghardt commented Aug 22, 2022

Description:

Rationale:

github-actions bot commented Aug 22, 2022

github-actions bot commented Aug 22, 2022 • edited

marcusburghardt commented Aug 22, 2022

jan-cerny Aug 22, 2022

Choose a reason for hiding this comment

marcusburghardt Aug 22, 2022 • edited

Choose a reason for hiding this comment

jan-cerny Aug 23, 2022

Choose a reason for hiding this comment

jan-cerny Aug 22, 2022

Choose a reason for hiding this comment

marcusburghardt Aug 22, 2022

Choose a reason for hiding this comment

marcusburghardt Aug 22, 2022

Choose a reason for hiding this comment

codeclimate bot commented Aug 22, 2022

openshift-ci bot commented Aug 22, 2022

jan-cerny left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 22, 2022 •

edited

marcusburghardt Aug 22, 2022 •

edited