[PATCH 06/11] evm: Allow setxattr() and setattr() if metadata digest won't change

[PATCH 06/11] evm: Allow setxattr() and setattr() if metadata digest won't change

[Roberto Sassu]

If metadata are immutable, they cannot be changed. If metadata are already
set to the final value before cp and tar restore the value from the source,
those applications display an error even if the operation is legitimate
(they don't change the value).

This patch determines whether setxattr()/setattr() change metadata and, if
not, allows the operations even if metadata are immutable.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/evm/evm_main.c | 72 +++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 30072030f05d..41cc6a4aaaab 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -18,6 +18,7 @@
 #include <linux/integrity.h>
 #include <linux/evm.h>
 #include <linux/magic.h>
+#include <linux/posix_acl_xattr.h>
 
 #include <crypto/hash.h>
 #include <crypto/hash_info.h>
@@ -305,6 +306,56 @@ static enum integrity_status evm_verify_current_integrity(struct dentry *dentry)
 	return evm_verify_hmac(dentry, NULL, NULL, 0, NULL);
 }
 
+static int evm_xattr_acl_change(struct dentry *dentry, const char *xattr_name,
+				const void *xattr_value, size_t xattr_value_len)
+{
+	umode_t mode;
+	struct posix_acl *acl = NULL, *acl_res;
+	struct inode *inode = d_backing_inode(dentry);
+	int rc;
+
+	/* UID/GID in ACL have been already converted from user to init ns */
+	acl = posix_acl_from_xattr(&init_user_ns, xattr_value, xattr_value_len);
+	if (!acl)
+		return 1;
+
+	acl_res = acl;
+	rc = posix_acl_update_mode(inode, &mode, &acl_res);
+
+	posix_acl_release(acl);
+
+	if (rc)
+		return 1;
+
+	if (acl_res && inode->i_mode != mode)
+		return 1;
+
+	return 0;
+}
+
+static int evm_xattr_change(struct dentry *dentry, const char *xattr_name,
+			    const void *xattr_value, size_t xattr_value_len)
+{
+	char *xattr_data = NULL;
+	int rc = 0;
+
+	if (posix_xattr_acl(xattr_name))
+		return evm_xattr_acl_change(dentry, xattr_name, xattr_value,
+					    xattr_value_len);
+
+	rc = vfs_getxattr_alloc(dentry, xattr_name, &xattr_data, 0, GFP_NOFS);
+	if (rc < 0)
+		return 1;
+
+	if (rc == xattr_value_len)
+		rc = memcmp(xattr_value, xattr_data, rc);
+	else
+		rc = 1;
+
+	kfree(xattr_data);
+	return rc;
+}
+
 /*
  * evm_protect_xattr - protect the EVM extended attribute
  *
@@ -361,6 +412,10 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
 	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
 		return 0;
 
+	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
+	    !evm_xattr_change(dentry, xattr_name, xattr_value, xattr_value_len))
+		return 0;
+
 	if (evm_status != INTEGRITY_PASS)
 		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 				    dentry->d_name.name, "appraise_metadata",
@@ -477,6 +532,19 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
 	evm_update_evmxattr(dentry, xattr_name, NULL, 0);
 }
 
+static int evm_attr_change(struct dentry *dentry, struct iattr *attr)
+{
+	struct inode *inode = d_backing_inode(dentry);
+	unsigned int ia_valid = attr->ia_valid;
+
+	if ((!(ia_valid & ATTR_UID) || uid_eq(attr->ia_uid, inode->i_uid)) &&
+	    (!(ia_valid & ATTR_GID) || gid_eq(attr->ia_gid, inode->i_gid)) &&
+	    (!(ia_valid & ATTR_MODE) || attr->ia_mode == inode->i_mode))
+		return 0;
+
+	return 1;
+}
+
 /**
  * evm_inode_setattr - prevent updating an invalid EVM extended attribute
  * @dentry: pointer to the affected dentry
@@ -506,6 +574,10 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
 	    (evm_status == INTEGRITY_FAIL_IMMUTABLE))
 		return 0;
 
+	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
+	    !evm_attr_change(dentry, attr))
+		return 0;
+
 	integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 			    dentry->d_name.name, "appraise_metadata",
 			    integrity_status_msg[evm_status], -EPERM, 0);
-- 
2.17.1

Re: [PATCH 06/11] evm: Allow setxattr() and setattr() if metadata digest won't change

[Mimi Zohar]

On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> If metadata are immutable, they cannot be changed. If metadata are already
> set to the final value before cp and tar restore the value from the source,
> those applications display an error even if the operation is legitimate
> (they don't change the value).

"metadata" is singular.   The first sentence would be clearer by using
the specific metadata.  What problem are you trying to solve?  It
doesn't look like you're trying to solve the problem of writing the EVM
portable signatures without an exiting HMAC.  

> 
> This patch determines whether setxattr()/setattr() change metadata and, if
> not, allows the operations even if metadata are immutable.

Doesn't setxattr/setattr always change file metadata?  Please describe
the real problem.

> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/evm/evm_main.c | 72 +++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 30072030f05d..41cc6a4aaaab 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -18,6 +18,7 @@
>  #include <linux/integrity.h>
>  #include <linux/evm.h>
>  #include <linux/magic.h>
> +#include <linux/posix_acl_xattr.h>
>  
>  #include <crypto/hash.h>
>  #include <crypto/hash_info.h>
> @@ -305,6 +306,56 @@ static enum integrity_status evm_verify_current_integrity(struct dentry *dentry)
>  	return evm_verify_hmac(dentry, NULL, NULL, 0, NULL);
>  }
>  
> +static int evm_xattr_acl_change(struct dentry *dentry, const char *xattr_name,
> +				const void *xattr_value, size_t xattr_value_len)
> +{
> +	umode_t mode;
> +	struct posix_acl *acl = NULL, *acl_res;
> +	struct inode *inode = d_backing_inode(dentry);
> +	int rc;
> +
> +	/* UID/GID in ACL have been already converted from user to init ns */
> +	acl = posix_acl_from_xattr(&init_user_ns, xattr_value, xattr_value_len);
> +	if (!acl)
> +		return 1;
> +
> +	acl_res = acl;
> +	rc = posix_acl_update_mode(inode, &mode, &acl_res);
> +
> +	posix_acl_release(acl);
> +
> +	if (rc)
> +		return 1;
> +
> +	if (acl_res && inode->i_mode != mode)
> +		return 1;
> +
> +	return 0;
> +}
> +
> +static int evm_xattr_change(struct dentry *dentry, const char *xattr_name,
> +			    const void *xattr_value, size_t xattr_value_len)
> +{
> +	char *xattr_data = NULL;
> +	int rc = 0;
> +
> +	if (posix_xattr_acl(xattr_name))
> +		return evm_xattr_acl_change(dentry, xattr_name, xattr_value,
> +					    xattr_value_len);
> +
> +	rc = vfs_getxattr_alloc(dentry, xattr_name, &xattr_data, 0, GFP_NOFS);
> +	if (rc < 0)
> +		return 1;
> +
> +	if (rc == xattr_value_len)
> +		rc = memcmp(xattr_value, xattr_data, rc);
> +	else
> +		rc = 1;
> +
> +	kfree(xattr_data);
> +	return rc;
> +}
> +
>  /*
>   * evm_protect_xattr - protect the EVM extended attribute
>   *
> @@ -361,6 +412,10 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
>  	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
>  		return 0;
>  
> +	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> +	    !evm_xattr_change(dentry, xattr_name, xattr_value, xattr_value_len))
> +		return 0;
> +
>  	if (evm_status != INTEGRITY_PASS)
>  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
>  				    dentry->d_name.name, "appraise_metadata",
> @@ -477,6 +532,19 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
>  	evm_update_evmxattr(dentry, xattr_name, NULL, 0);
>  }
> 
> +static int evm_attr_change(struct dentry *dentry, struct iattr *attr)

static functions don't normally require a function comment, but in this
case it wouldn't hurt to clarify why the uid, gid, mode bits are set,
but aren't being modified.
Similarly a function comment would help with the readability of
evm_xattr_acl_change().

Mimi

> +{
> +	struct inode *inode = d_backing_inode(dentry);
> +	unsigned int ia_valid = attr->ia_valid;
> +
> +	if ((!(ia_valid & ATTR_UID) || uid_eq(attr->ia_uid, inode->i_uid)) &&
> +	    (!(ia_valid & ATTR_GID) || gid_eq(attr->ia_gid, inode->i_gid)) &&
> +	    (!(ia_valid & ATTR_MODE) || attr->ia_mode == inode->i_mode))
> +		return 0;
> +
> +	return 1;
> +}
> +
>  /**
>   * evm_inode_setattr - prevent updating an invalid EVM extended attribute
>   * @dentry: pointer to the affected dentry
> @@ -506,6 +574,10 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
>  	    (evm_status == INTEGRITY_FAIL_IMMUTABLE))
>  		return 0;
>  
> +	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> +	    !evm_attr_change(dentry, attr))
> +		return 0;
> +
>  	integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
>  			    dentry->d_name.name, "appraise_metadata",
>  			    integrity_status_msg[evm_status], -EPERM, 0);

RE: [PATCH 06/11] evm: Allow setxattr() and setattr() if metadata digest won't change

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, August 24, 2020 2:17 PM
> On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> > If metadata are immutable, they cannot be changed. If metadata are
> already
> > set to the final value before cp and tar restore the value from the source,
> > those applications display an error even if the operation is legitimate
> > (they don't change the value).
> 
> "metadata" is singular.   The first sentence would be clearer by using
> the specific metadata.  What problem are you trying to solve?  It
> doesn't look like you're trying to solve the problem of writing the EVM
> portable signatures without an exiting HMAC.
> 
> >
> > This patch determines whether setxattr()/setattr() change metadata and,
> if
> > not, allows the operations even if metadata are immutable.
> 
> Doesn't setxattr/setattr always change file metadata?  Please describe
> the real problem.

Yes. The problem is that tar/cp change metadata even if its value is
already correct after the file has been created. These operations
will be denied because metadata is immutable and verification
succeeds.

> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  security/integrity/evm/evm_main.c | 72
> +++++++++++++++++++++++++++++++
> >  1 file changed, 72 insertions(+)
> >
> > diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> > index 30072030f05d..41cc6a4aaaab 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/integrity.h>
> >  #include <linux/evm.h>
> >  #include <linux/magic.h>
> > +#include <linux/posix_acl_xattr.h>
> >
> >  #include <crypto/hash.h>
> >  #include <crypto/hash_info.h>
> > @@ -305,6 +306,56 @@ static enum integrity_status
> evm_verify_current_integrity(struct dentry *dentry)
> >  	return evm_verify_hmac(dentry, NULL, NULL, 0, NULL);
> >  }
> >
> > +static int evm_xattr_acl_change(struct dentry *dentry, const char
> *xattr_name,
> > +				const void *xattr_value, size_t
> xattr_value_len)
> > +{
> > +	umode_t mode;
> > +	struct posix_acl *acl = NULL, *acl_res;
> > +	struct inode *inode = d_backing_inode(dentry);
> > +	int rc;
> > +
> > +	/* UID/GID in ACL have been already converted from user to init ns
> */
> > +	acl = posix_acl_from_xattr(&init_user_ns, xattr_value,
> xattr_value_len);
> > +	if (!acl)
> > +		return 1;
> > +
> > +	acl_res = acl;
> > +	rc = posix_acl_update_mode(inode, &mode, &acl_res);
> > +
> > +	posix_acl_release(acl);
> > +
> > +	if (rc)
> > +		return 1;
> > +
> > +	if (acl_res && inode->i_mode != mode)
> > +		return 1;
> > +
> > +	return 0;
> > +}
> > +
> > +static int evm_xattr_change(struct dentry *dentry, const char
> *xattr_name,
> > +			    const void *xattr_value, size_t xattr_value_len)
> > +{
> > +	char *xattr_data = NULL;
> > +	int rc = 0;
> > +
> > +	if (posix_xattr_acl(xattr_name))
> > +		return evm_xattr_acl_change(dentry, xattr_name,
> xattr_value,
> > +					    xattr_value_len);
> > +
> > +	rc = vfs_getxattr_alloc(dentry, xattr_name, &xattr_data, 0,
> GFP_NOFS);
> > +	if (rc < 0)
> > +		return 1;
> > +
> > +	if (rc == xattr_value_len)
> > +		rc = memcmp(xattr_value, xattr_data, rc);
> > +	else
> > +		rc = 1;
> > +
> > +	kfree(xattr_data);
> > +	return rc;
> > +}
> > +
> >  /*
> >   * evm_protect_xattr - protect the EVM extended attribute
> >   *
> > @@ -361,6 +412,10 @@ static int evm_protect_xattr(struct dentry
> *dentry, const char *xattr_name,
> >  	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
> >  		return 0;
> >
> > +	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> > +	    !evm_xattr_change(dentry, xattr_name, xattr_value,
> xattr_value_len))
> > +		return 0;
> > +
> >  	if (evm_status != INTEGRITY_PASS)
> >  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> d_backing_inode(dentry),
> >  				    dentry->d_name.name,
> "appraise_metadata",
> > @@ -477,6 +532,19 @@ void evm_inode_post_removexattr(struct dentry
> *dentry, const char *xattr_name)
> >  	evm_update_evmxattr(dentry, xattr_name, NULL, 0);
> >  }
> >
> > +static int evm_attr_change(struct dentry *dentry, struct iattr *attr)
> 
> static functions don't normally require a function comment, but in this
> case it wouldn't hurt to clarify why the uid, gid, mode bits are set,
> but aren't being modified.
> Similarly a function comment would help with the readability of
> evm_xattr_acl_change().

Ok.

Roberto

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

> Mimi
> 
> > +{
> > +	struct inode *inode = d_backing_inode(dentry);
> > +	unsigned int ia_valid = attr->ia_valid;
> > +
> > +	if ((!(ia_valid & ATTR_UID) || uid_eq(attr->ia_uid, inode->i_uid))
> &&
> > +	    (!(ia_valid & ATTR_GID) || gid_eq(attr->ia_gid, inode->i_gid)) &&
> > +	    (!(ia_valid & ATTR_MODE) || attr->ia_mode == inode->i_mode))
> > +		return 0;
> > +
> > +	return 1;
> > +}
> > +
> >  /**
> >   * evm_inode_setattr - prevent updating an invalid EVM extended
> attribute
> >   * @dentry: pointer to the affected dentry
> > @@ -506,6 +574,10 @@ int evm_inode_setattr(struct dentry *dentry,
> struct iattr *attr)
> >  	    (evm_status == INTEGRITY_FAIL_IMMUTABLE))
> >  		return 0;
> >
> > +	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> > +	    !evm_attr_change(dentry, attr))
> > +		return 0;
> > +
> >  	integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> d_backing_inode(dentry),
> >  			    dentry->d_name.name, "appraise_metadata",
> >  			    integrity_status_msg[evm_status], -EPERM, 0);
> 

[PATCH 07/11] evm: Set IMA_CHANGE_XATTR/ATTR bit if EVM_ALLOW_METADATA_WRITES is set

[Roberto Sassu]

When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation on
metadata. Its main purpose is to allow users to freely set metadata when
they are protected by a portable signature, until the HMAC key is loaded.

However, IMA is not notified about metadata changes and, after the first
appraisal, always allows access to the files without checking metadata
again.

This patch checks in evm_reset_status() if EVM_ALLOW_METADATA WRITES is
enabled and if it is, sets the IMA_CHANGE_XATTR/ATTR bits depending on the
operation performed. At the next appraisal, metadata are revalidated.

This patch also adds a call to evm_reset_status() in
evm_inode_post_setattr() so that EVM won't return the cached status the
next time appraisal is performed.

Cc: stable@vger.kernel.org # 4.16.x
Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/evm/evm_main.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 41cc6a4aaaab..d4d918183094 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -478,13 +478,17 @@ int evm_inode_removexattr(struct dentry *dentry, const char *xattr_name)
 	return evm_protect_xattr(dentry, xattr_name, NULL, 0);
 }
 
-static void evm_reset_status(struct inode *inode)
+static void evm_reset_status(struct inode *inode, int bit)
 {
 	struct integrity_iint_cache *iint;
 
 	iint = integrity_iint_find(inode);
-	if (iint)
+	if (iint) {
+		if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
+			set_bit(bit, &iint->atomic_flags);
+
 		iint->evm_status = INTEGRITY_UNKNOWN;
+	}
 }
 
 /**
@@ -507,7 +511,7 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
 				  && !posix_xattr_acl(xattr_name)))
 		return;
 
-	evm_reset_status(dentry->d_inode);
+	evm_reset_status(dentry->d_inode, IMA_CHANGE_XATTR);
 
 	evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len);
 }
@@ -527,7 +531,7 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
 	if (!evm_key_loaded() || !evm_protected_xattr(xattr_name))
 		return;
 
-	evm_reset_status(dentry->d_inode);
+	evm_reset_status(dentry->d_inode, IMA_CHANGE_XATTR);
 
 	evm_update_evmxattr(dentry, xattr_name, NULL, 0);
 }
@@ -600,6 +604,8 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
 	if (!evm_key_loaded())
 		return;
 
+	evm_reset_status(dentry->d_inode, IMA_CHANGE_ATTR);
+
 	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
 		evm_update_evmxattr(dentry, NULL, NULL, 0);
 }
-- 
2.17.1

Re: [PATCH 07/11] evm: Set IMA_CHANGE_XATTR/ATTR bit if EVM_ALLOW_METADATA_WRITES is set

[Mimi Zohar]

On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation on
> metadata. Its main purpose is to allow users to freely set metadata when
> they are protected by a portable signature, until the HMAC key is loaded.
> 
> However, IMA is not notified about metadata changes and, after the first
> appraisal, always allows access to the files without checking metadata
> again.

^after the first successful appraisal
> 
> This patch checks in evm_reset_status() if EVM_ALLOW_METADATA WRITES is
> enabled and if it is, sets the IMA_CHANGE_XATTR/ATTR bits depending on the
> operation performed. At the next appraisal, metadata are revalidated.

EVM modifying IMA bits crosses the boundary between EVM and IMA.  There
is already an IMA post_setattr hook.  IMA could reset its own bit
there.  If necessary EVM could export as a function it's status info.

Mimi

> 
> This patch also adds a call to evm_reset_status() in
> evm_inode_post_setattr() so that EVM won't return the cached status the
> next time appraisal is performed.
> 
> Cc: stable@vger.kernel.org # 4.16.x
> Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/evm/evm_main.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 41cc6a4aaaab..d4d918183094 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -478,13 +478,17 @@ int evm_inode_removexattr(struct dentry *dentry, const char *xattr_name)
>  	return evm_protect_xattr(dentry, xattr_name, NULL, 0);
>  }
>  
> -static void evm_reset_status(struct inode *inode)
> +static void evm_reset_status(struct inode *inode, int bit)
>  {
>  	struct integrity_iint_cache *iint;
>  
>  	iint = integrity_iint_find(inode);
> -	if (iint)
> +	if (iint) {
> +		if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> +			set_bit(bit, &iint->atomic_flags);
> +
>  		iint->evm_status = INTEGRITY_UNKNOWN;
> +	}
>  }
>  
>  /**:q
> @@ -507,7 +511,7 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
>  				  && !posix_xattr_acl(xattr_name)))
>  		return;
>  
> -	evm_reset_status(dentry->d_inode);
> +	evm_reset_status(dentry->d_inode, IMA_CHANGE_XATTR);
>  
>  	evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len);
>  }
> @@ -527,7 +531,7 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
>  	if (!evm_key_loaded() || !evm_protected_xattr(xattr_name))
>  		return;
>  
> -	evm_reset_status(dentry->d_inode);
> +	evm_reset_status(dentry->d_inode, IMA_CHANGE_XATTR);
>  
>  	evm_update_evmxattr(dentry, xattr_name, NULL, 0);
>  }
> @@ -600,6 +604,8 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
>  	if (!evm_key_loaded())
>  		return;
>  
> +	evm_reset_status(dentry->d_inode, IMA_CHANGE_ATTR);
> +
>  	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
>  		evm_update_evmxattr(dentry, NULL, NULL, 0);
>  }

RE: [PATCH 07/11] evm: Set IMA_CHANGE_XATTR/ATTR bit if EVM_ALLOW_METADATA_WRITES is set

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, August 24, 2020 2:18 PM
> On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> > When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation
> on
> > metadata. Its main purpose is to allow users to freely set metadata when
> > they are protected by a portable signature, until the HMAC key is loaded.
> >
> > However, IMA is not notified about metadata changes and, after the first
> > appraisal, always allows access to the files without checking metadata
> > again.
> 
> ^after the first successful appraisal
> >
> > This patch checks in evm_reset_status() if EVM_ALLOW_METADATA
> WRITES is
> > enabled and if it is, sets the IMA_CHANGE_XATTR/ATTR bits depending on
> the
> > operation performed. At the next appraisal, metadata are revalidated.
> 
> EVM modifying IMA bits crosses the boundary between EVM and IMA.
> There
> is already an IMA post_setattr hook.  IMA could reset its own bit
> there.  If necessary EVM could export as a function it's status info.

I wouldn't try to guess in IMA when EVM resets its status. We would have
to duplicate the logic to check if an EVM key is loaded, if the passed xattr
is a POSIX ACL, ...

I think it is better to set a flag, maybe a new one, directly in EVM, to notify
the integrity subsystem that iint->evm_status is no longer valid.

If the EVM flag is set, IMA would reset the appraisal flags, as it uses
iint->evm_status for appraisal. We can consider to reset also the measure
flags when we have a template that includes file metadata.

Roberto

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

> Mimi
> 
> >
> > This patch also adds a call to evm_reset_status() in
> > evm_inode_post_setattr() so that EVM won't return the cached status
> the
> > next time appraisal is performed.
> >
> > Cc: stable@vger.kernel.org # 4.16.x
> > Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of
> EVM-protected metadata")
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  security/integrity/evm/evm_main.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> > index 41cc6a4aaaab..d4d918183094 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -478,13 +478,17 @@ int evm_inode_removexattr(struct dentry
> *dentry, const char *xattr_name)
> >  	return evm_protect_xattr(dentry, xattr_name, NULL, 0);
> >  }
> >
> > -static void evm_reset_status(struct inode *inode)
> > +static void evm_reset_status(struct inode *inode, int bit)
> >  {
> >  	struct integrity_iint_cache *iint;
> >
> >  	iint = integrity_iint_find(inode);
> > -	if (iint)
> > +	if (iint) {
> > +		if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> > +			set_bit(bit, &iint->atomic_flags);
> > +
> >  		iint->evm_status = INTEGRITY_UNKNOWN;
> > +	}
> >  }
> >
> >  /**:q
> > @@ -507,7 +511,7 @@ void evm_inode_post_setxattr(struct dentry
> *dentry, const char *xattr_name,
> >  				  && !posix_xattr_acl(xattr_name)))
> >  		return;
> >
> > -	evm_reset_status(dentry->d_inode);
> > +	evm_reset_status(dentry->d_inode, IMA_CHANGE_XATTR);
> >
> >  	evm_update_evmxattr(dentry, xattr_name, xattr_value,
> xattr_value_len);
> >  }
> > @@ -527,7 +531,7 @@ void evm_inode_post_removexattr(struct dentry
> *dentry, const char *xattr_name)
> >  	if (!evm_key_loaded() || !evm_protected_xattr(xattr_name))
> >  		return;
> >
> > -	evm_reset_status(dentry->d_inode);
> > +	evm_reset_status(dentry->d_inode, IMA_CHANGE_XATTR);
> >
> >  	evm_update_evmxattr(dentry, xattr_name, NULL, 0);
> >  }
> > @@ -600,6 +604,8 @@ void evm_inode_post_setattr(struct dentry
> *dentry, int ia_valid)
> >  	if (!evm_key_loaded())
> >  		return;
> >
> > +	evm_reset_status(dentry->d_inode, IMA_CHANGE_ATTR);
> > +
> >  	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
> >  		evm_update_evmxattr(dentry, NULL, NULL, 0);
> >  }
> 

Re: [PATCH 07/11] evm: Set IMA_CHANGE_XATTR/ATTR bit if EVM_ALLOW_METADATA_WRITES is set

[Mimi Zohar]

On Tue, 2020-09-01 at 09:08 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Monday, August 24, 2020 2:18 PM
> > On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> > > When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation
> > on
> > > metadata. Its main purpose is to allow users to freely set metadata when
> > > they are protected by a portable signature, until the HMAC key is loaded.
> > >
> > > However, IMA is not notified about metadata changes and, after the first
> > > appraisal, always allows access to the files without checking metadata
> > > again.
> > 
> > ^after the first successful appraisal
> > >
> > > This patch checks in evm_reset_status() if EVM_ALLOW_METADATA
> > WRITES is
> > > enabled and if it is, sets the IMA_CHANGE_XATTR/ATTR bits depending on
> > the
> > > operation performed. At the next appraisal, metadata are revalidated.
> > 
> > EVM modifying IMA bits crosses the boundary between EVM and IMA.
> > There
> > is already an IMA post_setattr hook.  IMA could reset its own bit
> > there.  If necessary EVM could export as a function it's status info.
> 
> I wouldn't try to guess in IMA when EVM resets its status. We would have
> to duplicate the logic to check if an EVM key is loaded, if the passed xattr
> is a POSIX ACL, ...

Agreed, but IMA could call an EVM function.

> 
> I think it is better to set a flag, maybe a new one, directly in EVM, to notify
> the integrity subsystem that iint->evm_status is no longer valid.
> 
> If the EVM flag is set, IMA would reset the appraisal flags, as it uses
> iint->evm_status for appraisal. We can consider to reset also the measure
> flags when we have a template that includes file metadata.

When would IMA read the EVM flag?   Who would reset the flag?  At what
point would it be reset?   Just as EVM shouldn't be resetting the IMA
flag, IMA shouldn't be resetting the EVM flag.

Mimi

RE: [PATCH 07/11] evm: Set IMA_CHANGE_XATTR/ATTR bit if EVM_ALLOW_METADATA_WRITES is set

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Tuesday, September 1, 2020 1:05 PM
> On Tue, 2020-09-01 at 09:08 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > Sent: Monday, August 24, 2020 2:18 PM
> > > On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> > > > When EVM_ALLOW_METADATA_WRITES is set, EVM allows any
> operation
> > > on
> > > > metadata. Its main purpose is to allow users to freely set metadata
> when
> > > > they are protected by a portable signature, until the HMAC key is
> loaded.
> > > >
> > > > However, IMA is not notified about metadata changes and, after the
> first
> > > > appraisal, always allows access to the files without checking metadata
> > > > again.
> > >
> > > ^after the first successful appraisal
> > > >
> > > > This patch checks in evm_reset_status() if EVM_ALLOW_METADATA
> > > WRITES is
> > > > enabled and if it is, sets the IMA_CHANGE_XATTR/ATTR bits
> depending on
> > > the
> > > > operation performed. At the next appraisal, metadata are revalidated.
> > >
> > > EVM modifying IMA bits crosses the boundary between EVM and IMA.
> > > There
> > > is already an IMA post_setattr hook.  IMA could reset its own bit
> > > there.  If necessary EVM could export as a function it's status info.
> >
> > I wouldn't try to guess in IMA when EVM resets its status. We would have
> > to duplicate the logic to check if an EVM key is loaded, if the passed xattr
> > is a POSIX ACL, ...
> 
> Agreed, but IMA could call an EVM function.
> 
> >
> > I think it is better to set a flag, maybe a new one, directly in EVM, to notify
> > the integrity subsystem that iint->evm_status is no longer valid.
> >
> > If the EVM flag is set, IMA would reset the appraisal flags, as it uses
> > iint->evm_status for appraisal. We can consider to reset also the measure
> > flags when we have a template that includes file metadata.
> 
> When would IMA read the EVM flag?   Who would reset the flag?  At what
> point would it be reset?   Just as EVM shouldn't be resetting the IMA
> flag, IMA shouldn't be resetting the EVM flag.

IMA would read the flag in process_measurement() and behave similarly
to when it processes IMA_CHANGE_ATTR. The flag would be reset by
evm_verify_hmac().

Roberto

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

Re: [PATCH 07/11] evm: Set IMA_CHANGE_XATTR/ATTR bit if EVM_ALLOW_METADATA_WRITES is set

[Mimi Zohar]

> > > I think it is better to set a flag, maybe a new one, directly in EVM, to notify
> > > the integrity subsystem that iint->evm_status is no longer valid.
> > >
> > > If the EVM flag is set, IMA would reset the appraisal flags, as it uses
> > > iint->evm_status for appraisal. We can consider to reset also the measure
> > > flags when we have a template that includes file metadata.
> > 
> > When would IMA read the EVM flag?   Who would reset the flag?  At what
> > point would it be reset?   Just as EVM shouldn't be resetting the IMA
> > flag, IMA shouldn't be resetting the EVM flag.
> 
> IMA would read the flag in process_measurement() and behave similarly
> to when it processes IMA_CHANGE_ATTR. The flag would be reset by
> evm_verify_hmac().

Sounds good.

Mimi

[PATCH 08/11] ima: Allow imasig requirement to be satisfied by EVM portable signatures

[Roberto Sassu]

System administrators can require that all accessed files have a signature
by specifying appraise_type=imasig in a policy rule.

Currently, only IMA signatures satisfy this requirement. IMA signatures
ensure data source authentication for file content and prevent any change.
EVM signatures instead ensure data source authentication for file metadata.
Given that the digest or signature of the file content must be included in
the metadata, EVM signatures provide at least the same guarantees of IMA
signatures.

This patch lets systems protected with EVM signatures pass appraisal
verification if the appraise_type=imasig requirement is specified in the
policy. This facilitates deployment in the scenarios where only EVM
signatures are available.

The patch makes the following changes:

file xattr types:
security.ima: IMA_XATTR_DIGEST/IMA_XATTR_DIGEST_NG
security.evm: EVM_XATTR_PORTABLE_DIGSIG

execve(), mmap(), open() behavior (with appraise_type=imasig):
before: denied (file without IMA signature, imasig requirement not met)
after: allowed (file with EVM portable signature, imasig requirement met)

open(O_WRONLY) behavior (without appraise_type=imasig):
before: allowed (file without IMA signature, not immutable)
after: denied (file with EVM portable signature, immutable)

In addition, similarly to IMA signatures, this patch temporarily allows
new files without or with incomplete metadata to be opened so that content
can be written.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_appraise.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 21bda264fc30..9505bb390d90 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -219,12 +219,16 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
 		hash_start = 1;
 		/* fall through */
 	case IMA_XATTR_DIGEST:
