Comment 30 for bug 1476662

On 2015-09-16 21:15:13, Serge Hallyn wrote:
> Quoting Tyler Hicks (<email address hidden>):
> > 1) In mount_entry(), the call to mount(2) is changed to use target as
> > both the source and the destination. I can't see why this would be a
> > valid change.
>
> It appears to me to be more correct. Since we are doing a remount,
> the old source isn't really valid any more. I'm thinking of it in
> terms of
>
> mount --bind /ab /cd
> mount -o bind.remount,ro /cd
>
> But I'll test with that part undone, and change it back, as I may just
> not be thinking right.

Aha! It didn't click with me that you are doing a remount there. If we
look at the mount(2) man page, under the MS_REMOUNT section:

"target should be the same value specified in the initial mount() call;
source and filesystemtype are ignored."

I guess you could even pass in NULL for source and filesystemtype.

> > 2) In is_file_mount(), the usage of lstat(2) seems odd for a couple reasons:
> > - If path is a symlink to a directory, the mount will be incorrectly treated as a file mount in safe_mount(). I think stat(2) should be used.
>
> Hm, yeah. I'll change that.
>
> > - Error checking of lstat(2) is ignored. I think this should be
> treated as fatal for most (all?) cases.
>
> My tree has
>
> ret = lstat(path, &sb);
> if (ret)
> return false;
>
> Oh, you mean that if the lstat (or stat) fails, the whole safe_mount()
> should be aborted? That's probably a good thing to fix.

Yes, that's what I meant. At the moment, a return value of false could
mean that it isn't a file mount or that lstat(2) failed. Those feel like
two different conditions to me.

>
> > 3) In open_without_symlink(), why is curlen decremented after assigning
> > it the return value of strlen(prefix_skip)? Is it because prefix_skip is
> > somehow guaranteed to have a trailing '/' character?
>
> Hm, I don't think that's guaranteed.
>
> > 4) open_without_symlink() is documented to return -1 upon error but it
> > returns -EINVAL when target doesn't start with prefix_skip. (minor)
> >
> > 5) Is there a need to protect against ".." components in
> > open_without_symlink()? I'm thinking of a target of "/foo/../" and a
> > prefix_skip of "/foo".
>
> The container admin cannot change the path being passed to
> open_without_symlink() without having access to the container config
> file, in which case he owns the host (or parent container) end of
> story. But I think it's probably worth simply refusing any paths
> with '..' in them, yes.

I understand that the right to modify the container config essentially
equals host root privs so I'm fine either way and will leave that
decision up to you.