polyinstantited tmp fixes

[maage]

Description:

Search configs at /etc/security/namespace.d Instead of requiring only one config, allow other possible implementations.

Rationale:

Rules should search configs where they can be placed. Rules should allow admins multiple ways to implement the control. If strict implementation is required, then it should be own rule.

[openshift-ci[bot]]

Hi @maage. 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.

[github-actions[bot]]

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

[vojtapolasek]

/ok-to-test

[vojtapolasek]

Hello, ah I did not look into the commit message. Very nice explanation, thank you for that. It is true that these rules are quite basic and they could use some extending. I have some suggestions. And maybe @jan-cerny might have some as well as he implemented these rules.

1. your current implementation allows basically any arguments after definition of directories. I understand your point that current regex might be too strict, but I suggest you implement something like:

• for method, put there a regex which allows only allowed set of methods from the man page
• use XCCDF variable to define list of users. I suggest you leave the default value at "root,adm". If you make the variable interactive, user will be able to supply their own list of users. You can see an example of interactive variable here: https://github.com/ComplianceAsCode/content/blob/master/linux_os/guide/system/kernel_build_config/var_kernel_config_panic_timeout.var You can find example usage of variable in OVAL here: https://github.com/ComplianceAsCode/content/blob/master/linux_os/guide/system/accounts/accounts-session/accounts_logon_fail_delay/oval/shared.xml

2. Could you please enrich also the rule description / ocil in rule.yml?

[jan-cerny]

@vojtapolasek Unfortunately, I don't have any deep understanding on polyinstantiated directories. I was following https://www.redhat.com/en/blog/polyinstantiating-tmp-and-vartmp-directories

[maage]

It would be nice to see how others have actually implemented this control.

For it to work it requires modification in /etc/pam.d/systemd-user for this to work, see: https://bugzilla.redhat.com/show_bug.cgi?id=2053098 And that is not enabled by default for example RHEL8. Please be really careful when you modify the file as it can require system rescue when there is upgrade issues later.

Also it requires polyinstantiation_enabled sebool to be true.

All three things should be checked.

Full regexp to parser any valid config is just way too complex task.

1. There is "" and \ to handle and parser has many quirks for you to find out. Actually my current regexp does not allow all valid configs thru as you can have space in " ". But I decided that I don't want to have that kind of trickery.
2. Because of iscript=path method flag is enable in all methods, this means path can be any quoted path possible, see 1.
3. namespace.conf setup is not well thought out as list_of_uids is actually usernames and each listed username must exist for rule to work. Mitigation script should know about this. This probably means ~ rules should be one per user.
4. As I said (or tried to say) in commit, I see list like "root,adm" as something which does not work in real world modern system, it excludes any system users and anyways system users should be handled via systemd sandboxing. And as most services have their own users, list of users should be updated each time a new service is installed. Maybe this is something not happening in really hardened env, but not in others.
5. This leaves ~username list, and again until there is generic ways to manage different user lists I see listings of users for on subsystem just going to cause problems. At least following rules should have common methods:

• xccdf_org.ssgproject.content_rule_sshd_limit_user_access
• xccdf_org.ssgproject.content_rule_selinux_user_login_roles
• xccdf_org.ssgproject.content_rule_no_password_auth_for_systemaccounts And again these rules should always have mitigations where they check existing users only if required by subsystem.

6. There is patches to extend namespace.conf to support groups etc and that kind of setup would simplify setup somewhat. Config could say that polyinstantieted tmp is enabled only for users belonging to a group. Then there is no need to manage users tightly.
7. You can have multiple lines in namespace.conf and rule targets only first one to match. So any parameters are not going to achieve what is wanted.

If you need to have variables implemented, then it would be possible if your ansible first pulls info from host and then merges that with configuration and generates the per host tailoring file. Is this something that is actually done in real world hardened situations?

I think this control is nice, because it means data is not leaked via /tmp or /var/tmp between normal users and or snoopy service even if there was bad perms or other mistake.

One last very influential part of this saga is:

https://lists.fedoraproject.org/pipermail/devel/2015-January/206778.html

Generally I do not want to have unmanaged IPC over boundaries so I see them not working as definite target, not a problem at all. Any other issue are just implementation details and misunderstandings. If group of users need to share X11 or other, then just disable and that is that.