-		if (iint->flags & IMA_DIGSIG_REQUIRED) {
-			*cause = "IMA-signature-required";
-			*status = INTEGRITY_FAIL;
-			break;
+		if (*status != INTEGRITY_PASS_IMMUTABLE) {
+			if (iint->flags & IMA_DIGSIG_REQUIRED) {
+				*cause = "IMA-signature-required";
+				*status = INTEGRITY_FAIL;
+				break;
+			}
+			clear_bit(IMA_DIGSIG, &iint->atomic_flags);
+		} else {
+			set_bit(IMA_DIGSIG, &iint->atomic_flags);
 		}
-		clear_bit(IMA_DIGSIG, &iint->atomic_flags);
 		if (xattr_len - sizeof(xattr_value->type) - hash_start >=
 				iint->ima_hash->length)
 			/*
@@ -394,6 +398,8 @@ int ima_appraise_measurement(enum ima_hooks func,
 		cause = "missing-HMAC";
 		goto out;
 	case INTEGRITY_FAIL_IMMUTABLE:
+		set_bit(IMA_DIGSIG, &iint->atomic_flags);
+		fallthrough;
 	case INTEGRITY_FAIL:		/* Invalid HMAC/signature. */
 		cause = "invalid-HMAC";
 		goto out;
@@ -437,9 +443,9 @@ int ima_appraise_measurement(enum ima_hooks func,
 				status = INTEGRITY_PASS;
 		}
 
-		/* Permit new files with file signatures, but without data. */
+		/* Permit new files marked as immutable, but without data. */
 		if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
-		    xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
+		    test_bit(IMA_DIGSIG, &iint->atomic_flags)) {
 			status = INTEGRITY_PASS;
 		}
 
