Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 5 Apr 2022 19:11:48 +0000
From:   Eric Biggers <ebiggers@kernel.org>
To:     Mimi Zohar <zohar@linux.ibm.com>
Cc:     linux-integrity@vger.kernel.org,
        Stefan Berger <stefanb@linux.ibm.com>,
        linux-fscrypt@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 2/5] ima: define a new template field named 'd-ngv2'
 and templates
Message-ID: <YkyUdM5hmKf4noS7@gmail.com>
References: <20220325223824.310119-1-zohar@linux.ibm.com>
 <20220325223824.310119-3-zohar@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220325223824.310119-3-zohar@linux.ibm.com>
Precedence: bulk

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