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

Refactor audit_rules_privileged_commands to include in CIS #10326

Merged
merged 11 commits into from
Apr 4, 2023

Conversation

marcusburghardt
Copy link
Member

Description:

The 4.1.3.6 CIS requirement for RHEL8 and RHEL9 and the 4.1.11 CIS requirement for RHEL7 are now satisfied by the
audit_rules_privileged_commands rule.

Initially, the rule demanded an update to consider -F perm=x in audit rules since the rule is about execution of Privileged Commands which, according to the CIS Benchmark, are commands with suidbit or sgidbit.

However, when reviewing this rule, I found enough issues to consider a complete review and refactoring of this rule.
The main points of this refactoring are:

  • More accurate rule description, rationale, ocil and warning.
  • Much better readability and maintainability for OVAL.
  • Improve OVAL efficiency when searching for Privileged Commands only in relevant mount points.
  • More efficient Bash remediation aligned to common approach in the project
  • More efficient Ansible remediation taking advantage of Ansible Facts instead of shell commands.
  • Review and update of all test scenario scripts to ensure all cases are properly covered.

Rationale:

  • Remove rule inefficiency and consequently increasing performance
  • Better user experience
  • Better CIS coverage for RHEL.

Review Hints:

I refactored the rule step by step and committed the changes in a logical flow. Therefore, I recommend to review commit by commit in order and check the commit message for more context about the changes.

@marcusburghardt marcusburghardt added RHEL Red Hat Enterprise Linux product related. Ansible Ansible remediation update. OVAL OVAL update. Related to the systems assessments. Bash Bash remediation update. Update Rule Issues or pull requests related to Rules updates. CIS CIS Benchmark related. labels Mar 10, 2023
@marcusburghardt marcusburghardt added this to the 0.1.67 milestone Mar 10, 2023
@marcusburghardt marcusburghardt requested a review from a team as a code owner March 10, 2023 15:45
@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

github-actions bot commented Mar 10, 2023

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_privileged_commands'.
--- xccdf_org.ssgproject.content_rule_audit_rules_privileged_commands
+++ xccdf_org.ssgproject.content_rule_audit_rules_privileged_commands
@@ -3,29 +3,32 @@
 Ensure auditd Collects Information on the Use of Privileged Commands
 
 [description]:
-The audit system should collect information about usage of privileged
-commands for all users and root. To find the relevant setuid /
-setgid programs, run the following command for each local partition
-PART:
-$ sudo find PART -xdev -type f -perm -4000 -o -type f -perm -2000 2>/dev/null
-If the auditd daemon is configured to use the augenrules
-program to read audit rules during daemon startup (the default), add a line of
-the following form to a file with suffix .rules in the directory
-/etc/audit/rules.d for each setuid / setgid program on the system,
-replacing the SETUID_PROG_PATH part with the full path of that setuid /
-setgid program in the list:
--a always,exit -F path=SETUID_PROG_PATH -F auid>=1000 -F auid!=unset -F key=privileged
-If the auditd daemon is configured to use the auditctl
-utility to read audit rules during daemon startup, add a line of the following
-form to /etc/audit/audit.rules for each setuid / setgid program on the
-system, replacing the SETUID_PROG_PATH part with the full path of that
-setuid / setgid program in the list:
--a always,exit -F path=SETUID_PROG_PATH -F auid>=1000 -F auid!=unset -F key=privileged
+The audit system should collect information about usage of privileged commands for all users.
+These are commands with suid or sgid bits on and they are specially risky in local block
+device partitions not mounted with noexec and nosuid options. Therefore, these partitions
+should be first identified by the following command:
+findmnt -n -l -k -it $(awk '/nodev/ { print $2 }' /proc/filesystems | paste -sd,) | grep -Pv "noexec|nosuid"
+
+For all partitions listed by the previous command, it is necessary to search for
+setuid / setgid programs using the following command:
+$ sudo find PARTITION -xdev -perm /6000 -type f 2>/dev/null
+
+For each setuid / setgid program identified by the previous command, an audit rule must be
+present in the appropriate place using the following line structure:
+-a always,exit -F path=PROG_PATH -F perm=x -F auid>=1000 -F auid!=unset -F key=privileged
+
+If the auditd daemon is configured to use the augenrules program to read
+audit rules during daemon startup, add the line to a file with suffix .rules in the
+/etc/audit/rules.d directory, replacing the PROG_PATH part with the full path
+of that setuid / setgid identified program.
+
+If the auditd daemon is configured to use the auditctl utility instead, add
+the line to the /etc/audit/audit.rules file, also replacing the PROG_PATH part
+with the full path of that setuid / setgid identified program.
 
 [warning]:
-This rule checks for multiple syscalls related to privileged commands;
-it was written with DISA STIG in mind. Other policies should use a
-separate rule for each syscall that needs to be checked. For example:
+This rule checks for multiple syscalls related to privileged commands. If needed to check
+specific privileged commands, other more specific rules should be considered. For example:
 audit_rules_privileged_commands_suaudit_rules_privileged_commands_umountaudit_rules_privileged_commands_passwd
 
 [reference]:
@@ -446,19 +449,18 @@
 SRG-OS-000471-VMM-001910
 
 [reference]:
-4.1.13
+4.1.3.6
 
 [rationale]:
-Misuse of privileged functions, either intentionally or unintentionally by
-authorized users, or by unauthorized external entities that have compromised system accounts,
-is a serious and ongoing concern and can have significant adverse impacts on organizations.
-Auditing the use of privileged functions is one way to detect such misuse and identify
-the risk from insider and advanced persistent threats.
-
-Privileged programs are subject to escalation-of-privilege attacks,
-which attempt to subvert their normal role of providing some necessary but
-limited capability. As such, motivation exists to monitor these programs for
-unusual activity.
+Misuse of privileged functions, either intentionally or unintentionally by authorized users,
+or by unauthorized external entities that have compromised system accounts, is a serious and
+ongoing concern that can have significant adverse impacts on organizations.
+Auditing the use of privileged functions is one way to detect such misuse and identify the
+risk from insider and advanced persistent threats.
+
+Privileged programs are subject to escalation-of-privilege attacks, which attempt to subvert
+their normal role of providing some necessary but limited capability. As such, motivation
+exists to monitor these programs for unusual activity.
 
 [ident]:
 CCE-80724-8

OVAL for rule 'xccdf_org.ssgproject.content_rule_audit_rules_privileged_commands' differs.
--- oval:ssg-audit_rules_privileged_commands:def:1
+++ oval:ssg-audit_rules_privileged_commands:def:1
@@ -1,9 +1,9 @@
 criteria OR
 criteria AND
 extend_definition oval:ssg-audit_rules_augenrules:def:1
-criterion oval:ssg-test_arpc_suid_sgid_augenrules:tst:1
-criterion oval:ssg-test_arpc_bin_count_equals_rules_count_augenrules:tst:1
+criterion oval:ssg-test_augenrules_all_priv_cmds_covered:tst:1
+criterion oval:ssg-test_augenrules_count_matches_system_priv_cmds:tst:1
 criteria AND
 extend_definition oval:ssg-audit_rules_auditctl:def:1
