bin/127532: [patch] install(1): install -S Not Safe in Jail
with security.jail.chflags_allowed: 0
Jilles Tjoelker
jilles at stack.nl
Fri May 29 21:20:05 UTC 2009
The following reply was made to PR bin/127532; it has been noted by GNATS.
From: Jilles Tjoelker <jilles at stack.nl>
To: bug-followup at FreeBSD.org, jcw at highperformance.net
Cc: Jaakko Heinonen <jh at saunalahti.fi>
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--
More information about the freebsd-bugs
mailing list