-- 
2.17.1

Re: [PATCH 08/11] ima: Allow imasig requirement to be satisfied by EVM portable signatures

[Mimi Zohar]

On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> System administrators can require that all accessed files have a signature
> by specifying appraise_type=imasig in a policy rule.
> 
> Currently, only IMA signatures satisfy this requirement.

Appended signatures may also satisfy this requirement, but are not
applicable as ...

> IMA signatures
> ensure data source authentication for file content and prevent any change.
> EVM signatures instead ensure data source authentication for file metadata.
> Given that the digest or signature of the file content must be included in
> the metadata, EVM signatures provide at least the same guarantees of IMA
> signatures.

^provide the same file data guarantees of IMA signatures, as well as
providing file metadata guarantees.

> 
> This patch lets systems protected with EVM signatures pass appraisal
> verification if the appraise_type=imasig requirement is specified in the
> policy. This facilitates deployment in the scenarios where only EVM
> signatures are available.
> 
> The patch makes the following changes:
> 
> file xattr types:
> security.ima: IMA_XATTR_DIGEST/IMA_XATTR_DIGEST_NG
> security.evm: EVM_XATTR_PORTABLE_DIGSIG
> 
> execve(), mmap(), open() behavior (with appraise_type=imasig):
> before: denied (file without IMA signature, imasig requirement not met)
> after: allowed (file with EVM portable signature, imasig requirement met)
> 
> open(O_WRONLY) behavior (without appraise_type=imasig):
> before: allowed (file without IMA signature, not immutable)
> after: denied (file with EVM portable signature, immutable)
> 
> In addition, similarly to IMA signatures, this patch temporarily allows
> new files without or with incomplete metadata to be opened so that content
> can be written.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

