[Skiboot] [PATCH v2 4/5] secvar/backend: redo hash algorithm handling for auth structures

[Daniel Axtens]

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 - if I've got the mbedtls flow correct - in a
32 byte overread.

 - Verify that the hash algorithm in the PKCS#7 message is sha256.
 [there is an additional complexity in the spec which our code doesn't
  handle. the digest algorithms are specified in 2 places:
  DigestAlgorithmIdentifiers, and SignerInfos.<number>.digestAlgorithm.
  Having them not match would be a spec violation, but nonetheless fairly
  trivial to construct. However, mbedtls_pkcs7_signed_hash_verify
  considers only the first element of DigestAlgorithmIdentifiers, so this
  is safe for now at least.]

 - Stop passing hash lengths around, they're constant.

 - Pass 32 into mbedtls as the hash length. I contemplated using 0,
   which is code for "just use what the hash algorithm says the length
   is", but I'm worried about what might happen if we move to an mbedtls
   that supports pkcs7 messages with multiple signers: what if a message
   contains both a sha256 and then a sha512 signature? Our new check
   won't catch it. At least with passing 32 there is the possibility that
   mbedtls can do the right thing and not over-read. If we pass in 0, then
   mbedtls has no way of knowing that we have only 32 bytes allocated.

Hopefully the upshot of this is that the code is safer and if we do end up
using another hash algorithm, we're forced to at least consider the
possibility that the hash length is different.

[If we ever use a version of mbedtls that supports multiple digests
then we should of course review this code again.
Arguably multiple digests are also going to need an mbedtls API change,
it's difficult to figure out how a hash length value of 0 could be safe.]

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

---

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 | 23 ++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
index 3ef3cf73c8f7..089a4def4cf8 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"
@@ -455,11 +456,11 @@ out:
 
 /* Verify the PKCS7 signature on the signed data. */
 static int verify_signature(const struct efi_variable_authentication_2 *auth,
-			    const char *hash, const size_t hash_len,
-			    const struct secvar *avar)
+			    const char *hash, const struct secvar *avar)
 {
 	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 +479,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;
+
 	prlog(PR_INFO, "Load the signing certificate from the keystore");
 
 	eslvarsize = avar->data_size;
@@ -534,7 +547,7 @@ static int verify_signature(const struct efi_variable_authentication_2 *auth,
 		free(x509_buf);
 		x509_buf = NULL;
 
-		rc = mbedtls_pkcs7_signed_hash_verify(pkcs7, &x509, hash, hash_len);
+		rc = mbedtls_pkcs7_signed_hash_verify(pkcs7, &x509, hash, 32);
 
 		/* If you find a signing certificate, you are done */
 		if (rc == 0) {
@@ -745,8 +758,8 @@ int process_update(const struct secvar *update, char **newesl,
 		if (!avar || !avar->data_size)
 			continue;
 
-		/* Verify the signature. sha256 is 32 bytes long. */
-		rc = verify_signature(auth, hash, 32, avar);
+		/* Verify the signature. */
+		rc = verify_signature(auth, hash, avar);
 
 		/* Break if signature verification is successful */
 		if (rc == OPAL_SUCCESS) {
-- 
2.30.2