-criterion oval:ssg-test_arpc_suid_sgid_auditctl:tst:1
-criterion oval:ssg-test_arpc_bin_count_equals_rules_count_auditctl:tst:1
+criterion oval:ssg-test_auditctl_all_priv_cmds_covered:tst:1
+criterion oval:ssg-test_auditctl_count_matches_system_priv_cmds:tst:1

OCIL for rule 'xccdf_org.ssgproject.content_rule_audit_rules_privileged_commands' differs.
--- ocil:ssg-audit_rules_privileged_commands_ocil:questionnaire:1
+++ ocil:ssg-audit_rules_privileged_commands_ocil:questionnaire:1
@@ -1,11 +1,16 @@
-To verify that auditing of privileged command use is configured, run the
-following command for each local partition PART to find relevant
-setuid / setgid programs:
-$ sudo find PART -xdev -type f -perm -4000 -o -type f -perm -2000 2>/dev/null
-Run the following command to verify entries in the audit rules for all programs
-found with the previous command:
-$ sudo grep path /etc/audit/audit.rules
-All relevant setuid / setgid programs have a line
-in the audit rules.
+To verify that auditing of privileged command use is configured, run the following command
+to search privileged commands in relevant partitions and check if they are covered by auditd
+rules:
+
+FILTER_NODEV=$(awk '/nodev/ { print $2 }' /proc/filesystems | paste -sd,)
+PARTITIONS=$(findmnt -n -l -k -it $FILTER_NODEV | grep -Pv "noexec|nosuid" | awk '{ print $1 }')
+for PARTITION in $PARTITIONS; do
+ for PRIV_CMD in $(find "${PARTITION}" -xdev -perm /6000 -type f 2>/dev/null); do
+ grep -qr "${PRIV_CMD}" /etc/audit/rules.d /etc/audit/audit.rules &&
+ printf "OK: ${PRIV_CMD}\n" || printf "WARNING - rule not found for: ${PRIV_CMD}\n"
+ done
+done
+
+The output should not contain any WARNING.
 Is it the case that any setuid or setgid programs doesn't have a line in the audit rules?
 
bash remediation for rule 'xccdf_org.ssgproject.content_rule_audit_rules_privileged_commands' differs.
--- xccdf_org.ssgproject.content_rule_audit_rules_privileged_commands
+++ xccdf_org.ssgproject.content_rule_audit_rules_privileged_commands
@@ -1,243 +1,327 @@
 # Remediation is applicable only in certain platforms
 if [ ! -f /.dockerenv ] && [ ! -f /run/.containerenv ] && rpm --quiet -q audit; then
 
