misc/127532: Install -S Not Safe in Jail with security.jail.chflags_allowed: 0

Jaakko Heinonen jh at saunalahti.fi
Mon Sep 29 14:50:04 UTC 2008


The following reply was made to PR misc/127532; it has been noted by GNATS.

From: Jaakko Heinonen <jh at saunalahti.fi>
To: "Jason C. Wells" <jcw at highperformance.net>
Cc: bug-followup at FreeBSD.org
Subject: Re: misc/127532: Install -S Not Safe in Jail with
	security.jail.chflags_allowed: 0
Date: Mon, 29 Sep 2008 17:39:53 +0300

 --jRHKVT23PllUwdXP
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline
 
 On 2008-09-22, Jason C. Wells wrote:
 > The install command deleted libc when it should not have.  Running the
 > install command with '-fschg -S' deletes the install target when
 > security.jail.chflags_allowed=0 inside a jail.  I observed this during
 > installworld.  The problem can be avoided by setting
 > security.jail.chflags_allowed=1 before running make installworld.
 
 This is a bug in install(1). Here's a stripped down way to reproduce it:
 
 (in a jail)
 
 # sysctl security.jail.jailed security.jail.chflags_allowed
 security.jail.jailed: 1
 security.jail.chflags_allowed: 0
 # touch target
 # ls -lo target
 -rw-r--r--  1 root  wheel  - 0 Sep 29 09:54 target
 # install -fschg -S /bin/cat target
 install: target: chflags: Operation not permitted
 # ls -lo target
 ls: target: No such file or directory
 
 The problem is that install(1) unlinks the target file if fchflags(2)
 fails. This is not good especially with -S which is supposed to be safe.
 There are also three more unsafe unlink(2) calls in install() function.
 fstat(2) and fchown(2) and fchmod(2) are performed for the file after
 rename. Failure on those calls causes the target to be unlinked. This is
 how to reproduce the fchown(2) problem without jail:
 
 (use a non-root user)
 
 $ touch target
 $ ls target
 target
 $ install -S -o root /bin/cat target
 install: target: chown/chgrp: Operation not permitted
 $ ls target
 ls: target: No such file or directory
 
 
 Attached patch does following changes:
 
 * If safe copy is used perform fstat(2) and fchown(2) and fchmod(2)
   _before_ rename and on failure unlink the temporary copy instead of
   the target.
 * On fchlags(2) failure don't unlink the target. We still exit with error
   status. fchflags(2) can't be performed before rename because
   immutable flags may prevent renaming.
 
 -- 
 Jaakko
 
 --jRHKVT23PllUwdXP
 Content-Type: text/x-diff; charset=us-ascii
 Content-Disposition: attachment; filename="xinstall-unsafe--S.diff"
 
 Index: usr.bin/xinstall/xinstall.c
 ===================================================================
 --- usr.bin/xinstall/xinstall.c	(revision 183263)
 +++ usr.bin/xinstall/xinstall.c	(working copy)
 @@ -93,6 +93,7 @@ int	create_tempfile(const char *, char *
  void	install(const char *, const char *, u_long, u_int);
  void	install_dir(char *);
  u_long	numeric_id(const char *, const char *);
 +void	set_attributes(int, const char *);
  void	strip(const char *);
  int	trymmap(int);
  void	usage(void);
 @@ -353,6 +354,9 @@ install(const char *from_name, const cha
  			     tempcopy ? tempfile : to_name, from_sb.st_size);
  	}
  
 +	if (!devnull)
 +		(void)close(from_fd);
 +
  	if (dostrip) {
  		strip(tempcopy ? tempfile : to_name);
  
 @@ -369,9 +373,8 @@ install(const char *from_name, const cha
  	/*
  	 * Compare the stripped temp file with the target.
  	 */
 +	temp_fd = to_fd;
  	if (docompare && dostrip && target) {
 -		temp_fd = to_fd;
 -
  		/* Re-open to_fd using the real target name. */
  		if ((to_fd = open(to_name, O_RDONLY, 0)) < 0)
  			err(EX_OSERR, "%s", to_name);
 @@ -400,7 +403,6 @@ install(const char *from_name, const cha
  				files_match = 1;
  				(void)unlink(tempfile);
  			}
 -			(void) close(temp_fd);
  		}
  	}
  
 @@ -409,6 +411,12 @@ install(const char *from_name, const cha
  	 * and the files are different (or just not compared).
  	 */
  	if (tempcopy && !files_match) {
 +		/*
 +		 * Set attributes before rename so we can safely abort
 +		 * on failure and leave the target untouched.
 +		 */
 +		set_attributes(temp_fd, tempfile);
 +
  		/* Try to turn off the immutable bits. */
  		if (to_sb.st_flags & NOCHANGEBITS)
  			(void)chflags(to_name, to_sb.st_flags & ~NOCHANGEBITS);
 @@ -443,7 +451,11 @@ install(const char *from_name, const cha
  		(void) close(to_fd);
  		if ((to_fd = open(to_name, O_RDONLY, 0)) < 0)
  			err(EX_OSERR, "%s", to_name);
 -	}
 +	} else
 +		set_attributes(to_fd, to_name);
 +
 +	if (docompare && dostrip && target)
 +		(void)close(temp_fd);
  
  	/*
  	 * Preserve the timestamp of the source file if necessary.
 @@ -456,42 +468,6 @@ install(const char *from_name, const cha
  		(void)utimes(to_name, tvb);
  	}
  
 -	if (fstat(to_fd, &to_sb) == -1) {
 -		serrno = errno;
 -		(void)unlink(to_name);
 -		errno = serrno;
 -		err(EX_OSERR, "%s", to_name);
 -	}
 -
 -	/*
 -	 * Set owner, group, mode for target; do the chown first,
 -	 * chown may lose the setuid bits.
 -	 */
 -	if ((gid != (gid_t)-1 && gid != to_sb.st_gid) ||
 -	    (uid != (uid_t)-1 && uid != to_sb.st_uid) ||
 -	    (mode != (to_sb.st_mode & ALLPERMS))) {
 -		/* Try to turn off the immutable bits. */
 -		if (to_sb.st_flags & NOCHANGEBITS)
 -			(void)fchflags(to_fd, to_sb.st_flags & ~NOCHANGEBITS);
 -	}
 -
 -	if ((gid != (gid_t)-1 && gid != to_sb.st_gid) ||
 -	    (uid != (uid_t)-1 && uid != to_sb.st_uid))
 -		if (fchown(to_fd, uid, gid) == -1) {
 -			serrno = errno;
 -			(void)unlink(to_name);
 -			errno = serrno;
 -			err(EX_OSERR,"%s: chown/chgrp", to_name);
 -		}
 -
 -	if (mode != (to_sb.st_mode & ALLPERMS))
 -		if (fchmod(to_fd, mode)) {
 -			serrno = errno;
 -			(void)unlink(to_name);
 -			errno = serrno;
 -			err(EX_OSERR, "%s: chmod", to_name);
 -		}
 -
  	/*
  	 * If provided a set of flags, set them, otherwise, preserve the
  	 * flags, except for the dump flag.
 @@ -508,7 +484,7 @@ install(const char *from_name, const cha
  				warn("%s: chflags", to_name);
  			else {
  				serrno = errno;
 -				(void)unlink(to_name);
 +				(void)close(to_fd);
  				errno = serrno;
  				err(EX_OSERR, "%s: chflags", to_name);
  			}
 @@ -516,8 +492,6 @@ install(const char *from_name, const cha
  	}
  
  	(void)close(to_fd);
 -	if (!devnull)
 -		(void)close(from_fd);
  }
  
  /*
 @@ -701,6 +675,54 @@ copy(int from_fd, const char *from_name,
  }
  
  /*
 + * set_attributes --
 + *	set file mode, uid and gid
 + */
 +void
 +set_attributes(int fd, const char *filename)
 +{
 +	struct stat sb;
 +	int serrno;
 +
 +	if (fstat(fd, &sb) == -1) {
 +		serrno = errno;
 +		(void)unlink(filename);
 +		errno = serrno;
 +		err(EX_OSERR, "%s", filename);
 +	}
 +
 +	/*
 +	 * Set owner, group, mode for target; do the chown first,
 +	 * chown may lose the setuid bits.
 +	 */
 +	if ((gid != (gid_t)-1 && gid != sb.st_gid) ||
 +	    (uid != (uid_t)-1 && uid != sb.st_uid) ||
 +	    (mode != (sb.st_mode & ALLPERMS))) {
 +		/* Try to turn off the immutable bits. */
 +		if (sb.st_flags & NOCHANGEBITS)
 +			(void)fchflags(fd, sb.st_flags & ~NOCHANGEBITS);
 +	}
 +
 +	if ((gid != (gid_t)-1 && gid != sb.st_gid) ||
 +	    (uid != (uid_t)-1 && uid != sb.st_uid))
 +		if (fchown(fd, uid, gid) == -1) {
 +			serrno = errno;
 +			(void)unlink(filename);
 +			errno = serrno;
 +			err(EX_OSERR,"%s: chown/chgrp", filename);
 +		}
 +
 +
 +	if (mode != (sb.st_mode & ALLPERMS))
 +		if (fchmod(fd, mode)) {
 +			serrno = errno;
 +			(void)unlink(filename);
 +			errno = serrno;
 +			err(EX_OSERR, "%s: chmod", filename);
 +		}
 +}
 +
 +/*
   * strip --
   *	use strip(1) to strip the target file
   */
 
 --jRHKVT23PllUwdXP--


More information about the freebsd-bugs mailing list