| Message ID | 20210628193732.109871-4-nick.child@ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | secvar: ESL validation fixes | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (9d52c580d3fabbea6b276b98f925ab0fceebb96c) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
Nick Child <nnac123@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@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
Daniel Axtens <dja@axtens.net> writes: > I can't figure out whether this patch is too complex or you've just > explained some simple changes in a roundabout way. Good point. This commit does add in a bunch of semi related code changes. I was having trouble deciding whether to add everything in this commit or add another patch. > > --- > > 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 >> > > 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... My only concern with that idea is sometimes the calling function does not want anything to do with the ESL and only wants the size, see `verify_signature`. I guess this could be resolved by adding back `get_esl_singature_list_size`. Also, having a function to just determine if a data buffer is a valid ESL would require the data buffer to be typecast to an EFI_SIGNATURE_LIST twice (once in the function and again on success in the calling function). This isn't a bad/costly operation but I am just bringing awareness that the somewhat scary cast from data buffer to ESL struct would have to appear in several more places. Let me know your thoughts and I can go ahead and implement your recommendations. Additionally, let me know if I should put these changes in a separate patch and leave this patch as it was in V1. Thanks, Nick
Nick Child <nnac123@gmail.com> writes: > Daniel Axtens <dja@axtens.net> writes: > >> I can't figure out whether this patch is too complex or you've just >> explained some simple changes in a roundabout way. > > Good point. This commit does add in a bunch of semi related code > changes. I was having trouble deciding whether to add everything in > this commit or add another patch. > >> > --- >> > 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 >>> >> >> 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... > > My only concern with that idea is sometimes the calling function does not > want anything to do with the ESL and only wants the size, see > `verify_signature`. I guess this could be resolved by adding back > `get_esl_singature_list_size`. Also, having a function to just determine > if a data buffer is a valid ESL would require the data buffer to be typecast > to an EFI_SIGNATURE_LIST twice (once in the function and again > on success in the calling function). This isn't a bad/costly operation > but I am just bringing awareness that the somewhat scary cast from > data buffer to ESL struct would have to appear in several more places. Hmm OK. Something I will ponder after I return from leave. > Let me know your thoughts and I can go ahead and implement your > recommendations. Additionally, let me know if I should put these changes > in a separate patch and leave this patch as it was in V1. Yes, please do a minimal fix and then split the rework out. Whether or not the fix ends up being critical (it requires a signed update or physical presence IIUC?), it's good to at least structure the patches so that there's the option of pulling it in separate from a bigger restructure. Kind regards, Daniel > > Thanks, > Nick
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; } /* @@ -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);
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 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. Signed-off-by: Nick Child <nick.child@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(-)