[Skiboot] [PATCH] secvar/backend: require sha256 in our PKCS#7 messages

[Nayna]

On 6/22/21 10:22 AM, Daniel Axtens wrote:
> We only handle sha256 hashes in auth structures.
>
> In the process of verifying an auth structure, we extract the pkcs7
> message and we calculate the hopefully-matching hash, which is
> sha256(name || vendor guid || attributes || timestamp || newcontent)
> We then verify that the PKCS#7 signature matches that calculated hash.
>
> However, at no point do we check that the PKCS#7 hash algorithm is
> sha256. So if the PKCS#7 message says that it is a signature on a sha512,
> mbedtls will compare 64 bytes of hash from the signature with 64 bytes
> from our hash, resulting in a 32 byte overread.
>
> Verify that the hash algorithm in the PKCS#7 message is sha256.
>
> Signed-off-by: Daniel Axtens <dja at axtens.net>
>
> ---
>
> This is the minimal fix for the underlying bug. It should probably
> go in ahead of any potential future reworking of the area.
>
> As always, compile tested only because I don't have access to a box
> set up to test this.
> ---
>   libstb/secvar/backend/edk2-compat-process.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
> index 244f23403fe0..657100b6a021 100644
> --- a/libstb/secvar/backend/edk2-compat-process.c
> +++ b/libstb/secvar/backend/edk2-compat-process.c
> @@ -11,6 +11,7 @@
>   #include <stdint.h>
>   #include <ccan/endian/endian.h>
>   #include <mbedtls/error.h>
> +#include <mbedtls/oid.h>
>   #include <device.h>
>   #include <assert.h>
>   #include "libstb/crypto/pkcs7/pkcs7.h"
> @@ -460,6 +461,7 @@ static int verify_signature(const struct efi_variable_authentication_2 *auth,
>   {
>   	mbedtls_pkcs7 *pkcs7 = NULL;
>   	mbedtls_x509_crt x509;
> +	mbedtls_md_type_t md_alg;
>   	char *signing_cert = NULL;
>   	char *x509_buf = NULL;
>   	int signing_cert_size;
> @@ -478,6 +480,18 @@ static int verify_signature(const struct efi_variable_authentication_2 *auth,
>   	if (!pkcs7)
>   		return OPAL_PARAMETER;
>   
> +	/*
> +	 * We only support sha256, which has a hash length of 32.
> +	 * If the alg is not sha256, then we should bail now.
> +	 */
> +	rc = mbedtls_oid_get_md_alg(&pkcs7->signed_data.digest_alg_identifiers,
> +				    &md_alg);
> +	if (rc != 0)
> +		return OPAL_PARAMETER;
> +
> +	if (md_alg != MBEDTLS_MD_SHA256)
> +		return OPAL_PARAMETER;

Thanks Daniel !! Really sorry for missing to mention first time. I think 
it would be useful to have error message for this failure as 
OPAL_PARAMETER might mean many other types of failures as well.

Thanks & Regards,

       - Nayna