[Skiboot] [PATCH v2 1/4] secvar: ensure ESL buf size is at least what ESL header expects

[Nayna]

On 6/28/21 3:37 PM, Nick Child wrote:
> Currently, `get_esl_cert` receives a data buffer containing an ESL and its
> length. It is to return a data buffer of the certificate that is contained
> inside the ESL. The ESL has header info that contains the certificates
> `size`  and the size of the header (`sig_data_offset`). We use this
> information to copy `size` bytes starting `sig_data_offset` bytes after the
> given ESL buffer. Currently we are checking that the length of the ESL
> buffer is at least `sig_data_offset` bytes but we are not checking that it
> also has enough bytes to also contain `size` bytes of the certificate. This
> becomes problematic if some data at the end of the ESL gets lost. Since the
> ESL claims it has more than it actually does, this will lead to a buffer
> over-read. What is even worse, is that this buffer over-read can go
> unnoticed since the last 256 bytes of the ESL are usually the x509 2048 bit
> signature so the extra garbage bytes that are copied will appear to be a
> valid rsa signature.
>
> To resolve this, this commit ensures that the ESL buffer length is large
> enough to hold the data that it claims it contains.
>
> Lastly, a new test case is added to test the described condition. It
> includes a new test file `trimmedKEK.h` which contains a struct a valid KEK
> auth file minus 5 bytes, therefore making it invalid.
>
> Signed-off-by: Nick Child <nick.child at ibm.com>
> Reviewed-by: Daniel Axtens <dja at axtens.net>
> ---
>   libstb/secvar/backend/edk2-compat-process.c  |   4 +-
>   libstb/secvar/test/data/trimmedKEK.h         | 161 +++++++++++++++++++
>   libstb/secvar/test/secvar-test-edk2-compat.c |  16 ++
>   3 files changed, 180 insertions(+), 1 deletion(-)
>   create mode 100644 libstb/secvar/test/data/trimmedKEK.h
>
> diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
> index 244f2340..e1101a4c 100644
> --- a/libstb/secvar/backend/edk2-compat-process.c
> +++ b/libstb/secvar/backend/edk2-compat-process.c
> @@ -135,8 +135,10 @@ static int get_esl_cert(const char *buf, const size_t buflen, char **cert)
>   	sig_data_offset = sizeof(EFI_SIGNATURE_LIST)
>   			  + le32_to_cpu(list->SignatureHeaderSize)
>   			  + 16 * sizeof(uint8_t);
> -	if (sig_data_offset > buflen)
> +	if (sig_data_offset + size > buflen) {
> +		prlog(PR_ERR, "Number of bytes of ESL data is less than size specified\n");
>   		return OPAL_PARAMETER;
> +	}


Thanks Nick !!

I think the intent here is not to validate individual ESL against its 
bounds but to validate against user submitted buffer bound. In case of 
multiple ESLs in the buffer, individual ESLs can still be overread until 
the buffer length.

And I think we do not have a way also to identify individual ESL bounds. 
So, this is the only check that we can ensure.

I just think it would be helpful to have a single line comment here to 
make it explicitly clear and also mention in the patch description.

Apart from that..

Reviewed-by: Nayna Jain <nayna at linux.ibm.com>

Tested-by: Nayna Jain <nayna at linux.ibm.com>