Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1f76429bfe89ee37a19f2d8ad0cf1a31ff9da798.camel@linux.ibm.com>
Subject: Re: [PATCH v7 2/5] ima: define a new template field named 'd-ngv2'
 and templates
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     linux-integrity@vger.kernel.org,
        Stefan Berger <stefanb@linux.ibm.com>,
        linux-fscrypt@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Wed, 27 Apr 2022 22:03:51 -0400
In-Reply-To: <YkyUdM5hmKf4noS7@gmail.com>
References: <20220325223824.310119-1-zohar@linux.ibm.com>
         <20220325223824.310119-3-zohar@linux.ibm.com> <YkyUdM5hmKf4noS7@gmail.com>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 2022-04-05 at 19:11 +0000, Eric Biggers wrote:
> 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.

Agreed.

> 
> > +	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.

Using "%*s" prevents having a trailing NULL.

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

Violation records contain 0's in the file hash and the template data
hash fields. Changing the default hash algorithm would result in larger
violation digests without any real benefit other than cosmetic.  Will
make the change anyway in the next version.

thanks,

Mimi