[Skiboot] [PATCH v2 3/4] secvar: return error if validate_esl has extra data
Daniel Axtens
dja at axtens.net
Thu Jul 1 00:30:20 AEST 2021
Nick Child <nnac123 at gmail.com> writes:
> Currently, in `validate_esl_list`, the return code is initialized to zero
> (our success value). While looping though the ESL's in the submitted ESL
> chain, the loop will break if there is not enough data to meet minimum ESL
> requirements. This condition was not setting a return code, meaning that the
> successful return code can pass to the end of the function if there is extra
> data at the end of the ESL. As a consequence, any properly signed update can
> successfully commit any data (as long as it is less than the min size of an
> ESL) to the secvars.
>
> This commit will return an error if the described condition is met. This
> means all data in the appended ESL of an auth file must be accounted for. No
> extra bytes can be added to the end since, on success, this data will become
> the updated secvar.
>
> Also, a test case has been added to ensure that this commit
> addresses the issue correctly.
>
> Additionally, some changes have been made in `get_esl_signature_list` that
Always a bit of a worry when a commit message has an 'additionally'!
> adds a check to the ESL size (which was being done outside of all of the
> functions calls) and also combines its functionality with
> `get_esl_signature_list_size`. The purpose of this is to have for
> `get_esl_signature_list` to have at least some data checks and save
> repetitive code. The function now has a new set of input and output parameters
> as well as a different return value. All calling functions have been edited to
> respond to this change appropriately.
>
> Lastly, some size variables in `verify_signature` and `validate_esl_list`
> have been edited from being type integer to an unsigned type that better matches their specifications.
I can't figure out whether this patch is too complex or you've just
explained some simple changes in a roundabout way.
> Signed-off-by: Nick Child <nick.child at ibm.com>
> ---
> libstb/secvar/backend/edk2-compat-process.c | 82 +++++++++-----------
> libstb/secvar/test/secvar-test-edk2-compat.c | 15 ++++
> 2 files changed, 52 insertions(+), 45 deletions(-)
>
> diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
> index 7d83c912..ff445d45 100644
> --- a/libstb/secvar/backend/edk2-compat-process.c
> +++ b/libstb/secvar/backend/edk2-compat-process.c
> @@ -85,27 +85,28 @@ 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)
> +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 list;
> -}
> + return OPAL_PARAMETER;
>
> -/* 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);
> + list_tmp = (EFI_SIGNATURE_LIST *)buf;
> + /* Calculate the size of the ESL */
> + *list_size = le32_to_cpu(list_tmp->SignatureListSize);
>
> - 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;
> }
>
So if we're in the process of redesigning APIs, and apologies that it's
taking me a while to come around to what I think is the best design:
Would it make more sense to:
- have a function that verifies the list - that is, checks
SignatureListSize, and SignatureSize, and SignatureHeaderSize ---
because they're all from the data we use all of them later on, and
- have that function take a buffer and buffer len and return an error
code, and
- after that function runs, you just cast the buffer to an
EFI_SIGNATURE_LIST directly, which is what the function does anyway.
Saves us passing pointer-to-pointer around and allows for cleaner
names...
Feel free to push back if you have other design ideas that you think
would be better.
Kind regards,
Daniel
> /*
> @@ -116,17 +117,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",
> @@ -257,33 +261,24 @@ 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 size is %d offset is %lu\n", eslvarsize, size - eslvarsize);
> - if (eslvarsize < sizeof(EFI_SIGNATURE_LIST))
> - 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));
> + prlog(PR_DEBUG, "esl var size size is %zu offset is %zu\n", eslvarsize, size - eslvarsize);
> + if (eslvarsize < sizeof(EFI_SIGNATURE_LIST)) {
> + prlog(PR_ERR, "ESL has %zu unknown extra bytes\n", eslvarsize);
> rc = OPAL_PARAMETER;
> break;
> }
>
> + /* Check Supported ESL Type */
> + 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);
> if (dsize < 0) {
> @@ -466,9 +461,9 @@ static int verify_signature(const struct efi_variable_authentication_2 *auth,
> int signing_cert_size;
> int rc = 0;
> char *errbuf;
> - int eslvarsize;
> + size_t eslvarsize;
> int eslsize;
> - int offset = 0;
> + size_t offset = 0;
>
> if (!auth)
> return OPAL_PARAMETER;
> @@ -485,18 +480,15 @@ static int verify_signature(const struct efi_variable_authentication_2 *auth,
>
> /* Variable is not empty */
> while (eslvarsize > 0) {
> - prlog(PR_DEBUG, "esl var size size is %d offset is %d\n", eslvarsize, offset);
> + prlog(PR_DEBUG, "esl var size size is %zu offset is %zu\n", eslvarsize, offset);
> if (eslvarsize < sizeof(EFI_SIGNATURE_LIST))
> break;
>
> /* Calculate the size of the ESL */
> - eslsize = get_esl_signature_list_size(avar->data + offset,
> - eslvarsize);
> + 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,
> diff --git a/libstb/secvar/test/secvar-test-edk2-compat.c b/libstb/secvar/test/secvar-test-edk2-compat.c
> index 93ade920..d401f952 100644
> --- a/libstb/secvar/test/secvar-test-edk2-compat.c
> +++ b/libstb/secvar/test/secvar-test-edk2-compat.c
> @@ -165,6 +165,21 @@ int run_test()
> ASSERT(5 == list_length(&variable_bank));
> ASSERT(setup_mode);
>
> + /* Add PK with bad ESL. should fail since data is not big enough to be ESL*/
> + printf("Add PK with invalid appended ESL");
> + /* 1014 is length of appended ESL Header and its data */
> + tmp = new_secvar("PK", 3, PK_auth, PK_auth_len - 1014 + sizeof(EFI_SIGNATURE_LIST) - 1, 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(5 == list_length(&variable_bank));
> + ASSERT(0 == list_length(&update_bank));
> + rc = edk2_compat_post_process(&variable_bank, &update_bank);
> + ASSERT(5 == list_length(&variable_bank));
> + ASSERT(setup_mode);
> +
> +
> /* Add PK to update and .process(). */
> printf("Add PK");
> tmp = new_secvar("PK", 3, PK_auth, PK_auth_len, 0);
> --
> 2.25.1
More information about the Skiboot
mailing list