[Skiboot] [PATCH 2/3] secvar/backend: clarify variables in process_update

[Nayna]

On 5/24/21 11:34 PM, Daniel Axtens wrote:
> process_update() has tbhbuffer and tbhbuffersize. However, tbhbuffer
> doesn't contain to-be-hashed data, but a hash:
>
> /* Prepare the data to be verified */
> tbhbuffer = get_hash_to_verify(update->key, *newesl, *new_data_size,
> 			timestamp);
>
> And tbhbuffersize is initalised to zero and then never filled with
> the actual length, so 0 is passed through to verify_signature.
> verify_signature will in turn pass that to mbedtls, which will interpret
> it as "figure out the length yourself based on the hash type". So this
> has always worked, but by accident.
>
> Rename tbhbuffer to hash, as that's what it is.
>
> Drop tbhbuffersize, and pass 32 directly to verify_signature. We only
> support SHA-256, and SHA-256 is 32 bytes long.
>
> Signed-off-by: Daniel Axtens <dja at axtens.net>

Thanks Daniel for identifying and fixing it.


> ---
>   libstb/secvar/backend/edk2-compat-process.c | 14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
> index 8324dc068b8e..b39f35e0c3dc 100644
> --- a/libstb/secvar/backend/edk2-compat-process.c
> +++ b/libstb/secvar/backend/edk2-compat-process.c
> @@ -662,8 +662,7 @@ int process_update(const struct secvar *update, char **newesl,
>   	void *auth_buffer = NULL;
>   	int auth_buffer_size = 0;
>   	const char *key_authority[3];
> -	char *tbhbuffer = NULL;
> -	size_t tbhbuffersize = 0;
> +	char *hash = NULL;
>   	struct secvar *avar = NULL;
>   	int rc = 0;
>   	int i;
> @@ -723,9 +722,9 @@ int process_update(const struct secvar *update, char **newesl,
>   	}
>   
>   	/* Prepare the data to be verified */
> -	tbhbuffer = get_hash_to_verify(update->key, *newesl, *new_data_size,
> +	hash = get_hash_to_verify(update->key, *newesl, *new_data_size,
>   				timestamp);
> -	if (!tbhbuffer) {
> +	if (!hash) {
>   		rc = OPAL_INTERNAL_ERROR;
>   		goto out;
>   	}
> @@ -746,9 +745,8 @@ int process_update(const struct secvar *update, char **newesl,
>   		if (!avar || !avar->data_size)
>   			continue;
>   
> -		/* Verify the signature */
> -		rc = verify_signature(auth, tbhbuffer, tbhbuffersize,
> -				      avar);
> +		/* Verify the signature. sha256 is 32 bytes long. */
> +		rc = verify_signature(auth, hash, 32, avar);

This does work. I am thinking that we still pass here zero like it was 
previously and fix the documentation of 
mbedtls_pkcs7_signed_hash_verify() in pkcs7.h.

The documentation makes it explicit that caller can pass 0 or 
sizeof(hash).  This will also make it consistent with general behaviour 
of mbedtls API where it allows to pass zero and figures out by itself.

Thanks & Regards,

     - Nayna