[libcamera-devel] [PATCH 2/3] test: log/process: check CAP_SYS_ADMIN in test init

[paul.elder at ideasonboard.com]

Hello,

Sorry for the delay.

On Tue, Jul 28, 2020 at 10:39:34PM +0300, Laurent Pinchart wrote:
> Hi You-Sheng,
> 
> (CC'ing Paul Elder)
> 
> On Tue, Jul 28, 2020 at 11:39:38AM +0800, You-Sheng Yang wrote:
> > On 2020-07-28 07:58, Laurent Pinchart wrote:
> > > On Mon, Jul 27, 2020 at 11:50:39PM +0800, You-Sheng Yang wrote:
> > >> On 2020-07-27 07:47, Laurent Pinchart wrote:
> > >>> On Mon, Jul 27, 2020 at 02:42:11AM +0300, Laurent Pinchart wrote:
> > >>>> On Sat, Jul 25, 2020 at 08:24:41PM +0800, You-Sheng Yang wrote:
> > >>>>> diff --git a/test/meson.build b/test/meson.build
> > >>>>> index f41d6e7..b4db328 100644
> > >>>>> --- a/test/meson.build
> > >>>>> +++ b/test/meson.build
> > >>>>> @@ -1,5 +1,7 @@
> > >>>>>  # SPDX-License-Identifier: CC0-1.0
> > >>>>>  
> > >>>>> +libcap = dependency('libcap', required : true)
> > >>>>
> > >>>> 'true' is the default value for 'required', you can omit it. However,
> > >>>> I'd like to keep the dependency optional, as we try to also support
> > >>>> resource-constrainted embedded systems (based on musl or uclibc for
> > >>>> instance, and/or without udev).
> > >>>>
> > >>>> I have an idea how to do that, I'll try to submit a patch shortly.
> > >>>
> > >>> Actually, thinking about it some more, would it make sense to instead
> > >>> condition the call to unshare() to CAP_SYS_ADMIN in the
> > >>> Process:isolate() class ? Or turn it into a non-fatal error ?
> > >>
> > >> It's about API design, so your opinions matter most.
> > >>
> > >> I didn't have much idea about the rational behind the unshare() call
> > >> inside libcamera::Process, but I'm really suspect the necessity of it as
> > >> part of a, at least looks like, generic API. It implicitly adds a
> > >> constrain that any process tries to create a subprocess in libcamera
> > >> using libcamera::Process, its child process must be either executed by
> > >> root or have CAP_SYS_ADMIN. This doesn't really sound a good idea for
> > >> me, especially when I believe one should really build a multimedia
> > >> library to run as a normal user as possible.
> > > 
> > > The Process class is meant to run closed-source image processing
> > > algorithm (IPA) modules in a separate, isolated process. The unshare()
> > > call is a very first (mockup) step in that direction, and we know more
> > > work is needed to achieve a real sandboxing.
> > > 
> > > Now that I think about it, it may be better to instead rely on minijail
> > > or firejail instead of reinventing the wheel.
> > >
> > >> Anyway, the only user of this API in libcamera is ipa_proxy_linux, you
> > >> could have put unshare() into ipa_proxy_linux itself. This way you could
> > >> install some selinux/apparmor rules to grant such permission to this
> > >> executable explicitly. But again, is that really necessary? Is
> > >> ipa_proxy_linux really has to own its own network and uid namespace?
> > > 
> > > We want to isolate the IPA modules, limiting their access to the system
> > > as much as possible. They should only be able to access specific file
> > > system directories (in order to load configuration data and write logs),
> > > and nothing else (no device access, no network access, ...).
> > > Closed-source IPA modules are considered to be untrusted binaries.
> > 
> > I understand. But while ipa_linux_proxy is currently integrated into
> > libcamera source, you know and can setup constrains for it correctly.
> > When some other vendor adopts libcamera and creates similar plugin by
> > their own, that may become something blocking their normal function.
> > 
> > >>> Could you maybe elaborate a little bit on the failure this patch is
> > >>> trying to solve ? I haven't seen any such failure, how can they be
> > >>> reproduced ?
> > >>
> > >> Please see https://gitlab.com/vicamo/libcamera/-/jobs/650449281
> > > 
> > > That's lots of failures :-S
> > > 
> > > The process test has been part of our test suite for a long time, and
> > > it's not run as root or with CAP_SYS_ADMIN. As far as I can tell, we've
> > > never noticed any issue with unshare() failing. I'm not sure what's
> > > different in your environment.
> > 
> > This is executed in a unprivileged docker container. And since unshare()
> > takes CAP_SYS_ADMIN, if that doesn't fail in your setup, it follows
> > either that test has never been enrolled or its executed with
> > CAP_SYS_ADMIN somehow.
> 
> I've double-checked, and I can run the test successfully, without
> CAP_SYS_ADMIN. I expect the problem to be caused by calling
> unshared(CLONE_NEWUSER) within a docker container, which is something I
> haven't tried.
> 
> Nonetheless, we need to address this issue. Paul, you're the author of
> that code, do you think we could just drop the skeleton isolation for
> now, and bring it back in another form later ?

Yeah, I think we could just drop the skeleton isolation for now. It's
probably better to replace it with minijail (or something similar)
instead of reimplementing our own jail. I think I used unshare() just to
show how isolation would work/fit in.


Paul