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

Fix in audit_rules_systadmin_actions and new rule audit_rules_sysadmi… #10685

Conversation

rumch-se
Copy link
Contributor

@rumch-se rumch-se commented Jun 5, 2023

…n_scope

Description:

  • Fixes in ansible part. Creation of a new rule

Rationale:

  • The current rule includes 2 CIS recommendations. With this PR a new rule was created to cover the recommendation related to scope

@rumch-se rumch-se requested a review from a team as a code owner June 5, 2023 13:39
@openshift-ci openshift-ci bot added the needs-ok-to-test Used by openshift-ci bot. label Jun 5, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jun 5, 2023

Hi @rumch-se. 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
Copy link

github-actions bot commented Jun 5, 2023

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

github-actions bot commented Jun 5, 2023

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

Click here to see the full diff
bash remediation for rule 'xccdf_org.ssgproject.content_rule_audit_rules_sysadmin_actions' differs.
--- xccdf_org.ssgproject.content_rule_audit_rules_sysadmin_actions
+++ xccdf_org.ssgproject.content_rule_audit_rules_sysadmin_actions
@@ -2,6 +2,7 @@
 if [ ! -f /.dockerenv ] && [ ! -f /run/.containerenv ] && rpm --quiet -q audit; then
 
 # Perform the remediation for both possible tools: 'auditctl' and 'augenrules'
+
 # Create a list of audit *.rules files that should be inspected for presence and correctness
 # of a particular audit rule. The scheme is as follows:
 #
@@ -134,7 +135,6 @@
         echo "-w /etc/sudoers -p wa -k actions" >> "$audit_rules_file"
     fi
 done
-
 # Create a list of audit *.rules files that should be inspected for presence and correctness
 # of a particular audit rule. The scheme is as follows:
 #

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_audit_rules_sysadmin_actions' differs.
--- xccdf_org.ssgproject.content_rule_audit_rules_sysadmin_actions
+++ xccdf_org.ssgproject.content_rule_audit_rules_sysadmin_actions
@@ -1,155 +1,6 @@
 - name: Gather the package facts
   package_facts:
     manager: auto
