Message ID | 20210623025141.278900-1-dja@axtens.net |
---|---|
State | Superseded |
Headers | show |
Series | [v2] secvar/backend: require sha256 in our PKCS#7 messages | 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 6/22/21 10:51 PM, Daniel Axtens wrote: > We only handle sha256 hashes in auth structures. > > In the process of verifying an auth structure, we extract the pkcs7 > message and we calculate the hopefully-matching hash, which is > sha256(name || vendor guid || attributes || timestamp || newcontent) > We then verify that the PKCS#7 signature matches that calculated hash. > > However, at no point do we check that the PKCS#7 hash algorithm is > sha256. So if the PKCS#7 message says that it is a signature on a sha512, > mbedtls will compare 64 bytes of hash from the signature with 64 bytes > from our hash, resulting in a 32 byte overread. > > Verify that the hash algorithm in the PKCS#7 message is sha256. > > Signed-off-by: Daniel Axtens <dja@axtens.net> > > --- > > This is the minimal fix for the underlying bug. It should probably > go in ahead of any potential future reworking of the area. > > v2: thanks Nick and Nayna for your feedback. Added error messages > and properly cleaned up the pkcs7 structure. > > As always, compile tested only because I don't have access to a box > set up to test this. I think it would be good if you can update edk2 testcases as well for this fix. I am referring to - libstb/secvar/test/secvar-test-edk2-compat.c These can be used to verify the fixes in absence of real setup. Thanks & Regards, - Nayna
diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c index 244f23403fe0..df9753245014 100644 --- a/libstb/secvar/backend/edk2-compat-process.c +++ b/libstb/secvar/backend/edk2-compat-process.c @@ -11,6 +11,7 @@ #include <stdint.h> #include <ccan/endian/endian.h> #include <mbedtls/error.h> +#include <mbedtls/oid.h> #include <device.h> #include <assert.h> #include "libstb/crypto/pkcs7/pkcs7.h" @@ -460,6 +461,7 @@ static int verify_signature(const struct efi_variable_authentication_2 *auth, { mbedtls_pkcs7 *pkcs7 = NULL; mbedtls_x509_crt x509; + mbedtls_md_type_t md_alg; char *signing_cert = NULL; char *x509_buf = NULL; int signing_cert_size; @@ -478,6 +480,25 @@ static int verify_signature(const struct efi_variable_authentication_2 *auth, if (!pkcs7) return OPAL_PARAMETER; + /* + * We only support sha256, which has a hash length of 32. + * If the alg is not sha256, then we should bail now. + */ + rc = mbedtls_oid_get_md_alg(&pkcs7->signed_data.digest_alg_identifiers, + &md_alg); + if (rc != 0) { + prlog(PR_ERR, "Failed to get the Digest Algorithm Identifier: %d\n", rc); + rc = OPAL_PARAMETER; + goto err_pkcs7; + } + + if (md_alg != MBEDTLS_MD_SHA256) { + prlog(PR_ERR, "Unexpected digest algorithm: expected %d (SHA-256), got %d\n", + MBEDTLS_MD_SHA256, md_alg); + rc = OPAL_PARAMETER; + goto err_pkcs7; + } + prlog(PR_INFO, "Load the signing certificate from the keystore"); eslvarsize = avar->data_size; @@ -562,6 +583,7 @@ static int verify_signature(const struct efi_variable_authentication_2 *auth, } free(signing_cert); +err_pkcs7: mbedtls_pkcs7_free(pkcs7); free(pkcs7);
We only handle sha256 hashes in auth structures. In the process of verifying an auth structure, we extract the pkcs7 message and we calculate the hopefully-matching hash, which is sha256(name || vendor guid || attributes || timestamp || newcontent) We then verify that the PKCS#7 signature matches that calculated hash. However, at no point do we check that the PKCS#7 hash algorithm is sha256. So if the PKCS#7 message says that it is a signature on a sha512, mbedtls will compare 64 bytes of hash from the signature with 64 bytes from our hash, resulting in a 32 byte overread. Verify that the hash algorithm in the PKCS#7 message is sha256. Signed-off-by: Daniel Axtens <dja@axtens.net> --- This is the minimal fix for the underlying bug. It should probably go in ahead of any potential future reworking of the area. v2: thanks Nick and Nayna for your feedback. Added error messages and properly cleaned up the pkcs7 structure. As always, compile tested only because I don't have access to a box set up to test this. --- libstb/secvar/backend/edk2-compat-process.c | 22 +++++++++++++++++++++ 1 file changed, 22 insertions(+)