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

Fix UMASK hardening #14

Merged
merged 1 commit into from
May 11, 2022
Merged

Fix UMASK hardening #14

merged 1 commit into from
May 11, 2022

Conversation

alewando
Copy link

@alewando alewando commented May 9, 2022

Updated regex replacements in bashrc umask hardening. Hard-coded replacement with UMASK results in runtime failure because UMASK (all caps) does not exist. Also, whitespace was not being preserved.

Signed-off-by: Adam Lewandowski <adam.lewandowski@plxis.com>
@uk-bolly
Copy link
Member

Hi @alewando

Thank you again for continuing to work through this, as we mentioned feedback is so important to pick up all the little pieces.
To explain the regexp in this searching with case insensitive and matches both upper and lower.
This would then set it with upper and one space as per the current way CIS request that it is carried out.
However the space could be an issue and changing the regexp to pick up different space layouts is a definite improvement.
So i would add a double change to this

e.g.
regexp: '^(?i)(\s+UMASK|UMASK)\s+0[0-2][0-6]'

and your change for replace to pick up the first group.

Hopefully that makes sense?
I am happy to add if you like or just amend your PR unless you see any issues with it?

Best regards as always

uk-bolly

@alewando
Copy link
Author

I'm not sure I understand what you are saying about the upper case. Without this change, it produces the following in /etc/bashrc:

    if [ $UID -gt 199 ] && [ "`/usr/bin/id -gn`" = "`/usr/bin/id -un`" ]; then
UMASK 27
    else
UMASK 27
    fi

This is not valid. There is no uppercase UMASK command. If bashrc is left like this, when you have a interactive non-login shell, you get this:

# bash -ic "whoami"
bash: UMASK: command not found
root

As far as I can tell, this PR follows the guidance for /etc/bashrc and /etc/profile:
https://static.open-scap.org/ssg-guides/ssg-rhel9-guide-cis.html#xccdf_org.ssgproject.content_rule_accounts_umask_etc_bashrc

And it is what was done in your RHEL 8 playbook:
https://github.com/ansible-lockdown/RHEL8-CIS/blob/43a1938113fab5110d469d2f20df17af394020e5/tasks/section_5/cis_5.6.x.yml#L94

I think uppercase UMASK and no whitespace is necessary for /etc/login.defs, however, which is specified in the next rule.
But I don't see anything in this RHEL9 playbook for that rule. Maybe there was a mixup?
https://static.open-scap.org/ssg-guides/ssg-rhel9-guide-cis.html#xccdf_org.ssgproject.content_rule_accounts_umask_etc_login_defs

@uk-bolly
Copy link
Member

hi @alewando

Thank you once again for your very quick turnaround. We really appreciate the documents they do help a lot, I now have the context i missed and was myself confused with login.defs vs these files.

PR approval shortly

Best regards and thanks again

uk-bolly

Copy link
Member

@uk-bolly uk-bolly left a comment

Choose a reason for hiding this comment

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

great catch

@uk-bolly uk-bolly merged commit d5cce24 into ansible-lockdown:devel May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants