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

[Laurent Pinchart]

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:
> >>> While these tests may be executed as normal user at build time,
> >>> unshare() call will fail and so are tests log_process and process_test.
> >>> This change checks if one is granted with necessary capabilities so that
> >>> we don't fail the build unexpectedly.
> >>>
> >>> Signed-off-by: You-Sheng Yang <vicamo.yang at canonical.com>
> >>> ---
> >>>  test/log/log_process.cpp      | 20 ++++++++++++++++++++
> >>>  test/log/meson.build          |  2 +-
> >>>  test/meson.build              |  2 ++
> >>>  test/process/meson.build      |  2 +-
> >>>  test/process/process_test.cpp | 23 +++++++++++++++++++++++
> >>>  5 files changed, 47 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp
> >>> index d46d5e3..876da22 100644
> >>> --- a/test/log/log_process.cpp
> >>> +++ b/test/log/log_process.cpp
> >>> @@ -9,6 +9,7 @@
> >>>  #include <iostream>
> >>>  #include <random>
> >>>  #include <string.h>
> >>> +#include <sys/capability.h>
> >>>  #include <sys/stat.h>
> >>>  #include <sys/types.h>
> >>>  #include <unistd.h>
> >>> @@ -55,6 +56,25 @@ class LogProcessTest : public Test
> >>>  protected:
> >>>  	int init()
> >>>  	{
> >>> +		int ret = TestPass;
> >>> +
> >>> +		cap_t caps = cap_get_proc();
> >>> +		if (caps == NULL) {
> >>> +			cerr << "failed to check process capabilities" << endl;
> >>> +			return TestFail;
> >>> +		}
> >>> +
> >>> +		/* Check required permissions: CAP_SYS_ADMIN: unshare */
> >>> +		cap_flag_value_t fv;
> >>> +		if ((cap_get_flag(caps, CAP_SYS_ADMIN, CAP_EFFECTIVE, &fv) < 0) || (fv != CAP_SET)) {
> >>> +			cerr << "skip due to insufficient capability" << endl;
> >>> +			ret = TestSkip;
> >>> +		}
> >>
> >> Would it make sense to add this as a helper function to the base Test
> >> class ?
> 
> Will do. But probably after having a conclusion below.
> 
> >>> +
> >>> +		cap_free(caps);
> >>> +		if (ret != TestPass)
> >>> +			return ret;
> >>> +
> >>>  		random_device random;
> >>>  		num_ = random();
> >>>  		logPath_ = "/tmp/libcamera.worker.test." +
> >>> diff --git a/test/log/meson.build b/test/log/meson.build
> >>> index 8cd664e..000f980 100644
> >>> --- a/test/log/meson.build
> >>> +++ b/test/log/meson.build
> >>> @@ -7,7 +7,7 @@ log_test = [
> >>>  
> >>>  foreach t : log_test
> >>>      exe = executable(t[0], t[1],
> >>> -                     dependencies : libcamera_dep,
> >>> +                     dependencies : [libcamera_dep, libcap],
> >>>                       link_with : test_libraries,
> >>>                       include_directories : test_includes_internal)
> >>>  
> >>> 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.

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

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.

Do you have any recommendation ?

> 21/55 libcamera:process / process_test                 FAIL
> 0.01s (exit status 255 or signal 127 SIGinvalid)
> --- command ---
> 10:26:15
> /builds/vicamo/libcamera/debian/output/libcamera-0~git20200722+d929555/obj-x86_64-linux-gnu/test/process/process_test
> --- stderr ---
> 
> I was trying to fix debian packaging and to have a daily build based on
> master tip.

-- 
Regards,

Laurent Pinchart