-  tags:
-  - CCE-80743-8
-  - CJIS-5.4.1.1
-  - NIST-800-171-3.1.7
-  - NIST-800-53-AC-2(7)(b)
-  - NIST-800-53-AC-6(9)
-  - NIST-800-53-AU-12(c)
-  - NIST-800-53-AU-2(d)
-  - NIST-800-53-CM-6(a)
-  - PCI-DSS-Req-10.2.2
-  - PCI-DSS-Req-10.2.5.b
-  - PCI-DSSv4-10.2.1.5
-  - PCI-DSSv4-10.2.2
-  - audit_rules_sysadmin_actions
-  - low_complexity
-  - low_disruption
-  - medium_severity
-  - no_reboot_needed
-  - restrict_strategy
-
-- name: Check if watch rule for /etc/sudoers already exists in /etc/audit/rules.d/
-  find:
-    paths: /etc/audit/rules.d
-    contains: ^\s*-w\s+/etc/sudoers\s+-p\s+wa(\s|$)+
-    patterns: '*.rules'
-  register: find_existing_watch_rules_d
-  when:
-  - '"audit" in ansible_facts.packages'
-  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
-  tags:
-  - CCE-80743-8
-  - CJIS-5.4.1.1
-  - NIST-800-171-3.1.7
-  - NIST-800-53-AC-2(7)(b)
-  - NIST-800-53-AC-6(9)
-  - NIST-800-53-AU-12(c)
-  - NIST-800-53-AU-2(d)
-  - NIST-800-53-CM-6(a)
-  - PCI-DSS-Req-10.2.2
-  - PCI-DSS-Req-10.2.5.b
-  - PCI-DSSv4-10.2.1.5
-  - PCI-DSSv4-10.2.2
-  - audit_rules_sysadmin_actions
-  - low_complexity
-  - low_disruption
-  - medium_severity
-  - no_reboot_needed
-  - restrict_strategy
-
-- name: Search /etc/audit/rules.d for other rules with specified key actions
-  find:
-    paths: /etc/audit/rules.d
-    contains: ^.*(?:-F key=|-k\s+)actions$
-    patterns: '*.rules'
-  register: find_watch_key
-  when:
-  - '"audit" in ansible_facts.packages'
-  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
-  - find_existing_watch_rules_d.matched is defined and find_existing_watch_rules_d.matched
-    == 0
-  tags:
-  - CCE-80743-8
-  - CJIS-5.4.1.1
-  - NIST-800-171-3.1.7
-  - NIST-800-53-AC-2(7)(b)
-  - NIST-800-53-AC-6(9)
-  - NIST-800-53-AU-12(c)
-  - NIST-800-53-AU-2(d)
-  - NIST-800-53-CM-6(a)
-  - PCI-DSS-Req-10.2.2
-  - PCI-DSS-Req-10.2.5.b
-  - PCI-DSSv4-10.2.1.5
-  - PCI-DSSv4-10.2.2
-  - audit_rules_sysadmin_actions
-  - low_complexity
-  - low_disruption
-  - medium_severity
-  - no_reboot_needed
-  - restrict_strategy
-
-- name: Use /etc/audit/rules.d/actions.rules as the recipient for the rule
-  set_fact:
-    all_files:
-    - /etc/audit/rules.d/actions.rules
-  when:
-  - '"audit" in ansible_facts.packages'
-  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
-  - find_watch_key.matched is defined and find_watch_key.matched == 0 and find_existing_watch_rules_d.matched
-    is defined and find_existing_watch_rules_d.matched == 0
-  tags:
-  - CCE-80743-8
-  - CJIS-5.4.1.1
-  - NIST-800-171-3.1.7
-  - NIST-800-53-AC-2(7)(b)
-  - NIST-800-53-AC-6(9)
-  - NIST-800-53-AU-12(c)
-  - NIST-800-53-AU-2(d)
-  - NIST-800-53-CM-6(a)
-  - PCI-DSS-Req-10.2.2
-  - PCI-DSS-Req-10.2.5.b
-  - PCI-DSSv4-10.2.1.5
-  - PCI-DSSv4-10.2.2
-  - audit_rules_sysadmin_actions
-  - low_complexity
-  - low_disruption
-  - medium_severity
-  - no_reboot_needed
-  - restrict_strategy
-
-- name: Use matched file as the recipient for the rule
-  set_fact:
-    all_files:
-    - '{{ find_watch_key.files | map(attribute=''path'') | list | first }}'
-  when:
-  - '"audit" in ansible_facts.packages'
-  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
-  - find_watch_key.matched is defined and find_watch_key.matched > 0 and find_existing_watch_rules_d.matched
-    is defined and find_existing_watch_rules_d.matched == 0
-  tags:
-  - CCE-80743-8
-  - CJIS-5.4.1.1
-  - NIST-800-171-3.1.7
-  - NIST-800-53-AC-2(7)(b)
-  - NIST-800-53-AC-6(9)
-  - NIST-800-53-AU-12(c)
-  - NIST-800-53-AU-2(d)
-  - NIST-800-53-CM-6(a)
-  - PCI-DSS-Req-10.2.2
-  - PCI-DSS-Req-10.2.5.b
-  - PCI-DSSv4-10.2.1.5
-  - PCI-DSSv4-10.2.2
-  - audit_rules_sysadmin_actions
-  - low_complexity
-  - low_disruption
-  - medium_severity
-  - no_reboot_needed
-  - restrict_strategy
-
-- name: Add watch rule for /etc/sudoers in /etc/audit/rules.d/
-  lineinfile:
-    path: '{{ all_files[0] }}'
-    line: -w /etc/sudoers -p wa -k actions
-    create: true
-    mode: '0640'
-  when:
-  - '"audit" in ansible_facts.packages'
-  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
-  - find_existing_watch_rules_d.matched is defined and find_existing_watch_rules_d.matched
-    == 0
   tags:
   - CCE-80743-8
   - CJIS-5.4.1.1
@@ -231,10 +82,10 @@
   - no_reboot_needed
   - restrict_strategy
 
-- name: Check if watch rule for /etc/sudoers.d/ already exists in /etc/audit/rules.d/
+- name: Check if watch rule for /etc/sudoers already exists in /etc/audit/rules.d/
   find:
     paths: /etc/audit/rules.d
-    contains: ^\s*-w\s+/etc/sudoers.d/\s+-p\s+wa(\s|$)+
+    contains: ^\s*-w\s+/etc/sudoers\s+-p\s+wa(\s|$)+
     patterns: '*.rules'
   register: find_existing_watch_rules_d
   when:
@@ -349,10 +200,10 @@
   - no_reboot_needed
   - restrict_strategy
 
-- name: Add watch rule for /etc/sudoers.d/ in /etc/audit/rules.d/
+- name: Add watch rule for /etc/sudoers in /etc/audit/rules.d/
   lineinfile:
     path: '{{ all_files[0] }}'
-    line: -w /etc/sudoers.d/ -p wa -k actions
+    line: -w /etc/sudoers -p wa -k actions
     create: true
     mode: '0640'
   when:
@@ -440,3 +291,152 @@
   - medium_severity
   - no_reboot_needed
   - restrict_strategy
