Subject: bug#72828: closed (Re: bug#72828: Grafting breaks libcamera signatures)

From: [email-masked] (GNU bug Tracking System)
Date: Thu, 05 Sep 2024 07:06:02 +0000

Your bug report

#72828: Grafting breaks libcamera signatures

which was filed against the guix package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
72828: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=72828
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#72828: Grafting breaks libcamera signatures
From: Andrew Tropin <[email-masked]>
To: Ludovic Courtès <[email-masked]>, Jacopo Mondi <[email-masked]>
Cc: [email-masked]
Date: Thu, 05 Sep 2024 10:46:03 +0400
List-Id: <debbugs-submit.debbugs.gnu.org>

On 2024-09-04 21:42, Andrew Tropin wrote:

> On 2024-09-04 18:42, Ludovic Courtès wrote:
>
>> Hi Jacopo,
>>
>> Jacopo Mondi <[email-masked]> skribis:
>>
>>> Not exactly. In libcamera, apart from creating a library to ease all
>>> the camera stack plumbing, we're creating an ecosystem of open-source
>>> 3A algorithms (what we call the IPA modules).
>>>
>>> Camera vendors and ODMs which invested in products with specific
>>> camera features, consider 3A algorithms and their tuning their secret
>>> sauce and are usually not willing to consider releasing them as open
>>> source with, fortunately, notable exceptions such as RaspberryPi
>>>
>>> Please note that all the platforms libcamera supports have an
>>> open-source 3A algorithm module available part of the main code base,
>>> and we consider open source 3A modules our 'first class citizens' and
>>> we're willing to develop and maintain them in libcamera mainline
>>> branch as free software, but at this point we have to provide a way for
>>> third-parties to load binary modules if they want to.
>>>
>>> The alternative is to have them continue developing camera stacks
>>> fully behind closed doors as it has been done so far.
>>
>> OK, I see, thanks for explaining the context.
>>
>>> As said, modules not built against the libcamera sources will not be
>>> signed, as they are distributed by other means by a vendor in binary
>>> form. To establish if a module has been built with the libcamera
>>> sources or not, we sign it during the build with a volatile key and
>>> validate the signature at run-time, when the IPA module is loaded.
>>>
>>> IPA modules for which the signature is not valid (either because they
>>> are distributed as binaries or, as in this case, because the build
>>> system strips symbols before installing the objects) are loaded in an
>>> isolated process and instead of being operated with direct function
>>> calls, we have implemented an IPC mechanism to communicate with them.
>>> This path is way less tested by our regular users and in our daily
>>> work on libcamera. Vendors that are running their binaries as isolated
>>> might have fixed issues here and there but maybe they have not
>>> reported the issue and the associated fix upstream (we have no control
>>> over this).
>>>
>>> For this reason I don't suggest running modules as isolated, even more
>>> if you have no reasons to do so. If all it takes is re-signing IPA modules
>>> after stripping them as Andrew did I would really consider doing that.
>>
>> Yeah, got it.  The other option, with the understanding that IPA modules
>> are all going to be free software here, would be to dismiss both the
>> authentication and the isolation mechanism, possibly with a custom
>> patch.  It seems like the change wouldn’t be too intrusive and it would
>> solve the problem for “grafts” as well (grafts modify files in a
>> non-functional way).
>
> On 2024-09-02 10:45, Andrew Tropin via Bug reports for GNU Guix wrote:
>> Anyway, I think the current most reasonable solution is to remove
>> signing step at all, because the signaturs will be invalidated by
>> grafting anyway and make it work somehow (either by loading in
>> isolation if it's possible or by loading unsigned libraries without
>> signature check directly).
>
> Everything indicates that we need to disable module authentication.
>
> Jacopo, I think I'll patch IPAManager::isSignatureValid to always return
> true.
>
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/ipa_manager.cpp#n285
>
> Like that:
>
> From c99706475cde3d963a17f4f8871149711ce6c467 Mon Sep 17 00:00:00 2001
> From: Andrew Tropin <[email-masked]>
> Date: Wed, 4 Sep 2024 21:36:16 +0400
> Subject: [PATCH] libcamera: ipa_manager: Disable signature verification
>
> ---
>  src/libcamera/ipa_manager.cpp | 28 +++++-----------------------
>  1 file changed, 5 insertions(+), 23 deletions(-)
>
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index cfc24d38..4fd3cf3e 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -284,33 +284,15 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
>  
>  bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa) const
>  {
> -#if HAVE_IPA_PUBKEY
> -	char *force = utils::secure_getenv("LIBCAMERA_IPA_FORCE_ISOLATION");
> -	if (force && force[0] != '\0') {
> -		LOG(IPAManager, Debug)
> -			<< "Isolation of IPA module " << ipa->path()
> -			<< " forced through environment variable";
> -		return false;
> -	}
> -
> -	File file{ ipa->path() };
> -	if (!file.open(File::OpenModeFlag::ReadOnly))
> -		return false;
> -
> -	Span<uint8_t> data = file.map();
> -	if (data.empty())
> -		return false;
> -
> -	bool valid = pubKey_.verify(data, ipa->signature());
> +	LOG(IPAManager, Debug)
> +		<< "Signature verification is disabled by Guix. "
> +		<< "See https://issues.guix.gnu.org/72828 for more details.";
>  
>  	LOG(IPAManager, Debug)
>  		<< "IPA module " << ipa->path() << " signature is "
> -		<< (valid ? "valid" : "not valid");
> +		<< "not verified (verification skipped).";
>  
> -	return valid;
> -#else
> -	return false;
> -#endif
> +	return true;
>  }
>  
>  } /* namespace libcamera */
> -- 
> 2.45.2
>
>
> Everyone is ok with it?

