[Skiboot] [PATCH v4 1/2] secvar: return error if validate_esl has extra data

[Nick Child]

Hi Daniel,

Forgot to mention, these patches are still dependent on:
https://lists.ozlabs.org/pipermail/skiboot/2021-July/017641.html

They were once in the same patchset but I split them since these
patches will likely need more discussions before merging.

Sorry, I am still new to sending to mailing lists. Let me know if I should
take any actions.

Thanks,
Nick

On Wed, Jul 14, 2021 at 11:07 PM Daniel Axtens <dja at axtens.net> wrote:
>
> Hi Nick,
>
> I can't get either of these patches to apply to the upstream master
> branch - are there some dependencies I'm missing? From patch 2 it looks
> like the patch adding a trimmed KEK test is expected?
>
> Kind regards,
> Daniel
>
> > 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.
> >
> > Additionally, a test case has been added to ensure that this commit
> > addresses the issue correctly.
> >
> > Signed-off-by: Nick Child <nick.child at ibm.com>
> > Reviewed-by: Nayna Jain <nayna at linux.ibm.com>
> > Tested-by: Nayna Jain <nayna at linux.ibm.com>
> > ---
> >
> > Previous version at https://lists.ozlabs.org/pipermail/skiboot/2021-July/017643.html
> >
> > ---
> >
> >  libstb/secvar/backend/edk2-compat-process.c  |  5 ++++-
> >  libstb/secvar/test/secvar-test-edk2-compat.c | 15 +++++++++++++++
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
> > index 5204fdd5..983a3313 100644
> > --- a/libstb/secvar/backend/edk2-compat-process.c
> > +++ b/libstb/secvar/backend/edk2-compat-process.c
> > @@ -267,8 +267,11 @@ int validate_esl_list(const char *key, const char *esl, const size_t size)
> >
> >       while (eslvarsize > 0) {
> >               prlog(PR_DEBUG, "esl var size is %d offset is %lu\n", eslvarsize, size - eslvarsize);
> > -             if (eslvarsize < sizeof(EFI_SIGNATURE_LIST))
> > +             if (eslvarsize < sizeof(EFI_SIGNATURE_LIST)) {
> > +                     prlog(PR_ERR, "ESL with size %d is too small\n", eslvarsize);
> > +                     rc = OPAL_PARAMETER;
> >                       break;
> > +             }
> >
> >               /* Check Supported ESL Type */
> >               list = get_esl_signature_list(esl, eslvarsize);
> > diff --git a/libstb/secvar/test/secvar-test-edk2-compat.c b/libstb/secvar/test/secvar-test-edk2-compat.c
> > index 3cdc6ef2..1edce112 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