+
+- name: Check if watch rule for /etc/sudoers.d/ already exists in /etc/audit/rules.d/
+  find:
+    paths: /etc/audit/rules.d
+    contains: ^\s*-w\s+/etc/sudoers.d/\s+-p\s+wa(\s|$)+
+    patterns: '*.rules'
+  register: find_existing_watch_rules_d
+  when:
+  - '"audit" in ansible_facts.packages'
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  tags:
+  - CCE-80743-8
+  - CJIS-5.4.1.1
+  - NIST-800-171-3.1.7
+  - NIST-800-53-AC-2(7)(b)
+  - NIST-800-53-AC-6(9)
+  - NIST-800-53-AU-12(c)
+  - NIST-800-53-AU-2(d)
+  - NIST-800-53-CM-6(a)
+  - PCI-DSS-Req-10.2.2
+  - PCI-DSS-Req-10.2.5.b
+  - PCI-DSSv4-10.2.1.5
+  - PCI-DSSv4-10.2.2
+  - audit_rules_sysadmin_actions
+  - low_complexity
+  - low_disruption
+  - medium_severity
+  - no_reboot_needed
+  - restrict_strategy
+
+- name: Search /etc/audit/rules.d for other rules with specified key actions
+  find:
+    paths: /etc/audit/rules.d
+    contains: ^.*(?:-F key=|-k\s+)actions$
+    patterns: '*.rules'
+  register: find_watch_key
+  when:
+  - '"audit" in ansible_facts.packages'
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - find_existing_watch_rules_d.matched is defined and find_existing_watch_rules_d.matched
+    == 0
+  tags:
+  - CCE-80743-8
+  - CJIS-5.4.1.1
+  - NIST-800-171-3.1.7
+  - NIST-800-53-AC-2(7)(b)
+  - NIST-800-53-AC-6(9)
+  - NIST-800-53-AU-12(c)
+  - NIST-800-53-AU-2(d)
+  - NIST-800-53-CM-6(a)
+  - PCI-DSS-Req-10.2.2
+  - PCI-DSS-Req-10.2.5.b
+  - PCI-DSSv4-10.2.1.5
+  - PCI-DSSv4-10.2.2
+  - audit_rules_sysadmin_actions
+  - low_complexity
+  - low_disruption
+  - medium_severity
+  - no_reboot_needed
+  - restrict_strategy
+
+- name: Use /etc/audit/rules.d/actions.rules as the recipient for the rule
+  set_fact:
+    all_files:
+    - /etc/audit/rules.d/actions.rules
+  when:
+  - '"audit" in ansible_facts.packages'
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - find_watch_key.matched is defined and find_watch_key.matched == 0 and find_existing_watch_rules_d.matched
+    is defined and find_existing_watch_rules_d.matched == 0
+  tags:
+  - CCE-80743-8
+  - CJIS-5.4.1.1
+  - NIST-800-171-3.1.7
+  - NIST-800-53-AC-2(7)(b)
+  - NIST-800-53-AC-6(9)
+  - NIST-800-53-AU-12(c)
+  - NIST-800-53-AU-2(d)
+  - NIST-800-53-CM-6(a)
+  - PCI-DSS-Req-10.2.2
+  - PCI-DSS-Req-10.2.5.b
+  - PCI-DSSv4-10.2.1.5
+  - PCI-DSSv4-10.2.2
+  - audit_rules_sysadmin_actions
+  - low_complexity
+  - low_disruption
+  - medium_severity
+  - no_reboot_needed
+  - restrict_strategy
+
+- name: Use matched file as the recipient for the rule
+  set_fact:
+    all_files:
+    - '{{ find_watch_key.files | map(attribute=''path'') | list | first }}'
+  when:
+  - '"audit" in ansible_facts.packages'
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - find_watch_key.matched is defined and find_watch_key.matched > 0 and find_existing_watch_rules_d.matched
+    is defined and find_existing_watch_rules_d.matched == 0
+  tags:
+  - CCE-80743-8
+  - CJIS-5.4.1.1
+  - NIST-800-171-3.1.7
+  - NIST-800-53-AC-2(7)(b)
+  - NIST-800-53-AC-6(9)
+  - NIST-800-53-AU-12(c)
+  - NIST-800-53-AU-2(d)
+  - NIST-800-53-CM-6(a)
+  - PCI-DSS-Req-10.2.2
+  - PCI-DSS-Req-10.2.5.b
+  - PCI-DSSv4-10.2.1.5
+  - PCI-DSSv4-10.2.2
+  - audit_rules_sysadmin_actions
+  - low_complexity
+  - low_disruption
+  - medium_severity
+  - no_reboot_needed
+  - restrict_strategy
+
+- name: Add watch rule for /etc/sudoers.d/ in /etc/audit/rules.d/
+  lineinfile:
+    path: '{{ all_files[0] }}'
+    line: -w /etc/sudoers.d/ -p wa -k actions
+    create: true
+    mode: '0640'
+  when:
+  - '"audit" in ansible_facts.packages'
+  - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+  - find_existing_watch_rules_d.matched is defined and find_existing_watch_rules_d.matched
+    == 0
+  tags:
+  - CCE-80743-8
+  - CJIS-5.4.1.1
+  - NIST-800-171-3.1.7
+  - NIST-800-53-AC-2(7)(b)
+  - NIST-800-53-AC-6(9)
+  - NIST-800-53-AU-12(c)
+  - NIST-800-53-AU-2(d)
+  - NIST-800-53-CM-6(a)
+  - PCI-DSS-Req-10.2.2
+  - PCI-DSS-Req-10.2.5.b
+  - PCI-DSSv4-10.2.1.5
+  - PCI-DSSv4-10.2.2
+  - audit_rules_sysadmin_actions
+  - low_complexity
+  - low_disruption
+  - medium_severity
+  - no_reboot_needed
+  - restrict_strategy

