| Message ID | 20200421092418.25151-1-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: Allow imasig requirement to be satisfied by EVM portable signatures | expand |
On Tue, 2020-04-21 at 11:24 +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. However, also EVM > portable signatures can satisfy it. Metadata, including security.ima, are > signed and cannot change. Please expand this paragraph with a short comparison of the security guarantees provided by EVM immutable, portable signatures versus ima- sig. > > This patch helps in the scenarios where system administrators want to > enforce this restriction but only EVM portable signatures are available. Yes, I agree it "helps", but we still need to address the ability of setting/removing security.ima, which isn't possible with an IMA signature. This sounds like we need to define an immutable file hash. What do you think? > 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) > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > security/integrity/ima/ima_appraise.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > index a9649b04b9f1..69a6a958f811 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) > /* Nice! Mimi
> -----Original Message----- > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Thursday, April 23, 2020 10:52 PM > To: Roberto Sassu <roberto.sassu@huawei.com>; mjg59@google.com > Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org; > linux-kernel@vger.kernel.org; Silviu Vlasceanu > <Silviu.Vlasceanu@huawei.com> > Subject: Re: [PATCH] ima: Allow imasig requirement to be satisfied by EVM > portable signatures > > On Tue, 2020-04-21 at 11:24 +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. However, also > EVM > > portable signatures can satisfy it. Metadata, including security.ima, are > > signed and cannot change. > > Please expand this paragraph with a short comparison of the security > guarantees provided by EVM immutable, portable signatures versus ima- > sig. > > > > > This patch helps in the scenarios where system administrators want to > > enforce this restriction but only EVM portable signatures are available. > > Yes, I agree it "helps", but we still need to address the ability of > setting/removing security.ima, which isn't possible with an IMA > signature. This sounds like we need to define an immutable file hash. I didn't understand. Can you explain better? Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli > What do you think? > > > 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) > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > --- > > security/integrity/ima/ima_appraise.c | 14 +++++++++----- > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/security/integrity/ima/ima_appraise.c > b/security/integrity/ima/ima_appraise.c > > index a9649b04b9f1..69a6a958f811 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) > > /* > > Nice! > > Mimi
> -----Original Message----- > From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity- > owner@vger.kernel.org] On Behalf Of Roberto Sassu > Sent: Friday, April 24, 2020 12:40 PM > To: Mimi Zohar <zohar@linux.ibm.com>; mjg59@google.com > Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org; > linux-kernel@vger.kernel.org; Silviu Vlasceanu > <Silviu.Vlasceanu@huawei.com> > Subject: RE: [PATCH] ima: Allow imasig requirement to be satisfied by EVM > portable signatures > > > -----Original Message----- > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > Sent: Thursday, April 23, 2020 10:52 PM > > To: Roberto Sassu <roberto.sassu@huawei.com>; mjg59@google.com > > Cc: linux-integrity@vger.kernel.org; linux-security- > module@vger.kernel.org; > > linux-kernel@vger.kernel.org; Silviu Vlasceanu > > <Silviu.Vlasceanu@huawei.com> > > Subject: Re: [PATCH] ima: Allow imasig requirement to be satisfied by > EVM > > portable signatures > > > > On Tue, 2020-04-21 at 11:24 +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. However, also > > EVM > > > portable signatures can satisfy it. Metadata, including security.ima, are > > > signed and cannot change. > > > > Please expand this paragraph with a short comparison of the security > > guarantees provided by EVM immutable, portable signatures versus ima- > > sig. > > > > > > > > This patch helps in the scenarios where system administrators want to > > > enforce this restriction but only EVM portable signatures are available. > > > > Yes, I agree it "helps", but we still need to address the ability of > > setting/removing security.ima, which isn't possible with an IMA > > signature. This sounds like we need to define an immutable file hash. > > I didn't understand. Can you explain better? Ok, got it. I wouldn't grant access to new file depending on the security.ima type but depending on the IMA_DIGSIG bit. In both cases, IMA signature and EVM portable signature, the bit is set. There is one remaining issue. Maybe the signature is portable, but you don't get it from evm_verifyxattr() if verification fails. There is a legitimate case when it happens, which is when you extract a file with a portable signature with tar, and the inode uid/gid are not yet correct (fchown() is called later after the open()). In this case, IMA_DIGSIG is not set and the open() fails. To avoid this issue I would introduce the new status INTEGRITY_FAIL_IMMUTABLE, so that IMA_DIGSIG is set even if the verification of the portable signature fails. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli > Thanks > > Roberto > > HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 > Managing Director: Li Peng, Li Jian, Shi Yanli > > > > What do you think? > > > > > 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) > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > --- > > > security/integrity/ima/ima_appraise.c | 14 +++++++++----- > > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > > > diff --git a/security/integrity/ima/ima_appraise.c > > b/security/integrity/ima/ima_appraise.c > > > index a9649b04b9f1..69a6a958f811 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) > > > /* > > > > Nice! > > > > Mimi
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index a9649b04b9f1..69a6a958f811 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) /*
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. However, also EVM portable signatures can satisfy it. Metadata, including security.ima, are signed and cannot change. This patch helps in the scenarios where system administrators want to enforce this restriction but only EVM portable 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) Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- security/integrity/ima/ima_appraise.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)