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: [email protected] # 4.16.x
Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
Signed-off-by: Roberto Sassu <[email protected]>
---
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
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: [email protected] # 4.16.x
> Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
> Signed-off-by: Roberto Sassu <[email protected]>
> ---
> 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);
> }
> From: Mimi Zohar [mailto:[email protected]]
> 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: [email protected] # 4.16.x
> > Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of
> EVM-protected metadata")
> > Signed-off-by: Roberto Sassu <[email protected]>
> > ---
> > 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);
> > }
>
> From: Mimi Zohar [mailto:[email protected]]
> Sent: Tuesday, September 1, 2020 1:05 PM
> On Tue, 2020-09-01 at 09:08 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:[email protected]]
> > > 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
On Tue, 2020-09-01 at 09:08 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:[email protected]]
> > 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
> > > 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