[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