Date: Mon, 29 Sep 2008 14:50:04 GMT From: Jaakko Heinonen <jh@saunalahti.fi> To: freebsd-bugs@FreeBSD.org Subject: Re: misc/127532: Install -S Not Safe in Jail with security.jail.chflags_allowed: 0 Message-ID: <200809291450.m8TEo4qq064034@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR misc/127532; it has been noted by GNATS. From: Jaakko Heinonen <jh@saunalahti.fi> To: "Jason C. Wells" <jcw@highperformance.net> Cc: bug-followup@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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200809291450.m8TEo4qq064034>