New content has different text for rule 'xccdf_org.ssgproject.content_rule_audit_sudo_log_events'.
--- xccdf_org.ssgproject.content_rule_audit_sudo_log_events
+++ xccdf_org.ssgproject.content_rule_audit_sudo_log_events
@@ -13,10 +13,25 @@
 -w /var/log/sudo.log -p wa -k maintenance
 
 [reference]:
+BP28(R73)
+
+[reference]:
 CCI-000172
 
 [reference]:
 CCI-002884
+
+[reference]:
+Req-10.2.2
+
+[reference]:
+Req-10.2.5.b
+
+[reference]:
+10.2.1.5
+
+[reference]:
+10.2.2
 
 [reference]:
 SRG-OS-000392-GPOS-00172

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_audit_sudo_log_events' differs.
--- xccdf_org.ssgproject.content_rule_audit_sudo_log_events
+++ xccdf_org.ssgproject.content_rule_audit_sudo_log_events
@@ -3,6 +3,10 @@
     manager: auto
   tags:
   - CCE-86432-2
+  - PCI-DSS-Req-10.2.2
+  - PCI-DSS-Req-10.2.5.b
+  - PCI-DSSv4-10.2.1.5
+  - PCI-DSSv4-10.2.2
   - audit_sudo_log_events
   - low_complexity
   - low_disruption
@@ -21,6 +25,10 @@
   - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
   tags:
   - CCE-86432-2
+  - PCI-DSS-Req-10.2.2
+  - PCI-DSS-Req-10.2.5.b
+  - PCI-DSSv4-10.2.1.5
+  - PCI-DSSv4-10.2.2
   - audit_sudo_log_events
   - low_complexity
   - low_disruption
@@ -41,6 +49,10 @@
     == 0
   tags:
   - CCE-86432-2
+  - PCI-DSS-Req-10.2.2
+  - PCI-DSS-Req-10.2.5.b
+  - PCI-DSSv4-10.2.1.5
+  - PCI-DSSv4-10.2.2
   - audit_sudo_log_events
   - low_complexity
   - low_disruption
@@ -59,6 +71,10 @@
     is defined and find_existing_watch_rules_d.matched == 0
   tags:
   - CCE-86432-2
+  - PCI-DSS-Req-10.2.2
+  - PCI-DSS-Req-10.2.5.b
+  - PCI-DSSv4-10.2.1.5
+  - PCI-DSSv4-10.2.2
   - audit_sudo_log_events
   - low_complexity
   - low_disruption
@@ -77,6 +93,10 @@
     is defined and find_existing_watch_rules_d.matched == 0
   tags:
   - CCE-86432-2
+  - PCI-DSS-Req-10.2.2
+  - PCI-DSS-Req-10.2.5.b
+  - PCI-DSSv4-10.2.1.5
+  - PCI-DSSv4-10.2.2
   - audit_sudo_log_events
   - low_complexity
   - low_disruption
@@ -97,6 +117,10 @@
     == 0
   tags:
   - CCE-86432-2
+  - PCI-DSS-Req-10.2.2
+  - PCI-DSS-Req-10.2.5.b
+  - PCI-DSSv4-10.2.1.5
+  - PCI-DSSv4-10.2.2
   - audit_sudo_log_events
   - low_complexity
   - low_disruption
@@ -115,6 +139,10 @@
   - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
   tags:
   - CCE-86432-2
+  - PCI-DSS-Req-10.2.2
+  - PCI-DSS-Req-10.2.5.b
+  - PCI-DSSv4-10.2.1.5
+  - PCI-DSSv4-10.2.2
   - audit_sudo_log_events
   - low_complexity
   - low_disruption
