cp -p preserves, among others, the file flags. Unfortunately, this clashes with its attempt just a little bit later to retain a file's setuid/setgid/sticky bits if the user doing the copying is the file owner. (There was a similar PR about /sbin/restore's handling of utimes a while back.) Fix: Alright, so this might not be the best solution; arguably the best way is to move the whole setuid/setgid/sticky bits fixup into setfile() itself, where it belongs. But this works for me ;) How-To-Repeat: [root@ringwraith /usr/home/roam]# ls -lo /usr/bin/passwd -r-sr-xr-x 2 root wheel schg 26260 Aug 16 16:13 /usr/bin/passwd [root@ringwraith /usr/home/roam]# cp -p /usr/bin/passwd . cp: ./passwd: Operation not permitted [root@ringwraith /usr/home/roam]# ls -lo passwd -r-sr-xr-x 1 root wheel schg 26260 Aug 16 16:13 passwd [root@ringwraith /usr/home/roam]#
Responsible Changed From-To: freebsd-bugs->dwmalone I looked at a similar PR for restore - I'll try to have a look at this one.
On Wed, Aug 16, 2000 at 09:16:21PM +0200, Sheldon Hearn wrote: > > That's what I'm getting at. I just wanted to be sure that I understand. > The problem here is not with the end result on the filesystem, but > rather with the manner in which cp(1) interacts with the caller. > > I'll take a look and see if we can't come up with something simpler. > Don't take this badly; you yourself said that your patch may not embody > the best solution. I'd just like to explore possible alternatives, > especially if we can find a more elegant solution. > > Ciao, > Sheldon. > OK, I see your point. I'll sleep on it and think again tomorrow; I admin that today's patch was made somewhat in haste, because I *needed* a cp that would return success. I'm too sleepy right now to figure out under what conditions this breaks anyway :( [ 15 minutes later ;-] Actually, instead of hiding behind sleepiness, I sat down and figured out just when this setuid/setgid fixup occured. The result? A somewhat better patch, with a wholly different idea :) Look at it and tell me what you think. G'luck, Peter -- I've heard that this sentence is a rumor. diff -u -urN src/bin/cp/utils.c mysrc/bin/cp/utils.c --- src/bin/cp/utils.c Wed Aug 16 18:15:40 2000 +++ mysrc/bin/cp/utils.c Wed Aug 16 23:12:17 2000 @@ -178,22 +178,11 @@ if (pflag && setfile(fs, to_fd)) rval = 1; - /* - * If the source was setuid or setgid, lose the bits unless the - * copy is owned by the same user and group. - */ -#define RETAINBITS \ - (S_ISUID | S_ISGID | S_ISVTX | S_IRWXU | S_IRWXG | S_IRWXO) - else if (fs->st_mode & (S_ISUID | S_ISGID) && fs->st_uid == myuid) { - if (fstat(to_fd, &to_stat)) { - warn("%s", to.p_path); - rval = 1; - } else if (fs->st_gid == to_stat.st_gid && - fchmod(to_fd, fs->st_mode & RETAINBITS & ~myumask)) { - warn("%s", to.p_path); - rval = 1; - } - } + + /* We no longer need to reinstate the setuid/setgid bits - see + the 'chown failed' part of setfile() - they are no longer reset, + and we couldn't reinstate them on immutable files anyway. */ + (void)close(from_fd); if (close(to_fd)) { warn("%s", to.p_path); @@ -299,7 +288,20 @@ warn("chown: %s", to.p_path); rval = 1; } - fs->st_mode &= ~(S_ISUID | S_ISGID); + + /* the chown failed, lose only the privileges + we have no right to have */ + + if ((fs->st_mode & S_ISUID) && (fs->st_uid != myuid)) + fs->st_mode &= ~S_ISUID; + if ((fs->st_mode & S_ISGID) && (fs->st_gid != ts.st_gid)) + fs->st_mode &= ~S_ISGID; + + /* I'm not quite sure what to do about the sticky bit - + for a file it doesn't matter, for a dir - I think + it's a good thing to have, just in case the world + decides to storm into our little newborn dir ;) + Think I'll just leave it alone. */ } if (!gotstat || fs->st_mode != ts.st_mode)
On Wed, Aug 16, 2000 at 10:11:50PM +0200, Sheldon Hearn wrote: > > > On Wed, 16 Aug 2000 21:16:21 +0200, Sheldon Hearn wrote: > > > I'd just like to explore possible alternatives, especially if we can > > find a more elegant solution. > > Hi Peter, > > Could you give me a hand here? I'm looking at this excerpt from > utils.c: > > > /* > * Don't remove the target even after an error. The target might > * not be a regular file, or its attributes might be important, > * or its contents might be irreplaceable. It would only be safe > * to remove it if we created it and its length is 0. > */ > > if (pflag && setfile(fs, to_fd)) > rval = 1; > /* > * If the source was setuid or setgid, lose the bits unless the > * copy is owned by the same user and group. > */ > #define RETAINBITS \ > (S_ISUID | S_ISGID | S_ISVTX | S_IRWXU | S_IRWXG | S_IRWXO) > else if (fs->st_mode & (S_ISUID | S_ISGID) && fs->st_uid == myuid) { > if (fstat(to_fd, &to_stat)) { > warn("%s", to.p_path); > rval = 1; > } else if (fs->st_gid == to_stat.st_gid && > fchmod(to_fd, fs->st_mode & RETAINBITS & ~myumask)) { > warn("%s", to.p_path); > rval = 1; > } > } > [...] > > It's that fchmod() that generates the bogus error. My question, though, > is why the else if clause depend on (fs->st_uid == myuid) instead of > (fs->st_uid != myuid). The comment immediately preceding implies that > we don't need to lose any mode bits if the copy and the source are owned by > the same user and group. > > In fact, it also doesn't make sense to me why S_ISUID and S_ISGID are > included in RETAINBITS, since the comment implies that these bits aren't > wanted in this case. So clearly I'm missing something. > > Can you explain what's going on in there? :-) > > Thanks, > Sheldon. > Well, actually, with the 15 minutes I mentioned in my previous e-mail, and with half the 2-liter bottle of Coke here, I think I can :) The comment is misleading - this excerpt does not *lose* any bits, it tries to *reinstate* the ones lost in utils.c, line 302 - setfile(), the case when chown() fails. Apparently the idea is that if we cannot chown() the new file, we are not root, or it is not our file, so cp cannot afford to possibly let a cracker elevate his privileges in any way. Then, copy_file() thinks: well, this file was owned by me anyway, there can be no additional gain in a setuid. Same for groups. This is just what I'm addressing in the new patch I sent in my previous e-mail - setfile() only resets the setuid bit if the original file was not owned by me, and the setgid bit if it was not in the group I can create files as. No additional arguments, no ugly checks - straight to the point (and no, cp doesn't whine on schg anymore :) G'luck, Peter -- I had to translate this sentence into English because I could not read the original Sanskrit.
On Wed, Aug 16, 2000 at 11:26:58AM -0700, dwmalone@FreeBSD.org wrote: > > Responsible-Changed-From-To: freebsd-bugs->dwmalone > Responsible-Changed-By: dwmalone > Responsible-Changed-When: Wed Aug 16 11:26:13 PDT 2000 > Responsible-Changed-Why: > I looked at a similar PR for restore - I'll try to have a look at this one. Actually, Sheldon Hearn has been discussing this with me via e-mail for the past hour; Sheldon, care to summarize? :) G'luck, Peter
State Changed From-To: open->closed Fixed in -current rev.1.30 of utils.c, etc. Fixed in RELENG_4 in rev.1.27.2.2 utils.c, etc.