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