linux-kernel - RE: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata

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:   Mon, 3 May 2021 15:30:48 +0000
From:   Roberto Sassu <roberto.sassu@...wei.com>
To:     Mimi Zohar <zohar@...ux.ibm.com>,
        "mjg59@...gle.com" <mjg59@...gle.com>
CC:     "linux-integrity@...r.kernel.org" <linux-integrity@...r.kernel.org>,
        "linux-security-module@...r.kernel.org" 
        <linux-security-module@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Andreas Gruenbacher <agruenba@...hat.com>
Subject: RE: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for
 unmodified metadata

> From: Mimi Zohar [mailto:zohar@...ux.ibm.com]
> Sent: Monday, May 3, 2021 5:13 PM
> On Mon, 2021-05-03 at 14:48 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:zohar@...ux.ibm.com]
> > > Sent: Monday, May 3, 2021 3:00 PM
> > > On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> > >
> > > > diff --git a/security/integrity/evm/evm_main.c
> > > b/security/integrity/evm/evm_main.c
> > > > @@ -389,6 +473,11 @@ static int evm_protect_xattr(struct
> > > user_namespace *mnt_userns,
> > > >  	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
> > > >  		return 0;
> > > >
> > > > +	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> > > > +	    !evm_xattr_change(mnt_userns, dentry, xattr_name, xattr_value,
> > > > +			      xattr_value_len))
> > > > +		return 0;
> > > > +
> > >
> > > If the purpose of evm_protect_xattr() is to prevent allowing an invalid
> > > security.evm xattr from being re-calculated and updated, making it
> > > valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional.
> Any
> > > time there is an attr or xattr change, including setting it to the
> > > existing value, the status flag should be reset.
> >
> > The status is always reset if evm_protect_xattr() returns 0. This does not
> > change.
> >
> > Not making INTEGRITY_PASS_IMMUTABLE conditional would cause issues.
> > Suppose that the status is INTEGRITY_FAIL. Writing the same xattr would
> > cause evm_protect_xattr() to return 0 and the HMAC to be updated.
> 
> This example is mixing security.evm types.  Please clarify.

What I meant is that returning 0 when the xattr does not change should
be done only in the positive cases: for INTEGRITY_PASS it is not needed,
for INTEGRITY_PASS_IMMUTABLE it is needed as otherwise
evm_protect_xattr() would return -EPERM.

If your proposal was to return 0 only when the xattr does not change,
without checking the current status, we risk that someone does an
offline attack to corrupt xattrs and when the system is online, he simply
rewrites the same corrupted xattrs to obtain a valid HMAC.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> > > I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
> > > prevent the file from being resigned.
> >
> > INTEGRITY_FAIL_IMMUTABLE should be enough to continue the
> > operation.
> 
> Agreed.
> 
> Mimi