Received-SPF: pass (google.com: domain of linux-kernel+bounces-92740-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <9c739aae1677b2b7169025750f83c1126e91ebde.camel@huaweicloud.com>
Subject: Re: [PATCH v2 24/25] commoncap: use vfs fscaps interfaces
From: Roberto Sassu <roberto.sassu@huaweicloud.com>
To: "Seth Forshee (DigitalOcean)" <sforshee@kernel.org>
Cc: Christian Brauner <brauner@kernel.org>, Serge Hallyn <serge@hallyn.com>,
  Paul Moore <paul@paul-moore.com>, Eric Paris <eparis@redhat.com>, James
 Morris <jmorris@namei.org>,  Alexander Viro <viro@zeniv.linux.org.uk>, Jan
 Kara <jack@suse.cz>, Stephen Smalley <stephen.smalley.work@gmail.com>,
 Ondrej Mosnacek <omosnace@redhat.com>,  Casey Schaufler
 <casey@schaufler-ca.com>, Mimi Zohar <zohar@linux.ibm.com>, Roberto Sassu
 <roberto.sassu@huawei.com>,  Dmitry Kasatkin <dmitry.kasatkin@gmail.com>,
 Eric Snowberg <eric.snowberg@oracle.com>, "Matthew Wilcox (Oracle)"
 <willy@infradead.org>, Jonathan Corbet <corbet@lwn.net>, Miklos Szeredi
 <miklos@szeredi.hu>, Amir Goldstein <amir73il@gmail.com>, 
 linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, 
 linux-security-module@vger.kernel.org, audit@vger.kernel.org, 
 selinux@vger.kernel.org, linux-integrity@vger.kernel.org, 
 linux-doc@vger.kernel.org, linux-unionfs@vger.kernel.org
Date: Tue, 05 Mar 2024 18:08:41 +0100
In-Reply-To: <ZedQRThbc60h+VoA@do-x1extreme>
References: <20240221-idmap-fscap-refactor-v2-0-3039364623bd@kernel.org>
	 <20240221-idmap-fscap-refactor-v2-24-3039364623bd@kernel.org>
	 <dcbd9e7869d2fcce69546b53851d694b8ebad54e.camel@huaweicloud.com>
	 <ZeXpbOsdRTbLsYe9@do-x1extreme>
	 <a7124afa6bed2fcadcb66efa08e256828cd6f8ab.camel@huaweicloud.com>
	 <ZeX9MRhU/EGhHkCY@do-x1extreme>
	 <20240305-fachjargon-abmontieren-75b1d6c67a83@brauner>
	 <3098aef3e5f924e5717b4ba4a34817d9f22ec479.camel@huaweicloud.com>
	 <20240305-zyklisch-halluzinationen-98b782666cf8@brauner>
	 <133a912d05fb0790ab3672103a21a4f8bfb70405.camel@huaweicloud.com>
	 <ZedQRThbc60h+VoA@do-x1extreme>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
Precedence: bulk
MIME-Version: 1.0

On Tue, 2024-03-05 at 11:03 -0600, Seth Forshee (DigitalOcean) wrote:
> On Tue, Mar 05, 2024 at 05:35:11PM +0100, Roberto Sassu wrote:
> > On Tue, 2024-03-05 at 17:26 +0100, Christian Brauner wrote:
> > > On Tue, Mar 05, 2024 at 01:46:56PM +0100, Roberto Sassu wrote:
> > > > On Tue, 2024-03-05 at 10:12 +0100, Christian Brauner wrote:
> > > > > On Mon, Mar 04, 2024 at 10:56:17AM -0600, Seth Forshee (DigitalOc=
ean) wrote:
> > > > > > On Mon, Mar 04, 2024 at 05:17:57PM +0100, Roberto Sassu wrote:
> > > > > > > On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean=
) wrote:
> > > > > > > > On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wro=
te:
> > > > > > > > > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalO=
cean) wrote:
> > > > > > > > > > Use the vfs interfaces for fetching file capabilities f=
or killpriv
> > > > > > > > > > checks and from get_vfs_caps_from_disk(). While there, =
update the
> > > > > > > > > > kerneldoc for get_vfs_caps_from_disk() to explain how i=
t is different
> > > > > > > > > > from vfs_get_fscaps_nosec().
> > > > > > > > > >=20
> > > > > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@ke=
rnel.org>
> > > > > > > > > > ---
> > > > > > > > > >  security/commoncap.c | 30 +++++++++++++---------------=
--
> > > > > > > > > >  1 file changed, 13 insertions(+), 17 deletions(-)
> > > > > > > > > >=20
> > > > > > > > > > diff --git a/security/commoncap.c b/security/commoncap.=
c
> > > > > > > > > > index a0ff7e6092e0..751bb26a06a6 100644
> > > > > > > > > > --- a/security/commoncap.c
> > > > > > > > > > +++ b/security/commoncap.c
> > > > > > > > > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new,
> > > > > > > > > >   */
> > > > > > > > > >  int cap_inode_need_killpriv(struct dentry *dentry)
> > > > > > > > > >  {
> > > > > > > > > > -	struct inode *inode =3D d_backing_inode(dentry);
> > > > > > > > > > +	struct vfs_caps caps;
> > > > > > > > > >  	int error;
> > > > > > > > > > =20
> > > > > > > > > > -	error =3D __vfs_getxattr(dentry, inode, XATTR_NAME_CA=
PS, NULL, 0);
> > > > > > > > > > -	return error > 0;
> > > > > > > > > > +	/* Use nop_mnt_idmap for no mapping here as mapping i=
s unimportant */
> > > > > > > > > > +	error =3D vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry=
, &caps);
> > > > > > > > > > +	return error =3D=3D 0;
> > > > > > > > > >  }
> > > > > > > > > > =20
> > > > > > > > > >  /**
> > > > > > > > > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_i=
dmap *idmap, struct dentry *dentry)
> > > > > > > > > >  {
> > > > > > > > > >  	int error;
> > > > > > > > > > =20
> > > > > > > > > > -	error =3D __vfs_removexattr(idmap, dentry, XATTR_NAME=
_CAPS);
> > > > > > > > > > +	error =3D vfs_remove_fscaps_nosec(idmap, dentry);
> > > > > > > > >=20
> > > > > > > > > Uhm, I see that the change is logically correct... but th=
e original
> > > > > > > > > code was not correct, since the EVM post hook is not call=
ed (thus the
> > > > > > > > > HMAC is broken, or an xattr change is allowed on a portab=
le signature
> > > > > > > > > which should be not).
> > > > > > > > >=20
> > > > > > > > > For completeness, the xattr change on a portable signatur=
e should not
> > > > > > > > > happen in the first place, so cap_inode_killpriv() would =
not be called.
> > > > > > > > > However, since EVM allows same value change, we are here.
> > > > > > > >=20
> > > > > > > > I really don't understand EVM that well and am pretty hesit=
ant to try an
> > > > > > > > change any of the logic around it. But I'll hazard a though=
t: should EVM
> > > > > > > > have a inode_need_killpriv hook which returns an error in t=
his
> > > > > > > > situation?
> > > > > > >=20
> > > > > > > Uhm, I think it would not work without modifying
> > > > > > > security_inode_need_killpriv() and the hook definition.
> > > > > > >=20
> > > > > > > Since cap_inode_need_killpriv() returns 1, the loop stops and=
 EVM would
> > > > > > > not be invoked. We would need to continue the loop and let EV=
M know
> > > > > > > what is the current return value. Then EVM can reject the cha=
nge.
> > > > > > >=20
> > > > > > > An alternative way would be to detect that actually we are se=
tting the
> > > > > > > same value for inode metadata, and maybe not returning 1 from
> > > > > > > cap_inode_need_killpriv().
> > > > > > >=20
> > > > > > > I would prefer the second, since EVM allows same value change=
 and we
> > > > > > > would have an exception if there are fscaps.
> > > > > > >=20
> > > > > > > This solves only the case of portable signatures. We would ne=
ed to
> > > > > > > change cap_inode_need_killpriv() anyway to update the HMAC fo=
r mutable
> > > > > > > files.
> > > > > >=20
> > > > > > I see. In any case this sounds like a matter for a separate pat=
ch
> > > > > > series.
> > > > >=20
> > > > > Agreed.
> > > >=20
> > > > Christian, how realistic is that we don't kill priv if we are setti=
ng
> > > > the same owner?
> > >=20
> > > Uhm, I would need to see the wider context of the proposed change. Bu=
t
> > > iiuc then you would be comparing current and new fscaps and if they a=
re
> > > identical you don't kill privs? I think that would work. But again, I
> > > would need to see the actual context/change to say something meaningf=
ul.
> >=20
> > Ok, basically a software vendor can ship binaries with a signature over
> > file metadata, including UID/GID, etc.
> >=20
> > A system can verify the signature through the public key of the
> > software vendor.
> >=20
> > The problem is if someone (or even tar), executes chown on that binary,
> > fscaps are lost. Thus, signature verification will fail from now on.
> >=20
> > EVM locks file metadata as soon as signature verification succeeds
> > (i.e. metadata are the same of those signed by the software vendor).
> >=20
> > EVM locking works if someone is trying to set different metadata. But,
> > if I try to chown to the same owner as the one stored in the inode, EVM
> > allows it but the capability LSM removes security.capability, thus
> > invalidating the signature.
> >=20
> > At least, it would be desirable that security.capability is not removed
> > when setting the same owner. If the owner is different, EVM will handle
> > that.
>=20
> When you say EVM "locks" file metadata, does that mean it prevents
> modification to file metadata?

Yes, but only when metadata are in the final state (what the software
vendor signed). Otherwise, modifications are still allowed.

That was needed to let tar set everything (xattrs and attrs).

> What about changes to file data? This will also result in removing
> fscaps xattrs. Does EVM also block changes to file data when signature
> verification succeeds?

That would be the job of IMA. EVM would not be able to detect that. If
the file has an EVM immutable and portable signature, IMA denies
changes to it (assuming that an IMA appraisal policy was loaded, and
that file matches a policy rule).

Roberto