Refactors and rules for RHEL7 DISA STIG #7827
Conversation
|
Hi @lenox-joseph. 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 Once the patch is verified, the new status will be reflected by the 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. |
|
|
||
| references: | ||
| disa: CCI-000130,CCI-000169,CCI-000172,CCI-002884 | ||
| nist: AU-3,AU-3.1,AU-12(a),AU-12.1(ii),AU-12.1(iv)AU-12(c),MA-4(1)(a) | ||
| srg: SRG-OS-000037-GPOS-00015,SRG-OS-000042-GPOS-00020,SRG-OS-000062-GPOS-00031,SRG-OS-000392-GPOS-00172,SRG-OS-000462-GPOS-00206,SRG-OS-000471-GPOS-00215,SRG-OS-000471-GPOS-00216,SRG-OS-000477-GPOS-00222 | ||
| stigid@rhel7: RHEL-07-030840 |
There was a problem hiding this comment.
this is the only addition to the reference list and its only use.
openshift-ci
bot
added
ok-to-test
|
/ok-to-test |
From https://linux.die.net/man/5/sshd_config I see that the X11Forwarding is defaulted to "no". So I assume the default would be enough. Is that a different case in RHEL7?
There has been some conversations around what path to take here. We though at first that using accepting the default value for options that are
The rule is slightly different than DISA STIG RHEL8, we may have to adapt something to be strictly equal to DISA STIG RHEL7
|
linux_os/guide/system/accounts/accounts-pam/display_login_attempts/rule.yml
Outdated
Show resolved
Hide resolved
linux_os/guide/system/accounts/accounts-pam/display_login_attempts_disa/rule.yml
Outdated
Show resolved
Hide resolved
|
AFAIK I can do X11 forwarding with a default config on RHEL7. This falls under the same things as "DISA wants these checks explicit".
I know I have to live with DISA's interpretation of their rules in my world.
…________________________________
From: Gabriel Becker ***@***.***>
Sent: Thursday, November 4, 2021 5:32:58 AM
To: ComplianceAsCode/content ***@***.***>
Cc: Lenox, Joseph D Collins ***@***.***>; Mention ***@***.***>
Subject: [External] Re: [ComplianceAsCode/content] Refactors and rules for RHEL7 DISA STIG (PR #7827)
Description:
* `sshd_disable_x11_forwarding`: Set the parameter to fail if not present.
From https://linux.die.net/man/5/sshd_config<https://urldefense.com/v3/__https://linux.die.net/man/5/sshd_config__;!!MvWE!SJ5ZlmNnaIdi7wW3hodQsGNp-Q6uw9N6vIx8yPkl71gYJZHFHPqZzCa0oGzVhIUtcq8$> I see that the X11Forwarding is defaulted to "no". So I assume the default would be enough. Is that a different case in RHEL7?
X11Forwarding Specifies whether X11 forwarding is permitted. The argument must be ''yes'' or ''no''. The default is ''no''.
* Make numerous `ssh_server` checks explicit.
There has been some conversations around what path to take here. We though at first that using accepting the default value for options that are secure by default was a good idea. But then it makes misaligned with DISA content.
* Add audit rule for RHEL to audit kmod.
The rule is slightly different than DISA STIG RHEL8, we may have to adapt something to be strictly equal to DISA STIG RHEL7
Rationale:
* `sshd_disable_x11_forwarding`: On RHEL7, a missing parameter does not necessarily mean that X11 forwarding is disabled. DISA's SCAP checks will fail this step without the parameter set. For safety, we set the parameter explicitly.
* `display_login_attempts`: DISA has different requirements than CIS and the current rule fails DISA's SCAP checks.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/ComplianceAsCode/content/pull/7827*issuecomment-960632350__;Iw!!MvWE!SJ5ZlmNnaIdi7wW3hodQsGNp-Q6uw9N6vIx8yPkl71gYJZHFHPqZzCa0oGzVtzCkfmY$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AUM63VXE6R3H2I3NQA3VO33UKJONVANCNFSM5HKH5PRA__;!!MvWE!SJ5ZlmNnaIdi7wW3hodQsGNp-Q6uw9N6vIx8yPkl71gYJZHFHPqZzCa0oGzV-_BUsIU$>.
Triage notifications on the go with GitHub Mobile for iOS<https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!MvWE!SJ5ZlmNnaIdi7wW3hodQsGNp-Q6uw9N6vIx8yPkl71gYJZHFHPqZzCa0oGzVl0q5mkI$> or Android<https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!MvWE!SJ5ZlmNnaIdi7wW3hodQsGNp-Q6uw9N6vIx8yPkl71gYJZHFHPqZzCa0oGzVROy7Bs8$>.
|
No, you keep the old CCEs in the rule. And add new CCEs to the new rule. Also, we have already some rules suffixed by |
|
So there will be a CCE attached to a rule that doesn't have the platform attached to it at all? Ooookay
…________________________________
From: Gabriel Becker ***@***.***>
Sent: Thursday, November 4, 2021 8:47:42 AM
To: ComplianceAsCode/content ***@***.***>
Cc: Lenox, Joseph D Collins ***@***.***>; Mention ***@***.***>
Subject: [External] Re: [ComplianceAsCode/content] Refactors and rules for RHEL7 DISA STIG (PR #7827)
So if I am splitting a product out from the rule, the CCE gets burned? That doesn't seem to make a lot of sense to me
No, you keep the old CCEs in the rule. And add new CCEs to the new rule. Also, we have already some rules suffixed by _stig.yml instead of _disa.yml, so you may want to rename the rule.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/ComplianceAsCode/content/pull/7827*issuecomment-960966939__;Iw!!MvWE!RUcKNA7YLdfu_Vc0IpuLvojNxvqYj91TxYiJgOMCe2LveSL9sooC_IO8UR1a6jhzPxs$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AUM63VSEEBMWZ64T6NSPALDUKKFH5ANCNFSM5HKH5PRA__;!!MvWE!RUcKNA7YLdfu_Vc0IpuLvojNxvqYj91TxYiJgOMCe2LveSL9sooC_IO8UR1axZxvkB0$>.
Triage notifications on the go with GitHub Mobile for iOS<https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!MvWE!RUcKNA7YLdfu_Vc0IpuLvojNxvqYj91TxYiJgOMCe2LveSL9sooC_IO8UR1aCfxmgx8$> or Android<https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!MvWE!RUcKNA7YLdfu_Vc0IpuLvojNxvqYj91TxYiJgOMCe2LveSL9sooC_IO8UR1aJ6zZvQ0$>.
|
I'm not sure if I understand your platform concept here. But CCE is used to identify uniquely a rule. These CCE numbers are provided by NIST and we can use them to assign to rules and later we notify them which rules received which CCE and they put into their database. So it doesn't make sense to simply move the CCE to a new rule, since the older one is still being used by many other profiles and we would also lose the sense of uniqueness. |
lenox-joseph
force-pushed
the
upstream-merge
branch
from
a8cc077 to
e0dcc74
Compare
...re_rules/audit_kernel_module_loading/audit_rules_kernel_module_loading_create/bash/shared.sh
Outdated
Show resolved
Hide resolved
...re_rules/audit_kernel_module_loading/audit_rules_kernel_module_loading_create/bash/shared.sh
Outdated
Show resolved
Hide resolved
...ules/audit_kernel_module_loading/audit_rules_kernel_module_loading_create/ansible/shared.yml
Outdated
Show resolved
Hide resolved
...ditd_configure_rules/audit_privileged_commands/audit_rules_privileged_commands_kmod/rule.yml
Outdated
Show resolved
Hide resolved
products/rhel7/profiles/stig.profile
Outdated
| @@ -274,7 +276,7 @@ selections: | |||
| - sshd_disable_user_known_hosts | |||
| - chronyd_or_ntpd_set_maxpoll | |||
| - service_firewalld_enabled | |||
| - display_login_attempts | |||
| - display_login_attempts_stig | |||
There was a problem hiding this comment.
I still believe we can extend display_login_attempts to accommodate any change required for STIG. Can you point out the reason why you decided to split the rules?
There was a problem hiding this comment.
The commit is older than your last changes that stripped silent. I've removed the split rule.
| @@ -31,7 +31,7 @@ | |||
|
|
|||
| <ind:textfilecontent54_object id="obj_sshd_use_approved_ciphers_ordered_stig" version="1"> | |||
| <ind:filepath>/etc/ssh/sshd_config</ind:filepath> | |||
| <ind:pattern operation="pattern match">^[\s]*(?i)Ciphers(?-i)[\s]+(?=[\w]+)(aes256-ctr(?=[\w,]+|$),?)?(aes192-ctr(?=[\w,]+|$),?)?(aes128-ctr)?[\s]*(?:#.*)?$</ind:pattern> | |||
| <ind:pattern operation="pattern match">^[\s]*[Cc]iphers[\s]+(?=\w)(aes256-ctr(?=[\w,-@]+|$),?)?(aes192-ctr(?=[\w,-@]+|$),?)?(aes128-ctr(?=[\w,-@]+|$),?)?[\s]*(?:#.*)?$</ind:pattern> | |||
There was a problem hiding this comment.
From: https://man7.org/linux/man-pages/man5/sshd_config.5.html
The possible keywords and their meanings are as follows (note that
keywords are case-insensitive and arguments are case-sensitive):
It means that CIpHeRS should also be accepted for example.
Also your regex doesn't match a line with trailing whitespace. I don't know exactly why but the previous one accepted it
I use this website to test regexes: https://regex101.com/
There was a problem hiding this comment.
Can you explain what's the intent behind these changes? Thank you
There was a problem hiding this comment.
I've got a use case where I need to adhere more closely to DISA (after remediation, I expect DISA's upstream rules to be evaluated on the system by a separate auditor), and I was looking for a path of less resistance.
There was a problem hiding this comment.
I've sent an email up to DISA; what I'll do right now is add a couple tests with CIpHeRs and trailing whitespace and adjust the regex to match.
There was a problem hiding this comment.
Trailing whitespace isn't marked as "pass" for the original regex either. I think that it's allowed, but not referenced at all in sshd_config(5).
There was a problem hiding this comment.
^[\s]*(?i)Ciphers(?-i)[\s]+(?=\w)(aes256-ctr(?=[\w,-@]+|$),?)?(aes192-ctr(?=[\w,-@]+|$),?)?(aes128-ctr(?=[\s\w,-@]+|$),?)?[\s]*(?:#.*)?[\s]*$Does catch trailing whitespace. Since DISA wouldn't return pass for 'Ciphers aes256-ctr,aes192-ctr,aes128-ctr ', however, I don't think we should either without a strong reason to do so.
There was a problem hiding this comment.
I believe the original (?i)Ciphers(?-i) should be enough to cover the case-insensitivity, although I don't really believe that people will use CIpHeRs in their SSH configuration files.
linux_os/guide/services/ssh/ssh_server/disable_host_auth/rule.yml
Outdated
Show resolved
Hide resolved
lenox-joseph
force-pushed
the
upstream-merge
branch
from
015b020 to
005f94a
Compare
|
/retest |
|
Looking at the text it looks like it's in the cluster infrastructure? Not something I can test locally. |
|
/retest |
|
/retest |
lenox-joseph
force-pushed
the
upstream-merge
branch
2 times, most recently
from
2439eb0 to
1ec6270
Compare
|
/retest |
lenox-joseph
force-pushed
the
upstream-merge
branch
from
4957719 to
d13bfb2
Compare
|
Code Climate has analyzed commit d13bfb2 and detected 2 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
… different requirements than CIS, etc and the existing rule fails 3rd-party checks from DISA. Add rule for RHEL7 to cover auditing create_module. Add RHEL7 to list of production items for auditing kmod. Fixed STIG rule sshd_disable_x11_forwarding Updated sshd ciphers/macs regex's to fix false failure.
Implement Bash remediation for RHEL7.
…watch` and `skip_action` for audit_rules_privleged_commands. Revise template to use ssg.utils function.
lenox-joseph
force-pushed
the
upstream-merge
branch
from
d13bfb2 to
e33667a
Compare
|
@lenox-joseph: The following tests failed, say
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. |
marcusburghardt
added
STIG
|
@lenox-joseph: PR needs rebase. 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. |
|
Closing as DISA decided to change its check syntax instead and the changes that needs to be implemented can be easier done as single PRs. |
Description:
sshd_disable_x11_forwarding: Set the parameter to fail if not present.display_login_attempts: Refactor into a DISA specific rule for RHEL7.Make numerousssh_serverchecks explicit.create_moduleunder RHEL7.Rationale:
sshd_disable_x11_forwarding: On RHEL7, a missing parameter does not necessarily mean that X11 forwarding is disabled. DISA's SCAP checks will fail this step without the parameter set. For safety, we set the parameter explicitly.display_login_attempts: DISA has different requirements than CIS and the current rule fails DISA's SCAP checks.create_module.kmodRHEL-07-030840.