Message-ID: <3dc25195b0362b3e5b6d6964df021ff4e7e1b226.camel@huaweicloud.com>
Date: Wed, 27 Nov 2024 10:51:11 +0100
From: Roberto Sassu <roberto.sassu@...weicloud.com>
To: Luis Chamberlain <mcgrof@...nel.org>
Cc: zohar@...ux.ibm.com, dmitry.kasatkin@...il.com,
 eric.snowberg@...cle.com,  corbet@....net, petr.pavlu@...e.com,
 samitolvanen@...gle.com, da.gomez@...sung.com,  akpm@...ux-foundation.org,
 paul@...l-moore.com, jmorris@...ei.org,  serge@...lyn.com,
 shuah@...nel.org, mcoquelin.stm32@...il.com,  alexandre.torgue@...s.st.com,
 linux-integrity@...r.kernel.org,  linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org,  linux-api@...r.kernel.org,
 linux-modules@...r.kernel.org,  linux-security-module@...r.kernel.org,
 linux-kselftest@...r.kernel.org,  wufan@...ux.microsoft.com,
 pbrobinson@...il.com, zbyszek@...waw.pl, hch@....de,  mjg59@...f.ucam.org,
 pmatilai@...hat.com, jannh@...gle.com, dhowells@...hat.com, 
 jikos@...nel.org, mkoutny@...e.com, ppavlu@...e.com, petr.vorel@...il.com, 
 mzerqung@...inter.de, kgold@...ux.ibm.com, Roberto Sassu
 <roberto.sassu@...wei.com>
Subject: Re: [PATCH v6 07/15] digest_cache: Allow registration of digest
 list parsers

On Tue, 2024-11-26 at 11:04 -0800, Luis Chamberlain wrote:
> On Tue, Nov 26, 2024 at 11:25:07AM +0100, Roberto Sassu wrote:
> > On Mon, 2024-11-25 at 15:53 -0800, Luis Chamberlain wrote:
> > 
> > Firmware, eBPF programs and so on are supposed
> 
> Keyword: "supposed". 

It depends if they are in a policy. They can also be verified with
other methods, such as file signatures.

For eBPF programs we are also in a need for a better way to
measure/appraise them.

> > As far as the LSM infrastructure is concerned, I'm not adding new LSM
> > hooks, nor extending/modifying the existing ones. The operations the
> > Integrity Digest Cache is doing match the usage expectation by LSM (net
> > denying access, as discussed with Paul Moore).
> 
> If modules are the only proven exception to your security model you are
> not making the case for it clearly.

The Integrity Digest Cache is not implementing any security model, this
is demanded to other LSMs which might decide to use the Integrity
Digest Cache based on a policy.

If we want to be super picky, the ksys_finit_module() helper is not
calling security_kernel_module_request(), which is done when using
request_module(). On the other hand, ksys_finit_module() is not
triggering user space, as the description of the function states
(anyway, apologies for not bringing up this earlier).

Net this, and we can discuss if it is more appropriate to call the LSM
hook, the helper does not introduce any exception since
security_file_open() is called when the kernel opens the file
descriptor, and security_kernel_read_file() and
security_kernel_post_read_file() are called in the same way regardless
if it is user space doing insmod or the kernel calling
ksys_finit_module().

The only exception is that the Integrity Digest Cache is unable to
verify the kernel modules containing the parsers, but I believe this is
fine because they are verified with their appended signature.

If there are any other concerns I'm missing, please let me know.

> > The Integrity Digest Cache is supposed to be used as a supporting tool
> > for other LSMs to do regular access control based on file data and
> > metadata integrity. In doing that, it still needs the LSM
> > infrastructure to notify about filesystem changes, and to store
> > additional information in the inode and file descriptor security blobs.
> > 
> > The kernel_post_read_file LSM hook should be implemented by another LSM
> > to verify the integrity of a digest list, when the Integrity Digest
> > Cache calls kernel_read_file() to read that digest list.
> 
> If LSM folks *do* agree that this work is *suplementing* LSMS then sure,
> it was not clear from the commit logs. But then you need to ensure the
> parsers are special snowflakes which won't ever incur other additional
> kernel_read_file() calls.

The Integrity Digest Cache was originally called digest_cache LSM, but
was renamed due to Paul's concern that it is not a proper LSM enforcing
a security model. If you are interested, I gave a talk at LSS NA 2024:

https://www.youtube.com/watch?v=aNwlKYSksg8

Given that the Integrity Digest Cache could not be standalone and use
the LSM infrastructure facilities, it is going to be directly
integrated in IMA, although it is not strictly necessary.

I planned to support IPE and BPF LSM as other users.

Uhm, let me clarify your last sentence a bit.

Let's assume that IMA is asked to verify a parser, when invoked through
the kernel_post_read_file hook. IMA is not handling the exception, and
is calling digest_cache_get() as usual. Normally, this would succeed,
but because digest_cache_get() realizes that the file descriptor passed
as argument is marked (i.e. it was opened by the Integrity Digest Cache
itself), it returns NULL.

That means that IMA falls back on another verification method, which is
verifying the appended signature.

The most important design principle that I introduced is that users of
the Integrity Digest Cache don't need to be aware of any exception,
everything is handled by the Integrity Digest Cache itself.

The same occurs when a kernel read occurs with file ID
READING_DIGEST_LIST (introduced in this patch set). Yes, I forbid
specifying an IMA policy which requires the Integrity Digest Cache to
verify digest lists, but due to the need of handling kernel modules
I decided to handle the exceptions in the Integrity Digest Cache itself
(this is why now I'm passing a file descriptor to digest_cache_get()
instead of a dentry).

Now, I'm trying to follow you on the additional kernel_read_file()
calls. I agree with you, if a parser tries to open again the file that
is being verified it would cause a deadlock in IMA (since the inode
mutex is already locked for verifying the original file).

In the Integrity Digest Cache itself, this is not going to happen,
since the file being verified with a digest cache is known and an
internal open of the same file fails. If it is really necessary, we can
pass the information to the parsers so that they are aware, it is just
an additional parameter.

However, I was assuming that a parser just receives the data read by
the Integrity Digest Cache, and just calls the Parser API to add the
extracted digests to the new digest cache. Also this can be discussed,
but I guess there is no immediate need.

> > Supporting kernel modules opened the road for new deadlocks, since one
> > can ask a digest list to verify a kernel module, but that digest list
> > requires the same kernel module. That is why the in-kernel mechanism is
> > 100% reliable,
> 
> Are users of this infrastructure really in need of modules for these
> parsers?

I planned to postpone this to later, and introduced two parsers built-
in (TLV and RPM). However, due to Linus's concern regarding the RPM
parser, I moved it out in a kernel module.

Also, a parser cannot be in user space, since the trust anchor is in
the kernel (the public keys and the signature verification mechanism),
it is not something that can be established in the initial ram disk
since the Integrity Digest Cache will be continously used in the
running system (maybe more parsers will be loaded on demand depending
on requests from user space).

And finally, the parser cannot run in user space, since it would be at
the same level of what the kernel is verifying.

Thanks

Roberto

Powered by blists - more mailing lists