bzr clobbers file owner and group

[cheater]

create a file, assign it a different user and/or group than yours. Edit it. use bzr shelve to revert those changes. By now the ownership is clobbered.

Expected:
the group and user and all permissions stay the same.

Cause:

stracing "bzr unshelve" (I guess bzr shelve will behave the same), I see:

rename("/home/user/projects/dev/file.ext", "/home/user/projects/dev/.bzr/checkout/pending-deletion/new-1") = 0
rename("/home/user/projects/dev/.bzr/checkout/limbo/new-1", "/home/userprojects/dev/file.ext") = 0

That's wrong, it should open the existing file and replace the contents instead. Copying clobbers the ownership, and the ownership can often not be changed (chown and chgrp require escalated permissions for some uid/gid).

Tested against bzr 2.3.4:

$ bzr --version
Bazaar (bzr) 2.3.4
  Python interpreter: /usr/bin/python 2.6.5
  Python standard library: /usr/lib/python2.6
  Platform: Linux-2.6.35.4-rscloud-x86_64-with-Ubuntu-10.04-lucid
  bzrlib: /usr/lib/pymodules/python2.6/bzrlib

$ cat /etc/issue
Ubuntu 10.04.3 LTS \n \l

$ uname -a
Linux <censored> 2.6.35.4-rscloud <censored> SMP Mon Sep 20 15:54:33 UTC 2010 x86_64 GNU/Linux

This bug could be a security vulnerability because it changes the owner and group of the files affected. This could make the files accessible to users who shouldn't have access, a typical privilege escalation problem.

[cheater (cheater00)]

in the above steps, i think you need to add and commit the file before editing it for bzr shelve to catch it, but you probably should know that.

bzr unshelve does the same, by the way:

1. create file (and probably add+commit it)
2. edit it
3. bzr shelve
4. chown or chgrp
5. bzr unshelve

by now the ownership data is overwritten with, again, your own user/group.

I'm not sure if I should make this a separate bug.

[cheater (cheater00)]

Apparently this is "by design". It's not perfect, but one has to live with it. This has to do with bzr trying to do atomic updates of the file system by using renames. However, it is fixable by keeping track of the file permissions and doing chown (possibly asking for sudo if necessary). Can I suggest adding this as an option, and enabling this option by default? I cannot come up with any usecases that would break because of such an addition.

If the suggestion seems sane, please change this to a feature request instead.

log from #bzr:

<cheater__> i have isolated the reason for the bug
 https://bugs.launchpad.net/bzr/+bug/825732
<ubot5> Error: ubuntu bug 825732 not found
<cheater__> ah yes, it's a security bug, it won't be accessible publicly
 i'm not sure if i should make it public
<maxb> My inclination is that this is not a bug
<cheater__> how so? it clobbers ownership of the file
 no other operations do that
<maxb> Simply that your expectations disagree with the design goals of most version control tools
<cheater__> it's not homogenous in its behavior
 other operations do not do this
 just shelve (that i know of)
<maxb> No, try bzr revert for example
<cheater__> hm
 what others do that?
 and which ones don't?
<maxb> pretty much anything that changes the file via the generic treetransform code
<cheater__> is there a reason to overwrite the file instead of updating it?
 if not, maybe i can make a patch (if htat part is in python and not, say, C)
<maxb> Yes, bzr prepares all of the updates it is going to do within .bzr, and then moves files in and out to commit the result of the operation in a nearly atomic fashion
<cheater__> mhm
 i understand that a rename is atomic while a write doesn't have to be
 i wonder if there could be a plugin which takes note of the ownership, and chown's the files (and asks you to sudo if necessary)
<maxb> Not entirely impossible, I suppose, though it does sound like it would have to hook very deeply into the core. I doubt sufficient hooks are available now

[John A Meinel (jameinel)]

This isn't a security vulnerability, because you can only change the file if you already had permissions to do so. If we added a hook that invoked Audi, *that* would be a potential security hole.

[John A Meinel (jameinel)]

We the file replacement code already tries to check the file mode to preserve things like group rwx. We could possibly add user/group checks to that, but often you won't have permission anyway. You might look at "etckeeper" which tracks a permission file and sets more permissions than bzr does normally.

[John A Meinel (jameinel)]

I can't set the title from my mobile, but this happens with shelve, unshelve, update, pull, merge. Pretty much any time that *bzr* alters the contents of a file, rather than the user.