| Message ID | 20210622142245.140015-1-dja@axtens.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | 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 |
Hey Daniel, Is this patch in replacement of the one you posted earlier at https://lists.ozlabs.org/pipermail/skiboot/2021-June/017550.html ? Maybe I missed something. Thanks, Nick Child On Tue, Jun 22, 2021 at 10:23 AM Daniel Axtens <dja@axtens.net> 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. > > 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 | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c > index 244f23403fe0..657100b6a021 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,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; > -- > 2.30.2 > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
On 6/22/21 10:22 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 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. > > 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 | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c > index 244f23403fe0..657100b6a021 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,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; Thanks Daniel !! Really sorry for missing to mention first time. I think it would be useful to have error message for this failure as OPAL_PARAMETER might mean many other types of failures as well. Thanks & Regards, - Nayna
Hey Daniel, I tested this with a sha512 PKCS7 (did not do a full skiboot build though), it works well but valgrind shows unfree'd memory. Comments below: On Tue, Jun 22, 2021 at 10:23 AM Daniel Axtens <dja@axtens.net> 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. > > 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 | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c > index 244f23403fe0..657100b6a021 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,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; > + Don't forget to clean up here on error. Both pkcs7 and the struct should be freed > + if (md_alg != MBEDTLS_MD_SHA256) > + return OPAL_PARAMETER; > + Also cleanup here before returning. Hope I am helping, Nick
diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c index 244f23403fe0..657100b6a021 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,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;
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. 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 | 14 ++++++++++++++ 1 file changed, 14 insertions(+)