Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
MIME-Version: 1.0
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>
In-Reply-To: <019f134a-6ab4-48ca-991c-5a5c94e042ea@huaweicloud.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Tue, 12 Dec 2023 12:44:50 +0200
Message-ID: <CAOQ4uxgpNt7qKEF_NEJPsKU7-XhM7N_3eP68FrOpMpcRcHt4rQ@mail.gmail.com>
Subject: Re: [RFC][PATCH] overlayfs: Redirect xattr ops on security.evm to security.evm_overlayfs
To:     Roberto Sassu <roberto.sassu@huaweicloud.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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Dec 12, 2023 at 12:25=E2=80=AFPM 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, th=
e
> > 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-underlyin=
g-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 f=
ile
>       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 exten=
ded
>       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?

Thanks,
Amir.