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

[Daniel Axtens]

Nick Child <nnac123 at gmail.com> writes:

> Currently, `get_esl_cert` receives a data buffer containing an ESL and it's
(nit: its, not "it's")

> 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, a conditional is has been added to ensure that the ESL

Another nit: "conditional is has been" -> "conditional is", or even
better, simply "ensure that ..." - see
https://www.kernel.org/doc/html/v4.10/process/submitting-patches.html#describe-your-changes

> 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>
> ---
>  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;
> +	}
>  
>  	*cert = zalloc(size);
>  	if (!(*cert))
> diff --git a/libstb/secvar/test/data/trimmedKEK.h b/libstb/secvar/test/data/trimmedKEK.h
> new file mode 100644
> index 00000000..6600d254
> --- /dev/null
> +++ b/libstb/secvar/test/data/trimmedKEK.h
> @@ -0,0 +1,161 @@
> +unsigned char trimmedKEK_auth[]  = {
> +0xe4 ,0x07 ,0x09 ,0x0e ,0x0e ,0x22 ,0x2e ,0x00  ,0x00 ,0x00 ,0x00 ,0x00 ,0x00 ,0x00 ,0x00 ,0x00
> +,0xd3 ,0x05 ,0x00 ,0x00 ,0x00 ,0x02 ,0xf1 ,0x0e  ,0x9d ,0xd2 ,0xaf ,0x4a ,0xdf ,0x68 ,0xee ,0x49

How are you creating these? I ask because the commas are in weird
spots. 

FWIW I use 'xxd -i foo > foo.h'.

>  
> +	/* Add trimmed KEK, .process(), should fail. */
> +	printf("Add trimmed KEK\n");
> +	tmp = new_secvar("KEK", 4, trimmedKEK_auth, trimmedKEK_auth_len, 0);
> +	ASSERT(0 == edk2_compat_validate(tmp));

Intuitively it seems edk2_compat_validate() should catch this, but I see
looking at the code that _validate() currently simply validates that the
variable being updated is one of the recognised SB vars and that the
message is PKCS#7, so I guess there must be historical reasons for the
split.

All up, I have nits but none of them rise to a level that should prevent
this code from being merged. As such:

Reviewed-by: Daniel Axtens <dja at axtens.net>

Kind regards,
Daniel