I've disabled signature verification, tested with cam -l and qcam, it
works good so far, will check browsers and pipewire after ci finish
building them.

https://git.savannah.gnu.org/cgit/guix.git/commit/?id=b0e224566f

Thank you everyone for participating, it helped a lot in narrowing down
and fixing the problem!  

Marking issue as DONE.

I'll update corresponding ticket on libcamera bugtracker after I test
the rest of applications.

-- 
Best regards,
Andrew Tropin

Subject: Grafting breaks libraries signatures
From: Andrew Tropin <[email-masked]>
To: [email-masked]
Date: Tue, 27 Aug 2024 14:46:40 +0400
List-Id: <debbugs-submit.debbugs.gnu.org>

TLDR: libcamera checks the signatures of its own libraries, when loading
them, grafts are enabled by default in Guix, grafted libraries fails to
pass the check.

The full story:

For the last a few days I was updating and fixing libcamera package.

The last problem I faced with it is invalid signatures:

[0:44:16.200646504] [17247] DEBUG IPAManager ipa_manager.cpp:316 IPA module /gnu/store/pfh7adzzy8akkqsjj4wlnmvmbzmrfbvk-libcamera-0.3.1/lib/libcamera/ipa_soft_simple.so signature is not valid

The full log:
https://paste.sr.ht/~abcdw/09e6d4aa66c8fb70a1e8e7e91bfef4cae5aa4e24#cam2.log

I thought it was because of grafts and tried to build it with
--no-grafts, it didn't help.  After that I realized that the signature
created during install phase and before strip phases.  I added a phase
after 'shrink-runpath and it helped:

https://git.savannah.gnu.org/cgit/guix.git/commit/?id=4a19fe41c3

qcam now shows the image, after that I removed --no-grafts flag and
built the package again and the problem came back (now because of
grafts).

The issue with related discussion in libcamera backtracker:
https://bugs.libcamera.org/show_bug.cgi?id=231

How we can solve or workaround this problem?

-- 
Best regards,
Andrew Tropin