[lxc-devel] LXC security issue - affects all supported releases

[Serge Hallyn]

Quoting Thomas Moschny (thomas.moschny at gmail.com):
> 2015-10-24 4:37 GMT+02:00 Serge Hallyn <serge.hallyn at ubuntu.com>:
> > Quoting Thomas Moschny (thomas.moschny at gmail.com):
> >> 2015-10-02 15:50 GMT+02:00 Serge Hallyn <serge.hallyn at ubuntu.com>:
> >> > Can you tell me what happens when you do an openat with
> >> > O_PATH?  Does it simply return < 0?  If so then I think this is all ok.
> >>
> >> As far as I can see, it behaves as if O_PATH wasn't given at all - so
> >> it doesn't really make a difference whether one "copies" the value of
> >> O_PATH over from elsewhere, or defines it to 0. Both ways feel hackish
> >> though. The second openat() call in open_if_safe() should fail anyway,
> >> so...
> >>
> >> > (since an openat without O_PATH already failed, you shouldn't be allowed
> >> > to mount on it in this case)
> >>
> >> ... a really clean solution would be to #ifdef that code in
> >> open_if_safe(), so it compiles cleanly.
> >
> > Heh, a really clean solution would be a mountfd system call :)
> >
> > If you can send a patch along the lines of what you'r thinking that
> > would be great.
> 
> From a very pragmatic point of view, this commit:
> 
> https://github.com/lxc/lxc/commit/27ec06f9
> 
> already sort of "fixes" the issue also on RHEL/CentOS6...
> 
> What I had in mind would look more like this:
> 
> diff --git a/src/lxc/utils.c b/src/lxc/utils.c
> index 214c5a8..264b554 100644
> --- a/src/lxc/utils.c
> +++ b/src/lxc/utils.c
> @@ -1226,6 +1226,7 @@ static int open_if_safe(int dirfd, const char *nextpath)
>         if (errno == ELOOP)
>                 return newfd;
> 
> +#ifndef O_PATH

Did you mean ifdef?

>         if (errno == EPERM || errno == EACCES) {
>                 /* we're not root (cause we got EPERM) so
>                    try opening with O_PATH */
> @@ -1242,6 +1243,7 @@ static int open_if_safe(int dirfd, const char *nextpath)
>                         }
>                 }
>         }
> +#endif
> 
>         return newfd;
>  }

I'm fine either way.  If you send a patch I'll let stgraber decide.

Your way has the advantage of being clearer what's going on.  (And should
then add a comment by the ifdef saying "if o_path isn't defined, there are
no user namespaces so we expect to be real root")