[Skiboot] [PATCH 3/4] secvar: return error if validate_esl has extra data

[Daniel Axtens]

Nick Child <nnac123 at gmail.com> writes:

> Currently, in `validate_esl_list`, the return code is initialized to zero
> (our success value). While looping though the ESL's in the submitted ESL
> chain, the loop will break if there is not enough data to meet minimum ESL
> requirements. This condition was not setting a return code, meaning that the
> successful return code can pass to the end of the function if there is extra
> data at the end of the ESL. As a consequence, any properly signed update can
> successfully commit any data (as long as it is less than the min size of an
> ESL) to the secvars.

Ironically if we did not have the test, get_esl_signature_list would
return NULL and then we _would_ return OPAL_PARAMETER. There is perhaps
an argument for inlining and incorporating get_esl_signature_list
directly into all of the call sites and eliminating any duplicate tests
that result...

> This commit will return an error if the described condition is met. This
> means all data in the appended ESL of an auth file must be accounted for. No
> extra bytes can be added to the end since, on success, this data will become
> the updated secvar.
>
> Additionally, a test case has been added to ensure that this commit
> addresses the issue correctly.
>
> Signed-off-by: Nick Child <nick.child at ibm.com>
> ---
>  libstb/secvar/backend/edk2-compat-process.c  |  5 ++++-
>  libstb/secvar/test/secvar-test-edk2-compat.c | 15 +++++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
> index d12d1134..5cb54c95 100644
> --- a/libstb/secvar/backend/edk2-compat-process.c
> +++ b/libstb/secvar/backend/edk2-compat-process.c
> @@ -265,8 +265,11 @@ int validate_esl_list(const char *key, const char *esl, const size_t size)
>  
>  	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)) {
> +			prlog(PR_ERR, "ESL has %d unknown extra bytes\n", eslvarsize);
> +			rc = OPAL_PARAMETER;
>  			break;
> +		}
>

I wondered is this test was adequate. What happens, for example, if
eslvarsize == sizeof(EFI_SIGNATURE_LIST) or is just a little bit bigger?

It looks to me like you will compute eslsize and then pass it down with
eslvarsize to get_esl_cert(). get_esl_cert() will eventually fail. So I
think that's OK.

I would perhaps think it wise to change the eslsize test
though. Currently it reads:

		/* Calculate the size of the ESL */
		eslsize = le32_to_cpu(list->SignatureListSize);

		/* If could not extract the size */
		if (eslsize <= 0) {
			prlog(PR_ERR, "Invalid size of the ESL: %u\n",
					le32_to_cpu(list->SignatureListSize));
			rc = OPAL_PARAMETER;
			break;
		}

This is odd in a few ways: eslsize is an int but signaturelistsize does
not appear to be signed. Then we are testing if it's less than zero,
which would - if the underlying SignatureListSize is unsigned - occur
when the stated SignatureListSize is above 2GB. le32_to_cpu will not
fail with a negative error code. I am not aware of any way we could fail
to extract the size in any other way given that we've successfully
extracted `list`.

I would be more comfortable if we made eslsize a u32, and we checked if
eslsize > eslvarsize.

I think it could probably also be shoehorned into this patch, but it
could I guess go in another patch, perhaps even done later...

>  		/* Check Supported ESL Type */
>  		list = get_esl_signature_list(esl + offset, eslvarsize);
> diff --git a/libstb/secvar/test/secvar-test-edk2-compat.c b/libstb/secvar/test/secvar-test-edk2-compat.c
> index 93ade920..0e1a8211 100644
> --- a/libstb/secvar/test/secvar-test-edk2-compat.c
> +++ b/libstb/secvar/test/secvar-test-edk2-compat.c
> @@ -165,6 +165,21 @@ int run_test()
>  	ASSERT(5 == list_length(&variable_bank));
>  	ASSERT(setup_mode);
>  
> +	/* Add PK with bad ESL. should fail since data is not big enough to be ESL*/
> +	printf("Add PK with invalid appended ESL");
> +	/* 1014 is length of appended ESL */
> +	tmp = new_secvar("PK", 3, PK_auth, PK_auth_len - 1014 + sizeof(EFI_SIGNATURE_LIST) - 1, 0);

I'm not quite sure what the calculation here does. So if I remember all
the pieces correctly (and this is off the top of my head), PK_auth
contains:

auth header || pkcs7 message || esl header    || esl data
40 bytes    || ?? bytes      || sizeof(E_S_L) || 1014? bytes

I'm not sure if 1014 contains the esl header or not, I'm guessing it
does, because if so your calculation puts the buffer end just inside of
the esl header. If it doesn't, your calculation puts it inside the esl
data which I think would not be caught by this commit.

Perhaps clarifying the comment to say that 1014 contains both the header
and the data would be helpful.

Kind regards,
Daniel Axtens