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]

Message-ID: <8493d7e2b0fefa4cd3861bd6b7ee6f2340aa7434.camel@linux.ibm.com>
Date:   Mon, 03 May 2021 09:00:21 -0400
From:   Mimi Zohar <zohar@...ux.ibm.com>
To:     Roberto Sassu <roberto.sassu@...wei.com>, mjg59@...gle.com
Cc:     linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-fsdevel@...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

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.

I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
prevent the file from being resigned.

>  	if (evm_status != INTEGRITY_PASS)
>  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
>  				    dentry->d_name.name, "appraise_metadata",

This would then be updated to if not INTEGRITY_PASS or
INTEGRITY_PASS_IMMUTABLE.  The subsequent "return" would need to be
updated as well.

thanks,

Mimi