Summary: | [patch] install(1): install -S (safe copy) with -C or -p is not so safe | ||||||
---|---|---|---|---|---|---|---|
Product: | Base System | Reporter: | John E. Hein <jhein> | ||||
Component: | bin | Assignee: | freebsd-bugs (Nobody) <bugs> | ||||
Status: | Open --- | ||||||
Severity: | Affects Only Me | CC: | jcfyecrayz, jhein | ||||
Priority: | Normal | Keywords: | patch | ||||
Version: | 6.2-STABLE | ||||||
Hardware: | Any | ||||||
OS: | Any | ||||||
Attachments: |
|
Description
John E. Hein
2007-05-02 01:50:03 UTC
Here is a patch that implements 2 and was simpler than I thought it would be. It creates the temp file and does a rename, not only if the contents of the "from" & "to" files don't match, but also if perms/ownership don't match. One optimization could be to try to do the chown/chmod first and if that succceeds and the files match contents, don't bother with the temp file. But that seems to be needlessly complex for the relatively miniscule number of cases where we could gain from the added pedantry. And there may be edge case failures associated with an early chmod/chown. Any committers willing to take this one? Index: src/usr.bin/xinstall/xinstall.c =================================================================== RCS file: /base/FreeBSD-CVS/src/usr.bin/xinstall/xinstall.c,v retrieving revision 1.67 diff -u -p -r1.67 xinstall.c --- src/usr.bin/xinstall/xinstall.c 6 Mar 2006 21:52:59 -0000 1.67 +++ src/usr.bin/xinstall/xinstall.c 7 May 2007 20:52:18 -0000 @@ -317,7 +317,19 @@ install(const char *from_name, const cha if (docompare && !dostrip && target) { if ((to_fd = open(to_name, O_RDONLY, 0)) < 0) err(EX_OSERR, "%s", to_name); - if (devnull) + /* + * Even if the contents are the same, we want to rename + * temp files when doing a "safe" copy if the + * permissions and ownership need to change. We may + * not have permission to chown/chmod the "to" file + * directly. + */ + if (tempcopy && + ((gid != (gid_t)-1 && gid != to_sb.st_gid) || + (uid != (uid_t)-1 && uid != to_sb.st_uid) || + (mode != (to_sb.st_mode & ALLPERMS)))) + files_match = 0; + else if (devnull) files_match = to_sb.st_size == 0; else files_match = !(compare(from_fd, from_name, Here is an update to the patch to refresh it after a recent commit to xinstall.c and to additionally check euid which is important in some non-superuser cases. Index: xinstall.c =================================================================== RCS file: /base/FreeBSD-CVS/src/usr.bin/xinstall/xinstall.c,v retrieving revision 1.68 diff -u -p -r1.68 xinstall.c --- xinstall.c 14 Dec 2007 08:46:57 -0000 1.68 +++ xinstall.c 15 Dec 2007 20:21:35 -0000 @@ -278,6 +278,7 @@ install(const char *from_name, const cha int devnull, files_match, from_fd, serrno, target; int tempcopy, temp_fd, to_fd; char backup[MAXPATHLEN], *p, pathbuf[MAXPATHLEN], tempfile[MAXPATHLEN]; + uid_t euid; files_match = 0; from_fd = -1; @@ -322,7 +323,20 @@ install(const char *from_name, const cha if (docompare && !dostrip && target) { if ((to_fd = open(to_name, O_RDONLY, 0)) < 0) err(EX_OSERR, "%s", to_name); - if (devnull) + /* + * Even if the contents are the same, we want to rename + * temp files when doing a "safe" copy if the + * permissions and ownership need to change. We may + * not have permission to chown/chmod the "to" file + * directly. + */ + if (tempcopy && (euid = geteuid()) != 0 && + euid != to_sb.st_uid && + ((gid != (gid_t)-1 && gid != to_sb.st_gid) || + (uid != (uid_t)-1 && uid != to_sb.st_uid) || + (mode != (to_sb.st_mode & ALLPERMS)))) + files_match = 0; + else if (devnull) files_match = to_sb.st_size == 0; else files_match = !(compare(from_fd, from_name, For bugs matching the following criteria: Status: In Progress Changed: (is less than) 2014-06-01 Reset to default assignee and clear in-progress tags. Mail being skipped Keyword: patch or patch-ready – in lieu of summary line prefix: [patch] * bulk change for the keyword * summary lines may be edited manually (not in bulk). Keyword descriptions and search interface: <https://bugs.freebsd.org/bugzilla/describekeywords.cgi> Created attachment 246032 [details]
[patch] safe copy update for permission/ownership-only changes
Still a problem with -current.
Attached is a refresh on the patch against the latest code - just line numbering changes in the patch.
The patch still fixes the problem as originally described (deleted file when using -S in certain cases).
|