Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <59bf3530-2a6e-4caa-ac42-4d0dab9a71d1@huaweicloud.com>
Date:   Tue, 12 Dec 2023 14:13:02 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC][PATCH] overlayfs: Redirect xattr ops on security.evm to
 security.evm_overlayfs
Content-Language: en-US
To:     Amir Goldstein <amir73il@gmail.com>
Cc:     Christian Brauner <brauner@kernel.org>,
        Seth Forshee <sforshee@kernel.org>, miklos@szeredi.hu,
        linux-unionfs@vger.kernel.org, linux-kernel@vger.kernel.org,
        zohar@linux.ibm.com, paul@paul-moore.com, stefanb@linux.ibm.com,
        jlayton@kernel.org, linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-fsdevel@vger.kernel.org,
        Roberto Sassu <roberto.sassu@huawei.com>,
        Eric Snowberg <eric.snowberg@oracle.com>
References: <20231208172308.2876481-1-roberto.sassu@huaweicloud.com>
 <CAOQ4uxivpZ+u0A5kE962XST37-ey2Tv9EtddnZQhk3ohRkcQTw@mail.gmail.com>
 <20231208-tauziehen-zerfetzt-026e7ee800a0@brauner>
 <c95b24f27021052209ec6911d2b7e7b20e410f43.camel@huaweicloud.com>
 <20231211-fortziehen-basen-b8c0639044b8@brauner>
 <019f134a-6ab4-48ca-991c-5a5c94e042ea@huaweicloud.com>
 <CAOQ4uxgpNt7qKEF_NEJPsKU7-XhM7N_3eP68FrOpMpcRcHt4rQ@mail.gmail.com>
From:   Roberto Sassu <roberto.sassu@huaweicloud.com>
In-Reply-To: <CAOQ4uxgpNt7qKEF_NEJPsKU7-XhM7N_3eP68FrOpMpcRcHt4rQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 12.12.23 11:44, Amir Goldstein wrote:
> On Tue, Dec 12, 2023 at 12:25 PM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
>>
>> On 11.12.23 19:01, Christian Brauner wrote:
>>>> The second problem is that one security.evm is not enough. We need two,
>>>> to store the two different HMACs. And we need both at the same time,
>>>> since when overlayfs is mounted the lower/upper directories can be
>>>> still accessible.
>>>
>>> "Changes to the underlying filesystems while part of a mounted overlay
>>> filesystem are not allowed. If the underlying filesystem is changed, the
>>> behavior of the overlay is undefined, though it will not result in a
>>> crash or deadlock."
>>>
>>> https://docs.kernel.org/filesystems/overlayfs.html#changes-to-underlying-filesystems
>>>
>>> So I don't know why this would be a problem.
>>
>> + Eric Snowberg
>>
>> Ok, that would reduce the surface of attack. However, when looking at:
>>
>>        ovl: Always reevaluate the file signature for IMA
>>
>>        Commit db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the
>> i_version")
>>        partially closed an IMA integrity issue when directly modifying a file
>>        on the lower filesystem.  If the overlay file is first opened by a
>> user
>>        and later the lower backing file is modified by root, but the extended
>>        attribute is NOT updated, the signature validation succeeds with
>> the old
>>        original signature.
>>
>> Ok, so if the behavior of overlayfs is undefined if the lower backing
>> file is modified by root, do we need to reevaluate? Or instead would be
>> better to forbid the write from IMA (legitimate, I think, since the
>> behavior is documented)? I just saw that we have d_real_inode(), we can
>> use it to determine if the write should be denied.
>>
> 
> There may be several possible legitimate actions in this case, but the
> overall concept IMO should be the same as I said about EVM -
> overlayfs does not need an IMA signature of its own, because it
> can use the IMA signature of the underlying file.
> 
> Whether overlayfs reads a file from lower fs or upper fs, it does not
> matter, the only thing that matters is that the underlying file content
> is attested when needed.
> 
> The only incident that requires special attention is copy-up.
> This is what the security hooks security_inode_copy_up() and
> security_inode_copy_up_xattr() are for.
> 
> When a file starts in state "lower" and has security.ima,evm xattrs
> then before a user changes the file, it is copied up to upper fs
> and suppose that security.ima,evm xattrs are copied as is?
> 
> When later the overlayfs file content is read from the upper copy
> the security.ima signature should be enough to attest that file content
> was not tampered with between going from "lower" to "upper".
> 
> security.evm may need to be fixed on copy up, but that should be
> easy to do with the security_inode_copy_up_xattr() hook. No?

It is not yet clear to me. EVM will be seeing the creation of a new 
file, and for new files setting xattrs is already allowed.

Maybe the security_inode_copy_up*() would be useful for IMA/EVM to 
authorize writes by overlayfs, which would be otherwise denied to the 
others (according to my solution).

Still, would like to hear Mimi's opinion.

Thanks

Roberto