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

SLES-15-10352 rule #6822

Merged

Conversation

teacup-on-rockingchair
Copy link
Contributor

Description:

  • Verify that Shared Library Directories Have Restrictive Permissions

Rationale:

  • Clone implementation of file_permissions_library_dirs to dir_permissions_library_dirs
  • Add some tests to cover basic scenarios

@openscap-ci
Copy link
Collaborator

Can one of the admins verify this patch?

1 similar comment
@openscap-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@openshift-ci-robot
Copy link
Collaborator

Hi @teacup-on-rockingchair. 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.

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Used by openshift-ci bot. label Apr 11, 2021
Copy link
Contributor

@brett060102 brett060102 left a comment

Choose a reason for hiding this comment

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

See inline comments

@@ -0,0 +1,18 @@
# platform = SUSE Linux Enterprise 15
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be multi_platform_sle not "SUSE Linux Enterprise 15"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment - should be ok in 86173c7

documentation_complete: true

title: |-
{{% if product in ["sle15"] %}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that the title should ever vary by distro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks - removed that in 4e4ea53

<pre>$ sudo chmod go-w <i>DIR</i></pre>

rationale: |-
If the SUSE operating system were to allow any user to make changes to software libraries,
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think this should be SUSE specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks - removed that in 4e4ea53

<unix:filename operation="pattern match">^.*$</unix:filename>
<filter action="include">dir_state_perms_nogroupwrite_noworldwrite</filter>
<filter action="exclude">dir_perms_state_symlink</filter>
</unix:file_object>
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the dir_object_file_permissions_lib_files and dir_object_file_permissions_lib_dir are doing the same thing. Am I missing something here? I guess from test name that one is for dirs and files, but test look there are the same and the rule is for dir's only so. I feel uncomfortable if the oval does something different than what is in the rule file.

Copy link
Member

Choose a reason for hiding this comment

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

It probably needs to replace the unix:filename with <unix:filename xsi:nil="true" />. See how it's done in the file_permissions template

@@ -147,6 +147,7 @@ selections:
- dconf_db_up_to_date
- dconf_gnome_banner_enabled
- dconf_gnome_login_banner_text
- dir_permissions_library_dirs
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a conflict on this file that should be addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rebased the branch so conflict should be resolved now in 07143f3

@ggbecker
Copy link
Member

@openscap-ci ok to test

@openscap-ci
Copy link
Collaborator

openscap-ci commented Apr 13, 2021

Changes identified:
Rules:
 dir_permissions_library_dirs
Profiles:
 stig on sle15

Show details

Rule dir_permissions_library_dirs:
 Bash remediation is newly added.
 OVAL check is newly added.
 Ansible remediation newly added.
Profile stig on sle15:
 Rule dir_permissions_library_dirs added to stig profile.

Recommended tests to execute:
 build_product sle15
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using ansible --datastream build/ssg-sle15-ds.xml dir_permissions_library_dirs
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using bash --datastream build/ssg-sle15-ds.xml dir_permissions_library_dirs
 tests/test_suite.py profile --libvirt qemu:///system test-suite-vm --datastream build/ssg-sle15-ds.xml stig

<unix:file_object comment="library files" id="dir_object_file_permissions_lib_files" version="1">
<!-- Check the files within /lib, /lib64, /usr/lib, /usr/lib64 directories have safe permissions (go-w) -->
<unix:path operation="pattern match">^\/lib(|64)|^\/usr\/lib(|64)</unix:path>
<unix:filename operation="pattern match">^.*$</unix:filename>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<unix:filename operation="pattern match">^.*$</unix:filename>
<unix:filename xsi:nil="true" />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea of the rule dir_object_file_permissions_lib_files was to match all files in the directories, as far as I understood <unix:filename xsi:nil="true" /> will match only the directories, which should have been done in dir_object_file_permissions_lib_dir, so my point here is to match all files with dir_object_file_permissions_lib_files and dirs and subdirs with dir_object_file_permissions_lib_dir object. I guess once you are suggested changes I misunderstood something in the syntax. When I implemented it I had in mind the file_object description in the oval spec (https://oval.mitre.org/language/version5.11/ovaldefinition/documentation/unix-definitions-schema.html), where it is stated that:

...
For example, one would set xsi:nil to true if the desire was to test the attributes or permissions associated with a directory. Setting xsi:nil equal to true is different than using a .* pattern match, which says to collect every file under a given path.

This was my motivation but as far as I understand from your feedback it was wrong so you would suggest to change both rules <unix:filename xsi:nil="true" />, but then I will not need both objects but only one would be enough - am I right

Copy link
Member

Choose a reason for hiding this comment

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

I have probably misread the comment from brett. If the idea is to check for all files in the directories then it is probably fine. I'm will go through the PR once again soon, please bear with me.

Copy link
Member

Choose a reason for hiding this comment

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

Based on text from SLES-15-010351, I understand that no files should be checked within this rule, I think that was my original concern.

The library files test should be covered by this rule already:
https://github.com/ComplianceAsCode/content/blob/master/linux_os/guide/system/permissions/files/permissions_within_important_dirs/file_permissions_library_dirs/rule.yml#L43

My question is, do we really want to have the test dir_object_file_permissions_lib_files in this rule?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ggbecker @teacup-on-rockingchair SLES-15-010352 the STIG for this PR is explicit in covering directories only "-type d", SLES-15-010351 is explicit in covering files "-type f" So, in this case the oval should only be handling directories. So, if I understand this correctly <unix:filename xsi:nil="true" /> is what is needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @brett060102 and @ggbecker , it seems I was totally confused on this one. Dropped completely the rule for files so now(252fa36) only dirs are checked by this one.

Copy link
Contributor

@brett060102 brett060102 left a comment

Choose a reason for hiding this comment

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

test cases need to end in either .fail.sh or .pass.sh so for example:

rename:
linux_os/guide/system/permissions/files/permissions_within_important_dirs/dir_permissions_library_dirs/tests/world_writable_dir_on_usr_lib.sh

to
linux_os/guide/system/permissions/files/permissions_within_important_dirs/dir_permissions_library_dirs/tests/world_writable_dir_on_usr_lib.fail.sh
same for owner_only_writable_dir.sh, world_writable_dir_on_lib.sh, world_writable_dir_on_lib.sh, world_writable_dir_on_usr_lib.sh

# platform = multi_platform_sle
DIRS="/usr/lib /usr/lib64"
for dirPath in $DIRS; do
mkdir -p "$dirPath/testme" && chmod 777 "$dirPath/test"
Copy link
Contributor

Choose a reason for hiding this comment

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

"$dirPath/test needs to be "$dirPath/testme

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx should be ok in 8851e58

@openshift-ci openshift-ci bot added the needs-rebase Used by openshift-ci bot. label May 3, 2021
@ggbecker
Copy link
Member

ggbecker commented May 3, 2021

Unfortunately, there is a conflict with the stig.profile file, can you resolve it?

@ggbecker ggbecker self-assigned this May 3, 2021
@ggbecker ggbecker added this to the 0.1.56 milestone May 3, 2021
Clone implementation of file_permissions_library_dirs to dir_permissions_library_dirs
Thanks to @brett060102 for pointing it out
Remove the dir_object_file_permissions_lib_files from dir_permissions_library_dirs,
since this check is covered in separate rule.

Thanks to @ggbecker and @brett060102 for the help on this 🙇
@openshift-ci openshift-ci bot removed the needs-rebase Used by openshift-ci bot. label May 4, 2021
…ortant_dirs/dir_permissions_library_dirs/rule.yml


Thanx to @ggbecker for pointing this one

Co-authored-by: Gabriel Becker <ggasparb@redhat.com>
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.

@ggbecker ggbecker merged commit 424835a into ComplianceAsCode:master May 5, 2021
@teacup-on-rockingchair teacup-on-rockingchair deleted the suse_SLES-15-010352 branch May 10, 2021 04:19
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants