On 18/03/24 14:05, Mimi Zohar wrote:
> On Fri, 2024-03-08 at 11:49 +0100, Enrico Bravi wrote:
>> The template hash showed by the ascii_runtime_measurements and
>> binary_runtime_measurements is the one calculated using sha1 and there is
>> no possibility to change this value, despite the fact that the template
>> hash is calculated using the hash algorithms corresponding to all the PCR
>> banks configured in the TPM.
>>
>> Add the support to retrieve the ima log with the template data hash
>> calculated with a specific hash algorithm.
>> Add a new file in the securityfs ima directory for each hash algo
>> configured in a PCR bank of the TPM. Each new file has the name with
>> the following structure:
>>
>> {binary, ascii}_runtime_measurements_<hash_algo_name>
>>
>> Legacy files are kept, to avoid breaking existing applications, but as
>> symbolic links which point to {binary, ascii}_runtime_measurements_sha1
>> files. These two files are created even if a TPM chip is not detected or
>> the sha1 bank is not configure in the TPM.
>>
>> As example, in the case a TPM chip is present and sha256 is the only
>> configured PCR bank, the listing of the security/ima directory is the
>> following:
>>
>> lr--r--r-- [...] ascii_runtime_measurements -> ascii_runtime_measurements_sha1
>> -r--r----- [...] ascii_runtime_measurements_sha1
>> -r--r----- [...] ascii_runtime_measurements_sha256
>> lr--r--r-- [...] binary_runtime_measurements ->
>> binary_runtime_measurements_sha1
>> -r--r----- [...] binary_runtime_measurements_sha1
>> -r--r----- [...] binary_runtime_measurements_sha256
>> --w------- [...] policy
>> -r--r----- [...] runtime_measurements_count
>> -r--r----- [...] violations
>>
>> Signed-off-by: Enrico Bravi <enrico.bravi@xxxxxxxxx>
>> Signed-off-by: Silvia Sisinni <silvia.sisinni@xxxxxxxxx>
>>
>> ---
>>
>> v4:
>> - Added NULL check on m->file for measurements list dump called by
>> ima_dump_measurement_list() on kexec.
>> - Exported ima_algo_array and struct ima_algo_desc declaration from
>> ima_crypto.c to access this information in ima_fs.c.
>> - Added ima_measurements_files_count global variable to avoid extra
>> logic each time the number of measurements file is needed.
>>
>> v3:
>> - Added create_measurements_list_files function for measurements files
>> creation.
>> - Parameterized the remove_measurements_list_files function and add NULL
>> check before freeing files' list.
>> - Removed algorithm selection based on file name during ima_measurements_show
>> and ima_ascii_measurements_show, and selecting it comparing dentry address.
>> - Allocate also sha1 file following the schema
>> {binary, ascii}_runtime_measurements_<hash_algo_name> and keep legacy
>> files as symbolic links to those files.
>> - Allocate measurements files lists even if a TPM chip is not detected,
>> adding only sha1 files.
>>
>> v2:
>> - Changed the behavior of configuring at boot time the template data hash
>> algorithm.
>> - Removed template data hash algo name prefix.
>> - Removed ima_template_hash command line option.
>> - Introducing a new file in the securityfs ima subdir for each PCR banks
>> algorithm configured in the TPM.
>> (suggested by Roberto)
>>
>>
>> security/integrity/ima/ima.h | 9 ++
>> security/integrity/ima/ima_crypto.c | 7 +-
>> security/integrity/ima/ima_fs.c | 135 +++++++++++++++++++++++++---
>> 3 files changed, 131 insertions(+), 20 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index c29db699c996..81318e294175 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -54,6 +54,15 @@ extern int ima_hash_algo __ro_after_init;
>> extern int ima_sha1_idx __ro_after_init;
>> extern int ima_hash_algo_idx __ro_after_init;
>> extern int ima_extra_slots __ro_after_init;
>> +
>> +/* export hash algorithms configured in ima */
>> +struct ima_algo_desc {
>> + struct crypto_shash *tfm;
>> + enum hash_algo algo;
>> +};
>> +
>> +extern struct ima_algo_desc *ima_algo_array;
>> +
>> extern int ima_appraise;
>> extern struct tpm_chip *ima_tpm_chip;
>> extern const char boot_aggregate_name[];
>> diff --git a/security/integrity/ima/ima_crypto.c
>> b/security/integrity/ima/ima_crypto.c
>> index 51ad29940f05..d42ea0d350a1 100644
>> --- a/security/integrity/ima/ima_crypto.c
>> +++ b/security/integrity/ima/ima_crypto.c
>> @@ -57,11 +57,6 @@ MODULE_PARM_DESC(ahash_bufsize, "Maximum ahash buffer
>> size");
>> static struct crypto_shash *ima_shash_tfm;
>> static struct crypto_ahash *ima_ahash_tfm;
>>
>> -struct ima_algo_desc {
>> - struct crypto_shash *tfm;
>> - enum hash_algo algo;
>> -};
>> -
>> int ima_sha1_idx __ro_after_init;
>> int ima_hash_algo_idx __ro_after_init;
>> /*
>> @@ -70,7 +65,7 @@ int ima_hash_algo_idx __ro_after_init;
>> */
>> int ima_extra_slots __ro_after_init;
>>
>> -static struct ima_algo_desc *ima_algo_array;
>> +struct ima_algo_desc *ima_algo_array;
>>
>> static int __init ima_init_ima_crypto(void)
>> {
>> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
>> index cd1683dad3bf..475ab368e32f 100644
>> --- a/security/integrity/ima/ima_fs.c
>> +++ b/security/integrity/ima/ima_fs.c
>> @@ -116,9 +116,13 @@ void ima_putc(struct seq_file *m, void *data, int
>> datalen)
>> seq_putc(m, *(char *)data++);
>> }
>>
>> +static struct dentry **ima_ascii_measurements_files;
>> +static struct dentry **ima_binary_measurements_files;
>
> The variable naming isn't quite right. It's defined as a 'struct dentry', but
> the name is '*_files'. Why not just name the variables 'ima_{ascii, binary}
> _measurements'?
Hi Mimi,
thank you for pointing that out. What do you think of naming them 'ima_{ascii,
binary}_securityfs_measurement_lists', to have also coherence with the names of
the new functions defined.
> Variables that shouldn't change after '__init' should be annotated as
> __ro_after_init;
Thanks we will fix it.
>> +static int ima_measurements_files_count;
>
> Removing '_files', in this case, from the variable name could be confused with
> the number of measurements. Perhaps rename the variable to
> ima_measurement_list_count or ima_securityfs_measurement_list_count.
I think that 'ima_securityfs_measurement_list_count' is better, just to point
out that is the number of entries in the securityfs.
>> +
>> /* print format:
>> * 32bit-le=pcr#
>> - * char[20]=template digest
>> + * char[n]=template digest
>> * 32bit-le=template name size
>> * char[n]=template name
>> * [eventdata length]
>> @@ -130,9 +134,26 @@ int ima_measurements_show(struct seq_file *m, void *v)
>> struct ima_queue_entry *qe = v;
>> struct ima_template_entry *e;
>> char *template_name;
>> + struct dentry *dentry;
>> u32 pcr, namelen, template_data_len; /* temporary fields */
>> bool is_ima_template = false;
>> - int i;
>> + int i, algo_idx;
>> + enum hash_algo algo;
>> +
>> + algo_idx = ima_sha1_idx;
>> + algo = HASH_ALGO_SHA1;
>> +
>> + if (m->file != NULL) {
>> + dentry = file_dentry(m->file);
>> +
>> + for (i = 0; i < ima_measurements_files_count; i++) {
>> + if (dentry == ima_binary_measurements_files[i]) {
>> + algo_idx = i;
>> + algo = ima_algo_array[i].algo;
>> + break;
>> + }
>> + }
>> + }
>>
>> /* get entry */
>> e = qe->entry;
>> @@ -151,7 +172,7 @@ int ima_measurements_show(struct seq_file *m, void *v)
>> ima_putc(m, &pcr, sizeof(e->pcr));
>>
>> /* 2nd: template digest */
>> - ima_putc(m, e->digests[ima_sha1_idx].digest, TPM_DIGEST_SIZE);
>> + ima_putc(m, e->digests[algo_idx].digest, hash_digest_size[algo]);
>>
>> /* 3rd: template name size */
>> namelen = !ima_canonical_fmt ? strlen(template_name) :
>> @@ -220,7 +241,24 @@ static int ima_ascii_measurements_show(struct seq_file
>> *m, void *v)
>> struct ima_queue_entry *qe = v;
>> struct ima_template_entry *e;
>> char *template_name;
>> - int i;
>> + struct dentry *dentry;
>> + int i, algo_idx;
>> + enum hash_algo algo;
>> +
>> + algo_idx = ima_sha1_idx;
>> + algo = HASH_ALGO_SHA1;
>> +
>> + if (m->file != NULL) {
>> + dentry = file_dentry(m->file);
>> +
>> + for (i = 0; i < ima_measurements_files_count; i++) {
>> + if (dentry == ima_ascii_measurements_files[i]) {
>> + algo_idx = i;
>> + algo = ima_algo_array[i].algo;
>> + break;
>> + }
>> + }
>> + }
>>
>> /* get entry */
>> e = qe->entry;
>> @@ -233,8 +271,8 @@ static int ima_ascii_measurements_show(struct seq_file *m,
>> void *v)
>> /* 1st: PCR used (config option) */
>> seq_printf(m, "%2d ", e->pcr);
>>
>> - /* 2nd: SHA1 template hash */
>> - ima_print_digest(m, e->digests[ima_sha1_idx].digest, TPM_DIGEST_SIZE);
>> + /* 2nd: template hash */
>> + ima_print_digest(m, e->digests[algo_idx].digest,
>> hash_digest_size[algo]);
>>
>> /* 3th: template name */
>> seq_printf(m, " %s", template_name);
>> @@ -379,6 +417,71 @@ static const struct seq_operations ima_policy_seqops = {
>> };
>> #endif
>>
>> +static void remove_measurements_list_files(struct dentry **files)
>
> And remove '_files' from the function name. Perhaps rename it
> remove_measurement_lists or remove_securityfs_measurement_lists.
>
>> +{
>> + int i;
>> +
>> + if (files) {
>> + for (i = 0; i < ima_measurements_files_count; i++)
>> + securityfs_remove(files[i]);
>> +
>> + kfree(files);
>> + }
>> +}
>> +
>> +static int create_measurements_list_files(void)
>
> And remove '_files' from the function name. Perhaps rename it to
> create_measurement_lists or create_securityfs_measurement_lists.
I think that keeping this structure for the names
'remove_securityfs_measurement_lists' and 'create_securityfs_measurement_lists'
makes sense.
>> +{
>> + int i;
>> + u16 algo;
>> + char file_name[NAME_MAX+1];
>> + struct dentry *dfile;
>
> Use 'dentry', if needed.>
>> + enum hash_algo sha1_algo_idx = -1;
>> +
>> + ima_measurements_files_count = NR_BANKS(ima_tpm_chip);
>> +
>> + if (ima_sha1_idx >= NR_BANKS(ima_tpm_chip))
>> + ima_measurements_files_count++;
>> +
>> + ima_ascii_measurements_files = kcalloc(ima_measurements_files_count,
>> + sizeof(struct dentry *),
>> GFP_KERNEL);
>> + if (!ima_ascii_measurements_files)
>> + return -ENOMEM;
>> +
>> + ima_binary_measurements_files = kcalloc(ima_measurements_files_count,
>> + sizeof(struct dentry *),
>> GFP_KERNEL);
>> + if (!ima_binary_measurements_files)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < ima_measurements_files_count; i++) {
>> + algo = ima_algo_array[i].algo;
>> +
>> + if (algo == HASH_ALGO_SHA1)
>> + sha1_algo_idx = i;
>> +
>> + sprintf(file_name, "ascii_runtime_measurements_%s",
>> + hash_algo_name[algo]);
>> + dfile = securityfs_create_file(file_name,
>> + S_IRUSR | S_IRGRP, ima_dir, NULL,
>> + &ima_ascii_measurements_ops);
>> + if (IS_ERR(dfile))
>> + return PTR_ERR(dfile);
>> +
>> + ima_ascii_measurements_files[i] = dfile;
>> +
>> + sprintf(file_name, "binary_runtime_measurements_%s",
>> + hash_algo_name[algo]);
>> + dfile = securityfs_create_file(file_name,
>> + S_IRUSR | S_IRGRP, ima_dir, NULL,
>> + &ima_measurements_ops);
>> + if (IS_ERR(dfile))
>> + return PTR_ERR(dfile);
>> +
>> + ima_binary_measurements_files[i] = dfile;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> /*
>> * ima_open_policy: sequentialize access to the policy file
>> */
>> @@ -465,19 +568,21 @@ int __init ima_fs_init(void)
>
> The new functions should be annotated with '__init' as well.
Yes you are right, we will add it.
>> goto out;
>> }
>>
>> - binary_runtime_measurements =
>> - securityfs_create_file("binary_runtime_measurements",
>> - S_IRUSR | S_IRGRP, ima_dir, NULL,
>> - &ima_measurements_ops);
>> + ret = create_measurements_list_files();
>> + if (ret != 0)
>> + goto out;
>> +
>> + binary_runtime_measurements = securityfs_create_symlink(
>> + "binary_runtime_measurements", ima_dir,
>> + "binary_runtime_measurements_sha1", NULL);
>
> Please use scripts/checkpatch.pl to check formatting.
Yes thank you, we will fix the format.
>> if (IS_ERR(binary_runtime_measurements)) {
>> ret = PTR_ERR(binary_runtime_measurements);
>> goto out;
>> }
>>
>> - ascii_runtime_measurements =
>> - securityfs_create_file("ascii_runtime_measurements",
>> - S_IRUSR | S_IRGRP, ima_dir, NULL,
>> - &ima_ascii_measurements_ops);
>> + ascii_runtime_measurements = securityfs_create_symlink(
>
> And here ...
>
> thanks,
Thanks to you for your valuable feedback.
Enrico
> Mimi
>
>> + "ascii_runtime_measurements", ima_dir,
>> + "ascii_runtime_measurements_sha1", NULL);
>> if (IS_ERR(ascii_runtime_measurements)) {
>> ret = PTR_ERR(ascii_runtime_measurements);
>> goto out;
>> @@ -515,6 +620,8 @@ int __init ima_fs_init(void)
>> securityfs_remove(runtime_measurements_count);
>> securityfs_remove(ascii_runtime_measurements);
>> securityfs_remove(binary_runtime_measurements);
>> + remove_measurements_list_files(ima_ascii_measurements_files);
>> + remove_measurements_list_files(ima_binary_measurements_files);
>> securityfs_remove(ima_symlink);
>> securityfs_remove(ima_dir);
>>
>> base-commit: 88035e5694a86a7167d490bb95e9df97a9bb162b