@@ -136,6 +164,10 @@
     == 0
   tags:
   - CCE-86432-2
+  - PCI-DSS-Req-10.2.2
+  - PCI-DSS-Req-10.2.5.b
+  - PCI-DSSv4-10.2.1.5
+  - PCI-DSSv4-10.2.2
   - audit_sudo_log_events
   - low_complexity
   - low_disruption

@rumch-se
Copy link
Contributor Author

rumch-se commented Jun 5, 2023

Hello @vojtapolasek
I think that macros created by you ansible_audit_augenrules_add_watch_rule, and ansible_audit_auditctl_add_watch_rule do not work as expected in a specific situation like this.
1/
On SUSE we have to add 4 records for auditing in audit.rules located in /etc/audit or to created 2 files into /etc/audit/rules.d/ - scope.rules and privileged-actions.rules which include these records

these records are:
-w /etc/sudoers -p wa -k actions
-w /etc/sudoers.d -p wa -k actions
-w /etc/sudoers -p wa -k scope
-w /etc/sudoers.d -p wa -k scope
2/When I checked for existence of the actions with oscap xccdf eval command - my test failed because I did not have records and file - this was expected result
3/I executed ansible remediation - and my system was fixed - this was expected result
4/I checked for the existence of the scope via oscap xccdf eval command - my test failed because I did not have records and file for the keyword scope - this was expected result
5/I executed ansible remediation - and my system was not fixed

You can simulate this manually
add these records based on keyword scope
-w /etc/sudoers -p wa -k scope
-w /etc/sudoers.d -p wa -k scope
to /etc/audit/audit.rules
and
create a file scope.rules in /etc/audit/rules.d/ which include these records

Now execute oscap xccdf eval command which checks for the rule audit_rules_sysadmin_actions - it should fail when you don't have expected file and records
Now execute a command for ansible remediation againts the rule audit_rules_sysadmin_actions and to see if your system will be remediated.
I think that NO, and the problem is coming from the fact that in your macro you check only the beginning of the line: -w /etc/sudoers -p wa, but you don't check for the remaining part with the key.

Because of that when we have similar rules which begin with same string only the 1st rule will make remediation (will add rules), but the remaining rules will not. For example I tried this - to check/remediate my system with the keyword scope - it worked, but after I was not able to remediated my system with the keyword actions.

NOTE: ALL TESTS WHICH I DID WERE IN ANSIBLE, I DID NOT TEST BASH.

Have a nice day
Rumen

@Mab879 Mab879 added this to the 0.1.69 milestone Jun 5, 2023
@Mab879 Mab879 added SLES SUSE Linux Enterprise Server product related. New Rule Issues or pull requests related to new Rules. Update Profile Issues or pull requests related to Profiles updates. labels Jun 5, 2023
@teacup-on-rockingchair
Copy link
Contributor

If I understand correctly the patches in the PR do not address the issue right?

As far as I could see an example of the problem is that if one is applied remediation for the audit_rules_sysadmin_actions rule for example, later on the audit_rules_sysadmin_scope remediation will not work, since the key param is not checked by the bash and ansible macros as part of the audit rules statements.

Also can confirm that the above is valid for bash remediation also.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rumch-se I am not sure what happens here but do you get different results if you change the order of the macros, because otherwise the files are identical right?

Copy link
Contributor Author

@rumch-se rumch-se Jun 6, 2023

Choose a reason for hiding this comment

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

Hello @teacup-on-rockingchair,

No I did not get different results. i.e. if you execute 1st audit_rules_sysadmin_scope , in this case audit_rules_sysadmin_actions will not work. I changed the order of macros in ansible, to correspond to the order of macros in bash. But the main problem is in macros, because if we have 2+ rules which have identical parts - only the 1st executed rule will remediate the system. At the same time documentation of the macros shows that we have to be able to add new rows - which is not correct statement. This is the reason why I have addressed the issue to the creator of the macros.

Have a nice day.
Rumen

@vojtapolasek
Copy link
Collaborator

Hello @rumch-se, I think the Ansible macros are working correctly.
If I understand your question correctly, your goal is to add two pairs of rules into two different files in /etc/audit/rules.d directory. The rules are nearly identical, the only part which is different is their key.
But macros disregard the key because it does not affect the behavior of the rule - the only purpose of the key is to make the log searching easier. Therefore, only one instance of those rules will be present. The key parameter is even not checked in OVAL.
Honestly, I am not sure what is the purpose of adding those rules twice. I am afraid that you will actually don't get the expected result, you can try it your self:

