[Skiboot] [PATCH 6/7] secvar/backend: get_pkcs7_len should return a signed type

[Daniel Axtens]

get_pkcs7_len parses a efi_variable_authentication2 and tells us how
long the embedded PKCS#7 message is.

This process involves reading a number out of the auth header and
subtracting a number of constants. As such, this process can fail: if the
resultant value is less than 0, this is an error: a pkcs7 message cannot
have negative size!

Currently, we don't catch this: we calculate with an unsigned type, so it
experiences integer overflow and becomes enormous.

The field we're reading from the header is 32 bit, so we don't need all
our 64 bits as data bits. Instead, change the type to be ssize_t, and if
the result of the subtraction is less than 0, return OPAL_PARAMETER.

Adjust call-sites to use the different return type and specifically catch
return values that are errors.

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

---

I am fairly sure this cannot cause a bug. If you could get to get_pkcs7
then you'd get to mbedtls_pkcs7_parse_der with a huge len, but the only
caller is verify_signature(), which is only called by process_update().
However process_update() first calls get_auth_descriptor2(), which will
helpfully call get_pkcs7_len() and compare the result with buf_len. The
underflowed negative number will be bigger than buf_len - so the caller
(get_auth_descriptor2()) will fail, and process_update() will then bail
before calling into verify_signature().

So this patch isn't as urgent as the others.
---
 libstb/secvar/backend/edk2-compat-process.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
index f6831f1f7ccf..580bd4ebdcc8 100644
--- a/libstb/secvar/backend/edk2-compat-process.c
+++ b/libstb/secvar/backend/edk2-compat-process.c
@@ -162,11 +162,13 @@ static int get_esl_cert(const char *buf, const size_t buflen, char **cert)
 /* 
  * Extracts size of the PKCS7 signed data embedded in the
  * struct Authentication 2 Descriptor Header.
+ *
+ * Returns a ssize_t: positive values are OK, negative values are error.
  */
-static size_t get_pkcs7_len(const struct efi_variable_authentication_2 *auth)
+static ssize_t get_pkcs7_len(const struct efi_variable_authentication_2 *auth)
 {
 	uint32_t dw_length;
-	size_t size;
+	ssize_t size;
 
 	assert(auth != NULL);
 
@@ -176,14 +178,17 @@ static size_t get_pkcs7_len(const struct efi_variable_authentication_2 *auth)
 			+ sizeof(auth->auth_info.hdr.w_certificate_type)
 			+ sizeof(auth->auth_info.cert_type));
 
+	if (size < 0)
+		return OPAL_PARAMETER;
+
 	return size;
 }
 
 int get_auth_descriptor2(const void *buf, const size_t buflen, void **auth_buffer)
 {
 	const struct efi_variable_authentication_2 *auth = buf;
-	int auth_buffer_size;
-	size_t len;
+	ssize_t auth_buffer_size;
+	ssize_t len;
 
 	assert(auth_buffer != NULL);
 	if (buflen < sizeof(struct efi_variable_authentication_2)
@@ -192,7 +197,7 @@ int get_auth_descriptor2(const void *buf, const size_t buflen, void **auth_buffe
 
 	len = get_pkcs7_len(auth);
 	/* pkcs7 content length cannot be greater than buflen */ 
-	if (len > buflen)
+	if (len < 0 || len > buflen)
 		return OPAL_PARAMETER;
 
 	auth_buffer_size = sizeof(auth->timestamp) + sizeof(auth->auth_info.hdr)
@@ -417,11 +422,13 @@ int check_timestamp(const char *key, const struct efi_time *timestamp,
 static mbedtls_pkcs7* get_pkcs7(const struct efi_variable_authentication_2 *auth)
 {
 	char *checkpkcs7cert = NULL;
-	size_t len;
+	ssize_t len;
 	mbedtls_pkcs7 *pkcs7 = NULL;
 	int rc;
 
 	len = get_pkcs7_len(auth);
+	if (len < 0)
+		return NULL;
 
 	pkcs7 = malloc(sizeof(struct mbedtls_pkcs7));
 	if (!pkcs7)
-- 
2.30.2