Comment 29 for bug 1476662

Revision history for this message
Serge Hallyn (serge-hallyn) wrote : Re: [Bug 1476662] Re: lxc-start symlink vulnerabilities may allow guest to read host filesystem, interfere with apparmor

Quoting Tyler Hicks (<email address hidden>):
> Hi Serge - I've reviewed the patch against git HEAD and have a couple
> questions.

Thanks, Tyler.

> 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.

> 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.

> 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.