[Skiboot] [PATCH v3 3/3] secvar: Fix signedness issues when validating ESL

Nick Child nnac123 at gmail.com
Fri Jul 2 05:12:40 AEST 2021


In both `validate_esl_list` and `verify_signature`, the submitted ESL
chain and the current secvar ESL chains (respectively) were having
their sizes typecast from their actual type.

Mainly, the length of the buffer, referred to as `eslvarsize` in both
functions, was cast from a type `size_t` to an integer. This was likely
done to ensure the loop `while (eslvarsize > 0)` terminates. A better
solution would be to keep `eslvarsize` as unsigned and ensure that the
current ESL is never bigger than the remaining buffer length. As an
effect, `eslvarsize` could never be decremented past 0. This makes sure
that all bytes are accounted for.

Currently, the length of a single ESL, referred to as `eslsize` in both
functions, is an integer so it would be difficult to compare the two
due to more signedess issues. Rather than cast a signed variable to
unsigned, it would be better to treat `eslsize` as it actually is
stored in the ESL struct: `uint32_t`. This requires a redesign of the
function `get_esl_signature_list_size`.

For these reasons, this commit merges the function
`get_esl_signature_list_size` with `get_esl_signature_list`.
`get_esl_signature_list` now returns an error code or success number.
Its parameters have also changed to return the `uint32_t` size of the
ESL as well as a pointer to the ESL struct if the user gives a pointer
to write to. The check to ensure that the ESL structures specified size
is never larger than the data buffer has been added as a minimal check
that the ESL being returned is valid.

Signed-off-by: Nick Child <nick.child at ibm.com>
---
 libstb/secvar/backend/edk2-compat-process.c | 93 ++++++++++-----------
 1 file changed, 45 insertions(+), 48 deletions(-)

diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
index bb1205a5..bcd43c5d 100644
--- a/libstb/secvar/backend/edk2-compat-process.c
+++ b/libstb/secvar/backend/edk2-compat-process.c
@@ -85,30 +85,37 @@ static void get_key_authority(const char *ret[3], const char *key)
 	ret[i] = NULL;
 }
 
-static EFI_SIGNATURE_LIST* get_esl_signature_list(const char *buf, size_t buflen)
+/*
+ * Returns success or error number if buffer is not a valid ESL
+ * If success, `list` will point to ESL struct which is a `buf` internal pointer
+ * If success, `list_size` will be assigned the size of the ESL header + its data
+ * `list` can be NULL if only size of ESL is requested
+ */
+static int get_esl_signature_list(const char *buf, size_t buflen, EFI_SIGNATURE_LIST** list, uint32_t *list_size)
 {
-	EFI_SIGNATURE_LIST *list = NULL;
+	EFI_SIGNATURE_LIST *list_tmp = NULL;
 
 	if (buflen < sizeof(EFI_SIGNATURE_LIST) || !buf)
-		return NULL;
-
-	list = (EFI_SIGNATURE_LIST *)buf;
+		return OPAL_PARAMETER;
 
-	return list;
-}
+	list_tmp = (EFI_SIGNATURE_LIST *)buf;
+	/* Calculate the size of the ESL */
+	*list_size = le32_to_cpu(list_tmp->SignatureListSize);
 
-/* Returns the size of the complete ESL. */
-static int32_t get_esl_signature_list_size(const char *buf, const size_t buflen)
-{
-	EFI_SIGNATURE_LIST *list = get_esl_signature_list(buf, buflen);
-
-	if (!list)
+	/* If could not extract the size or it is larger than available data */
+	if (*list_size == 0 || *list_size > (uint32_t) buflen) {
+		prlog(PR_ERR, "Invalid size of the ESL: %u\n",
+				*list_size);
 		return OPAL_PARAMETER;
+	}
+
+	if (list != NULL)
+		*list = list_tmp;
 
-	return le32_to_cpu(list->SignatureListSize);
+	return OPAL_SUCCESS;
 }
 
-/* 
+/*
  * Copies the certificate from the ESL into cert buffer and returns the size
  * of the certificate
  */