-# Perform the remediation for both possible tools: 'auditctl' and 'augenrules'
-files_to_inspect=()
-
-# If the audit tool is 'auditctl', then:
-# * add '/etc/audit/audit.rules'to the list of files to be inspected,
-# * specify '/etc/audit/audit.rules' as the output audit file, where
-# missing rules should be inserted
-files_to_inspect=("/etc/audit/audit.rules")
-output_audit_file="/etc/audit/audit.rules"
-
-# Obtain the list of SUID/SGID binaries on the particular system (split by newline)
-# into privileged_binaries array
-privileged_binaries=()
-readarray -t privileged_binaries < <(find / -not \( -fstype afs -o -fstype ceph -o -fstype cifs -o -fstype smb3 -o -fstype smbfs -o -fstype sshfs -o -fstype ncpfs -o -fstype ncp -o -fstype nfs -o -fstype nfs4 -o -fstype gfs -o -fstype gfs2 -o -fstype glusterfs -o -fstype gpfs -o -fstype pvfs2 -o -fstype ocfs2 -o -fstype lustre -o -fstype davfs -o -fstype fuse.sshfs \) -type f \( -perm -4000 -o -perm -2000 \) 2> /dev/null)
-
-# Keep list of SUID/SGID binaries that have been already handled within some previous iteration
-sbinaries_to_skip=()
-
-# For each found sbinary in privileged_binaries list
-for sbinary in "${privileged_binaries[@]}"
-do
-
- # Check if this sbinary wasn't already handled in some of the previous sbinary iterations
- # Return match only if whole sbinary definition matched (not in the case just prefix matched!!!)
- if [[ $(sed -ne "\|${sbinary}|p" <<< "${sbinaries_to_skip[*]}") ]]
+ACTION_ARCH_FILTERS="-a always,exit"
+AUID_FILTERS="-F auid>=1000 -F auid!=unset"
+SYSCALL=""
+KEY="privileged"
+SYSCALL_GROUPING=""
+
+FILTER_NODEV=$(awk '/nodev/ { print $2 }' /proc/filesystems | paste -sd,)
+PARTITIONS=$(findmnt -n -l -k -it $FILTER_NODEV | grep -Pv "noexec|nosuid" | awk '{ print $1 }')
+for PARTITION in $PARTITIONS; do
+ PRIV_CMDS=$(find "${PARTITION}" -xdev -perm /6000 -type f 2>/dev/null)
+ for PRIV_CMD in $PRIV_CMDS; do
+ OTHER_FILTERS="-F path=$PRIV_CMD -F perm=x"
+ # Perform the remediation for both possible tools: 'auditctl' and 'augenrules'
+ unset syscall_a
+ unset syscall_grouping
+ unset syscall_string
+ unset syscall
+ unset file_to_edit
+ unset rule_to_edit
+ unset rule_syscalls_to_edit
+ unset other_string
+ unset auid_string
+ unset full_rule
+
+ # Load macro arguments into arrays
+ read -a syscall_a <<< $SYSCALL
+ read -a syscall_grouping <<< $SYSCALL_GROUPING
+
+ # 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:
+ #
+ # -----------------------------------------------------------------------------------------
+ # Tool used to load audit rules | Rule already defined | Audit rules file to inspect |
+ # -----------------------------------------------------------------------------------------
+ # auditctl | Doesn't matter | /etc/audit/audit.rules |
+ # -----------------------------------------------------------------------------------------
+ # augenrules | Yes | /etc/audit/rules.d/*.rules |
+ # augenrules | No | /etc/audit/rules.d/$key.rules |
+ # -----------------------------------------------------------------------------------------
+ #
+ files_to_inspect=()
+
+
+ # If audit tool is 'augenrules', then check if the audit rule is defined
+ # If rule is defined, add '/etc/audit/rules.d/*.rules' to the list for inspection
+ # If rule isn't defined yet, add '/etc/audit/rules.d/$key.rules' to the list for inspection
+ default_file="/etc/audit/rules.d/$KEY.rules"
+ # As other_filters may include paths, lets use a different delimiter for it
+ # The "F" script expression tells sed to print the filenames where the expressions matched
+ readarray -t files_to_inspect < <(sed -s -n -e "/^$ACTION_ARCH_FILTERS/!d" -e "\#$OTHER_FILTERS#!d" -e "/$AUID_FILTERS/!d" -e "F" /etc/audit/rules.d/*.rules)
+ # Case when particular rule isn't defined in /etc/audit/rules.d/*.rules yet
+ if [ ${#files_to_inspect[@]} -eq "0" ]
 then
- # If so, don't process it second time & go to process next sbinary
- continue
+ file_to_inspect="/etc/audit/rules.d/$KEY.rules"
+ files_to_inspect=("$file_to_inspect")
+ if [ ! -e "$file_to_inspect" ]
+ then
+ touch "$file_to_inspect"
+ chmod 0640 "$file_to_inspect"
+ fi
 fi
 
- # Reset the counter of inspected files when starting to check
- # presence of existing audit rule for new sbinary
- count_of_inspected_files=0
-
- # Define expected rule form for this binary
- expected_rule="-a always,exit -F path=${sbinary} -F auid>=1000 -F auid!=unset -F key=privileged"
-
- # If list of audit rules files to be inspected is empty, just add new rule and move on to next binary
- if [[ ${#files_to_inspect[@]} -eq 0 ]]; then
- echo "$expected_rule" >> "$output_audit_file"
- continue
+ # After converting to jinja, we cannot return; therefore we skip the rest of the macro if needed instead
+ skip=1
+
+ for audit_file in "${files_to_inspect[@]}"
+ do
+ # Filter existing $audit_file rules' definitions to select those that satisfy the rule pattern,
+ # i.e, collect rules that match:
+ # * the action, list and arch, (2-nd argument)
+ # * the other filters, (3-rd argument)
+ # * the auid filters, (4-rd argument)
+ readarray -t similar_rules < <(sed -e "/^$ACTION_ARCH_FILTERS/!d" -e "\#$OTHER_FILTERS#!d" -e "/$AUID_FILTERS/!d" "$audit_file")
+
+ candidate_rules=()
+ # Filter out rules that have more fields then required. This will remove rules more specific than the required scope
+ for s_rule in "${similar_rules[@]}"
+ do
+ # Strip all the options and fields we know of,
+ # than check if there was any field left over
+ extra_fields=$(sed -E -e "s/^$ACTION_ARCH_FILTERS//" -e "s#$OTHER_FILTERS##" -e "s/$AUID_FILTERS//" -e "s/((:?-S [[:alnum:],]+)+)//g" -e "s/-F key=\w+|-k \w+//"<<< "$s_rule")
+ grep -q -- "-F" <<< "$extra_fields" || candidate_rules+=("$s_rule")
+ done
+
+ if [[ ${#syscall_a[@]} -ge 1 ]]
+ then
+ # Check if the syscall we want is present in any of the similar existing rules
+ for rule in "${candidate_rules[@]}"
+ do
+ rule_syscalls=$(echo "$rule" | grep -o -P '(-S [\w,]+)+' | xargs)
+ all_syscalls_found=0
+ for syscall in "${syscall_a[@]}"
+ do
+ grep -q -- "\b${syscall}\b" <<< "$rule_syscalls" || {
+ # A syscall was not found in the candidate rule
+ all_syscalls_found=1
+ }
+ done
+ if [[ $all_syscalls_found -eq 0 ]]
+ then
+ # We found a rule with all the syscall(s) we want; skip rest of macro
+ skip=0
+ break
+ fi
+
+ # Check if this rule can be grouped with our target syscall and keep track of it
+ for syscall_g in "${syscall_grouping[@]}"
+ do
+ if grep -q -- "\b${syscall_g}\b" <<< "$rule_syscalls"
+ then
+ file_to_edit=${audit_file}
+ rule_to_edit=${rule}
+ rule_syscalls_to_edit=${rule_syscalls}
+ fi
+ done
+ done
+ else
+ # If there is any candidate rule, it is compliant; skip rest of macro
+ if [ "${#candidate_rules[@]}" -gt 0 ]
+ then
+ skip=0
+ fi
+ fi
+
+ if [ "$skip" -eq 0 ]; then
+ break
+ fi
+ done
+
+ if [ "$skip" -ne 0 ]; then
+ # We checked all rules that matched the expected resemblance pattern (action, arch & auid)
+ # At this point we know if we need to either append the $full_rule or group
+ # the syscall together with an exsiting rule
+
+ # Append the full_rule if it cannot be grouped to any other rule
+ if [ -z ${rule_to_edit+x} ]
+ then
+ # Build full_rule while avoid adding double spaces when other_filters is empty
+ if [ "${#syscall_a[@]}" -gt 0 ]
+ then
+ syscall_string=""
+ for syscall in "${syscall_a[@]}"
+ do
+ syscall_string+=" -S $syscall"
+ done
+ fi
+ other_string=$([[ $OTHER_FILTERS ]] && echo " $OTHER_FILTERS") || /bin/true
+ auid_string=$([[ $AUID_FILTERS ]] && echo " $AUID_FILTERS") || /bin/true
+ full_rule="$ACTION_ARCH_FILTERS${syscall_string}${other_string}${auid_string} -F key=$KEY" || /bin/true
+ echo "$full_rule" >> "$default_file"
+ chmod o-rwx ${default_file}
+ else
+ # Check if the syscalls are declared as a comma separated list or
+ # as multiple -S parameters
+ if grep -q -- "," <<< "${rule_syscalls_to_edit}"
+ then
+ delimiter=","
+ else
+ delimiter=" -S "
+ fi
+ new_grouped_syscalls="${rule_syscalls_to_edit}"
+ for syscall in "${syscall_a[@]}"
+ do
+ grep -q -- "\b${syscall}\b" <<< "${rule_syscalls_to_edit}" || {
+ # A syscall was not found in the candidate rule
+ new_grouped_syscalls+="${delimiter}${syscall}"
+ }
+ done
+
+ # Group the syscall in the rule
+ sed -i -e "\#${rule_to_edit}#s#${rule_syscalls_to_edit}#${new_grouped_syscalls}#" "$file_to_edit"
+ fi
 fi
-
- # Replace possible slash '/' character in sbinary definition so we could use it in sed expressions below
- sbinary_esc=${sbinary//$'/'/$'\/'}
-
- # For each audit rules file from the list of files to be inspected
- for afile in "${files_to_inspect[@]}"
+ unset syscall_a
+ unset syscall_grouping
+ unset syscall_string
+ unset syscall
+ unset file_to_edit
+ unset rule_to_edit
+ unset rule_syscalls_to_edit
+ unset other_string
+ unset auid_string
+ unset full_rule
+
+ # Load macro arguments into arrays
+ read -a syscall_a <<< $SYSCALL
+ read -a syscall_grouping <<< $SYSCALL_GROUPING
+
+ # 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:
+ #
+ # -----------------------------------------------------------------------------------------
+ # Tool used to load audit rules | Rule already defined | Audit rules file to inspect |
+ # -----------------------------------------------------------------------------------------
+ # auditctl | Doesn't matter | /etc/audit/audit.rules |
+ # -----------------------------------------------------------------------------------------
+ # augenrules | Yes | /etc/audit/rules.d/*.rules |
+ # augenrules | No | /etc/audit/rules.d/$key.rules |
+ # -----------------------------------------------------------------------------------------
+ #
+ files_to_inspect=()
+
+
+
+ # If audit tool is 'auditctl', then add '/etc/audit/audit.rules'
+ # file to the list of files to be inspected
+ default_file="/etc/audit/audit.rules"
+ files_to_inspect+=('/etc/audit/audit.rules' )
+
+ # After converting to jinja, we cannot return; therefore we skip the rest of the macro if needed instead
+ skip=1
+
+ for audit_file in "${files_to_inspect[@]}"
 do
- # Search current audit rules file's content for match. Match criteria:
- # * existing rule is for the same SUID/SGID binary we are currently processing (but
- # can contain multiple -F path= elements covering multiple SUID/SGID binaries)
- # * existing rule contains all arguments from expected rule form (though can contain
- # them in arbitrary order)
-
- base_search=$(sed -e '/-a always,exit/!d' -e '/-F path='"${sbinary_esc}"'[^[:graph:]]/!d' \
- -e '/-F path=[^[:space:]]\+/!d' \
- -e '/-F auid>='"1000"'/!d' -e '/-F auid!=\(4294967295\|unset\)/!d' \
- -e '/-k \|-F key=/!d' "$afile")
-
- # Increase the count of inspected files for this sbinary
- count_of_inspected_files=$((count_of_inspected_files + 1))
-
- # Search current audit rules file's content for presence of rule pattern for this sbinary
- if [[ $base_search ]]
- then
-
- # Current audit rules file already contains rule for this binary =>
- # Store the exact form of found rule for this binary for further processing
- concrete_rule=$base_search
-
- # Select all other SUID/SGID binaries possibly also present in the found rule
-
- readarray -t handled_sbinaries < <(grep -o -e "-F path=[^[:space:]]\+" <<< "$concrete_rule")
- handled_sbinaries=("${handled_sbinaries[@]//-F path=/}")
-
- # Merge the list of such SUID/SGID binaries found in this iteration with global list ignoring duplicates
- readarray -t sbinaries_to_skip < <(for i in "${sbinaries_to_skip[@]}" "${handled_sbinaries[@]}"; do echo "$i"; done | sort -du)
-
- # if there is a -F perm flag, remove it
- if grep -q '.*-F\s\+perm=[rwxa]\+.*' <<< "$concrete_rule"; then
-
- # Separate concrete_rule into three sections using hash '#'
- # sign as a delimiter around rule's permission section borders
- # note that the trailing space after perm flag is captured because there would be
- # two consecutive spaces after joining remaining parts of the rule together
- concrete_rule="$(echo "$concrete_rule" | sed -n "s/\(.*\)\+\(-F perm=[rwax]\+\ \?\)\+/\1#\2#/p")"
-
- # Split concrete_rule into head and tail sections using hash '#' delimiter
- # The second column contains the permission section, which we don't need to extract
- rule_head=$(cut -d '#' -f 1 <<< "$concrete_rule")
- rule_tail=$(cut -d '#' -f 3 <<< "$concrete_rule")
-
- # Remove permissions section from existing rule in the file
- sed -i "s#${rule_head}\(.*\)${rule_tail}#${rule_head}${rule_tail}#" "$afile"
- fi
- # If the required audit rule for particular sbinary wasn't found yet, insert it under following conditions:
- #
- # * in the "auditctl" mode of operation insert particular rule each time
- # (because in this mode there's only one file -- /etc/audit/audit.rules to be inspected for presence of this rule),
- #
- # * in the "augenrules" mode of operation insert particular rule only once and only in case we have already
- # searched all of the files from /etc/audit/rules.d/*.rules location (since that audit rule can be defined
- # in any of those files and if not, we want it to be inserted only once into /etc/audit/rules.d/privileged.rules file)
- #
- 
- else
- # Check if this sbinary wasn't already handled in some of the previous afile iterations
- # Return match only if whole sbinary definition matched (not in the case just prefix matched!!!)
- if [[ ! $(sed -ne "\|${sbinary}|p" <<< "${sbinaries_to_skip[*]}") ]]
- then
- # Current audit rules file's content doesn't contain expected rule for this
- # SUID/SGID binary yet => append it
- echo "$expected_rule" >> "$output_audit_file"
- fi
- continue
+ # Filter existing $audit_file rules' definitions to select those that satisfy the rule pattern,
+ # i.e, collect rules that match:
+ # * the action, list and arch, (2-nd argument)
+ # * the other filters, (3-rd argument)
+ # * the auid filters, (4-rd argument)
+ readarray -t similar_rules < <(sed -e "/^$ACTION_ARCH_FILTERS/!d" -e "\#$OTHER_FILTERS#!d" -e "/$AUID_FILTERS/!d" "$audit_file")
+
+ candidate_rules=()
+ # Filter out rules that have more fields then required. This will remove rules more specific than the required scope
+ for s_rule in "${similar_rules[@]}"
+ do
+ # Strip all the options and fields we know of,
+ # than check if there was any field left over
+ extra_fields=$(sed -E -e "s/^$ACTION_ARCH_FILTERS//" -e "s#$OTHER_FILTERS##" -e "s/$AUID_FILTERS//" -e "s/((:?-S [[:alnum:],]+)+)//g" -e "s/-F key=\w+|-k \w+//"<<< "$s_rule")
+ grep -q -- "-F" <<< "$extra_fields" || candidate_rules+=("$s_rule")
+ done
+
+ if [[ ${#syscall_a[@]} -ge 1 ]]
+ then
+ # Check if the syscall we want is present in any of the similar existing rules
+ for rule in "${candidate_rules[@]}"
+ do
+ rule_syscalls=$(echo "$rule" | grep -o -P '(-S [\w,]+)+' | xargs)
+ all_syscalls_found=0
+ for syscall in "${syscall_a[@]}"
+ do
+ grep -q -- "\b${syscall}\b" <<< "$rule_syscalls" || {
+ # A syscall was not found in the candidate rule
+ all_syscalls_found=1
+ }
+ done
+ if [[ $all_syscalls_found -eq 0 ]]
+ then
+ # We found a rule with all the syscall(s) we want; skip rest of macro
+ skip=0
+ break
+ fi
+
+ # Check if this rule can be grouped with our target syscall and keep track of it
+ for syscall_g in "${syscall_grouping[@]}"
+ do
+ if grep -q -- "\b${syscall_g}\b" <<< "$rule_syscalls"
+ then
+ file_to_edit=${audit_file}
+ rule_to_edit=${rule}
+ rule_syscalls_to_edit=${rule_syscalls}
+ fi
+ done
+ done
+ else
+ # If there is any candidate rule, it is compliant; skip rest of macro
+ if [ "${#candidate_rules[@]}" -gt 0 ]
+ then
+ skip=0
+ fi
+ fi
+
+ if [ "$skip" -eq 0 ]; then
+ break
 fi
 done
-done
-files_to_inspect=()
-# If the audit tool is 'augenrules', then:
-# * add '/etc/audit/rules.d/*.rules' to the list of files to be inspected
-# (split by newline),
-# * specify /etc/audit/rules.d/privileged.rules' as the output file, where
-# missing rules should be inserted
-readarray -t files_to_inspect < <(find /etc/audit/rules.d -maxdepth 1 -type f -name '*.rules' -print)
-output_audit_file="/etc/audit/rules.d/privileged.rules"
-
-# Obtain the list of SUID/SGID binaries on the particular system (split by newline)
-# into privileged_binaries array
-privileged_binaries=()
-readarray -t privileged_binaries < <(find / -not \( -fstype afs -o -fstype ceph -o -fstype cifs -o -fstype smb3 -o -fstype smbfs -o -fstype sshfs -o -fstype ncpfs -o -fstype ncp -o -fstype nfs -o -fstype nfs4 -o -fstype gfs -o -fstype gfs2 -o -fstype glusterfs -o -fstype gpfs -o -fstype pvfs2 -o -fstype ocfs2 -o -fstype lustre -o -fstype davfs -o -fstype fuse.sshfs \) -type f \( -perm -4000 -o -perm -2000 \) 2> /dev/null)
-
-# Keep list of SUID/SGID binaries that have been already handled within some previous iteration
-sbinaries_to_skip=()
-
-# For each found sbinary in privileged_binaries list
-for sbinary in "${privileged_binaries[@]}"
-do
-
- # Check if this sbinary wasn't already handled in some of the previous sbinary iterations
- # Return match only if whole sbinary definition matched (not in the case just prefix matched!!!)
- if [[ $(sed -ne "\|${sbinary}|p" <<< "${sbinaries_to_skip[*]}") ]]
- then
- # If so, don't process it second time & go to process next sbinary
- continue
+
+ if [ "$skip" -ne 0 ]; then
+ # We checked all rules that matched the expected resemblance pattern (action, arch & auid)
+ # At this point we know if we need to either append the $full_rule or group
+ # the syscall together with an exsiting rule
+
+ # Append the full_rule if it cannot be grouped to any other rule
+ if [ -z ${rule_to_edit+x} ]
+ then
+ # Build full_rule while avoid adding double spaces when other_filters is empty
+ if [ "${#syscall_a[@]}" -gt 0 ]
+ then
+ syscall_string=""
+ for syscall in "${syscall_a[@]}"
+ do
+ syscall_string+=" -S $syscall"
+ done
+ fi
+ other_string=$([[ $OTHER_FILTERS ]] && echo " $OTHER_FILTERS") || /bin/true
+ auid_string=$([[ $AUID_FILTERS ]] && echo " $AUID_FILTERS") || /bin/true
+ full_rule="$ACTION_ARCH_FILTERS${syscall_string}${other_string}${auid_string} -F key=$KEY" || /bin/true
+ echo "$full_rule" >> "$default_file"
+ chmod o-rwx ${default_file}
+ else
+ # Check if the syscalls are declared as a comma separated list or
+ # as multiple -S parameters
+ if grep -q -- "," <<< "${rule_syscalls_to_edit}"
+ then
+ delimiter=","
+ else
+ delimiter=" -S "
+ fi
+ new_grouped_syscalls="${rule_syscalls_to_edit}"
+ for syscall in "${syscall_a[@]}"
+ do
+ grep -q -- "\b${syscall}\b" <<< "${rule_syscalls_to_edit}" || {
+ # A syscall was not found in the candidate rule
+ new_grouped_syscalls+="${delimiter}${syscall}"
+ }
+ done
+
+ # Group the syscall in the rule
+ sed -i -e "\#${rule_to_edit}#s#${rule_syscalls_to_edit}#${new_grouped_syscalls}#" "$file_to_edit"
+ fi
 fi
-
- # Reset the counter of inspected files when starting to check
- # presence of existing audit rule for new sbinary
- count_of_inspected_files=0
-
- # Define expected rule form for this binary
- expected_rule="-a always,exit -F path=${sbinary} -F auid>=1000 -F auid!=unset -F key=privileged"
-
- # If list of audit rules files to be inspected is empty, just add new rule and move on to next binary
- if [[ ${#files_to_inspect[@]} -eq 0 ]]; then
- echo "$expected_rule" >> "$output_audit_file"
- continue
- fi
-
- # Replace possible slash '/' character in sbinary definition so we could use it in sed expressions below
- sbinary_esc=${sbinary//$'/'/$'\/'}
-
- # For each audit rules file from the list of files to be inspected
- for afile in "${files_to_inspect[@]}"
- do
- # Search current audit rules file's content for match. Match criteria:
- # * existing rule is for the same SUID/SGID binary we are currently processing (but
- # can contain multiple -F path= elements covering multiple SUID/SGID binaries)
- # * existing rule contains all arguments from expected rule form (though can contain
- # them in arbitrary order)
-
- base_search=$(sed -e '/-a always,exit/!d' -e '/-F path='"${sbinary_esc}"'[^[:graph:]]/!d' \
- -e '/-F path=[^[:space:]]\+/!d' \
- -e '/-F auid>='"1000"'/!d' -e '/-F auid!=\(4294967295\|unset\)/!d' \
- -e '/-k \|-F key=/!d' "$afile")
-
- # Increase the count of inspected files for this sbinary
- count_of_inspected_files=$((count_of_inspected_files + 1))
-
- # Search current audit rules file's content for presence of rule pattern for this sbinary
- if [[ $base_search ]]
- then
-
- # Current audit rules file already contains rule for this binary =>
- # Store the exact form of found rule for this binary for further processing
- concrete_rule=$base_search
-
- # Select all other SUID/SGID binaries possibly also present in the found rule
-
- readarray -t handled_sbinaries < <(grep -o -e "-F path=[^[:space:]]\+" <<< "$concrete_rule")
- handled_sbinaries=("${handled_sbinaries[@]//-F path=/}")
-
- # Merge the list of such SUID/SGID binaries found in this iteration with global list ignoring duplicates
- readarray -t sbinaries_to_skip < <(for i in "${sbinaries_to_skip[@]}" "${handled_sbinaries[@]}"; do echo "$i"; done | sort -du)
-
- # if there is a -F perm flag, remove it
- if grep -q '.*-F\s\+perm=[rwxa]\+.*' <<< "$concrete_rule"; then
-
- # Separate concrete_rule into three sections using hash '#'
- # sign as a delimiter around rule's permission section borders
- # note that the trailing space after perm flag is captured because there would be
- # two consecutive spaces after joining remaining parts of the rule together
- concrete_rule="$(echo "$concrete_rule" | sed -n "s/\(.*\)\+\(-F perm=[rwax]\+\ \?\)\+/\1#\2#/p")"
-
- # Split concrete_rule into head and tail sections using hash '#' delimiter
- # The second column contains the permission section, which we don't need to extract
- rule_head=$(cut -d '#' -f 1 <<< "$concrete_rule")
- rule_tail=$(cut -d '#' -f 3 <<< "$concrete_rule")
-
- # Remove permissions section from existing rule in the file
- sed -i "s#${rule_head}\(.*\)${rule_tail}#${rule_head}${rule_tail}#" "$afile"
- fi
- # If the required audit rule for particular sbinary wasn't found yet, insert it under following conditions:
- #
- # * in the "auditctl" mode of operation insert particular rule each time
- # (because in this mode there's only one file -- /etc/audit/audit.rules to be inspected for presence of this rule),
- #
- # * in the "augenrules" mode of operation insert particular rule only once and only in case we have already
- # searched all of the files from /etc/audit/rules.d/*.rules location (since that audit rule can be defined
- # in any of those files and if not, we want it to be inserted only once into /etc/audit/rules.d/privileged.rules file)
- #
- elif [[ $count_of_inspected_files -eq "${#files_to_inspect[@]}" ]]
- then
- 
- # Check if this sbinary wasn't already handled in some of the previous afile iterations
- # Return match only if whole sbinary definition matched (not in the case just prefix matched!!!)
- if [[ ! $(sed -ne "\|${sbinary}|p" <<< "${sbinaries_to_skip[*]}") ]]
- then
- # Current audit rules file's content doesn't contain expected rule for this
- # SUID/SGID binary yet => append it
- echo "$expected_rule" >> "$output_audit_file"
- fi
- continue
- fi
- done
+ done
 done
 
 else

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_audit_rules_privileged_commands' differs.
--- xccdf_org.ssgproject.content_rule_audit_rules_privileged_commands
+++ xccdf_org.ssgproject.content_rule_audit_rules_privileged_commands
@@ -12,22 +12,17 @@
 - NIST-800-53-CM-6(a)
 - PCI-DSS-Req-10.2.2
 - audit_rules_privileged_commands
+ - configure_strategy
 - low_complexity
 - low_disruption
 - medium_severity
 - no_reboot_needed
- - restrict_strategy
 
-- name: Search for privileged commands
- shell: |
- set -o pipefail
- find / -not \( -fstype afs -o -fstype ceph -o -fstype cifs -o -fstype smb3 -o -fstype smbfs -o -fstype sshfs -o -fstype ncpfs -o -fstype ncp -o -fstype nfs -o -fstype nfs4 -o -fstype gfs -o -fstype gfs2 -o -fstype glusterfs -o -fstype gpfs -o -fstype pvfs2 -o -fstype ocfs2 -o -fstype lustre -o -fstype davfs -o -fstype fuse.sshfs \) -type f \( -perm -4000 -o -perm -2000 \) 2> /dev/null
- args:
- executable: /bin/bash
- check_mode: false
- register: find_result
- changed_when: false
- failed_when: false
+- name: Ensure auditd Collects Information on the Use of Privileged Commands - Set
+ List of Mount Points Which Permits Execution of Privileged Commands
+ ansible.builtin.set_fact:
+ privileged_mount_points: '{{(ansible_facts.mounts | rejectattr(''options'', ''search'',
+ ''noexec|nosuid'') | map(attribute=''mount'') | list ) }}'
 when:
 - '"audit" in ansible_facts.packages'
 - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
@@ -42,21 +37,20 @@
 - NIST-800-53-CM-6(a)
 - PCI-DSS-Req-10.2.2
 - audit_rules_privileged_commands
+ - configure_strategy
 - low_complexity
 - low_disruption
 - medium_severity
 - no_reboot_needed
- - restrict_strategy
 
-- name: Search /etc/audit/rules.d for audit rule entries
- find:
- paths: /etc/audit/rules.d
- recurse: false
- contains: ^.*path={{ item }} .*$
- patterns: '*.rules'
- with_items:
- - '{{ find_result.stdout_lines }}'
- register: files_result
+- name: Ensure auditd Collects Information on the Use of Privileged Commands - Search
+ for Privileged Commands in Eligible Mount Points
+ ansible.builtin.shell:
+ cmd: find {{ item }} -xdev -perm /6000 -type f 2>/dev/null
+ register: result_privileged_commands_search
+ changed_when: false
+ failed_when: false
+ with_items: '{{ privileged_mount_points }}'
 when:
 - '"audit" in ansible_facts.packages'
 - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
@@ -71,22 +65,17 @@
 - NIST-800-53-CM-6(a)
 - PCI-DSS-Req-10.2.2
 - audit_rules_privileged_commands
+ - configure_strategy
 - low_complexity
 - low_disruption
 - medium_severity
 - no_reboot_needed
- - restrict_strategy
 
-- name: Overwrites the rule in rules.d
- lineinfile:
- path: '{{ item.1.path }}'
- line: -a always,exit -F path={{ item.0.item }} -F auid>=1000 -F auid!=unset -F
- key=privileged
- create: false
- regexp: ^.*path={{ item.0.item }} .*$
- with_subelements:
- - '{{ files_result.results }}'
- - files
+- name: Ensure auditd Collects Information on the Use of Privileged Commands - Set
+ List of Privileged Commands Found in Eligible Mount Points
+ ansible.builtin.set_fact:
+ privileged_commands: '{{( result_privileged_commands_search.results | map(attribute=''stdout_lines'')
+ | select() | list )[-1] }}'
 when:
 - '"audit" in ansible_facts.packages'
 - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
@@ -101,23 +90,64 @@
 - NIST-800-53-CM-6(a)
 - PCI-DSS-Req-10.2.2
 - audit_rules_privileged_commands
+ - configure_strategy
 - low_complexity
 - low_disruption
 - medium_severity
 - no_reboot_needed
- - restrict_strategy
 
-- name: Adds the rule in rules.d
- lineinfile:
- path: /etc/audit/rules.d/privileged.rules
- line: -a always,exit -F path={{ item.item }} -F auid>=1000 -F auid!=unset -F key=privileged
- create: true
- with_items:
- - '{{ files_result.results }}'
+- name: Ensure auditd Collects Information on the Use of Privileged Commands - Privileged
+ Commands are Present in the System
+ block:
+
+ - name: Ensure auditd Collects Information on the Use of Privileged Commands - Ensure
+ Rules for All Privileged Commands in augenrules Format
+ ansible.builtin.lineinfile:
+ path: /etc/audit/rules.d/privileged.rules
+ line: -a always,exit -F path={{ item }} -F perm=x -F auid>=1000 -F auid!=unset
+ -F key=privileged
+ regexp: ^.*path={{ item | regex_escape() }} .*$
+ create: true
+ with_items:
+ - '{{ privileged_commands }}'
+
+ - name: Ensure auditd Collects Information on the Use of Privileged Commands - Ensure
+ Rules for All Privileged Commands in auditctl Format
+ ansible.builtin.lineinfile:
+ path: /etc/audit/audit.rules
+ line: -a always,exit -F path={{ item }} -F perm=x -F auid>=1000 -F auid!=unset
+ -F key=privileged
+ regexp: ^.*path={{ item | regex_escape() }} .*$
+ create: true
+ with_items:
+ - '{{ privileged_commands }}'
+
+ - name: Ensure auditd Collects Information on the Use of Privileged Commands - Search
+ for Duplicated Rules in Other Files
+ ansible.builtin.find:
+ paths: /etc/audit/rules.d
+ recurse: false
+ contains: ^-a always,exit -F path={{ item }} .*$
+ patterns: '*.rules'
+ with_items:
+ - '{{ privileged_commands }}'
+ register: result_augenrules_files
+
+ - name: Ensure auditd Collects Information on the Use of Privileged Commands - Ensure
+ Rules for Privileged Commands are Defined Only in One File
+ ansible.builtin.lineinfile:
+ path: '{{ item.1.path }}'
+ regexp: ^-a always,exit -F path={{ item.0.item }} .*$
+ state: absent
+ with_subelements:
+ - '{{ result_augenrules_files.results }}'
+ - files
+ when:
+ - item.1.path != '/etc/audit/rules.d/privileged.rules'
 when:
 - '"audit" in ansible_facts.packages'
 - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
- - files_result.results is defined and item.matched == 0
+ - privileged_commands is defined
 tags:
 - CCE-80724-8
 - CJIS-5.4.1.1
@@ -129,36 +159,8 @@
 - NIST-800-53-CM-6(a)
 - PCI-DSS-Req-10.2.2
 - audit_rules_privileged_commands
+ - configure_strategy
 - low_complexity
 - low_disruption
 - medium_severity
 - no_reboot_needed
- - restrict_strategy
-
-- name: Inserts/replaces the rule in audit.rules
- lineinfile:
- path: /etc/audit/audit.rules
- line: -a always,exit -F path={{ item.item }} -F auid>=1000 -F auid!=unset -F key=privileged
- create: true
- regexp: ^.*path={{ item.item }} .*$
- with_items:
- - '{{ files_result.results }}'
- when:
- - '"audit" in ansible_facts.packages'
- - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
- tags:
- - CCE-80724-8
- - CJIS-5.4.1.1
- - NIST-800-171-3.1.7
- - NIST-800-53-AC-2(4)
- - 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
- - audit_rules_privileged_commands
- - low_complexity
- - low_disruption
- - medium_severity
- - no_reboot_needed
- - restrict_strategy

@marcusburghardt marcusburghardt marked this pull request as draft March 10, 2023 17:21
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Mar 10, 2023
The OVAL was very confusing with inaccurate comments and not intuitive
organization of elements. This commit doesn't change any logic but
improve the comments, the elements ids and the ordering of elements.
The OVAL was also aligned to the Project Style Guide.
The rule description was reviewed and simplified to be more easily
consumable by readers. The OCIL section was also improved by introducing
a small script as reference to audit the system.
The same regex was used twice. Considering it was a relatively long
regex, it was not easy to identify the similarities and also the
maintainability was compromissed when the regex needed updates.
It was introduced a variable for the regex and used this variable as
reference in the two places.
This rule is about execution of privilleged commands. It should not care
about reading or writing in these files since there are other more
specific ways to check changes in these files. However, it is not
possible to say the rule without a filter for perm is wrong because it
would implicitly collect all permissions and not only execution. In
short, a rule without a perm filter would do much more than necessary
and it could lead to performance issues, but wouldn't be wrong in the
context of this rule. Based on this context, this commit includes the
perm filter in the check but makes it optional. The remdiation will be
updated to ensure the perm=x is actually present.
The header was updated to not restrict the remedition to bash and also
to include multi_platform_rhel since they are also applicable for RHEL9,
which was not listed in plaform header.
The rules created in test scenarios are now including -F perm=x, in
alignement to the OVAL.
The Bash remediation was using the a macro specific for this rule which
was very complex to be maintained and was not reutilizing existing
macros to avoid code duplication. This macro was removed from the bash
remediation and a much simpler remediation was created using existing
macros shared by other rules. The remediation is aligned to the approach
used in audit_rules_privileged_commands template. Note that some bash
variables are empty but were intentionally kept to preserv the
readability when calling the macro.
It was hard-coded in OVAL to ignore files if their file paths start
with some strings, which basically referred to pseudo filesystems or
filesystems that presumably should be ignored. This approach was not
robust and reliable enough. Instead, the partitions should be
dinamically filtered based on their mount options. This commit improves
the OVAL by dinamically detecting filtering out not relevant devices
and partitions mounted with noexec or nosuid mount options. This
restricts the scope of the probes to find suid and sgid files and
ultimately increases the check performance.
@marcusburghardt marcusburghardt marked this pull request as ready for review March 10, 2023 22:26
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Mar 10, 2023
The Ansible remediation was collecting information in an inefficient
way and repeating similar tasks unnecessarily. The Playbook was
simplified to improve readability and also optimized to be more
efficient. From the ansible_facts is filtered only the mount points
eligible to execute privileged commands. The privileged commands are
search only in these mount points. Finally, the decision to include a
new rule or update an existing rule is left to Ansible lineinfile
module.
The 4.1.3.6 CIS requirement for RHEL8 and RHEL9 and the 4.1.11 CIS
requirement for RHEL7 are now satisfied by the
audit_rules_privileged_commands rule.
@marcusburghardt
Copy link
Member Author

It is expected the tests fail in containers (Automatus tests) since the partitions in these test containers are not relevant for this rule.

@marcusburghardt
Copy link
Member Author

/retest

@Mab879 Mab879 modified the milestones: 0.1.67, 0.1.68 Mar 27, 2023
@yuumasato yuumasato self-assigned this Mar 28, 2023
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.

Thank you for the nice commit messages.

I miss tests to ensure that the filtering partition with nosuid and noexec works. But I wonder if it is feasible to test this.
Would it be possible to have a binary with suid set in a partition mounted with nosuid? I guess not, right?

Also, do you know why the Automatus CS tests fail?
The test scenarios pass locally.

Co-authored-by: Watson Yuuma Sato <wsato@redhat.com>
@marcusburghardt
Copy link
Member Author

Thank you for the nice commit messages.

I miss tests to ensure that the filtering partition with nosuid and noexec works. But I wonder if it is feasible to test this. Would it be possible to have a binary with suid set in a partition mounted with nosuid? I guess not, right?

Possible it should be but I don't think it is feasible. At least I didn't find a safe way to do that when working on test scenarios. There are some extra details. For example, depending on the file system, the -o remount might not work and a complete umount/mount would be necessary. I believe it is not worth at the moment. But I did manual tests with local VMs to ensure this is working.

Also, do you know why the Automatus CS tests fail? The test scenarios pass locally.

Our containers only have nodev filesystems. It is expected to fail there. You can check with this command:
cat /proc/filesystems

See #10326 (comment)

@codeclimate
Copy link

codeclimate bot commented Mar 31, 2023

Code Climate has analyzed commit 3c3fc0d 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 51.8% (0.0% change).

View more on Code Climate.

@yuumasato
Copy link
Member

yuumasato commented Apr 3, 2023

Also, do you know why the Automatus CS tests fail? The test scenarios pass locally.

Our containers only have nodev filesystems. It is expected to fail there. You can check with this command: cat /proc/filesystems

Could you clarify why nodev filesystems are excluded in the remediation?
The check doesn't exclude filesystems with nodev option.

It seems that the failure happens because the container doesn't have any binary with SUID or SGID bit set.
i was able to reproduce this on my VM.

I: oscap:     Variable 'oval:ssg-var_audit_rules_privileged_commands_priv_cmds:var:1' has no values. [oscap(48):oscap(7f4f7871c3c0):oval_variable.c:461:_dump_variable_values]
I: oscap:     Referenced variable has no values (oval:ssg-var_audit_rules_privileged_commands_priv_cmds:var:1). [oscap(48):oscap(7f4f7871c3c0):oval_sexp.c:250:oval_varref_elm    _to_sexp] 

This happens previous to this PR as well.

@marcusburghardt
Copy link
Member Author

Also, do you know why the Automatus CS tests fail? The test scenarios pass locally.

Our containers only have nodev filesystems. It is expected to fail there. You can check with this command: cat /proc/filesystems

Could you clarify why nodev filesystems are excluded in the remediation? The check doesn't exclude filesystems with nodev option.

It is not about filesystems mounted with nodev option. But nodev file systems found in /proc/filesystems. This file is consulted by the Bash remediation. Here is the related quote from the proc(5) man:

/proc/filesystems
              A text listing of the filesystems which are supported by
              the kernel, namely filesystems which were compiled into
              the kernel or whose kernel modules are currently loaded.
              (See also [filesystems(5)](https://man7.org/linux/man-pages/man5/filesystems.5.html).)  If a filesystem is marked with
              "nodev", this means that it does not require a block
              device to be mounted (e.g., virtual filesystem, network
              filesystem).

              Incidentally, this file may be used by [mount(8)](https://man7.org/linux/man-pages/man8/mount.8.html) when no
              filesystem is specified and it didn't manage to determine
              the filesystem type.  Then filesystems contained in this
              file are tried (excepted those that are marked with
              "nodev").

It seems that the failure happens because the container doesn't have any binary with SUID or SGID bit set. i was able to reproduce this on my VM.

This can be another argument about the expectation these rules fail in containers. But I was thinking one step ahead. Even if there is files with SUID or SGID bit set, they might not be fixed because the containers have mainly pseudo-filesystems (nodev). In any case, containers can have these corner cases and the failure is expected.

If you wondering yourself about the Ansible remediation, it works a little bit different (does not consult the /proc/filesystems) because it consults the Ansible facts instead and this proved enough during the tests. Actually, the approach in Bash was a low-hanging-fruit with a potential performance improvement for the Bash remediation.

I: oscap:     Variable 'oval:ssg-var_audit_rules_privileged_commands_priv_cmds:var:1' has no values. [oscap(48):oscap(7f4f7871c3c0):oval_variable.c:461:_dump_variable_values]
I: oscap:     Referenced variable has no values (oval:ssg-var_audit_rules_privileged_commands_priv_cmds:var:1). [oscap(48):oscap(7f4f7871c3c0):oval_sexp.c:250:oval_varref_elm    _to_sexp] 

This happens previous to this PR as well.

Yes, not sure if it would be worth to change this in this PR. A test scenario would demand to remove these special permissions in the whole system and the impact should be better tested. Supposing it is still reliable and sane to test the rule after removal of all special permissions, the OVAL would still demand some updates. Maybe we can consider this in another PR, after this refactoring be landed and some additional research specific to this approach.

@yuumasato
Copy link
Member

Could you clarify why nodev filesystems are excluded in the remediation? The check doesn't exclude filesystems with nodev option.

It is not about filesystems mounted with nodev option. But nodev file systems found in /proc/filesystems. This file is consulted by the Bash remediation. Here is the related quote from the proc(5) man:

/proc/filesystems
              A text listing of the filesystems which are supported by
              the kernel, namely filesystems which were compiled into
              the kernel or whose kernel modules are currently loaded.
              (See also [filesystems(5)](https://man7.org/linux/man-pages/man5/filesystems.5.html).)  If a filesystem is marked with
              "nodev", this means that it does not require a block
              device to be mounted (e.g., virtual filesystem, network
              filesystem).

              Incidentally, this file may be used by [mount(8)](https://man7.org/linux/man-pages/man8/mount.8.html) when no
              filesystem is specified and it didn't manage to determine
              the filesystem type.  Then filesystems contained in this
              file are tried (excepted those that are marked with
              "nodev").

@marcusburghardt Thanks, I understand the code better now, do you think it would make sense to comment somewhere that virtual filesystems are not scanned by the rule?
Also, considering that sometimes /tmp can be a tmpfs file system, wouldn't it miss binaries there that have SUID or GUID set?

The rule description was subtly updated to make it clear that only block
device partitions will be checked.
@marcusburghardt
Copy link
Member Author

Could you clarify why nodev filesystems are excluded in the remediation? The check doesn't exclude filesystems with nodev option.

It is not about filesystems mounted with nodev option. But nodev file systems found in /proc/filesystems. This file is consulted by the Bash remediation. Here is the related quote from the proc(5) man:

/proc/filesystems
              A text listing of the filesystems which are supported by
              the kernel, namely filesystems which were compiled into
              the kernel or whose kernel modules are currently loaded.
              (See also [filesystems(5)](https://man7.org/linux/man-pages/man5/filesystems.5.html).)  If a filesystem is marked with
              "nodev", this means that it does not require a block
              device to be mounted (e.g., virtual filesystem, network
              filesystem).

              Incidentally, this file may be used by [mount(8)](https://man7.org/linux/man-pages/man8/mount.8.html) when no
              filesystem is specified and it didn't manage to determine
              the filesystem type.  Then filesystems contained in this
              file are tried (excepted those that are marked with
              "nodev").

@marcusburghardt Thanks, I understand the code better now, do you think it would make sense to comment somewhere that virtual filesystems are not scanned by the rule?

It is a fair point. I subtly updated the rule description. 684ccf9

Also, considering that sometimes /tmp can be a tmpfs file system, wouldn't it miss binaries there that have SUID or GUID set?

The /tmp and maybe all other "nodev" partitions have nosuid and or noexec option set. This filter to exclude "nodev" partitions is also part of the latest CIS versions for RHEL8 and RHEL9. I unfortunately couldn't locate the history of this requirement in CIS, but I am also inclined to agree this is a safe approach in favor of performance. But we can start a discussion in CIS community. Maybe this is not a risk in a profile because other requirements will ensure the nosuid and other options in /tmp. But thinking in this rule isolated, I wonder if there is any realistic case. I started a discussion in CIS:
https://workbench.cisecurity.org/community/101/discussions/9467

If necessary, we can update the rule after this discussion, but I don't think we need to wait the outcome of this discussion to merge this PR. Do you agree?

@yuumasato
Copy link
Member

If necessary, we can update the rule after this discussion, but I don't think we need to wait the outcome of this discussion to merge this PR. Do you agree?

Sure, thank you for the clarifications and for starting the discussion.

@yuumasato
Copy link
Member

As discussed, the Automatus tests don't run well on containers and it is not easy to change them to be.

@yuumasato yuumasato merged commit 48c4559 into ComplianceAsCode:master Apr 4, 2023
23 of 29 checks passed
@marcusburghardt marcusburghardt deleted the cis_audit branch April 4, 2023 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ansible Ansible remediation update. Bash Bash remediation update. CIS CIS Benchmark related. OVAL OVAL update. Related to the systems assessments. RHEL Red Hat Enterprise Linux product related. Update Rule Issues or pull requests related to Rules updates.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants