#18 Always set the home directory permissions according to HOME_MODE
Merged a year ago by abbra. Opened a year ago by casantos.
casantos/oddjob master  into  master

file modified
+21 -24
@@ -67,6 +67,7 @@ 

  {

  	uid_t uid = pwd->pw_uid;

  	gid_t gid = pwd->pw_gid;

+ 	mode_t mode = sb->st_mode & ~override_umask;

  	int sfd, dfd, i, res;

  	char target[PATH_MAX + 1], newpath[PATH_MAX + 1];

  	unsigned char buf[BUFSIZ];
@@ -112,8 +113,7 @@ 

  			oddjob_set_selinux_file_creation_context(newpath,

  								 sb->st_mode |

  								 S_IFREG);

- 			dfd = open(newpath, O_WRONLY | O_CREAT | O_EXCL,

- 				   sb->st_mode & ~override_umask);

+ 			dfd = open(newpath, O_WRONLY | O_CREAT | O_EXCL, mode);

  			if (dfd != -1) {

  				while ((i = read(sfd, buf, sizeof(buf))) > 0) {

  					retry_write(dfd, buf, i);
@@ -156,20 +156,22 @@ 

  		}

  		return 0;

  	case FTW_D:

- 		/* It's the home directory itself. Don't give it to the

- 		 * target user just yet to avoid potential race conditions

- 		 * involving symlink attacks when we copy over the skeleton

- 		 * tree. */

- 		if (status->level == 0 && !owner_mkdir_first) {

- 			uid = 0;

- 			gid = 0;

- 		}

- 

  		/* It's a directory.  Make one with the same name and

  		 * permissions, but owned by the target user. */

- 		res = oddjob_selinux_mkdir(newpath,

- 					   sb->st_mode & ~override_umask,

- 					   uid, gid);

+ 		if (status->level == 0) {

+ 			/* It's the home directory itself.  Use the configured

+ 			 * (or overriden) mode, not the source mode & umask. */

+ 			mode = 0777 & ~override_umask;

+ 

+ 			/* Don't give it to the target user just yet to avoid

+ 			 * potential race conditions involving symlink attacks

+ 			 * when we copy over the skeleton tree. */

+ 			if (!owner_mkdir_first) {

+ 				uid = 0;

+ 				gid = 0;

+ 			}

+ 		}

+ 		res = oddjob_selinux_mkdir(newpath, mode, uid, gid);

  

  		/* on unexpected errors, or if the home directory itself

  		 * suddenly already exists, abort the copy operation. */
@@ -248,12 +250,8 @@ 

  

  				return res;

  			} else {

- 				if (stat(skel, &st) != 0) {

- 					st.st_mode = S_IRWXU;

- 				}

  				if ((oddjob_selinux_mkdir(pwd->pw_dir,

- 							  st.st_mode &

- 							  ~override_umask,

+ 							  0777 & ~override_umask,

  							  pwd->pw_uid,

  							  pwd->pw_gid) != 0) &&

  				    (errno != EEXIST)) {
@@ -269,11 +267,11 @@ 

  }

  

  static mode_t

- get_umask(int *configured, const char *variable)

+ get_umask(int *configured, const char *variable, mode_t default_value)

  {

  	FILE *fp;

  	char buf[BUFSIZ], *p, *end;

- 	mode_t mask = umask(0777);

+ 	mode_t mask = default_value;

  	long tmp;

  	size_t vlen = strlen(variable);

  
@@ -315,11 +313,10 @@ 

  

  	openlog(PACKAGE "-mkhomedir", LOG_PID, LOG_DAEMON);

  	/* Unlike UMASK, HOME_MODE is the file mode, so needs to be reverted */

- 	override_umask = 0777 & ~get_umask(&configured_umask, "HOME_MODE");

+ 	override_umask = 0777 & ~get_umask(&configured_umask, "HOME_MODE", 0);

  	if (configured_umask == 0) {

- 		override_umask = get_umask(&configured_umask, "UMASK");

+ 		override_umask = get_umask(&configured_umask, "UMASK", 022);

  	}

- 	umask(override_umask);

  	skel_dir = "/etc/skel";

  

  	while ((i = getopt(argc, argv, "nqfs:u:")) != -1) {

Currently the home directory permissions are set by taking the /etc/skel
mode and masking it with HOME_MODE:

override_umask = 0777 & ~get_umask(&configured_umask, "HOME_MODE");
stat(skel, &sb); /* performed by nftw() */
oddjob_selinux_mkdir(newpath, sb->st_mode & ~override_umask, uid, gid);

The problem is that when HOME_MODE is more permissive than /etc/skel,
the masking will not produce the desired result, e.g.

skel_mode = 0755
HOME_MODE = 0775
override_umask = 0777 & ~HOME_MODE /* 0002 */
mode = skel_mode & ~override_umask /* 0755 & 0775 = 0755 */

In order to fix the problem, always use 0777 & ~override_umask for the
top home directory.

Signed-off-by: Carlos Santos casantos@redhat.com
Fixes: https://pagure.io/oddjob/issue/17

thanks, @casantos. I am currently on vacation due to school holidays and will look at this next week.

Metadata Update from @abbra:
- Request assigned

a year ago

LGTM. Thank you for the elaborate description.

Pull-Request has been merged by abbra

a year ago
Metadata