[Skiboot] [PATCH v3 2/3] secvar: return error if verify_signature runs out of ESLs

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


Currently, in `verify_signature`, the return code `rc` is initialized
as 0 (our success value). While looping through the ESL's in the given
secvar, the function will break if the remaining data in the secvar is
not enough to contain another ESL. This break from the loop was not
setting a return code, this means that the successful return code
can pass to the end of the function if the first iteration meets
this condition. In other words, if a current secvar has a size that
is less than minimum size for an ESL, than it will approve any update.

In response to this bug, this commit will return an error code if
the described condition is met. Additionally, a test case has been
added to ensure that this unlikely event is handled correctly.

Signed-off-by: Nick Child <nick.child at ibm.com>
---
 libstb/secvar/backend/edk2-compat-process.c  |  5 +++-
 libstb/secvar/test/secvar-test-edk2-compat.c | 25 ++++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
index 03547875..bb1205a5 100644
--- a/libstb/secvar/backend/edk2-compat-process.c
+++ b/libstb/secvar/backend/edk2-compat-process.c
@@ -492,8 +492,11 @@ 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);
-		if (eslvarsize < sizeof(EFI_SIGNATURE_LIST))
+		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,
diff --git a/libstb/secvar/test/secvar-test-edk2-compat.c b/libstb/secvar/test/secvar-test-edk2-compat.c
index 1edce112..100fda7d 100644
--- a/libstb/secvar/test/secvar-test-edk2-compat.c
+++ b/libstb/secvar/test/secvar-test-edk2-compat.c
@@ -89,6 +89,7 @@ int run_test()
 {
 	int rc = -1;
 	struct secvar *tmp;
+	size_t tmp_size;
 	char empty[64] = {0};
 
 	/* The sequence of test cases here is important to ensure that
@@ -213,6 +214,30 @@ int run_test()
 	tmp = find_secvar("db", 3, &variable_bank);
 	ASSERT(NULL != tmp);
 
+	/* Add db, should fail with no KEK and invalid PK size */
+	printf("Add db, corrupt PK");
+	/* Somehow PK gets assigned wrong size */
+	tmp = find_secvar("PK", 3, &variable_bank);
+	ASSERT(NULL != tmp);
+	tmp_size = tmp->data_size;
+	tmp->data_size = sizeof(EFI_SIGNATURE_LIST) - 1;
+	tmp = new_secvar("db", 3, DB_auth, DB_auth_len, 0);
+	ASSERT(0 == edk2_compat_validate(tmp));
+	list_add_tail(&update_bank, &tmp->link);
+	ASSERT(1 == list_length(&update_bank));
+
+	rc = edk2_compat_process(&variable_bank, &update_bank);
+	ASSERT(OPAL_INTERNAL_ERROR == rc);
+	ASSERT(5 == list_length(&variable_bank));
+	ASSERT(0 == list_length(&update_bank));
+	tmp = find_secvar("db", 3, &variable_bank);
+	ASSERT(NULL != tmp);
+	ASSERT(0 == tmp->data_size);
+	/* Restore PK data size */
+	tmp = find_secvar("PK", 3, &variable_bank);
+	ASSERT(NULL != tmp);
+	tmp->data_size = tmp_size;
+
 	/* Add trimmed KEK, .process(), should fail. */
 	printf("Add trimmed KEK\n");
 	tmp = new_secvar("KEK", 4, trimmedKEK_auth, trimmedKEK_auth_len, 0);
-- 
2.25.1



More information about the Skiboot mailing list