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

Create new rules for ANSSI R39 #6495

Merged
merged 8 commits into from
Jan 8, 2021

Conversation

jan-cerny
Copy link
Collaborator

Description:

ANSSI R39: Temporary directories dedicated to each account
Each user or service account must have its own temporary directory and dispose of it exclusively.

We can satisfy this requirement by setting up the temporary directories as polyinstantiated, according to https://access.redhat.com/blogs/766093/posts/3169121.

This PR adds new rules that check for the configuration changes.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Dec 18, 2020
@openshift-ci-robot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

jan-cerny added a commit to jan-cerny/scap-security-guide that referenced this pull request Dec 18, 2020
The new rules are created in
ComplianceAsCode#6495
@openscap-ci
Copy link
Collaborator

openscap-ci commented Dec 18, 2020

Changes identified:
Rules:
 enable_pam_namespace
 accounts_polyinstantiated_tmp
 accounts_polyinstantiated_var_tmp

Show details

Rule enable_pam_namespace:
 OVAL check is newly added.
 Ansible remediation newly added.
 The rule doesn't occur in any profile nor product.
 Bash remediation is newly added.
Rule accounts_polyinstantiated_tmp:
 OVAL check is newly added.
 Ansible remediation newly added.
 The rule doesn't occur in any profile nor product.
 Bash remediation is newly added.
Rule accounts_polyinstantiated_var_tmp:
 OVAL check is newly added.
 Ansible remediation newly added.
 The rule doesn't occur in any profile nor product.
 Bash remediation is newly added.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Used by openshift-ci bot. label Dec 25, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Used by openshift-ci bot. label Jan 4, 2021
@jan-cerny jan-cerny marked this pull request as ready for review January 4, 2021 12:45
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Jan 4, 2021
@yuumasato yuumasato self-assigned this Jan 6, 2021
Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

The rules look good and the test scenarios pass, but I think you forgot to enable the SELinux boolean polyinstantiation_enabled=1.
Without it the polyinstantiation didn't work, and no other user than root was able to login.

Nitick: please ensure the files have a newline at the end.

@DraugurHundur
Copy link

I'm confused why create the inst directory in /var/tmp under /var/tmp while you create the inst directory for /tmp under / (which may be a different partition) ?

@yuumasato
Copy link
Member

yuumasato commented Jan 6, 2021

That's a good point. I guess it would be safer (in terms of partition capacity) to have the instance in /tmp/tmp-inst?

@DraugurHundur
Copy link

I'm guessing because /tmp is wiped on reboot (whereas /var/tmp is not) this can cause an issue as the inst directory would have to be recreated on restart.

@openshift-merge-robot
Copy link
Collaborator

@jan-cerny: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-ocp4-cis c77942e link /test e2e-aws-ocp4-cis

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.

@@ -4,6 +4,7 @@
<criteria operator="AND" comment="Check Polyinstantiation of /tmp Directories">
<criterion comment="Check that /tmp-inst exists and has mode 000" test_ref="test_tmp_inst" />
<criterion comment="Check configuration of /tmp in /etc/security/namespace.conf file" test_ref="test_tmp_in_namespace_conf" />
<criterion comment="Check SELinux boolean polyinstantiation_enabled is enabled" test_ref="test_accounts_polyinstantiated_tmp_sebool" />
Copy link
Member

Choose a reason for hiding this comment

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

Should the check and remediation for sebool polyinstantiation_enabled be present in multiple rules?
It makes more sense embed it on enable_pam_namespace.
It could also become a rule of its own.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but we already have a rule for this selinux boolean, it's a templated rule, its ID is sebool_polyinstantiation_enabled https://github.com/ComplianceAsCode/content/blob/master/linux_os/guide/system/selinux/selinux-booleans/sebool_polyinstantiation_enabled/rule.yml. But we're adding this rule to the profile.

I like the idea about embedding the enable_pam_namespace, but I would even go firther and merge the check from enable_pam_namespace into accounts_polyinstantiated_tmp and accounts_polyinstantiated_var_tmp.

What is your advise?

Copy link
Member

@yuumasato yuumasato Jan 7, 2021

Choose a reason for hiding this comment

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

Oh, I missed that a rule for sebool polyinstantiaion_enabled already existed, and it uses an XCCDF Value for the value.

In this case, just selecting it in the profile with the appropriate value should do, right?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the confusion and extra work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should be enough when the profile selects all 4 rules.

@yuumasato
Copy link
Member

yuumasato commented Jan 7, 2021

I'm confused why create the inst directory in /var/tmp under /var/tmp while you create the inst directory for /tmp under / (which may be a different partition) ?

@jan-cerny What do you think of the suggestion to create the instance dir in /tmp/tmp-inst? So that data intended for polyinstantiated /tmp, is actually put in the same partition as /tmp would.

jan-cerny added a commit to jan-cerny/scap-security-guide that referenced this pull request Jan 8, 2021
The new rules are created in
ComplianceAsCode#6495
jan-cerny added a commit to jan-cerny/scap-security-guide that referenced this pull request Jan 8, 2021
The new rules are created in
ComplianceAsCode#6495
@openshift-ci
Copy link

openshift-ci bot commented Jan 8, 2021

@jan-cerny: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-ocp4-cis-node 151c6bd link /test e2e-aws-ocp4-cis-node

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.

Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

LGTM, thank you, @jan-cerny :)

@yuumasato yuumasato added this to the 0.1.54 milestone Jan 8, 2021
@yuumasato yuumasato merged commit 31772a6 into ComplianceAsCode:master Jan 8, 2021
@jan-cerny jan-cerny deleted the anssi_r39 branch January 8, 2021 13:19
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants