linux-kernel - Re: [PATCH 7/8] ima: remove usage of filename parameter

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <54071778.8050204@samsung.com>
Date:	Wed, 03 Sep 2014 16:28:24 +0300
From:	Dmitry Kasatkin <d.kasatkin@...sung.com>
To:	Mimi Zohar <zohar@...ux.vnet.ibm.com>
Cc:	linux-ima-devel@...ts.sourceforge.net,
	linux-security-module@...r.kernel.org,
	linux-kernel@...r.kernel.org, dmitry.kasatkin@...il.com
Subject: Re: [PATCH 7/8] ima: remove usage of filename parameter

On 03/09/14 16:16, Mimi Zohar wrote:
> On Wed, 2014-09-03 at 10:20 +0300, Dmitry Kasatkin wrote: 
>> In all cases except ima_bprm_check() filename was not defined and
>> ima_d_path() was used to find full path.
>>
>> ima_bprm_check() used to select between bprm->interp and bprm->filename.
>> Following dump demonstrates differences between using filename and interp.
>>
>> bprm->filename
>>  filename: ./foo.sh, pathname: /root/bin/foo.sh
>>  filename: ./foo.sh, pathname: /bin/dash
>>
>> bprm->interp
>>  filename: ./foo.sh, pathname: /root/bin/foo.sh
>>  filename: /bin/sh, pathname: /bin/dash
>>
>> In both cases pathnames are the same.
>> This patch removes usage of filename and interp in favor of d_path.
>>
>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@...sung.com>
> Thanks, this has been on my list to do.  My only concern is whether we
> should be using d_path() or one of the other variants (eg.
> dentry_path(), d_absolute_path()).  For namespaces, we would want to be
> able to differentiate the files.
>
> Please include in this patch description why d_path(), if it is the
> case, the best option.
>
> thanks,

Hi,

Actually as we discussed, we can also in this patch change ima_d_path to
use d_absolute_path().
It will work for "chroot" cases and will show real path...

Should I switch to 'd_absolute_path'?

In the case of namespaces, neither d_path nor d_absolute_path works....
Usage of dentry_path() would eliminate mount tree and requires device
prefix.
But it will 'break' clients, reading process measurement list.
That would require essentially more agreement.

- Dmitry


> Mimi
>
>> ---
>>  security/integrity/ima/ima_main.c | 19 ++++++++-----------
>>  1 file changed, 8 insertions(+), 11 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index aaf5552..673a37e 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -156,8 +156,8 @@ void ima_file_free(struct file *file)
>>  	ima_check_last_writer(iint, inode, file);
>>  }
>>
>> -static int process_measurement(struct file *file, const char *filename,
>> -			       int mask, int function, int opened)
>> +static int process_measurement(struct file *file, int mask, int function,
>> +			       int opened)
>>  {
>>  	struct inode *inode = file_inode(file);
>>  	struct integrity_iint_cache *iint;
>> @@ -218,7 +218,7 @@ static int process_measurement(struct file *file, const char *filename,
>>  		goto out_digsig;
>>  	}
>>
>> -	pathname = filename ?: ima_d_path(&file->f_path, &pathbuf);
>> +	pathname = ima_d_path(&file->f_path, &pathbuf);
>>
>>  	if (action & IMA_MEASURE)
>>  		ima_store_measurement(iint, file, pathname,
>> @@ -254,7 +254,7 @@ out:
>>  int ima_file_mmap(struct file *file, unsigned long prot)
>>  {
>>  	if (file && (prot & PROT_EXEC))
>> -		return process_measurement(file, NULL, MAY_EXEC, MMAP_CHECK, 0);
>> +		return process_measurement(file, MAY_EXEC, MMAP_CHECK, 0);
>>  	return 0;
>>  }
>>
>> @@ -273,10 +273,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)
>>   */
>>  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, 0);
>> +	return process_measurement(bprm->file, MAY_EXEC, BPRM_CHECK, 0);
>>  }
>>
>>  /**
>> @@ -292,7 +289,7 @@ int ima_bprm_check(struct linux_binprm *bprm)
>>  int ima_file_check(struct file *file, int mask, int opened)
>>  {
>>  	ima_rdwr_violation_check(file);
>> -	return process_measurement(file, NULL,
>> +	return process_measurement(file,
>>  				   mask & (MAY_READ | MAY_WRITE | MAY_EXEC),
>>  				   FILE_CHECK, opened);
>>  }
>> @@ -317,7 +314,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, 0);
>> +	return process_measurement(file, MAY_EXEC, MODULE_CHECK, 0);
>>  }
>>
>>  int ima_fw_from_file(struct file *file, char *buf, size_t size)
>> @@ -328,7 +325,7 @@ int ima_fw_from_file(struct file *file, char *buf, size_t size)
>>  			return -EACCES;	/* INTEGRITY_UNKNOWN */
>>  		return 0;
>>  	}
>> -	return process_measurement(file, NULL, MAY_EXEC, FIRMWARE_CHECK, 0);
>> +	return process_measurement(file, MAY_EXEC, FIRMWARE_CHECK, 0);
>>  }
>>
>>  static int __init init_ima(void)
>
>

--
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/