content icon indicating copy to clipboard operation
content copied to clipboard

SUSE fix file_groupownership_home_directories

Open brett060102 opened this issue 3 years ago • 9 comments

The oval check in file_groupownership_home_directories fails for SLE12 and SLE12.

The rule.yml has followins check, which returns the following on my test system. ec2-user@ip-10-0-0-226:~> ls -ld $(awk -F: '($3>=1000)&&($7 !~ /nologin/){print $6}' /etc/passwd) drwxr-x--- 9 ec2-user users 218 Aug 9 15:31 /home/ec2-user drwxr-xr-x 2 nobody nobody 6 May 7 20:40 /var/lib/nobody

The rule does list any other checks to perform. But, current oval test code: 1: skips gids < gid_nim (1000) 2: skips user nobody 3: requires at least 1 interactive login account 4: requires all users be in unique groups

A number of problems with this. 1: multiple users can be in same group (i.e. the "users" group which is the default when adding a local user on some distros). Oval would fail because more that account in the same group.

2: The "users" GID is 100 which is less than gid_min, which means on distros where local users are added with "users" as the login group , the code would not check the group ownership for the login directory

3: it is possible for a system to have no interactive login accounts except root (uid:0, gid:0), Since GID < 1000 are skipped, oval would fail because no interactive accounts were found.

4: It is possible to have noninteractive account attached to a group >= 1000

Our passwd file contains one: rpc:x:491:65534:user for rpcbind:/var/lib/empty:/sbin/nologin

Proposed change: 1: only check accounts with UID >= uid_min and is not the nobody account 2: allow multiple accounts with same GID. 3: allow for no inactive accounts. (no accounts other than nobody with uid >= uid_min)

Description:

  • Fix file_groupownership_home_directories for SLE12/15

Rationale:

  • Allow file_groupownership_home_directories to work on distros like SLE12/15

brett060102 avatar Aug 09 '22 17:08 brett060102

Hi @brett060102. Thanks for your PR.

I'm waiting for a ComplianceAsCode member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

openshift-ci[bot] avatar Aug 09 '22 17:08 openshift-ci[bot]

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

github-actions[bot] avatar Aug 09 '22 17:08 github-actions[bot]

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

Click here to see the full diff
OVAL definition oval:ssg-file_groupownership_home_directories:def:1 differs:
--- old datastream
+++ new datastream
- criteria AND
- criterion oval:ssg-test_file_groupownership_home_directories_duplicated:tst:1
+ criteria None

github-actions[bot] avatar Aug 09 '22 17:08 github-actions[bot]

I see two failed tests: Automatus CS8 / Run Tests (pull_request) Failing after 8m — Run Tests Automatus CS9 / Run Tests (pull_request) Failing after 6m — Run Tests

Error in both cases is an exit 1 from remediation. I am not sure how my change could have caused this since there are no changes to remediation.

brett060102 avatar Aug 09 '22 19:08 brett060102

I see two failed tests: Automatus CS8 / Run Tests (pull_request) Failing after 8m — Run Tests Automatus CS9 / Run Tests (pull_request) Failing after 6m — Run Tests

Error in both cases is an exit 1 from remediation. I am not sure how my change could have caused this since there are no changes to remediation.

I don't see there anything related to "exit 1 from remediation".

Instead, I can see that the test scenario home_dirs_with_same_groupowner.fail.sh fails during initial stage, which means it fails the first scan by oscap, in which no remediation is executed. It happens in all 4 cases.

ERROR - Script home_dirs_with_same_groupowner.fail.sh using profile (all) found issue:
ERROR - Rule evaluation resulted in pass, instead of expected fail during initial stage 
ERROR - The initial scan failed for rule 'xccdf_org.ssgproject.content_rule_file_groupownership_home_directories'.

Taking a closer look, the test scenario home_dirs_with_same_groupowner.fail.sh tests the situation that 2 home directories on the system are owned by the same group. And it expects that the rule will fail in this situation. That's exactly the situation you wanted to change by this PR (see your problem 1 in the PR description).

You can easily align this by renaming the test scenario file home_dirs_with_same_groupowner.fail.sh to home_dirs_with_same_groupowner.pass.sh.

jan-cerny avatar Aug 10 '22 07:08 jan-cerny

@jan-cerny thanks and fixed. I saw the exit 1 at the end of the details for two two tests and that it was a from the remediation run log. I had not seen the errors in the middle of the log. I will make sure I check that in the future.

brett060102 avatar Aug 10 '22 13:08 brett060102

Code Climate has analyzed commit e7213183 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.7%.

View more on Code Climate.

qlty-cloud-legacy[bot] avatar Aug 10 '22 13:08 qlty-cloud-legacy[bot]

Still seeing the Script home_dirs_with_same_groupowner.fail.sh failure, but I have renamed that to home_dirs_with_same_groupowner.pass.sh, is that change not getting picked up?

brett060102 avatar Aug 10 '22 14:08 brett060102

It looks like it isn't renamed but created a new file and the old file still exists.

jan-cerny avatar Aug 10 '22 15:08 jan-cerny

Thank, Now show as rename without change. I used "git mv" to rename the file. Not sure what happened. File was removed from my local branch, but the push made a copy instead.

Deleted in local branch and things look right now.

brett060102 avatar Aug 10 '22 16:08 brett060102

@jan-cerny Thank you, finally looks clean.

brett060102 avatar Aug 10 '22 23:08 brett060102