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

Only allow root and members of group wheel to use su #134

Merged
merged 2 commits into from
Jun 25, 2018
Merged

Only allow root and members of group wheel to use su #134

merged 2 commits into from
Jun 25, 2018

Conversation

timstoop
Copy link
Contributor

@timstoop timstoop commented Jun 3, 2018

This fixes the benchmark cis-dil-benchmark-5.6.

Copy link
Member

@mcgege mcgege left a comment

Choose a reason for hiding this comment

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

And please squash after implementing your changes ;-)

README.md Outdated
@@ -68,6 +68,8 @@ This Puppet module provides secure configuration of your base OS with hardening.
the number of last passwords (e.g. 5 will prevent user to reuse any of her last 5 passwords)
* `allow_change_user = false`
if a user may use `su` to change his login
* `only_root_may_su = true`
Copy link
Member

Choose a reason for hiding this comment

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

I would better set the default to false, because this change might otherwise break existing installations

@@ -37,6 +37,7 @@
Boolean $manage_pam_unix = false,
Boolean $enable_pw_history = true,
Integer $pw_remember_last = 5,
Boolean $only_root_may_su = true,
Copy link
Member

Choose a reason for hiding this comment

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

default = false

manifests/pam.pp Outdated
@@ -17,6 +17,7 @@
Boolean $manage_pam_unix = false,
Boolean $enable_pw_history = false,
Integer $pw_remember_last = 5,
Boolean $only_root_may_su = true,
Copy link
Member

Choose a reason for hiding this comment

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

default = false

@@ -0,0 +1,68 @@
#
Copy link
Member

Choose a reason for hiding this comment

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

This template is only valid for debian / ubuntu ... please also distinguish between the distributions. I'll attach the default settings for SUSE and RedHat/Centos.

@mcgege
Copy link
Member

mcgege commented Jun 4, 2018

@timstoop
Copy link
Contributor Author

timstoop commented Jun 4, 2018

I'm not entirely happy with how the templates are provided now. Should I make subdirectories per OS instead?

@mcgege
Copy link
Member

mcgege commented Jun 5, 2018

@timstoop Concerning the templates: that's fine for me ... it's not crowded yet in the template folder

I just saw that you also need to pass $only_root_may_su to class os_hardening::pam in init.pp, line 182.
And I would also suggest that you include the commented line (with else) in pam_su_debian_ubuntu.erb and pam_su_suse.erb (as done in pam_su_redhat_centos.erb)

@timstoop
Copy link
Contributor Author

timstoop commented Jun 5, 2018

Will do, but might take a few weeks before I'm able to pick this up again (see my comment in PR #131).

HardeningFramework-DCO-1.1-Signed-off-by: Tim Stoop <github@timstoop.nl> (github: timstoop)
@timstoop
Copy link
Contributor Author

I think that fixes all complaints!

manifests/pam.pp Outdated
@@ -131,6 +133,32 @@
}
}

#only allow root and members of the group wheel to su
if $only_root_may_su {
case $::operatingsystem {
Copy link
Member

Choose a reason for hiding this comment

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

this case isn't working here, because you'll never reach this if you are not debian or ubuntu --> see line 48
So the whole block must be copied, customized and pasted into the end / line 167

I see this is hard to setup & test if you don't have access to those environments ... let me suggest this: reduce your solution once again (sorry) to ubuntu and I'll adapt this for the other platforms and insert another pr myself afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sorry, I'm only testing on Debian Stretch due to time constraints :/ I'm testing my work on a modified version of https://github.com/dev-sec/hardening/tree/master/puppet (as I want to use the contrib-stretch image, for instance). If there's a similar solution for RedHat/Suse, I'm happy to add it to my testing before I send in my patches! It's just that RedHat and Suse are fairly unknown to me (been using Debian since 1997), so it's not that easy for me to setup (and time does not permit it either).

For now, I'll do as requested and move it down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

s/move it down/reduce to debian/

manifests/pam.pp Outdated
@@ -131,6 +133,17 @@
}
}

#only allow root and members of the group wheel to su
if $only_root_may_su {
Copy link
Member

Choose a reason for hiding this comment

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

This if clause should be left out here - if you switch back only_root_may_su to false, the file won't get an update.

@artem-sidorenko
Copy link
Member

@mcgege does it make sense to port this to chef and ansible implementations?

@mcgege
Copy link
Member

mcgege commented Jun 25, 2018

@artem-sidorenko I'm undecided here as I don't know how often this feature is used ... and you have to pay attention that you don't lock out yourself (su doesn't work any more if root is not in this group) :-)

@timstoop Can you implement this last open change?

HardeningFramework-DCO-1.1-Signed-off-by: Tim Stoop <github@timstoop.nl> (github: timstoop)
@timstoop
Copy link
Contributor Author

Done!

@mcgege mcgege merged commit 86c7a91 into dev-sec:master Jun 25, 2018
@mcgege
Copy link
Member

mcgege commented Jun 25, 2018

@timstoop Thanks a lot!

@timstoop timstoop deleted the cis-dil-benchmark-5.6 branch June 27, 2018 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants