Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: Roberto Sassu <roberto.sassu@huawei.com>
To: Mimi Zohar <zohar@linux.ibm.com>, "mjg59@google.com" <mjg59@google.com>
Cc: "linux-integrity@vger.kernel.org"
	<linux-integrity@vger.kernel.org>,
	"linux-security-module@vger.kernel.org" 
	<linux-security-module@vger.kernel.org>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Christian Brauner <christian.brauner@ubuntu.com>,
	Andreas Gruenbacher <agruenba@redhat.com>
Subject: RE: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
Date: Mon, 3 May 2021 15:30:48 +0000
Message-ID: <d599ddac80a94e4bbcd7973d1fa32235@huawei.com> (raw)
In-Reply-To: <33ddeb6108699f47ba47d5f002403ffeca5f9531.camel@linux.ibm.com>

> From: Mimi Zohar [mailto:zohar@linux.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@linux.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


  reply index

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-07 10:52 [PATCH v5 00/12] evm: Improve usability of portable signatures Roberto Sassu
2021-04-07 10:52 ` [PATCH v5 01/12] evm: Execute evm_inode_init_security() only when an HMAC key is loaded Roberto Sassu
2021-04-07 10:52 ` [PATCH v5 02/12] evm: Load EVM key in ima_load_x509() to avoid appraisal Roberto Sassu
2021-04-07 10:52 ` [PATCH v5 03/12] evm: Refuse EVM_ALLOW_METADATA_WRITES only if an HMAC key is loaded Roberto Sassu
2021-04-30 20:28   ` Mimi Zohar
2021-04-07 10:52 ` [PATCH v5 04/12] ima: Move ima_reset_appraise_flags() call to post hooks Roberto Sassu
2021-04-07 16:17   ` Casey Schaufler
2021-04-07 16:31     ` Roberto Sassu
2021-04-26 19:49     ` Mimi Zohar
2021-04-07 10:52 ` [PATCH v5 05/12] evm: Introduce evm_status_revalidate() Roberto Sassu
2021-04-07 10:52 ` [PATCH v5 06/12] evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are safe Roberto Sassu
2021-05-03  0:12   ` Mimi Zohar
2021-05-03  7:55     ` Roberto Sassu
2021-05-03 12:07       ` Mimi Zohar
2021-05-03 14:15       ` Roberto Sassu
2021-05-03 14:34         ` Mimi Zohar
2021-05-04 13:16           ` Roberto Sassu
2021-05-04 13:45             ` Mimi Zohar
2021-04-07 10:52 ` [PATCH v5 07/12] evm: Allow xattr/attr operations for portable signatures Roberto Sassu
2021-05-03  0:12   ` Mimi Zohar
2021-05-04 14:28     ` Roberto Sassu
2021-05-04 14:49       ` Mimi Zohar
2021-04-07 10:52 ` [PATCH v5 08/12] evm: Pass user namespace to set/remove xattr hooks Roberto Sassu
2021-04-07 12:06   ` Christian Brauner
2021-04-07 10:52 ` [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata Roberto Sassu
2021-04-07 12:05   ` Christian Brauner
2021-04-07 15:23   ` kernel test robot
2021-04-07 18:14   ` kernel test robot
2021-04-07 19:28     ` [RESEND][PATCH " Roberto Sassu
2021-05-03 13:00   ` [PATCH " Mimi Zohar
2021-05-03 14:48     ` Roberto Sassu
2021-05-03 15:13       ` Mimi Zohar
2021-05-03 15:30         ` Roberto Sassu [this message]
2021-05-03 15:11     ` Roberto Sassu
2021-05-03 15:26       ` Mimi Zohar
2021-05-03 15:32         ` Roberto Sassu
2021-05-03 15:48           ` Mimi Zohar
2021-04-07 10:52 ` [PATCH v5 10/12] ima: Allow imasig requirement to be satisfied by EVM portable signatures Roberto Sassu
2021-04-07 10:52 ` [PATCH v5 11/12] ima: Introduce template field evmsig and write to field sig as fallback Roberto Sassu
2021-04-07 10:52 ` [PATCH v5 12/12] ima: Don't remove security.ima if file must not be appraised Roberto Sassu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d599ddac80a94e4bbcd7973d1fa32235@huawei.com \
    --to=roberto.sassu@huawei.com \
    --cc=agruenba@redhat.com \
    --cc=christian.brauner@ubuntu.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=mjg59@google.com \
    --cc=zohar@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git