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.
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
> 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@xxxxxxxxxxx>
>> ---
>> 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-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
