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

[Nick Child]

Hey Daniel,
I tested this with a sha512 PKCS7 (did not do a full skiboot build though),
it works well but valgrind shows unfree'd memory.
Comments below:

On Tue, Jun 22, 2021 at 10:23 AM Daniel Axtens <dja at axtens.net> 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;
> +

Don't forget to clean up here on error. Both pkcs7 and the struct
should be freed

> +       if (md_alg != MBEDTLS_MD_SHA256)
> +               return OPAL_PARAMETER;
> +

Also cleanup here before returning.

Hope I am helping,
Nick