| 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
| ||
|
Message-ID: <1405510640.1466.16.camel@dhcp-9-2-203-236.watson.ibm.com>
Date: Wed, 16 Jul 2014 07:37:20 -0400
From: Mimi Zohar <zohar@...ux.vnet.ibm.com>
To: Dmitry Kasatkin <d.kasatkin@...sung.com>
Cc: linux-ima-devel@...ts.sourceforge.net,
linux-security-module@...r.kernel.org,
linux-kernel@...r.kernel.org, viro@...iv.linux.org.uk,
linux-fsdevel@...r.kernel.org, dmitry.kasatkin@...il.com
Subject: Re: [PATCH v1 3/3] ima: pass 'opened' flag to identify newly
created files
On Wed, 2014-07-16 at 11:25 +0300, Dmitry Kasatkin wrote:
> On 16/07/14 01:12, Mimi Zohar wrote:
> > On Fri, 2014-07-11 at 14:47 +0300, Dmitry Kasatkin wrote:
> >> Empty file size and missing xattrs do not guaranty that file
> > ^guarantee
> >
> >> was just created. It could be originally made empty and labeled
> >> with needed LSM labels. Current implementation makes it possible
> >> to remove security.ima, and set arbitrary LSM related attribute.
> >> On open, IMA would be forced to update security.evm to 'fake' LSM
> >> xattrs.
> > Only in 'fix' mode, is the security.ima value written out on file
> > open. The previous patch introduced the ability to set "arbitrary LSM
> > related attributes" without a security.evm label.
>
> Comment is a bit unclear to me...
>
> Previous patch does not allow to set arbitrary LSM value,
> but if runtime permission allows, it allows to set "initial" xattrs for
> newly created files...
>
> I think description I made is a bit unclear..
>
> What I wanted to tell is that..
>
> "Assuming that empty file is a newly created file, IMA skips EVM
> verification which allows "offline" removing security.ima and set
> arbitrary security xattrs. Updating and closing file will make EVM to
> update security.evm with forged secirity xattrs.
Sure the security IMA and EVM xattrs can be removed offline, but can not
be replaced one without the other. The file size indicates a "new"
file, only if there aren't any existing xattrs. The question is whether
just removing an xattr provides a benefit.
> The question is if making file empty on purpose, clearing security.ima
> and changing xattrs will allow any attack as file is empty.
> If so, then using FILE_CREATED flag is safe choice.
>
> - Dmitry
Can we say, "Clearly identifying new files further limits possible
attacks."
Mimi
>
> > The patch itself is fine. Please update the patch description.
> >
> > thanks,
> >
> > Mimi
> >
> >> This patch passes FILE_CREATED flag to IMA to reliably identify new
> >> files.
> >>
> >> Signed-off-by: Dmitry Kasatkin <d.kasatkin@...sung.com>
> >> ---
> >> fs/namei.c | 2 +-
> >> fs/nfsd/vfs.c | 2 +-
> >> include/linux/ima.h | 4 ++--
> >> security/integrity/ima/ima.h | 4 ++--
> >> security/integrity/ima/ima_appraise.c | 4 ++--
> >> security/integrity/ima/ima_main.c | 14 +++++++-------
> >> 6 files changed, 15 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/fs/namei.c b/fs/namei.c
> >> index 985c6f3..005771f 100644
> >> --- a/fs/namei.c
> >> +++ b/fs/namei.c
> >> @@ -3058,7 +3058,7 @@ opened:
> >> error = open_check_o_direct(file);
> >> if (error)
> >> goto exit_fput;
> >> - error = ima_file_check(file, op->acc_mode);
> >> + error = ima_file_check(file, op->acc_mode, *opened);
> >> if (error)
> >> goto exit_fput;
> >>
> >> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> >> index 140c496..d49c778 100644
> >> --- a/fs/nfsd/vfs.c
> >> +++ b/fs/nfsd/vfs.c
> >> @@ -709,7 +709,7 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type,
> >> host_err = PTR_ERR(*filp);
> >> *filp = NULL;
> >> } else {
> >> - host_err = ima_file_check(*filp, may_flags);
> >> + host_err = ima_file_check(*filp, may_flags, 0);
> >>
> >> if (may_flags & NFSD_MAY_64BIT_COOKIE)
> >> (*filp)->f_mode |= FMODE_64BITHASH;
> >> diff --git a/include/linux/ima.h b/include/linux/ima.h
> >> index 1b7f268..23a87a4 100644
> >> --- a/include/linux/ima.h
> >> +++ b/include/linux/ima.h
> >> @@ -15,7 +15,7 @@ struct linux_binprm;
> >>
> >> #ifdef CONFIG_IMA
> >> extern int ima_bprm_check(struct linux_binprm *bprm);
> >> -extern int ima_file_check(struct file *file, int mask);
> >> +extern int ima_file_check(struct file *file, int mask, int opened);
> >> extern void ima_file_free(struct file *file);
> >> extern int ima_file_mmap(struct file *file, unsigned long prot);
> >> extern int ima_module_check(struct file *file);
> >> @@ -26,7 +26,7 @@ static inline int ima_bprm_check(struct linux_binprm *bprm)
> >> return 0;
> >> }
> >>
> >> -static inline int ima_file_check(struct file *file, int mask)
> >> +static inline int ima_file_check(struct file *file, int mask, int opened)
> >> {
> >> return 0;
> >> }
> >> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> >> index 3e9be3d..9337aa9 100644
> >> --- a/security/integrity/ima/ima.h
> >> +++ b/security/integrity/ima/ima.h
> >> @@ -175,7 +175,7 @@ void ima_delete_rules(void);
> >> int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> >> struct file *file, const unsigned char *filename,
> >> struct evm_ima_xattr_data *xattr_value,
> >> - int xattr_len);
> >> + int xattr_len, int opened);
> >> int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func);
> >> void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
> >> enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
> >> @@ -191,7 +191,7 @@ static inline int ima_appraise_measurement(int func,
> >> struct file *file,
> >> const unsigned char *filename,
> >> struct evm_ima_xattr_data *xattr_value,
> >> - int xattr_len)
> >> + int xattr_len, int opened)
> >> {
> >> return INTEGRITY_UNKNOWN;
> >> }
> >> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> >> index 3a4beb3..10679c8 100644
> >> --- a/security/integrity/ima/ima_appraise.c
> >> +++ b/security/integrity/ima/ima_appraise.c
> >> @@ -175,7 +175,7 @@ int ima_read_xattr(struct dentry *dentry,
> >> int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> >> struct file *file, const unsigned char *filename,
> >> struct evm_ima_xattr_data *xattr_value,
> >> - int xattr_len)
> >> + int xattr_len, int opened)
> >> {
> >> static const char op[] = "appraise_data";
> >> char *cause = "unknown";
> >> @@ -195,7 +195,7 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> >>
> >> cause = "missing-hash";
> >> status = INTEGRITY_NOLABEL;
> >> - if (inode->i_size == 0) {
> >> + if (opened & FILE_CREATED) {
> >> iint->flags |= IMA_NEW_FILE;
> >> status = INTEGRITY_PASS;
> >> }
> >> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> >> index 5a870e7..3384036 100644
> >> --- a/security/integrity/ima/ima_main.c
> >> +++ b/security/integrity/ima/ima_main.c
> >> @@ -157,7 +157,7 @@ void ima_file_free(struct file *file)
> >> }
> >>
> >> static int process_measurement(struct file *file, const char *filename,
> >> - int mask, int function)
> >> + int mask, int function, int opened)
> >> {
> >> struct inode *inode = file_inode(file);
> >> struct integrity_iint_cache *iint;
> >> @@ -238,7 +238,7 @@ static int process_measurement(struct file *file, const char *filename,
> >> if (action & IMA_APPRAISE_SUBMASK) {
> >> mutex_lock(&inode->i_mutex);
> >> rc = ima_appraise_measurement(_func, iint, file, pathname,
> >> - xattr_value, xattr_len);
> >> + xattr_value, xattr_len, opened);
> >> mutex_unlock(&inode->i_mutex);
> >> }
> >> if (action & IMA_AUDIT)
> >> @@ -269,7 +269,7 @@ out_unlocked:
> >> int ima_file_mmap(struct file *file, unsigned long prot)
> >> {
> >> if (file && (prot & PROT_EXEC))
> >> - return process_measurement(file, NULL, MAY_EXEC, MMAP_CHECK);
> >> + return process_measurement(file, NULL, MAY_EXEC, MMAP_CHECK, 0);
> >> return 0;
> >> }
> >>
> >> @@ -291,7 +291,7 @@ int ima_bprm_check(struct linux_binprm *bprm)
> >> return process_measurement(bprm->file,
> >> (strcmp(bprm->filename, bprm->interp) == 0) ?
> >> bprm->filename : bprm->interp,
> >> - MAY_EXEC, BPRM_CHECK);
> >> + MAY_EXEC, BPRM_CHECK, 0);
> >> }
> >>
> >> /**
> >> @@ -304,12 +304,12 @@ int ima_bprm_check(struct linux_binprm *bprm)
> >> * On success return 0. On integrity appraisal error, assuming the file
> >> * is in policy and IMA-appraisal is in enforcing mode, return -EACCES.
> >> */
> >> -int ima_file_check(struct file *file, int mask)
> >> +int ima_file_check(struct file *file, int mask, int opened)
> >> {
> >> ima_rdwr_violation_check(file);
> >> return process_measurement(file, NULL,
> >> mask & (MAY_READ | MAY_WRITE | MAY_EXEC),
> >> - FILE_CHECK);
> >> + FILE_CHECK, opened);
> >> }
> >> EXPORT_SYMBOL_GPL(ima_file_check);
> >>
> >> @@ -332,7 +332,7 @@ int ima_module_check(struct file *file)
> >> #endif
> >> return 0; /* We rely on module signature checking */
> >> }
> >> - return process_measurement(file, NULL, MAY_EXEC, MODULE_CHECK);
> >> + return process_measurement(file, NULL, MAY_EXEC, MODULE_CHECK, 0);
> >> }
> >>
> >> static int __init init_ima(void)
> >
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists