About passwd, deluser and delgroup

Tito farmatito at tiscali.it
Sat Apr 8 06:33:46 UTC 2006


On Saturday 8 April 2006 04:27, Rob Landley wrote:
> 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.)

I'm locking the original file to avoid other programs to change it
while i'm deleting the user.

> 
> > 	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.

Why???
 
> 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...

How can there be two updates if the original file is locked and 
the new file is opened with O_EXCL ???

> > 				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...

I don't, I'm just doing deluser or delgroup so far, but if the code is 
ok from a security point of view  it would be easy to pass one 
or two more args (maybe a flag DELETE/CHANGE_PW and
the new password) and do passwd also. So this could
be moved to libbb and we have one pretty
function dealing with all passwd/group files
handling.

> > 	/* 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

Ciao,
Tito



More information about the busybox mailing list