linux-kernel - Re: [PATCH v10 9/9] ima: add FIRMWARE_PARTIAL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 7 Jul 2020 10:13:42 -0700
From:   Scott Branden <scott.branden@...adcom.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Luis Chamberlain <mcgrof@...nel.org>,
        Wolfram Sang <wsa@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        David Brown <david.brown@...aro.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Shuah Khan <shuah@...nel.org>, bjorn.andersson@...aro.org,
        Shuah Khan <skhan@...uxfoundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        Mimi Zohar <zohar@...ux.ibm.com>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-fsdevel@...r.kernel.org,
        BCM Kernel Feedback <bcm-kernel-feedback-list@...adcom.com>,
        Olof Johansson <olof@...om.net>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Colin Ian King <colin.king@...onical.com>,
        Takashi Iwai <tiwai@...e.de>, linux-kselftest@...r.kernel.org,
        Andy Gross <agross@...nel.org>,
        linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org
Subject: Re: [PATCH v10 9/9] ima: add FIRMWARE_PARTIAL_READ support

Hi Kees,

You and others are certainly more experts in the filesystem and security
infrastructure of the kernel.
What I am trying to accomplish is a simple operation:
request part of a file into a buffer rather than the whole file.
If someone could add such support I would be more than happy to use it.

This has now bubbled into many other designs issues in the existing 
codebase.
I will need more details on your comments - see below.


On 2020-07-06 8:08 p.m., Kees Cook wrote:
> On Mon, Jul 06, 2020 at 04:23:09PM -0700, Scott Branden wrote:
>> Add FIRMWARE_PARTIAL_READ support for integrity
>> measurement on partial reads of firmware files.
> Hi,
>
> Several versions ago I'd suggested that the LSM infrastructure handle
> the "full read" semantics so that individual LSMs don't need to each
> duplicate the same efforts. As it happens, only IMA is impacted (SELinux
> ignores everything except modules, and LoadPin only cares about origin
> not contents).
Does your patch series "Fix misused kernel_read_file() enums" handle this
because this suggestion is outside the scope of my change?
>
> Next is the problem that enum kernel_read_file_id is an object
> TYPE enum, not a HOW enum. (And it seems I missed the addition of
> READING_FIRMWARE_PREALLOC_BUFFER, which may share a similar problem.)
> That it's a partial read doesn't change _what_ you're reading: that's an
> internal API detail. What happens when I attempt to do a partial read of
> a kexec image?
It does not appear there is any user of partial reads of kexec images?
I have been informed by Greg K-H to not add apis that are not used so 
such support
doesn't make sense to add at this time.
>   I'll use kernel_pread_file() and pass READING_KEXEC_IMAGE,
> but the LSMs will have no idea it's a partial read.
The addition I am adding is for request_partial_firmware_into_buf.
In order to do so it adds internal support for partial reads of firmware 
files,
not kexec image.

The above seems outside the scope of my patch?
>
> Finally, what keeps the contents of the file from changing between the
> first call (which IMA will read the entire file for) and the next reads
> which will bypass IMA?
The request is for a partial read.  IMA ensures the whole file integrity 
even though I only do a partial read.
The next partial read will re-read and check integrity of file.
>   I'd suggested that the open file must have writes
> disabled on it (as execve() does).
The file will be reopened and integrity checked on the next partial read 
(if there is one).
So I don't think there is any change to be made here.
If writes aren't already disabled for a whole file read then that is 
something that needs to be fixed in the existing code.
>
> So, please redesign this:
> - do not add an enum
I used existing infrastructure provided by Mimi but now looks like it 
will have to fit with your patches from yesterday.
> - make the file unwritable for the life of having the handle open
It's no different than a full file read so no change to be made here.
> - make the "full read" happen as part of the first partial read so the
>    LSMs don't have to reimplement everything
Each partial read is an individual operation so I think a "full read" is 
performed every time
if your security IMA is enabled.  If someone wants to add a file lock 
and then partial reads in the kernel
then that would be different than what is needed by the kernel driver.
>
> -Kees
>
Regards,
Scott