touch /root/test
echo "-w /root/test -p wa -k hello" > /etc/audit/rules.d/test.rules
echo "-w /root/test -p wa -k world" >> /etc/audit/rules.d/test.rules
augenrules --load
echo "testing" > /root/test
ausearch -k hello
ausearch -k world

I got different results for each key, although the rules were the same.
I checked latest CIS benchmarks for SLE 15 and SLE 12 and I see there the rule ending with the "scope" key. But the rule checking for "actions" key actually has different watch part - it checks for the Sudo log file, not for the Sudo configuration.
In conclusion, trying to remediate two Audit rules where the only difference is the key is something which should not happen.

@rumch-se
Copy link
Contributor Author

rumch-se commented Jun 6, 2023

Hello @vojtapolasek

There are two rules which are nearly identical, and this macro does not manage them well. The pictures are attached. Also @teacup-on-rockingchair can confirm that bash part also does not work as expected. The problem is that you don't check the whole string but only part of it. The expected behavior should be when the audit rule is not exist it should be added, when exist - do nothing. But when you check for the existence you check only the first part of the audit rule, and when they are equal, but key is different - the audit rules with different key will not be added. Please see the pictures and simulate a case when you have already one audit rule and try to add another rule but with a different key.
Note: In case of CIS - adding rules for actions and scope will work, but DISA has a different view about actions .. and in this case adding of rules for action and scope will be not possible.
macro-1
macro-2

@vojtapolasek
Copy link
Collaborator

Hello @rumch-se. Thank you for the screenshots. I still think that the way in which the macro works now is reasonable, and mainly that rules audit_rules_sysadmin_scope and audit_rules_sysadmin_actions should NOT be in the same profile.
I understand the problem you are facing, but I think that the problem is not due to macros but to incorrect composition of rules.
In which profile do you face the problem you describe?

Please see rationale and discussion in this PR: #9463
Moreover, I still think that utilizing two audit rules which watch for the same file and same permissions and the only difference is the key is bad practice. @stevegrubb, could you please confirm what is the result of of using two Audit watch rules watching the same path and same permission changes with only difference being the key? My understanding is that only one of those rules wil get processed or results will not be consistent.

@rumch-se
Copy link
Contributor Author

rumch-se commented Jun 7, 2023

Hello @vojtapolasek, @stevegrubb

I am not agree with you @vojtapolasek, because now there is a risk of an "open door", if I modify the file /etc/audit/audit.rules by using this rule: -w /etc/sudoers -p wa -k skip and have an actions.file in /etc/audit/rules.d/ with same rules - the oval check will pass, and remediation will not add a record .i.e a potential attacker can modify audit part of the system in his favor, and like that the automation will be broken. In the information security we have the principle - "Expect the unexpected" and we as security experts want to control the situation via hardening. You can see the screenshot - which shows that.

Have a nice day
Rumen

macro_3

@teacup-on-rockingchair
Copy link
Contributor

ausearch -k world

@vojtapolasek if I understand correctly your point is that, the audit rules main point is to watch access to files, and the oval should be following that.

At the same time -k somekey is just convenience for searching/deleting the auditing rules.

If we are following those guidance the new rule oval will be better off just checking the main part of the rule , instead of the key and the same probably should be valid of remediations also?

@vojtapolasek
Copy link
Collaborator

@teacup-on-rockingchair yes, my point is that the key specified after the -k argument is there only for humans. It does not affect the event which the rule is looking for.
It is explained here: https://www.man7.org/linux/man-pages/man7/audit.rules.7.html#NOTES
My point is that our automation should NOT enforce usage of specific keys - this is up to the system administrator.

@rumch-se
Copy link
Contributor Author

rumch-se commented Jun 7, 2023

Hello @vojtapolasek

I would like to clarify my point here.

If somebody in moment t0 succeeded to include -w /etc/sudoers -p wa -k skip, then the administrator via automation will not include a rule with his desired key - for example action in a moment t1 - because the audit will show pass.
In this case when the logs should be analyses by the administrator in moment tn - he will use an automatic log system, or he will use grep with the key defined by him, and he will not find any log records, because these records will have key skip instead of correct key action. In other words the activities of the attacker will be obfuscated.

Have a nice day
Rumen

@teacup-on-rockingchair
Copy link
Contributor

@teacup-on-rockingchair yes, my point is that the key specified after the -k argument is there only for humans. It does not affect the event which the rule is looking for. It is explained here: https://www.man7.org/linux/man-pages/man7/audit.rules.7.html#NOTES My point is that our automation should NOT enforce usage of specific keys - this is up to the system administrator.