So I just don't think there is benefit to modify my patch regarding to regexp.

[vojtapolasek]

Hello. I can see that enabling proper polyinstantiation is a multistep and quite intricate process. Also checking of namespace.conf is only one of many steps how to get this working. But I still don't think that your regexp increases security posture of a system. I still think that using an XCCDF variable is better than giving a bianco check for usage of any users, the same stands for methods. It could be interesting to sum up configuration changes for getting all the steps needed to enable polyinstantiation and you could open an upstream issue for that.

[vojtapolasek]

Hello @maage we discussed this PR in our team and I have some questions for you:

1. We understand introduction of support for /etc/namespace.conf.d directory. But by reading man pages, it is not clear which of the entries gets the priority. Are you sure that the current check will capture the entry which will be in fact used?
2. We acknowledge that the "method" field can become very complex. But we still think that it should be somehow restricted. How about disallowing the "shared" flag? From our understanding this completely undermines the purpose. Thank you for your responses.

[jan-cerny]

@maage Could you please check @vojtapolasek 's questions above?

[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-accounts_polyinstantiated_tmp:def:1 differs:
--- old datastream
+++ new datastream
- criterion oval:ssg-test_tmp_inst:tst:1
+ criterion oval:ssg-test_tmp_tmp_inst:tst:1
OCIL for rule 'xccdf_org.ssgproject.content_rule_accounts_polyinstantiated_tmp' differs:
--- old datastream
+++ new datastream
@@ -1,7 +1,8 @@
-Run the following command to ensure that /tmp is configured as a
-polyinstantiated directory:
-$ sudo grep /tmp /etc/security/namespace.conf
+Run the following command to ensure that
+/tmp
+is configured as a polyinstantiated directory:
+$ sudo grep -hr '^[^#]' /etc/security/namespace.conf /etc/security/namespace.d
 The output should return the following:
-/tmp /tmp/tmp-inst/ level root,adm
+/tmp /tmp/tmp-inst/ level root,adm
 Is it the case that is not configured?
 
bash remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_polyinstantiated_tmp' differs:
--- old datastream
+++ new datastream
@@ -2,11 +2,11 @@
 mkdir --mode 000 /tmp/tmp-inst
 fi
 chmod 000 /tmp/tmp-inst
-chcon --reference=/tmp /tmp/tmp-inst
+chcon --reference=/tmp/ /tmp/tmp-inst
 
-if ! grep -Eq '^\s*/tmp\s+/tmp/tmp-inst/\s+level\s+root,adm$' /etc/security/namespace.conf ; then
+if ! grep -Eq '^\s*/tmp\s+/tmp/tmp-inst/\s+level\s+root,adm\s*$' /etc/security/namespace.conf ; then
 if grep -Eq '^\s*/tmp\s+' /etc/security/namespace.conf ; then
 sed -i '/^\s*\/tmp/d' /etc/security/namespace.conf
 fi
- echo "/tmp /tmp/tmp-inst/ level root,adm" >> /etc/security/namespace.conf
+ echo "/tmp /tmp/tmp-inst/ level root,adm" >> /etc/security/namespace.conf
 fi

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_polyinstantiated_tmp' differs:
--- old datastream
+++ new datastream
@@ -3,9 +3,9 @@
 path: /tmp/tmp-inst
 state: directory
 mode: '000'
- seuser: system_u
- serole: object_r
- setype: tmp_t
+ seuser: _default
+ serole: _default
+ setype: _default
 tags:
 - CCE-83732-8
 - accounts_polyinstantiated_tmp
@@ -19,9 +19,12 @@
 lineinfile:
 path: /etc/security/namespace.conf
 create: false
- regexp: ^\s*/tmp\s+/tmp/tmp-inst/\s+level\s+root,adm$
- line: /tmp /tmp/tmp-inst/ level root,adm
+ regexp: ^\s*{{{ basedir }}}\s+{{{ basedir }}}/tmp-inst/\s+level\s+root,adm\s*$
+ line: '{{{ basedir }}} {{{ basedir }}}/tmp-inst/ level root,adm'
 state: present
+ seuser: _default
+ serole: _default
+ setype: _default
 tags:
 - CCE-83732-8
 - accounts_polyinstantiated_tmp

OCIL for rule 'xccdf_org.ssgproject.content_rule_accounts_polyinstantiated_var_tmp' differs:
--- old datastream
+++ new datastream
@@ -1,6 +1,7 @@
-Run the following command to ensure that /var/tmp is configured as a
-polyinstantiated directory:
-$ sudo grep /var/tmp /etc/security/namespace.conf
+Run the following command to ensure that
+/var/tmp
+is configured as a polyinstantiated directory:
+$ sudo grep -hr '^[^#]' /etc/security/namespace.conf /etc/security/namespace.d
 The output should return the following:
 /var/tmp /var/tmp/tmp-inst/ level root,adm
 Is it the case that is not configured?

bash remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_polyinstantiated_var_tmp' differs:
--- old datastream
+++ new datastream
@@ -1,12 +1,12 @@
-if ! [ -d /tmp-inst ] ; then
+if ! [ -d /var/tmp/tmp-inst ] ; then
 mkdir --mode 000 /var/tmp/tmp-inst
 fi
 chmod 000 /var/tmp/tmp-inst
 chcon --reference=/var/tmp/ /var/tmp/tmp-inst
 
-if ! grep -Eq '^\s*/var/tmp\s+/var/tmp/tmp-inst/\s+level\s+root,adm$' /etc/security/namespace.conf ; then
+if ! grep -Eq '^\s*/var/tmp\s+/var/tmp/tmp-inst/\s+level\s+root,adm\s*$' /etc/security/namespace.conf ; then
 if grep -Eq '^\s*/var/tmp\s+' /etc/security/namespace.conf ; then
- sed -i '/^\s*\/var\/tmp/d' /etc/security/namespace.conf
+ sed -i '/^\s*\/var/tmp/d' /etc/security/namespace.conf
 fi
 echo "/var/tmp /var/tmp/tmp-inst/ level root,adm" >> /etc/security/namespace.conf
 fi

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_polyinstantiated_var_tmp' differs:
--- old datastream
+++ new datastream
@@ -3,9 +3,9 @@
 path: /var/tmp/tmp-inst
 state: directory
 mode: '000'
- seuser: system_u
- serole: object_r
- setype: tmp_t
+ seuser: _default
+ serole: _default
+ setype: _default
 tags:
 - CCE-83778-1
 - accounts_polyinstantiated_var_tmp
@@ -19,9 +19,12 @@
 lineinfile:
 path: /etc/security/namespace.conf
 create: false
- regexp: ^\s*/var/tmp\s+/var/tmp/tmp-inst/\s+level\s+root,adm$
- line: /var/tmp /var/tmp/tmp-inst/ level root,adm
+ regexp: ^\s*{{{ basedir }}}\s+{{{ basedir }}}/tmp-inst/\s+level\s+root,adm\s*$
+ line: '{{{ basedir }}} {{{ basedir }}}/tmp-inst/ level root,adm'
 state: present
+ seuser: _default
+ serole: _default
+ setype: _default
 tags:
 - CCE-83778-1
 - accounts_polyinstantiated_var_tmp

[maage]

Now updates

First added ansbile_lineinfile feature to enforce selinux directives to be _default, by default this is now off. pam_namespace is part of login process and if it has problems, then you can not login. One problem can be wrong selinux context. Ansible at least as of 2.9 does not honour target default type, if file has wrong context it leaved wrong context and if file is moved it uses source context. This can cause problems. One defensive measurement that should be used when handling any file should be to set selinux directives to be _default. I think this should be default in all ansible remediations. Maybe this could be own PR.

Converted to "semitemplate" implementation. Only difference between accounts_polyinstantiated_tmp and accounts_polyinstantiated_var_tmp is per file basedir. Maybe we could use templates here, but there probably will not be more instances. Found some minor issues, copy-paste-and-modify is just hard.

Updated documentation and mentioned possibilities of problems.

Converted regexp to partially conform config. Handle shared parameter and test it. pass case is part of user-selected.pass.sh and fail has new test.

Noticed bash remediation does not actually do the right thing but decided not to do anything about that. Polyinstantiation can have multiple rules per polydir (for example /tmp). Logic starts from user as it is user that is logging in. So actual key of rule is uid, currently parsed from list_of_uids field, not polydir field. One polydir can have multiple different implementations. First hit per user wins. See: https://github.com/linux-pam/linux-pam/blob/510e825ef130a843663115ec510b1237ea4708f4/modules/pam_namespace/pam_namespace.c#L1862 Now bash remediation allows only one rule per directory.

[maage]

Do not merge. Centos 7 and maybe others exposed real issue. I used jinja trim filter, but older versions do not support to set character.

[maage]

Use python methods instead of jinja filters.

[maage]

"Gate / Build, Test on OpenSUSE Leap 15" seems to be temporary issue maybe. "Automatus CS9 / Run Tests" seem to fail on unrelated rules chronyd_no_chronyc_network and chronyd_client_only AFAIK

[qlty-cloud-legacy[bot]]

Code Climate has analyzed commit 6e350f0f 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.4% (0.0% change).

View more on Code Climate.

[openshift-ci[bot]]

@maage: 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-ocp4-e8	6e350f0f30c4017005a8ea30098692f1adbd4011	link	true	/test e2e-aws-ocp4-e8
ci/prow/e2e-aws-ocp4-cis	6e350f0f30c4017005a8ea30098692f1adbd4011	link	true	/test e2e-aws-ocp4-cis
ci/prow/e2e-aws-ocp4-stig-node	6e350f0f30c4017005a8ea30098692f1adbd4011	link	true	/test e2e-aws-ocp4-stig-node
ci/prow/e2e-aws-rhcos4-moderate	6e350f0f30c4017005a8ea30098692f1adbd4011	link	true	/test e2e-aws-rhcos4-moderate
ci/prow/e2e-aws-ocp4-cis-node	6e350f0f30c4017005a8ea30098692f1adbd4011	link	true	/test e2e-aws-ocp4-cis-node
ci/prow/e2e-aws-rhcos4-high	6e350f0f30c4017005a8ea30098692f1adbd4011	link	true	/test e2e-aws-rhcos4-high

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.

[maage]

Ok, seems Automatus CS8 and maybe others are failing in Ansible remediation.

Issue source is the same as I noticed above with bash remediations. Ansible remediation essentially adds/normalizes rule. But does not change bad rule.

But how to remediate bad rule correctly?

I guess remediation should target list_of_uids + polydir per user. There is 3 possibilities:

• with ~
• without ~
• empty: all users

BP28(R39) states:

Each user or service account must have its own temporary directory and dispose of it exclusively.

This requirement is quite bad. It can not be fulfilled by systemd services use own /tmp PrivateTmp=yes as that setting is per service, not per user.

Service users generally do not create new session, so there is no pam involvement as such.

I think there should be new systemd rule version to require PrivateTmp=yes for all services except listed exceptions. See: https://github.com/ComplianceAsCode/content/issues/9422

And with support of no_shelllogin_for_systemaccounts rule this leaves only root user. And I would prefer not to involve root with this kind of stuff.

So that makes empty case something I see as not desired and I'd drop support for that case here, so no tests involving it and no remediation capabilities.

Remediations should also fail if config has " " or \n etc. This can be safely tested, but how can I test case where remediation is expected to fail?

Other cases need to preserve order. There could be rule without ~ and list all system usernames (uid < 1000) there to set default setting for users. And then before the default case is per user exceptions ~username format. This would be so much safer and nicer if namespace.conf would support his kind of uid comparisons (or groups).

So remediation could parse system users and then create default entry as last and remove any entry where there is ~ and system user. And then allow any ~username rules before it.

I guess we are still missing multiple rules test cases.

I think main issue with remediation is to keep system in runnable state. To force new session reboot is needed after polyinstantiated tmp is configured. So tests should test this too.

[vojtapolasek]

Hello @maage thank you for the extensive comment. I can read that the configuration of this feature is quite problematic if we want to ensure that the system works correctly and that a decent level of security is offered. Do you plan to continue working on this? Or do you expect any help from us?

[vojtapolasek]

Hello @maage , since I haven't received any response from you for several months, I am closing this PR. Feel free to reopen it if you decide to continue working on this problem.