About passwd, deluser and delgroup

[Rob Landley]

On Thursday 06 April 2006 3:14 pm, Tito wrote:

> > If the new file is a constant filename (which is not a security problem
> > if /etc isn't world writeable) then O_EXCL should fail if it already
> > exists.
>
> OK.
>
> > The theory is to create a new version and then atomic rename it over the
> > old one.  (The backup is done via a hard link first, so the old one
> > should remain unmodified.  No need to actually copy the data and set the
> > date and all that.)
> >
> >     // Create new file, as one atomic operation with the right
> > permissions and // ownership because doing chmod or chown after create
> > opens a race.
> >
> >     if ((out_fd = bb_xopen(dest, O_CREAT|O_EXCL,
> >             ENABLE_FEATURE_SHADOWPASSWDS ? 0600 : 0644)) < 0) return 0;
>
> I' m trying to use about the same routine in deluser and delgroup which
> by the way I'm unifying in one file together with delline.c.
> The code is:
>
> static void del_line_matching(const char *login, const char *filename)
> {
> 	char *line;
> 	FILE *passwd;
> 	FILE *temp;
> 	char *new_file = "/etc/ptmp";
> 	int fd;
> 	int found = 0;
> 	struct stat statbuf;
> 	struct flock lock;
>
> 	/* open file */
> 	passwd = bb_xfopen(filename, "r+");
>
> 	/*get mode */
> 	xstat(filename, &statbuf);
>
> 	/* Lock the password file before updating */
> 	lock.l_type = F_WRLCK;
> 	lock.l_whence = SEEK_SET;
> 	lock.l_start = 0;
> 	lock.l_len = 0;

If we're using the O_EXCL thing then we don't need to mess with file locking 
at all.  (The kernel doesn't even have to support it.)


> 	if (fcntl(fileno(passwd), F_SETLK, &lock) == 0) {
> 		/* Be paranoid about who's going to own this file. */
> 		setuid(0);
> 		setgid(0);
> 		/* get fd */
> 		if ((fd = open(new_file, O_WRONLY | O_CREAT | O_EXCL, statbuf.st_mode))
> != -1) { /* get stream */
> 			if ((temp = fdopen(fd,"w"))) {
> 				/* Loop through entries, copying each one except the one we want to
> remove */ while ((line = bb_get_line_from_file(passwd)) != NULL) {
> 					if ((strncmp(login, line, strlen(login)) == 0) && line[strlen(login)]
> == ':') { found++;
> 					} else {
> 						fputs(line, temp);
> 					}
> 					free(line);
> 				}

This reorders lines.  Bad thing.

You're also discarding everything we already know about the user, meaning two 
close updates (one updating password and one updating the command shell that 
user uses) will stomp each other, since your lock only protects the actual 
update of the file and not the construction of the new user info structure...

> 				if (!found) {
> 					bb_error_msg("'%s' not found in '%s'", login, filename);
> 				} else {
> 					/* Be sure the new file was written to disk */
> 					if (!fflush(temp) && !fsync(fileno(temp)) && !fclose(temp)) {
> 						if (rename(new_file, filename)) {
> 							bb_error_msg("Can't rename %s to %s", new_file, filename);
> 						}
> 					}
> 				}
> 			}
> 		}
> 		/* Delete new copy in case it didn't get renamed. */
> 		unlink(new_file);
> 	}

I seem to have missed where were you were writing out the new user data at 
all...

> 	/* Remove lock */
> 	lock.l_type = F_UNLCK;
> 	fcntl(fileno(passwd), F_SETLK, &lock);
> 	/* close original file */
> 	fclose(passwd);
> }
>
> This should work with /etc/passwd /etc/shadow, /etc/group and /etc/gshadow
>
> Comments and hints form the list members about the security of
> this code are wellcome as I'm not that good in this things....

I'm already fiddling in this area. :)

Rob
-- 
Never bet against the cheap plastic solution.