Quoting Miklos Szeredi (miklos@xxxxxxxxxx): > On Thu, 4 Sep 2008, Serge E. Hallyn wrote: > > Quoting Miklos Szeredi (miklos@xxxxxxxxxx): > > > On Thu, 4 Sep 2008, Serge E. Hallyn wrote: > > > > but you're still doing > > > > > > > > if (IS_MNT_SHARED(old_nd.path.mnt) && !capable(CAP_SYS_ADMIN)) > > > > goto out; > > > > > > > > shouldn't it be something like > > > > > > > > if (IS_MNT_SHARED(old_nd.path.mnt) && (old_nd.path.mnt & MNT_USER)) > > > > goto out; > > > > > > > > ? > > > > > > Why would that be an error? There's no real security gain to be had > > > from restricting a privileged user, but could cause a lot of > > > annoyance. If we think this is dangerous, then protection should be > > > built into mount(8) with an option to override. But not into the > > > kernel, IMO. > > > > We disagree on that. But can we agree that the check you added is wrong? > > No :) > > > There is no reason why a user mount should not be able to do shared > > mounts, is there? > > I don't know. It's something to think about in the future, but not > essential. We know that without the above check the user can do bad > things: propagate mounts back into the source, and we don't want that. When is propagating mounts back into the source bad? Because you are not preventing it. You are preventing future propagation back into the user's own mounts, but not into mounts not owned by the user. It's not right. > We could allow binding a shared mount if > > a) the owners of the source and destination match > b) the destination is made a slave of the source Well that's what I've been saying... > But the current patchset doesn't allow _any_ changes to propagation > without CAP_SYS_ADMIN, so why should bind be an exception? Because it's not a change in propagation among existing mounts, instead it's defining propagation for the new user mounts. And since user mounts don't currently exist, we're in no position to talk about exceptions to existing behavior. > And yes, this is something to think about, but I think it's a rather > uncommon corner case, and so the patchset very much makes sense > without having to deal with unprivileged mount propagation changes. I'm willing to accept that if we simply leave the patchset as it was before, but your new check just adds inconsistencies for absolutely zero security gain. > > So should the check above just go away then? > > No, we'd be back with the original problem. We still have the original problem. When root does mount -bind /mnt /mnt mount --make-rshared /mnt mount --bind -o user=hallyn /mnt /home/hallyn/mnt and hallyn does mount --bind /usr /home/hallyn/mnt/usr then the kernel happily propagates the mount to /mnt/usr. Now if hallyn does mount --bind /home/hallyn/mnt/usr /home/hallyn/mnt/usr2 THAT gives him a -EPERM. To what end? -serge -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
