| Message ID | 20210621082641.26476-5-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 6/21/21 4:26 AM, 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 - if I've got the mbedtls flow correct - in a > 32 byte overread. This seems good check to add. Thanks for looking at it. > > - Verify that the hash algorithm in the PKCS#7 message is sha256. > [there is an additional complexity in the spec which our code doesn't > handle. the digest algorithms are specified in 2 places: > DigestAlgorithmIdentifiers, and SignerInfos.<number>.digestAlgorithm. > Having them not match would be a spec violation, but nonetheless fairly > trivial to construct. However, mbedtls_pkcs7_signed_hash_verify > considers only the first element of DigestAlgorithmIdentifiers, so this > is safe for now at least.] > - Stop passing hash lengths around, they're constant. > > - Pass 32 into mbedtls as the hash length. I contemplated using 0, > which is code for "just use what the hash algorithm says the length > is", but I'm worried about what might happen if we move to an mbedtls > that supports pkcs7 messages with multiple signers: what if a message > contains both a sha256 and then a sha512 signature? Our new check > won't catch it. At least with passing 32 there is the possibility that > mbedtls can do the right thing and not over-read. If we pass in 0, then > mbedtls has no way of knowing that we have only 32 bytes allocated. > > Hopefully the upshot of this is that the code is safer and if we do end up > using another hash algorithm, we're forced to at least consider the > possibility that the hash length is different. > > [If we ever use a version of mbedtls that supports multiple digests > then we should of course review this code again. > Arguably multiple digests are also going to need an mbedtls API change, > it's difficult to figure out how a hash length value of 0 could be safe.] I would go with assuming only single signer support now. Multiple signers support might need not only hash handling but some other changes also in pkcs7 code. Nick and I are going to test the patches. Thanks & Regards, - Nayna
On 6/22/21 6:59 AM, Nayna wrote: > > On 6/21/21 4:26 AM, 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 - if I've got the mbedtls flow correct - in a >> 32 byte overread. > > .../... >> >> [If we ever use a version of mbedtls that supports multiple digests >> then we should of course review this code again. >> Arguably multiple digests are also going to need an mbedtls API change, >> it's difficult to figure out how a hash length value of 0 could be safe.] > > I would go with assuming only single signer support now. Multiple signers > support might need not only hash handling but some other changes also in pkcs7 > code. > > Nick and I are going to test the patches. Did you guys test this series? Any update? -Vasant
On Tue, Jul 20, 2021 at 5:05 AM Vasant Hegde <hegdevasant@linux.vnet.ibm.com> wrote: > > Did you guys test this series? Any update? > > -Vasant This specific patch seems to be superseded by 9d52c580 upstream. I will review the other patches in the meantime. Best, Nick Child
diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c index 3ef3cf73c8f7..089a4def4cf8 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" @@ -455,11 +456,11 @@ out: /* Verify the PKCS7 signature on the signed data. */ static int verify_signature(const struct efi_variable_authentication_2 *auth, - const char *hash, const size_t hash_len, - const struct secvar *avar) + const char *hash, const struct secvar *avar) { 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 +479,18 @@ 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) + return OPAL_PARAMETER; + + if (md_alg != MBEDTLS_MD_SHA256) + return OPAL_PARAMETER; + prlog(PR_INFO, "Load the signing certificate from the keystore"); eslvarsize = avar->data_size; @@ -534,7 +547,7 @@ static int verify_signature(const struct efi_variable_authentication_2 *auth, free(x509_buf); x509_buf = NULL; - rc = mbedtls_pkcs7_signed_hash_verify(pkcs7, &x509, hash, hash_len); + rc = mbedtls_pkcs7_signed_hash_verify(pkcs7, &x509, hash, 32); /* If you find a signing certificate, you are done */ if (rc == 0) { @@ -745,8 +758,8 @@ int process_update(const struct secvar *update, char **newesl, if (!avar || !avar->data_size) continue; - /* Verify the signature. sha256 is 32 bytes long. */ - rc = verify_signature(auth, hash, 32, avar); + /* Verify the signature. */ + rc = verify_signature(auth, hash, avar); /* Break if signature verification is successful */ if (rc == OPAL_SUCCESS) {
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 - if I've got the mbedtls flow correct - in a 32 byte overread. - Verify that the hash algorithm in the PKCS#7 message is sha256. [there is an additional complexity in the spec which our code doesn't handle. the digest algorithms are specified in 2 places: DigestAlgorithmIdentifiers, and SignerInfos.<number>.digestAlgorithm. Having them not match would be a spec violation, but nonetheless fairly trivial to construct. However, mbedtls_pkcs7_signed_hash_verify considers only the first element of DigestAlgorithmIdentifiers, so this is safe for now at least.] - Stop passing hash lengths around, they're constant. - Pass 32 into mbedtls as the hash length. I contemplated using 0, which is code for "just use what the hash algorithm says the length is", but I'm worried about what might happen if we move to an mbedtls that supports pkcs7 messages with multiple signers: what if a message contains both a sha256 and then a sha512 signature? Our new check won't catch it. At least with passing 32 there is the possibility that mbedtls can do the right thing and not over-read. If we pass in 0, then mbedtls has no way of knowing that we have only 32 bytes allocated. Hopefully the upshot of this is that the code is safer and if we do end up using another hash algorithm, we're forced to at least consider the possibility that the hash length is different. [If we ever use a version of mbedtls that supports multiple digests then we should of course review this code again. Arguably multiple digests are also going to need an mbedtls API change, it's difficult to figure out how a hash length value of 0 could be safe.] Signed-off-by: Daniel Axtens <dja@axtens.net> --- 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 | 23 ++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)