After addressing the comments above and below,

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

> ---
>  security/integrity/ima/ima_appraise.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 21bda264fc30..9505bb390d90 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -219,12 +219,16 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
>  		hash_start = 1;
>  		/* fall through */
>  	case IMA_XATTR_DIGEST:
> -		if (iint->flags & IMA_DIGSIG_REQUIRED) {
> -			*cause = "IMA-signature-required";
> -			*status = INTEGRITY_FAIL;
> -			break;
> +		if (*status != INTEGRITY_PASS_IMMUTABLE) {
> +			if (iint->flags & IMA_DIGSIG_REQUIRED) {
> +				*cause = "IMA-signature-required";
> +				*status = INTEGRITY_FAIL;
> +				break;
> +			}
> +			clear_bit(IMA_DIGSIG, &iint->atomic_flags);
> +		} else {
> +			set_bit(IMA_DIGSIG, &iint->atomic_flags);
>  		}
> -		clear_bit(IMA_DIGSIG, &iint->atomic_flags);
>  		if (xattr_len - sizeof(xattr_value->type) - hash_start >=
>  				iint->ima_hash->length)
>  			/*
> @@ -394,6 +398,8 @@ int ima_appraise_measurement(enum ima_hooks func,
>  		cause = "missing-HMAC";
>  		goto out;
>  	case INTEGRITY_FAIL_IMMUTABLE:
> +		set_bit(IMA_DIGSIG, &iint->atomic_flags);
> +		fallthrough;
>  	case INTEGRITY_FAIL:		/* Invalid HMAC/signature. */
>  		cause = "invalid-HMAC";
>  		goto out;
> @@ -437,9 +443,9 @@ int ima_appraise_measurement(enum ima_hooks func,
>  				status = INTEGRITY_PASS;
>  		}
>  
> -		/* Permit new files with file signatures, but without data. */
> +		/* Permit new files marked as immutable, but without data. */

This comment isn't quite right.

>  		if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
> -		    xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
> +		    test_bit(IMA_DIGSIG, &iint->atomic_flags)) {
>  			status = INTEGRITY_PASS;
>  		}
>  

