linux-kernel - Re: [PATCH v7 2/5] ima: define a new template field named 'd-ngv2' and templates

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]

Date:   Tue, 5 Apr 2022 19:11:48 +0000
From:   Eric Biggers <ebiggers@...nel.org>
To:     Mimi Zohar <zohar@...ux.ibm.com>
Cc:     linux-integrity@...r.kernel.org,
        Stefan Berger <stefanb@...ux.ibm.com>,
        linux-fscrypt@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 2/5] ima: define a new template field named 'd-ngv2'
 and templates

On Fri, Mar 25, 2022 at 06:38:21PM -0400, Mimi Zohar wrote:
>  static int ima_eventdigest_init_common(const u8 *digest, u32 digestsize,
> -				       u8 hash_algo,
> +				       u8 digest_type, u8 hash_algo,
>  				       struct ima_field_data *field_data)
>  {
>  	/*
>  	 * digest formats:
>  	 *  - DATA_FMT_DIGEST: digest
>  	 *  - DATA_FMT_DIGEST_WITH_ALGO: [<hash algo>] + ':' + '\0' + digest,
> +	 *  - DATA_FMT_DIGEST_WITH_TYPE_AND_ALGO:
> +	 *	[<digest type> + ':' + <hash algo>] + ':' + '\0' + digest,
> +	 *    where <hash type> is either "ima" or "verity",
>  	 *    where <hash algo> is provided if the hash algorithm is not
>  	 *    SHA1 or MD5

This says both "hash type" and "digest type".  It should be one or the other.

The square brackets are meant to indicate that the part within it is optional,
right?  Are they in the right place?  I don't see how this matches the code.
There is also no explanation for why or when <digest type> is optional with
DATA_FMT_DIGEST_WITH_TYPE_AND_ALGO.

> +	if (digest_type < DIGEST_TYPE__LAST && hash_algo < HASH_ALGO__LAST) {
> +		fmt = DATA_FMT_DIGEST_WITH_TYPE_AND_ALGO;
> +		offset += snprintf(buffer, DIGEST_TYPE_NAME_LEN_MAX +
> +				   CRYPTO_MAX_ALG_NAME + 1, "%*s:%s",
> +				   (int)strlen(digest_type_name[digest_type]),
> +				   digest_type_name[digest_type],
>  				   hash_algo_name[hash_algo]);
>  		buffer[offset] = ':';
>  		offset += 2;

There's no need to use %*s if the length argument is just going to be strlen().
It should just use %s.

Also, this is not correct use of snprintf(), given that the string is
unconditionally appended to at the offset which snprintf() returns.  So it is
not providing buffer overflow protection.  It might as well just be:

                offset += 1 + sprintf(buffer, "%s:%s:",
                                      digest_type_name[digest_type],
                                      hash_algo_name[hash_algo]);

and likewise for the other case:

                offset += 1 + sprintf(buffer, "%s:", hash_algo_name[hash_algo]);

> +/*
> + * This function writes the digest of an event (without size limit),
> + * prefixed with both the hash type and algorithm.
> + */
> +int ima_eventdigest_ngv2_init(struct ima_event_data *event_data,
> +			      struct ima_field_data *field_data)
> +{
> +	u8 *cur_digest = NULL, hash_algo = HASH_ALGO_SHA1;

Why is this defaulting to SHA-1?

- Eric