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