I tend to agree with @rumch-se with that the scenario with the skip keys maybe a valid one, but also due to the cited notes in the manual we obviously cannot enforce that policy to the common framework.
So I would suggest to SLES specific solution for this CIS rule, which will both enforce the key as integral part of the oval check and remediations and also will not affect other platforms. Of course the only possible problem would be executing audit_rules_systadmin_actions and new rule audit_rules_sysadmin_scope one after another, but as long as we keep those rules in separate profiles, this should not be an issue. I am guessing somebody from @ComplianceAsCode/content-development can also give opinon if that looks as workable solution?

@vojtapolasek
Copy link
Collaborator

Hello @rumch-se @teacup-on-rockingchair, thank you for explaining your point of view.
I am glad that we all agree that putting both rules in one profile is not a good idea.

If somebody in moment t0 succeeded to include -w /etc/sudoers -p wa -k skip, then the administrator via automation wil not include a rule with his desired key - for example action in a moment t1 - because the audit will show pass.

I think that if an attacker manages to write into that file which is only writable by root, you have much bigger problem than missing Audit records.

In this case when the logs should be analyses by the administrator in moment tn - he will use an automatic log system, or he will use grep with the key defined by him, and he will not find any log records, because these records will have key skip instead of correct key action. In other words the activities of the attacker will be obfuscated.

The key is not the only parameter which you can use for searching in Audit logs.

I still think that having such a rule can bring more problems than it is worth it, but I respect your approach.
However, I don't think the change you suggest should happen for ALL audit rules in the project. I can imagine that the macro can get an additional parameter which will indicate if the remediation will check for the exact line or not.
Feel free to extend the macro along those lines.
Also please mention the fact that you are checking for the exact line either in the rule title or in the rule description.
I hope this is acceptable for all of us.

@marcusburghardt
Copy link
Member

Regarding the discussion about the key value, the value informed as key is just used for convenience. It has no technical impact in the other parameters of the rule. If the admin informs anything or nothing, the events are still collected according to the other parameters in the rule. So I agree these macros are correct for not caring about the key. The value entered as a key or even the use of keys is at the discretion of the administrator.

Furthermore, relying exclusively on keys to search logs seems to be a weak approach in the case of a real audit.
The requirement asks to ensure that events are collected, but does not (and should not) dictate how an auditor must investigate these logs.

I would be fine if you extend the macro to optionally consider keys in audit rules since this does not affect the current behavior. I would also respect if you desire to enforce specific keys in SUSE, but I would recommend to reconsider this approach because I can imagine it bringing more issues than value to the users.

@rumch-se
Copy link
Contributor Author

Hello @marcusburghardt

I understand that the key is used for convenience, and its definition is in discretion of administrator - i.e. he/she has to control the usage of the key. We will have a problem when the administrator has to manage a lot of systems - in my practice we had an administrator responsible for 300+ servers on daily basis - and he had to use automation tools to succeeded with his tasks. According to the current realization of the oval check (which we use for audit) - the test will pass when we have any key after the -k option. There is a risk the administrator of the system not to know that for a specific type of event the key is not equal to the key he expected to be used. Usually after each patch (security update) - the administrator have to execute again audit+remediation. If the audit checks also for the key, the administrator will know that something is not OK with a specific system. From a security point of view we have to control input and not to allow something to be injected - otherwise we will have impact on the results ... and the results are important for the next actions. In the bank industry there is a classical example - when the programmer of the banking system used the exchange rate to his benefits - he transferred from customer accounts to his account the value after the 2nd position of the decimal point. (when the exchange rate is 1.23465 - he kept 1.23 in the customer account and took the remaining part). From such of rounding operations "he made" ~1000 daily.

Have a nice day
Rumen

@marcusburghardt
Copy link
Member

Hello @marcusburghardt

I understand that the key is used for convenience, and its definition is in discretion of administrator - i.e. he/she has to control the usage of the key. We will have a problem when the administrator has to manage a lot of systems - in my practice we had an administrator responsible for 300+ servers on daily basis - and he had to use automation tools to succeeded with his tasks. According to the current realization of the oval check (which we use for audit) - the test will pass when we have any key after the -k option. There is a risk the administrator of the system not to know that for a specific type of event the key is not equal to the key he expected to be used. Usually after each patch (security update) - the administrator have to execute again audit+remediation. If the audit checks also for the key, the administrator will know that something is not OK with a specific system. From a security point of view we have to control input and not to allow something to be injected - otherwise we will have impact on the results ... and the results are important for the next actions. In the bank industry there is a classical example - when the programmer of the banking system used the exchange rate to his benefits - he transferred from customer accounts to his account the value after the 2nd position of the decimal point. (when the exchange rate is 1.23465 - he kept 1.23 in the customer account and took the remaining part). From such of rounding operations "he made" ~1000 daily.

Have a nice day Rumen

