[Skiboot] [PATCH 4/4] secvar: return error if verify_signature runs out of ESLs

[Daniel Axtens]

Nick Child <nnac123 at gmail.com> writes:

> Currently, in `verify_signature`, the return code `rc` is initialized
> as 0 (our success value). While looping through the ESL's in the given

Should we maybe not do that then? Maybe we should initialise it to some
sort of internal error code, or leave it unintialised and turn on
-Wuninitialized or some such. This patch, while it does fix a problem,
arguably leaves the underlying foot-gun around to cause us trouble
another day.

> secvar, the function will break if the remaining data in the secvar is
> not enough to contain another ESL. This break from the loop was not
> setting a return code, this means that the successful return code
> can pass to the end of the function if the first iteration meets
> this condition. In other words, if a current secvar has a size that
> is less than minimum size for an ESL, than it will approve any update.
>
> In response to this bug, this commit will return an error code if
> the described condition is met. Additionally, a test case has been
> added to ensure that this unlikely event is handled correctly.
>
> Signed-off-by: Nick Child <nick.child at ibm.com>
> ---
>
> Notice that the successful return code wil still pass through if 
> the variable length is 0. I did not patch this condition because it 
> is caught by its calling function `process_update`.
>
> ---
>  libstb/secvar/backend/edk2-compat-process.c  |  5 +++-
>  libstb/secvar/test/secvar-test-edk2-compat.c | 25 ++++++++++++++++++++
>  2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
> index 5cb54c95..0ed275bb 100644
> --- a/libstb/secvar/backend/edk2-compat-process.c
> +++ b/libstb/secvar/backend/edk2-compat-process.c
> @@ -490,8 +490,11 @@ static int verify_signature(const struct efi_variable_authentication_2 *auth,
>  	/* Variable is not empty */
>  	while (eslvarsize > 0) {
>  		prlog(PR_DEBUG, "esl var size size is %d offset is %d\n", eslvarsize, offset);
> -		if (eslvarsize < sizeof(EFI_SIGNATURE_LIST))
> +		if (eslvarsize < sizeof(EFI_SIGNATURE_LIST)) {
> +			rc = OPAL_INTERNAL_ERROR;
> +			prlog(PR_ERR, "ESL data is corrupted\n");
>  			break;
> +		}
>

This function also suffers from a different but related quirkyness
around esl size, the get_esl_signature_list_size returns -1 or
le32_to_cpu(signature list size), again it would be better to inline
this, make the types the same signedness and test if claimed size > size
remaining. Again, not sure quite how we want to handle this in terms of
bug fixes vs code improvements.

Kind regards,
Daniel

>  		/* Calculate the size of the ESL */
>  		eslsize = get_esl_signature_list_size(avar->data + offset,
> diff --git a/libstb/secvar/test/secvar-test-edk2-compat.c b/libstb/secvar/test/secvar-test-edk2-compat.c
> index 0e1a8211..62dcae80 100644
> --- a/libstb/secvar/test/secvar-test-edk2-compat.c
> +++ b/libstb/secvar/test/secvar-test-edk2-compat.c
> @@ -89,6 +89,7 @@ int run_test()
>  {
>  	int rc = -1;
>  	struct secvar *tmp;
> +	size_t tmp_size;
>  	char empty[64] = {0};
>  
>  	/* The sequence of test cases here is important to ensure that
> @@ -213,6 +214,30 @@ int run_test()
>  	tmp = find_secvar("db", 3, &variable_bank);
>  	ASSERT(NULL != tmp);
>  
> +	/* Add db, should fail with no KEK and invalid PK size */
> +	printf("Add db, corrupt PK");
> +	/* Somehow PK gets assigned wrong size */
> +	tmp = find_secvar("PK", 3, &variable_bank);
> +	ASSERT(NULL != tmp);
> +	tmp_size = tmp->data_size;
> +	tmp->data_size = sizeof(EFI_SIGNATURE_LIST) - 1;
> +	tmp = new_secvar("db", 3, DB_auth, DB_auth_len, 0);
> +	ASSERT(0 == edk2_compat_validate(tmp));
> +	list_add_tail(&update_bank, &tmp->link);
> +	ASSERT(1 == list_length(&update_bank));
> +
> +	rc = edk2_compat_process(&variable_bank, &update_bank);
> +	ASSERT(OPAL_INTERNAL_ERROR == rc);
> +	ASSERT(5 == list_length(&variable_bank));
> +	ASSERT(0 == list_length(&update_bank));
> +	tmp = find_secvar("db", 3, &variable_bank);
> +	ASSERT(NULL != tmp);
> +	ASSERT(0 == tmp->data_size);
> +	/* Restore PK data size */
> +	tmp = find_secvar("PK", 3, &variable_bank);
> +	ASSERT(NULL != tmp);
> +	tmp->data_size = tmp_size;
> +
>  	/* Add trimmed KEK, .process(), should fail. */
>  	printf("Add trimmed KEK\n");
>  	tmp = new_secvar("KEK", 4, trimmedKEK_auth, trimmedKEK_auth_len, 0);
> -- 
> 2.17.1