From owner-freebsd-bugs@FreeBSD.ORG Fri May 29 21:20:04 2009 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C08C31065670 for ; Fri, 29 May 2009 21:20:04 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id AD9F58FC17 for ; Fri, 29 May 2009 21:20:04 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.14.3/8.14.3) with ESMTP id n4TLK4tL067594 for ; Fri, 29 May 2009 21:20:04 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.3/8.14.3/Submit) id n4TLK4AR067593; Fri, 29 May 2009 21:20:04 GMT (envelope-from gnats) Date: Fri, 29 May 2009 21:20:04 GMT Message-Id: <200905292120.n4TLK4AR067593@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Jilles Tjoelker Cc: Subject: Re: bin/127532: [patch] install(1): install -S Not Safe in Jail with security.jail.chflags_allowed: 0 X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Jilles Tjoelker List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 May 2009 21:20:05 -0000 The following reply was made to PR bin/127532; it has been noted by GNATS. From: Jilles Tjoelker To: bug-followup@FreeBSD.org, jcw@highperformance.net Cc: Jaakko Heinonen Subject: Re: bin/127532: [patch] install(1): install -S Not Safe in Jail with security.jail.chflags_allowed: 0 Date: Fri, 29 May 2009 23:16:27 +0200 --PEIAKu/WMn1b1Hv9 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline The patch works but introduces a new problem: install -S -m 0 src dst (installing to an unreadable destination, probably as non-root) no longer works. I have fixed this particular issue (attachment and http://www.stack.nl/~jilles/unix/install-S-safe.patch ), but I think the code is too hard to understand. The install() function was already fairly twisted and the patch has not improved it. -- Jilles Tjoelker --PEIAKu/WMn1b1Hv9 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="install-S-safe.patch" Index: head/usr.bin/xinstall/xinstall.c =================================================================== --- head/usr.bin/xinstall/xinstall.c (revision 192913) +++ head/usr.bin/xinstall/xinstall.c (working copy) @@ -93,6 +93,7 @@ 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 @@ 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 @@ /* * 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 @@ files_match = 1; (void)unlink(tempfile); } - (void) close(temp_fd); } } @@ -409,6 +411,12 @@ * 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); @@ -441,10 +449,14 @@ /* Re-open to_fd so we aren't hosed by the rename(2). */ (void) close(to_fd); - if ((to_fd = open(to_name, O_RDONLY, 0)) < 0) - err(EX_OSERR, "%s", to_name); - } + to_fd = temp_fd; + temp_fd = -1; + } else + set_attributes(to_fd, to_name); + if (temp_fd != to_fd) + (void)close(temp_fd); + /* * Preserve the timestamp of the source file if necessary. */ @@ -456,43 +468,7 @@ (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. * NFS does not support flags. Ignore EOPNOTSUPP flags if we're just @@ -508,7 +484,7 @@ 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 @@ } (void)close(to_fd); - if (!devnull) - (void)close(from_fd); } /* @@ -701,6 +675,54 @@ } /* + * 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 */ --PEIAKu/WMn1b1Hv9--