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

Check only local users home directories #10825

Merged
merged 13 commits into from
Jul 17, 2023

Conversation

jan-cerny
Copy link
Collaborator

@jan-cerny jan-cerny commented Jul 12, 2023

The rule accounts_user_interactive_home_directory_exists states in rationale that we need to ensure that home directories of interactive users exist. The text indicates that this requirement is relevant to local interactive users.

However, the current implementation of the OVAL check uses the unix:password_object element, which also returns non-local (LDAP) users, because the implementation of OpenSCAP makes use of the getpwent() system call, which browses all users provides by the NSS.

In this commit, we will change the implementation so that only local interactive users will be considered. We will achieve this by parsing the data directly from /etc/passwd using the OVAL ind:textfilecontent54_object instead of using the unix:password_object.

Also, the rule description is clarified.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2203791

Then, a similar change has been done also in these rules:

  • accounts_users_home_files_groupownership
  • accounts_user_dot_group_ownership
  • accounts_users_home_files_permissions
  • accounts_umask_interactive_users
  • accounts_user_dot_user_ownership
  • file_permissions_home_directories
  • file_groupownership_home_directories
  • file_ownership_home_directories
  • accounts_users_home_files_ownership

@jan-cerny jan-cerny added bugfix Fixes to reported bugs. OVAL OVAL update. Related to the systems assessments. labels Jul 12, 2023
@jan-cerny jan-cerny added this to the 0.1.69 milestone Jul 12, 2023
@jan-cerny
Copy link
Collaborator Author

QUESTION: This rule used to use macro create_interactive_users_list_object which is used in many other rules. That means they're affected by the same problem, ie. they include non-local users to the evaluation. Should we report it as an issue?

@github-actions
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

@github-actions
Copy link

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_accounts_user_interactive_home_directory_exists'.
--- xccdf_org.ssgproject.content_rule_accounts_user_interactive_home_directory_exists
+++ xccdf_org.ssgproject.content_rule_accounts_user_interactive_home_directory_exists
@@ -3,7 +3,7 @@
 All Interactive Users Home Directories Must Exist
 
 [description]:
-Create home directories to all interactive users that currently do not
+Create home directories to all local interactive users that currently do not
 have a home directory assigned. Use the following commands to create the user
 home directory assigned in /etc/passwd:
 $ sudo mkdir /home/USER

@vojtapolasek vojtapolasek self-assigned this Jul 12, 2023
@vojtapolasek
Copy link
Collaborator

Hello @jan-cerny and thank you. I have a concern. Previously, we checked for the user Id as well so that it is not below 1000. I think that this check is missing from the implementation. Could you add it back?

@vojtapolasek
Copy link
Collaborator

@jan-cerny I have quickly analyzed rules which use the create_interactive_users_list_object macro.
Following rules might be worth changing as well. Remaining rules do not have local users clearly mentioned in the description / rationale / ocil.

  • accounts_users_home_files_groupownership
  • accounts_user_dot_group_ownership
  • accounts_users_home_files_permissions
  • accounts_umask_interactive_users
  • accounts_user_dot_user_ownership (the description does not state it exactly, but since we would modify accounts_user_dot_group_ownership it makes sense to modify both... or none of them)
  • file_permissions_home_directories
  • file_groupownership_home_directories
  • file_ownership_home_directories
  • accounts_users_home_files_ownership

@jan-cerny
Copy link
Collaborator Author

Previously, we checked for the user Id as well so that it is not below 1000. I think that this check is missing from the implementation. Could you add it back?

@vojtapolasek It's still there, but it's implemented in a hackish way. The regular expression object_{{{ rule_id }}}_local_interactive_users requires that the UID field consists of at least 4 digits. Therefore, it won't match uers with UID consisting of 3 digits, ie less than or equal 999. I did it this way because in OVAL textfilecontet54_object you can't have multiple different subexpression capturing groups in the regex, and I already need to capture the user name field in a subexpression. I think that to have a proper comparison with 1000 in the OVAL I would have to create one more layer of objects and variables. What do you think? Is my current solution sufficient in this situation?

@vojtapolasek
Copy link
Collaborator

@jan-cerny That sounds reasonable for now. Maybe it is worth creating upstream RFE to enhance it in future.

@jan-cerny
Copy link
Collaborator Author

I have performed a similar change in other rules. To do that, the OVAL macro has been refactored even more which leads to the situation that there are now 3 new macros for working with local interactive users: 1. macro retrieving home directories paths, 2. macro retrieving UIDs, 3. macro retrieving GIDs.

@jan-cerny
Copy link
Collaborator Author

I have reported the problem as RFE in #10845

The rule `accounts_user_interactive_home_directory_exists` states
in rationale that we need to ensure that home directories of
interactive users exist. The text indicates that this requirement
is relevant to local interactive users.

However, the current implementation of the OVAL check uses
the `unix:password_object` element, which also returns non-local
(LDAP) users, because the implementation of OpenSCAP makes use of
the `getpwent()` system call, which browses all users provides by
the NSS.

In this commit, we will change the implementation so that only
local interactive users will be considered. We will achieve this
by parsing the data directly from `/etc/passwd` using the OVAL
`ind:textfilecontent54_object` instead of using the `unix:password_object`.

Also, the rule description is clarified.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2203791
Extract the internal part of the
create_local_interactive_users_home_dirs_list_object macro to
a new low-level macro so that it could be used later in new similar
high-level macros.
Change the check so that only local interactive users will be
considered by the OVAL check.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2203791
Change the check so that only local interactive users will be
considered by the OVAL check.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2203791
Change the check so that only local interactive users will be
considered by the OVAL check.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2203791
Change the check so that only local interactive users will be
considered by the OVAL check.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2203791
Change the check so that only local interactive users will be
considered by the OVAL check.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2203791
Change the check so that only local interactive users will be
considered by the OVAL check.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2203791
Change the check so that only local interactive users will be
considered by the OVAL check.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2203791
Change the check so that only local interactive users will be
considered by the OVAL check.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2203791
Change the check so that only local interactive users will be
considered by the OVAL check.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2203791
@jan-cerny
Copy link
Collaborator Author

I have rebased this PR on the top of the latest upstream master branch.

@codeclimate
Copy link

codeclimate bot commented Jul 14, 2023

Code Climate has analyzed commit e0bd595 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 53.4% (0.0% change).

View more on Code Climate.

Copy link
Collaborator

@vojtapolasek vojtapolasek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you for this @jan-cerny .

@vojtapolasek vojtapolasek merged commit afbd589 into ComplianceAsCode:master Jul 17, 2023
33 of 34 checks passed
@jan-cerny jan-cerny deleted the rhbz2203791 branch July 17, 2023 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes to reported bugs. OVAL OVAL update. Related to the systems assessments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants