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

Allow audit syscall rules remediations to group the syscalls #7329

Merged
merged 31 commits into from Aug 19, 2021

Conversation

yuumasato
Copy link
Member

@yuumasato yuumasato commented Aug 4, 2021

Description:

  • Improve and extend rule skeleton matching with more explicit rule options for action, list, arch, auid and other filters.
  • Make explicit the syscalls that can through the syscall_groupings parameter.
    • When syscall_groupings is empty, it will always add a new rule if not found.
  • Make they key to use more explicit, instead of implicit through 'group'.
  • Update the Ansible remediations (8af4ced)
  • Add tests for fix_audit_syscall_rule I think the unit tests are not suited for this function. The function directly edits the rule files in /etc/audit and would be better run on a VM. If will need a VM anyway, I'd rather check and improve the test scenarios if needed.
  • Improve/adapt test scenarios (I believe the current test scenarios served pretty well to find bugs).

Rationale:

  • The function actually separated the syscalls into individual lines.

@yuumasato yuumasato requested a review from ggbecker August 4, 2021 13:17
@openshift-ci
Copy link

openshift-ci bot commented Aug 4, 2021

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

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Aug 4, 2021
@yuumasato
Copy link
Member Author

Given the '-F auid=0` changes in #6910, I'm considering bringing the input to the Bash template directly to the 'rule.yml'.

@yuumasato
Copy link
Member Author

I think I'll rebase this PR on top of #6910 to take advantage of the rule selections in the profile.

@pep8speaks
Copy link

pep8speaks commented Aug 5, 2021

Hello @yuumasato! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-08-19 13:36:08 UTC

@yuumasato yuumasato mentioned this pull request Aug 5, 2021
5 tasks
The function actually separated the syscalls into individual lines.
* Improve and extend rule skeleton matching with more explicit rule
  options for action, arch, auid and other filters.
* Make explicit the syscalls that can be grouped through the
  'syscall_groupings' parameter.
* Make they key to use more explicit, instead of implicit through
  'group'.
The ";F" was not a typo!
Hopefully this makes it more explicit the function of '-e "F"'.
When syscall is not set, just don't add the -S parameter.
The audit privileged commands use the fix_audit_syscall_rule despite
not adding a -S syscall.
Same situation happens for directory_access_var_log_audit.
Some rules deal with single handedly with multiple profiles.
These rules expect to use the fix_audit_syscall_rule to add a rule with
muliple syscalls at a time.
Enhance the bash function to nicely handle calls without auid filters
defined.
And updated the remediations of rules calling fix_audit_syscall_rule to
the new parameters.
The OVAL check was also updated to accept the key as a Field parameter.
Update rules audit_rules_time_clock_settime and bash shared
remediation perform_audit_adjtimex_settimeofday_stime_remediation
to group their syscalls.
@yuumasato yuumasato marked this pull request as ready for review August 6, 2021 09:51
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Aug 6, 2021
@yuumasato
Copy link
Member Author

I think the Bash remediations are in good shape and good for review.

The macros now group the syscall rule according to the grouping argument
The Ansible macros follow same argument pattern as the Bash remediations
(soon to become macros).
Use Ansible macro ansible_audit_augenrules_add_syscall_rule and
ansible_audit_auditctl_add_syscall_rule that group the syscalls
according to defined grouping.
…macro

Use Ansible macro ansible_audit_augenrules_add_syscall_rule and
ansible_audit_auditctl_add_syscall_rule that group the syscalls
according to defined grouping.
The Ansible macros for audit syscall rules should check the target
syscall and the groupable syscalls during 'find' task.

When 'syscall_grouping' was empty, the remediation would simply
execute the 'Add a new rule' task.
If the key was different, a new duplicate rule would be added.

Also removes extra syscalls declaration task.
Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

Any chance this could be added to the kubernetes templates as well?

@JAORMX
Copy link
Contributor

JAORMX commented Aug 18, 2021

@yuumasato some of them are using straight up MachineConfigs... while we could now replace those entries with jinja to show the actual audit rule. I could help with that if you want.

@yuumasato
Copy link
Member Author

Any chance this could be added to the kubernetes templates as well?

Do you mean to add the grouping capability to the kubernetes remediations?
I don't know how that could be done, but we can discuss it if you have any idea.

@yuumasato some of them are using straight up MachineConfigs... while we could now replace those entries with jinja to show the actual audit rule. I could help with that if you want.

@JAORMX You mean with url_encode()?

When running a playbook profile, they were accumulating over the entire
run.
@ggbecker ggbecker self-assigned this Aug 18, 2021
@JAORMX
Copy link
Contributor

JAORMX commented Aug 18, 2021

@yuumasato and I talked, and there's nothing to do for the kubernetes remediations as of now.

@openshift-ci
Copy link

openshift-ci bot commented Aug 19, 2021

@yuumasato: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-ocp4-moderate 34a6691 link /test e2e-aws-ocp4-moderate

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Otherwise the rule will be added with two spaces between other_filters
and auid_filters.
Copy link
Member

@ggbecker ggbecker left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for these changes. It was a really nice improvement to the project.

Variables should have spaces before and after
@ggbecker ggbecker merged commit 049506a into ComplianceAsCode:master Aug 19, 2021
@yuumasato yuumasato deleted the group_audit_syscalls branch August 19, 2021 14:44
@marcusburghardt marcusburghardt added RHEL8 Red Hat Enterprise Linux 8 product related. STIG STIG Benchmark related. labels Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RHEL8 Red Hat Enterprise Linux 8 product related. STIG STIG Benchmark related.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants