| Message ID | 20210525033425.972519-3-dja@axtens.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | secvar cleanups and fixes | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (1cdde9466ab658fb5b5a53af8c4e6a8929eef698) |
| 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 |
On 5/24/21 11:34 PM, Daniel Axtens wrote: > process_update() has tbhbuffer and tbhbuffersize. However, tbhbuffer > doesn't contain to-be-hashed data, but a hash: > > /* Prepare the data to be verified */ > tbhbuffer = get_hash_to_verify(update->key, *newesl, *new_data_size, > timestamp); > > And tbhbuffersize is initalised to zero and then never filled with > the actual length, so 0 is passed through to verify_signature. > verify_signature will in turn pass that to mbedtls, which will interpret > it as "figure out the length yourself based on the hash type". So this > has always worked, but by accident. > > Rename tbhbuffer to hash, as that's what it is. > > Drop tbhbuffersize, and pass 32 directly to verify_signature. We only > support SHA-256, and SHA-256 is 32 bytes long. > > Signed-off-by: Daniel Axtens <dja@axtens.net> Thanks Daniel for identifying and fixing it. > --- > libstb/secvar/backend/edk2-compat-process.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c > index 8324dc068b8e..b39f35e0c3dc 100644 > --- a/libstb/secvar/backend/edk2-compat-process.c > +++ b/libstb/secvar/backend/edk2-compat-process.c > @@ -662,8 +662,7 @@ int process_update(const struct secvar *update, char **newesl, > void *auth_buffer = NULL; > int auth_buffer_size = 0; > const char *key_authority[3]; > - char *tbhbuffer = NULL; > - size_t tbhbuffersize = 0; > + char *hash = NULL; > struct secvar *avar = NULL; > int rc = 0; > int i; > @@ -723,9 +722,9 @@ int process_update(const struct secvar *update, char **newesl, > } > > /* Prepare the data to be verified */ > - tbhbuffer = get_hash_to_verify(update->key, *newesl, *new_data_size, > + hash = get_hash_to_verify(update->key, *newesl, *new_data_size, > timestamp); > - if (!tbhbuffer) { > + if (!hash) { > rc = OPAL_INTERNAL_ERROR; > goto out; > } > @@ -746,9 +745,8 @@ int process_update(const struct secvar *update, char **newesl, > if (!avar || !avar->data_size) > continue; > > - /* Verify the signature */ > - rc = verify_signature(auth, tbhbuffer, tbhbuffersize, > - avar); > + /* Verify the signature. sha256 is 32 bytes long. */ > + rc = verify_signature(auth, hash, 32, avar); This does work. I am thinking that we still pass here zero like it was previously and fix the documentation of mbedtls_pkcs7_signed_hash_verify() in pkcs7.h. The documentation makes it explicit that caller can pass 0 or sizeof(hash). This will also make it consistent with general behaviour of mbedtls API where it allows to pass zero and figures out by itself. Thanks & Regards, - Nayna
On 5/25/21 8:49 AM, Nayna wrote: > > On 5/24/21 11:34 PM, Daniel Axtens wrote: >> process_update() has tbhbuffer and tbhbuffersize. However, tbhbuffer >> doesn't contain to-be-hashed data, but a hash: >> >> /* Prepare the data to be verified */ >> tbhbuffer = get_hash_to_verify(update->key, *newesl, *new_data_size, >> timestamp); >> >> And tbhbuffersize is initalised to zero and then never filled with >> the actual length, so 0 is passed through to verify_signature. >> verify_signature will in turn pass that to mbedtls, which will interpret >> it as "figure out the length yourself based on the hash type". So this >> has always worked, but by accident. >> >> Rename tbhbuffer to hash, as that's what it is. >> >> Drop tbhbuffersize, and pass 32 directly to verify_signature. We only >> support SHA-256, and SHA-256 is 32 bytes long. >> >> Signed-off-by: Daniel Axtens <dja@axtens.net> > > Thanks Daniel for identifying and fixing it. > > >> --- >> libstb/secvar/backend/edk2-compat-process.c | 14 ++++++-------- >> 1 file changed, 6 insertions(+), 8 deletions(-) >> >> diff --git a/libstb/secvar/backend/edk2-compat-process.c >> b/libstb/secvar/backend/edk2-compat-process.c >> index 8324dc068b8e..b39f35e0c3dc 100644 >> --- a/libstb/secvar/backend/edk2-compat-process.c >> +++ b/libstb/secvar/backend/edk2-compat-process.c >> @@ -662,8 +662,7 @@ int process_update(const struct secvar *update, >> char **newesl, >> void *auth_buffer = NULL; >> int auth_buffer_size = 0; >> const char *key_authority[3]; >> - char *tbhbuffer = NULL; >> - size_t tbhbuffersize = 0; >> + char *hash = NULL; >> struct secvar *avar = NULL; >> int rc = 0; >> int i; >> @@ -723,9 +722,9 @@ int process_update(const struct secvar *update, >> char **newesl, >> } >> /* Prepare the data to be verified */ >> - tbhbuffer = get_hash_to_verify(update->key, *newesl, >> *new_data_size, >> + hash = get_hash_to_verify(update->key, *newesl, *new_data_size, >> timestamp); >> - if (!tbhbuffer) { >> + if (!hash) { >> rc = OPAL_INTERNAL_ERROR; >> goto out; >> } >> @@ -746,9 +745,8 @@ int process_update(const struct secvar *update, >> char **newesl, >> if (!avar || !avar->data_size) >> continue; >> - /* Verify the signature */ >> - rc = verify_signature(auth, tbhbuffer, tbhbuffersize, >> - avar); >> + /* Verify the signature. sha256 is 32 bytes long. */ >> + rc = verify_signature(auth, hash, 32, avar); > > This does work. I am thinking that we still pass here zero like it was > previously and fix the documentation of > mbedtls_pkcs7_signed_hash_verify() in pkcs7.h. > > The documentation makes it explicit that caller can pass 0 or > sizeof(hash). This will also make it consistent with general > behaviour of mbedtls API where it allows to pass zero and figures out > by itself. > > Oops, I wanted to write length of hash.. but by mistake wrote it as sizeof(hash). Thanks & Regards, - Nayna
diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c index 8324dc068b8e..b39f35e0c3dc 100644 --- a/libstb/secvar/backend/edk2-compat-process.c +++ b/libstb/secvar/backend/edk2-compat-process.c @@ -662,8 +662,7 @@ int process_update(const struct secvar *update, char **newesl, void *auth_buffer = NULL; int auth_buffer_size = 0; const char *key_authority[3]; - char *tbhbuffer = NULL; - size_t tbhbuffersize = 0; + char *hash = NULL; struct secvar *avar = NULL; int rc = 0; int i; @@ -723,9 +722,9 @@ int process_update(const struct secvar *update, char **newesl, } /* Prepare the data to be verified */ - tbhbuffer = get_hash_to_verify(update->key, *newesl, *new_data_size, + hash = get_hash_to_verify(update->key, *newesl, *new_data_size, timestamp); - if (!tbhbuffer) { + if (!hash) { rc = OPAL_INTERNAL_ERROR; goto out; } @@ -746,9 +745,8 @@ int process_update(const struct secvar *update, char **newesl, if (!avar || !avar->data_size) continue; - /* Verify the signature */ - rc = verify_signature(auth, tbhbuffer, tbhbuffersize, - avar); + /* Verify the signature. sha256 is 32 bytes long. */ + rc = verify_signature(auth, hash, 32, avar); /* Break if signature verification is successful */ if (rc == OPAL_SUCCESS) { @@ -759,7 +757,7 @@ int process_update(const struct secvar *update, char **newesl, out: free(auth_buffer); - free(tbhbuffer); + free(hash); return rc; }
process_update() has tbhbuffer and tbhbuffersize. However, tbhbuffer doesn't contain to-be-hashed data, but a hash: /* Prepare the data to be verified */ tbhbuffer = get_hash_to_verify(update->key, *newesl, *new_data_size, timestamp); And tbhbuffersize is initalised to zero and then never filled with the actual length, so 0 is passed through to verify_signature. verify_signature will in turn pass that to mbedtls, which will interpret it as "figure out the length yourself based on the hash type". So this has always worked, but by accident. Rename tbhbuffer to hash, as that's what it is. Drop tbhbuffersize, and pass 32 directly to verify_signature. We only support SHA-256, and SHA-256 is 32 bytes long. Signed-off-by: Daniel Axtens <dja@axtens.net> --- libstb/secvar/backend/edk2-compat-process.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)