[PATCH 09/11] ima: Don't remove security.ima if file must not be appraised

[Roberto Sassu]

Files might come from a remote source and might have xattrs, including
security.ima. It should not be IMA task to decide whether security.ima
should be kept or not. This patch removes the removexattr() system
call in ima_inode_post_setattr().

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_appraise.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 9505bb390d90..83c62eaf342d 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -504,8 +504,6 @@ void ima_inode_post_setattr(struct dentry *dentry)
 		return;
 
 	action = ima_must_appraise(inode, MAY_ACCESS, POST_SETATTR);
-	if (!action)
-		__vfs_removexattr(dentry, XATTR_NAME_IMA);
 	iint = integrity_iint_find(inode);
 	if (iint) {
 		set_bit(IMA_CHANGE_ATTR, &iint->atomic_flags);
-- 
2.17.1

Re: [PATCH 09/11] ima: Don't remove security.ima if file must not be appraised

[Mimi Zohar]

On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> Files might come from a remote source and might have xattrs, including
> security.ima. It should not be IMA task to decide whether security.ima
> should be kept or not. This patch removes the removexattr() system
> call in ima_inode_post_setattr().
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Yes, this has been previously discussed.

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

[PATCH 10/11] ima: Don't ignore errors from crypto_shash_update()

[Roberto Sassu]

Errors returned by crypto_shash_update() are not checked in
ima_calc_boot_aggregate_tfm() and thus can be overwritten at the next
iteration of the loop. This patch adds a check after calling
crypto_shash_update() and returns immediately if the result is not zero.

Cc: stable@vger.kernel.org
Fixes: 3323eec921efd ("integrity: IMA as an integrity service provider")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_crypto.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 220b14920c37..47897fbae6c6 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -829,6 +829,8 @@ static int ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
 		/* now accumulate with current aggregate */
 		rc = crypto_shash_update(shash, d.digest,
 					 crypto_shash_digestsize(tfm));
+		if (rc != 0)
+			return rc;
 	}
 	if (!rc)
 		crypto_shash_final(shash, digest);