@@ -116,17 +123,20 @@ static int get_esl_cert(const char *buf, const size_t buflen, char **cert)
 {
 	size_t sig_data_offset;
 	size_t size;
-	EFI_SIGNATURE_LIST *list = get_esl_signature_list(buf, buflen);
+	uint32_t list_size;
+	int rc;
+	EFI_SIGNATURE_LIST *list;
 
-	if (!list)
-		return OPAL_PARAMETER;
+	rc = get_esl_signature_list(buf, buflen, &list, &list_size);
+	if (rc)
+		return rc;
 
 	assert(cert != NULL);
 
 	size = le32_to_cpu(list->SignatureSize) - sizeof(uuid_t);
 
 	prlog(PR_DEBUG,"size of signature list size is %u\n",
-			le32_to_cpu(list->SignatureListSize));
+			list_size);
 	prlog(PR_DEBUG, "size of signature header size is %u\n",
 			le32_to_cpu(list->SignatureHeaderSize));
 	prlog(PR_DEBUG, "size of signature size is %u\n",
@@ -260,35 +270,23 @@ int validate_esl_list(const char *key, const char *esl, const size_t size)
 	int count = 0;
 	int dsize;
 	char *data = NULL;
-	int eslvarsize = size;
-	int eslsize;
+	size_t eslvarsize = size;
+	uint32_t eslsize;
 	int rc = OPAL_SUCCESS;
 	EFI_SIGNATURE_LIST *list = NULL;
 
 	while (eslvarsize > 0) {
-		prlog(PR_DEBUG, "esl var size is %d offset is %lu\n", eslvarsize, size - eslvarsize);
+		prlog(PR_DEBUG, "esl var size is %zu offset is %zu\n", eslvarsize, size - eslvarsize);
 		if (eslvarsize < sizeof(EFI_SIGNATURE_LIST)) {
-			prlog(PR_ERR, "ESL has %d unknown extra bytes\n", eslvarsize);
+			prlog(PR_ERR, "ESL has %zu unknown extra bytes\n", eslvarsize);
 			rc = OPAL_PARAMETER;
 			break;
 		}
 
 		/* Check Supported ESL Type */
-		list = get_esl_signature_list(esl, eslvarsize);
-
-		if (!list)
-			return OPAL_PARAMETER;
-
-		/* Calculate the size of the ESL */
-		eslsize = le32_to_cpu(list->SignatureListSize);
-
-		/* If could not extract the size */
-		if (eslsize <= 0) {
-			prlog(PR_ERR, "Invalid size of the ESL: %u\n",
-					le32_to_cpu(list->SignatureListSize));
-			rc = OPAL_PARAMETER;
-			break;
-		}
+		rc = get_esl_signature_list(esl, eslvarsize, &list, &eslsize);
+		if (rc)
+			return rc;
 
 		/* Extract the certificate from the ESL */
 		dsize = get_esl_cert(esl, eslvarsize, &data);
@@ -316,6 +314,7 @@ int validate_esl_list(const char *key, const char *esl, const size_t size)
 
 		/* Look for the next ESL */
 		esl = esl + eslsize;
+		/* We know eslsize < eslvarsize so no unsigned drama */
 		eslvarsize = eslvarsize - eslsize;
 		free(data);
 		/* Since we are going to allocate again in the next iteration */
@@ -472,9 +471,9 @@ static int verify_signature(const struct efi_variable_authentication_2 *auth,
 	int signing_cert_size;
 	int rc = 0;
 	char *errbuf;
-	int eslvarsize;
-	int eslsize;
-	int offset = 0;
+	size_t eslvarsize;
+	uint32_t eslsize;
+	size_t offset = 0;
 
 	if (!auth)
 		return OPAL_PARAMETER;
@@ -491,21 +490,18 @@ static int verify_signature(const struct efi_variable_authentication_2 *auth,
 
 	/* Variable is not empty */
 	while (eslvarsize > 0) {
-		prlog(PR_DEBUG, "esl var size is %d offset is %d\n", eslvarsize, offset);
+		prlog(PR_DEBUG, "esl var size is %zu offset is %zu\n", eslvarsize, offset);
 		if (eslvarsize < sizeof(EFI_SIGNATURE_LIST)) {
 			rc = OPAL_INTERNAL_ERROR;
 			prlog(PR_ERR, "ESL data is corrupted\n");
 			break;
 		}
 
-		/* Calculate the size of the ESL */
-		eslsize = get_esl_signature_list_size(avar->data + offset,
-						      eslvarsize);
+		/* Get size of ESL, do not request pointer to ESL struct */
+		rc = get_esl_signature_list(avar->data + offset, eslvarsize, NULL, &eslsize);
 		/* If could not extract the size */
-		if (eslsize <= 0) {
-			rc = OPAL_PARAMETER;
+		if (rc)
 			break;
-		}
 
 		/* Extract the certificate from the ESL */
 		signing_cert_size = get_esl_cert(avar->data + offset,
@@ -563,6 +559,7 @@ static int verify_signature(const struct efi_variable_authentication_2 *auth,
 
 		/* Look for the next ESL */
 		offset = offset + eslsize;
+		/* We know eslsize < eslvarsize so no unsigned drama */
 		eslvarsize = eslvarsize - eslsize;
 		mbedtls_x509_crt_free(&x509);
 		free(signing_cert);
-- 
2.25.1



More information about the Skiboot mailing list