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

pam_mkhomedir: call umask(2) and let the OS do the masking #129

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

elric1
Copy link

@elric1 elric1 commented Aug 1, 2019

rather than chmod(2)ing everything that we create. This fixes a bug
where the sgid bit from the parent directory was not inherited. We also
try to preserve the permissions of the sub-directories from the skeleton
directory.

rather than chmod(2)ing everything that we create.  This fixes a bug
where the sgid bit from the parent directory was not inherited.  We also
try to preserve the permissions of the sub-directories from the skeleton
directory.
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

If this changed code is accidentally run with umask set to allow others to write it will probably enable race condition attacks. Not that using such umask with pam_mkhomedir would not be security issue by itself. However I'd still prefer if we prevent such accident.

@@ -26,21 +26,20 @@
#include <security/pam_ext.h>
#include <security/pam_modutil.h>

static unsigned long u_mask = 0022;
Copy link
Member

Choose a reason for hiding this comment

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

Please do not drop the 0022 default umask handling.

errno = 0;
u_mask = strtoul(argv[2], &eptr, 0);
if (errno != 0 || *eptr != '\0') {
pam_syslog(NULL, LOG_ERR, "Bogus umask value %s", argv[2]);
return PAM_SESSION_ERR;
}

umask(u_mask);
Copy link
Member

Choose a reason for hiding this comment

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

Please call umask with the default 0022 value when the umask argument is not specified.

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.

None yet

2 participants