This administrator can check the same events regardless of the keys values. But as I said, I understand if you prefer to enforce a specific key for audit rules, but please don't change this behavior for all rules. You can make it optional and not enabled by default.

@stevegrubb
Copy link
Contributor

Late to the game...

  1. having 2 rules to the same file with different key is a waste on memory. The first rule will always win. However, if the watch is on a directory and one has the trailing slash and the other doesn't, they are different rules. One watches the files in the directory and the other watches the directory entries itself to see if anything gets added or deleted. It is for this reason that '-w' should be avoided and '-F path' and '-F dir' should be used to make this very clear. -F path on a directory watches the directory entries. -F dir watches the files in the directory.
  2. "-k argument" is there to aid in investigations. Aureport --summary --key can help see what has been happening on the system. Then you can see what users are triggering the rule, ausearch --key some-key --raw | aurport --summary --user -i. It can also be used to tie several rules together that work together looking for a specific problem.
  3. The audit package tries to ship rules with opinionated keys to meet specific requirements. You can always add a second or third key to any syscall rule.

@vojtapolasek vojtapolasek modified the milestones: 0.1.69, 0.1.70 Jul 18, 2023
@rumch-se
Copy link
Contributor Author

Hello @marcusburghardt and @teacup-on-rockingchair

Would be possible this PR to be merged to the master?
Have a nice day
Rumen

Copy link
Member

@marcusburghardt marcusburghardt left a comment

Choose a reason for hiding this comment

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

This PR is basically duplicating the audit_rules_sysadmin_actions rule without adding value to the project. The audit_rules_sysadmin_scope rule is unnecessary and including both audit_rules_sysadmin_scope and audit_rules_sysadmin_actions in the same profile, as tried in this PR, will cause avoidable waste of resources and confusion.

If you want to create a mechanism where the key is assessed, you should implement this mechanism optionally in the existing OVAL. You can also introduce a variable if you want more flexibility for keys names in remediation.

Finally, I strongly recommend to avoid creating duplicated audit rules with different keys. If the Benchmark is asking this, the wiser approach would be to update the benchmark instead. As mentioned by the author of the auditd, it is a waste of memory with no real benefit because the first processed matching rule will always win.

@marcusburghardt
Copy link
Member

Hello @marcusburghardt and @teacup-on-rockingchair

Would be possible this PR to be merged to the master? Have a nice day Rumen

I don't think so @rumch-se . There are serious concerns about this approach, as mentioned in the comments. They should first be solved.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Used by openshift-ci bot. label Jul 21, 2023
@rumch-se rumch-se force-pushed the fix_in_audit_rules_sysadmin_actions branch from 225b07e to 0f47fbc Compare July 21, 2023 10:46
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Used by openshift-ci bot. label Jul 21, 2023
@rumch-se
Copy link
Contributor Author

Hello @marcusburghardt

I have considered your last proposals and

  • removed from the PR the rule audit_rules_sysadmin_scope
  • adapted the rule audit_sudo_log_events

After these changes the rule audit_rules_sysadmin_actions covers SLE12/15 CIS requirement 4.1.14 - Ensure changes to system administration scope (sudoers) is collected (Automated), and the rule audit_sudo_log_events covers SLE12/15 CIS requirement Ensure system administrator actions (sudolog) are collected (Automated)

Have a nice day
Rumen

@marcusburghardt
Copy link
Member

Hello @marcusburghardt

I have considered your last proposals and

  • removed from the PR the rule audit_rules_sysadmin_scope
  • adapted the rule audit_sudo_log_events

After these changes the rule audit_rules_sysadmin_actions covers SLE12/15 CIS requirement 4.1.14 - Ensure changes to system administration scope (sudoers) is collected (Automated), and the rule audit_sudo_log_events covers SLE12/15 CIS requirement Ensure system administrator actions (sudolog) are collected (Automated)

Have a nice day Rumen

Thanks for considering @rumch-se . The concerns were solved with this last commit. I will wait the CI tests to finish but it seems good to me.

@codeclimate
Copy link

codeclimate bot commented Jul 21, 2023

Code Climate has analyzed commit 0f47fbc 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.2% (0.0% change).

View more on Code Climate.

Copy link
Member

@marcusburghardt marcusburghardt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @rumch-se .
It is up to you now @teacup-on-rockingchair

Copy link
Contributor

@teacup-on-rockingchair teacup-on-rockingchair left a comment

Choose a reason for hiding this comment

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

👍

@teacup-on-rockingchair teacup-on-rockingchair merged commit 55ddc6a into ComplianceAsCode:master Jul 26, 2023
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ok-to-test Used by openshift-ci bot. New Rule Issues or pull requests related to new Rules. SLES SUSE Linux Enterprise Server product related. Update Profile Issues or pull requests related to Profiles updates.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants