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

[Laurent Pinchart]

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 ?

> > We can also consider dropping the unshare() call for now, as it's only a
> > partial implementation of process isolation. We would need to implement
> > that feature down the line though. Wrapping the ipa_proxy_worker with
> > minijail or firejail, or implementing isolation in the worker itself,
> > are two possible candidates. Another option would be to run the proxy
> > worker as a system daemon, but at this point we would like to avoid
> > going down that route if possible.
> 
> minijail/firejail seem sufficient to me. It's already a separate
> process. There are many other ways to contain it without touching the
> source code.

I think that's the direction we'll take.

> > Do you have any recommendation ?
> 
> Not really. But as an end user, I would really love to see you drop all
> those incomplete/error-prone EventBlahBlah/Signal/Timer stuff with Boost
> io_service/process/... since you're already using boost.

Boost is only needed for the Raspberry Pi IPA module, for the JSON
parser, and it's something we may consider. libcamera aims at supporting
resource-constrained embedded systems (as much as we can reasonably do),
hence the attempt to limit the number of dependencies. That being said,
this decision could be reconsidered later.

Regarding timers and event notifiers, I think we'll simply drop them
from the public API, and make them internal only, as libcamera's goal
isn't to provide a generic implementation of those concepts.

-- 
Regards,

Laurent Pinchart