-- 
2.17.1

Re: [PATCH 10/11] ima: Don't ignore errors from crypto_shash_update()

[Mimi Zohar]

On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> Errors returned by crypto_shash_update() are not checked in
> ima_calc_boot_aggregate_tfm() and thus can be overwritten at the next
> iteration of the loop. This patch adds a check after calling
> crypto_shash_update() and returns immediately if the result is not zero.
> 
> Cc: stable@vger.kernel.org
> Fixes: 3323eec921efd ("integrity: IMA as an integrity service provider")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Verification of the boot_aggregate will fail, but yes this should be
fixed.  This patch  and the next should be moved up front to the
beginning of the patch set.

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

thanks,

Mimi

[PATCH 11/11] ima: Remove semicolon at the end of ima_get_binary_runtime_size()

[Roberto Sassu]

This patch removes the unnecessary semicolon at the end of
ima_get_binary_runtime_size().

Cc: stable@vger.kernel.org
Fixes: d158847ae89a2 ("ima: maintain memory size needed for serializing the measurement list")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_queue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index fb4ec270f620..c096ef8945c7 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -133,7 +133,7 @@ unsigned long ima_get_binary_runtime_size(void)
 		return ULONG_MAX;
 	else
 		return binary_runtime_size + sizeof(struct ima_kexec_hdr);
-};
+}
 
 static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
 {
-- 
2.17.1