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

[You-Sheng Yang]

Hi Laurent,

On 2020-07-28 07:58, Laurent Pinchart wrote:
> Hi You-Sheng,
> 
> 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.

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

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

You-Sheng Yang

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200728/00a5fc42/attachment-0001.sig>