Fix auditd rule to watch apparmor instead of selinux on Ubuntu

[mpurg]

Description:

• Fix auditd rule audit_rules_mac_modifications to watch apparmor dirs /etc/apparmor and /etc/apparmor.d instead of /etc/selinux on Ubuntu

[openshift-ci]

Hi @mpurg. 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-sigs/prow repository.

[github-actions]

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]

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_audit_rules_mac_modification'.
--- xccdf_org.ssgproject.content_rule_audit_rules_mac_modification
+++ xccdf_org.ssgproject.content_rule_audit_rules_mac_modification
@@ -7,10 +7,13 @@
 augenrules program to read audit rules during daemon startup (the
 default), add the following line to a file with suffix .rules in the
 directory /etc/audit/rules.d:
+
 -w /etc/selinux/ -p wa -k MAC-policy
+
 If the auditd daemon is configured to use the auditctl
 utility to read audit rules during daemon startup, add the following line to
 /etc/audit/audit.rules file:
+
 -w /etc/selinux/ -p wa -k MAC-policy
 
 [reference]:
@@ -371,7 +374,7 @@
 10.3
 
 [rationale]:
-The system's mandatory access policy (SELinux) should not be
+The system's mandatory access policy (SELinux or Apparmor) should not be
 arbitrarily changed by anything other than administrator action. All changes to
 MAC policy should be audited.
 

[vojtapolasek]

/ok-to-test

[vojtapolasek]

@mpurg thank you for the PR. I reviewed it and I understand its purpose. However, I wonder if this is an optimal way.
The PR introduces lots of conditions into the rule. I think that it would be easier to create two new rules, one checking for /etc/apparmor and another checking for /etc/apparmor.d. Both rules could be templated, therefore minimizing chance of error. You could use audit_rules_watch template for this. What do you think about this suggestion?

[mpurg]

@vojtapolasek thanks for reviewing. I agree that your solution is cleaner. To clarify, the reason I adapted the existing rule was to avoid introducing a breaking change downstream when using customized tailoring files. I.e. if a user disabled this rule using a tailoring file, and the rule was now renamed, the rule would no longer be disabled with the same tailoring file. Additionally, I assumed it was ok to modify the existing rule since the rule id contains mac_modifications and not selinux_modifications. Do you think that these rationales could justify the less clean code? Alternatively, I can implement your cleaner solution upstream and implement the current patch downstream only.

[vojtapolasek]

Hello @mpurg I understand you. However, I think it is not a problem.
If I understand it correctly, currently, the audit_rules_mac_modifications without modification you introduce here does not make sense on Ubuntu systems, because there is no Selinux.
Therefore, I believe people are using tailoring to remove this rule from the profile rather than adding it to a profile.
When you create a new rule, let's say audit_rules_apparmor_modifications and replace the rule audit_rules_mac_modifications in all its occurences in Ubuntu profiles, there would be no change for users regarding the audit_rules_mac_modifications. The rule would not exist in the profile, that is the same effect as if it would be tailored out.
The tailoring does not break if the tailored rule is not present in the profile. We can even ensure that the rule stays in the datastream if this is needed.
Would it be OK in this case?

[mpurg]

Hi @vojtapolasek

You're right that it does not make sense on Ubuntu, but the rule exists in the Ubuntu CIS profile nevertheless:

content/products/ubuntu2204/profiles/cis_level2_server.profile

Line 169 in 4e72fdd

- audit_rules_mac_modification

Since the rule exists in the profile and contains a reference to the Ubuntu CIS control 4.1.3.14, I believe that the users will by default assume that the rule correctly implements the CIS control, and will not tailor the rule out. Does that make sense?

[vojtapolasek]

@mpurg I am sorry, but I fail to understand the problem.
You wrote:

the reason I adapted the existing rule was to avoid introducing a breaking change downstream when using customized tailoring files. I.e. if a user disabled this rule using a tailoring file, and the rule was now renamed, the rule would no longer be disabled with the same tailoring file.

And then you wrote:

Since the rule exists in the profile and contains a reference to the Ubuntu CIS control 4.1.3.14, I believe that the users will by default assume that the rule correctly implements the CIS control, and will not tailor the rule out.

I assume that the rule would be removed from affected Ubuntu profiles and replaced with the new one. Could you please describe the situation in which it would cause problem?

[mpurg]

Apologies for the confusion, I should have added in my last comment, that the users will not tailor the rule out "because it is SELinux" as you suggested. They will likely assume that the rule is correctly implementing the apparmor check as stated in 4.1.3.14 and might tailor it out for another reason.

This is the scenario I'm concerned about:

1. user excludes the rule audit_rules_mac_modification because they don't want to monitor apparmor modifications
2. a new rule audit_rules_apparmor_modification is added to the profile, which is enabled by default in the profile and is not tailored out, thus enabling the monitoring of apparmor modifications

All that said, since we will soon release the Ubuntu 24.04 benchmark and update the 22.04 benchmark, I think it might be much cleaner to just make new rules as you suggested. Sorry for the back and forth :)

[vojtapolasek]

I understand. I will test and merge the modification. But I suggest we create new rules for both selinux and apparmor. I will create new rules for rhel10 and I suggest you also create new rules which you will use in upcoming updates.

[vojtapolasek]

@mpurg please rebase because there were some fixes for CI failures in recent PRs.

[vojtapolasek]

I created new rule for rhel10 which is more specific and less bloated in #12826

[mpurg]

@vojtapolasek I rebased and moved the apparmor logic out of the shared Bash and OVAL files and into Ubuntu-specific files, to avoid bloating the shared file unnecessarily.

[vojtapolasek]

LTM, thank you. The solution you propose keeps the rule less bloated.
I waive the failing tests because audit is not available in those containers.

[codeclimate]

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

View more on Code Climate.

[vojtapolasek]

/packit retest-failed