| Message ID | 20220119201612.421953-1-erichte@linux.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | libstb/create-container: avoid using deprecated APIs when compiling with OpenSSL 3.0 | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-Docker_builds_and_checks | success | Successfully ran 9 jobs. |
On 1/19/22 21:16, Eric Richter wrote: > OpenSSL 3.0 has deprecated functions that operate on raw key data, however the > closest replacement function are not available in OpenSSL 1.x. This patch > attempts to maintain compatibility with both 3.0 and 1.x versions. > > Avoids using the following deprecated functions when compiling with 3.0: > - EC_KEY_get0_group > - EC_KEY_get0_public_key > - EC_POINT_point2bn > - EC_KEY_free > > Signed-off-by: Eric Richter <erichte@linux.ibm.com> > --- > > NOTE: While this patch should work, I have not yet been able to adequately > test this on actual hardware. The resulting data that stored in pubKeyData[] > appears to be identical when compiling with both versions of OpenSSL (minus > the one byte header that is removed anyway), thus it should work as expected. Would you prefer to have some tests on HW confirm that this patch is safe merge ? or should we proceed ? Thanks C.
On 1/24/22 9:55 AM, Cédric Le Goater wrote: > On 1/19/22 21:16, Eric Richter wrote: >> OpenSSL 3.0 has deprecated functions that operate on raw key data, however the >> closest replacement function are not available in OpenSSL 1.x. This patch >> attempts to maintain compatibility with both 3.0 and 1.x versions. >> >> Avoids using the following deprecated functions when compiling with 3.0: >> - EC_KEY_get0_group >> - EC_KEY_get0_public_key >> - EC_POINT_point2bn >> - EC_KEY_free >> >> Signed-off-by: Eric Richter <erichte@linux.ibm.com> >> --- >> >> NOTE: While this patch should work, I have not yet been able to adequately >> test this on actual hardware. The resulting data that stored in pubKeyData[] >> appears to be identical when compiling with both versions of OpenSSL (minus >> the one byte header that is removed anyway), thus it should work as expected. > > Would you prefer to have some tests on HW confirm that this patch is safe > merge ? or should we proceed ? > I'd prefer to err on the side of caution on this, I am no expert with OpenSSL nor this particular piece of secure boot code. That said, I had initially assumed that this would need a full op-build to test. However, I just managed to test this by using three skiboot images: - using OpenSSL 1.x - using OpenSSL 3.x - commenting out the changed code, leaving the container pubkey as zeros. The first two images booted up to Petitboot without issue. The third failed as expected, which mostly served as a sanity check that secure boot errors would be enforced with development firmware. I would prefer having a larger scale test of this, as I don't want this to be a potential source of problems in production in the future. That is not a strong preference though, if this testing method sounds relatively sane to more than just me, I am fine with moving forward.
On Tue, Feb 01, 2022 at 12:56:52PM -0600, Eric Richter wrote: >However, I just managed to test this by using three skiboot images: > - using OpenSSL 1.x > - using OpenSSL 3.x > - commenting out the changed code, leaving the container pubkey as zeros. > >The first two images booted up to Petitboot without issue. The third failed as >expected, which mostly served as a sanity check that secure boot errors would >be enforced with development firmware. > >I would prefer having a larger scale test of this, as I don't want this to be >a potential source of problems in production in the future. That is not a >strong preference though, if this testing method sounds relatively sane to >more than just me, I am fine with moving forward. Seems like a reasonable test. Applied to master, as more people are starting to build on newer distros and this resolves a sticking point.
Hi,
I'm seeing the same type of error when running "make check" on latest
ubuntu. Don't we need the same type of fix for print-container.c?
# make libstb-check
[ RUN-TEST ] libstb/test/run-stb-container
[ HOSTCC ] libstb/print-container.c
libstb/print-container.c: In function 'verify_signature':
libstb/print-container.c:405:9: error: 'EC_KEY_new' is deprecated: Since
OpenSSL 3.0 [-Werror=deprecated-declarations]
405 | ec_key = EC_KEY_new();
| ^~~~~~
[ cutting more errors... ]
Fred
On 19/01/2022 21:16, Eric Richter wrote:
> OpenSSL 3.0 has deprecated functions that operate on raw key data, however the
> closest replacement function are not available in OpenSSL 1.x. This patch
> attempts to maintain compatibility with both 3.0 and 1.x versions.
>
> Avoids using the following deprecated functions when compiling with 3.0:
> - EC_KEY_get0_group
> - EC_KEY_get0_public_key
> - EC_POINT_point2bn
> - EC_KEY_free
>
> Signed-off-by: Eric Richter <erichte@linux.ibm.com>
> ---
>
> NOTE: While this patch should work, I have not yet been able to adequately
> test this on actual hardware. The resulting data that stored in pubKeyData[]
> appears to be identical when compiling with both versions of OpenSSL (minus
> the one byte header that is removed anyway), thus it should work as expected.
>
>
> libstb/create-container.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/libstb/create-container.c b/libstb/create-container.c
> index 0c7bf13b..4e198dab 100644
> --- a/libstb/create-container.c
> +++ b/libstb/create-container.c
> @@ -11,6 +11,9 @@
> #include <openssl/ec.h>
> #include <openssl/ecdsa.h>
> #include <openssl/evp.h>
> +#if OPENSSL_VERSION_NUMBER >= 0x30000000
> +#include <openssl/core_names.h>
> +#endif
> #include <openssl/opensslv.h>
> #include <openssl/ossl_typ.h>
> #include <openssl/pem.h>
> @@ -45,7 +48,7 @@ void usage(int status);
> void getPublicKeyRaw(ecc_key_t *pubkeyraw, char *filename)
> {
> EVP_PKEY* pkey;
> - unsigned char pubkeyData[1 + 2 * EC_COORDBYTES];
> + unsigned char pubkeyData[1 + 2 * EC_COORDBYTES] = {0};
>
> FILE *fp = fopen(filename, "r");
> if (!fp)
> @@ -64,6 +67,10 @@ void getPublicKeyRaw(ecc_key_t *pubkeyraw, char *filename)
> }
>
> if (pkey) {
> +#if OPENSSL_VERSION_NUMBER >= 0x30000000
> + size_t sz;
> + EVP_PKEY_get_octet_string_param(pkey, OSSL_PKEY_PARAM_ENCODED_PUBLIC_KEY, pubkeyData, sizeof(pubkeyData), &sz);
> +#else
> EC_KEY *key;
> const EC_GROUP *ecgrp;
> const EC_POINT *ecpoint;
> @@ -87,6 +94,7 @@ void getPublicKeyRaw(ecc_key_t *pubkeyraw, char *filename)
>
> BN_free(pubkeyBN);
> EC_KEY_free(key);
> +#endif
> EVP_PKEY_free(pkey);
> }
> else {
diff --git a/libstb/create-container.c b/libstb/create-container.c index 0c7bf13b..4e198dab 100644 --- a/libstb/create-container.c +++ b/libstb/create-container.c @@ -11,6 +11,9 @@ #include <openssl/ec.h> #include <openssl/ecdsa.h> #include <openssl/evp.h> +#if OPENSSL_VERSION_NUMBER >= 0x30000000 +#include <openssl/core_names.h> +#endif #include <openssl/opensslv.h> #include <openssl/ossl_typ.h> #include <openssl/pem.h> @@ -45,7 +48,7 @@ void usage(int status); void getPublicKeyRaw(ecc_key_t *pubkeyraw, char *filename) { EVP_PKEY* pkey; - unsigned char pubkeyData[1 + 2 * EC_COORDBYTES]; + unsigned char pubkeyData[1 + 2 * EC_COORDBYTES] = {0}; FILE *fp = fopen(filename, "r"); if (!fp) @@ -64,6 +67,10 @@ void getPublicKeyRaw(ecc_key_t *pubkeyraw, char *filename) } if (pkey) { +#if OPENSSL_VERSION_NUMBER >= 0x30000000 + size_t sz; + EVP_PKEY_get_octet_string_param(pkey, OSSL_PKEY_PARAM_ENCODED_PUBLIC_KEY, pubkeyData, sizeof(pubkeyData), &sz); +#else EC_KEY *key; const EC_GROUP *ecgrp; const EC_POINT *ecpoint; @@ -87,6 +94,7 @@ void getPublicKeyRaw(ecc_key_t *pubkeyraw, char *filename) BN_free(pubkeyBN); EC_KEY_free(key); +#endif EVP_PKEY_free(pkey); } else {
OpenSSL 3.0 has deprecated functions that operate on raw key data, however the closest replacement function are not available in OpenSSL 1.x. This patch attempts to maintain compatibility with both 3.0 and 1.x versions. Avoids using the following deprecated functions when compiling with 3.0: - EC_KEY_get0_group - EC_KEY_get0_public_key - EC_POINT_point2bn - EC_KEY_free Signed-off-by: Eric Richter <erichte@linux.ibm.com> --- NOTE: While this patch should work, I have not yet been able to adequately test this on actual hardware. The resulting data that stored in pubKeyData[] appears to be identical when compiling with both versions of OpenSSL (minus the one byte header that is removed anyway), thus it should work as expected. libstb/create-container.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)