Re: [PATCH 1/1] lib/ecdsa: Fix LibreSSL before v2.7.0
Hi Artem,
I'm re-adding the u-boot mailing list to the CC field, as I see your email contains no sensitive information.
On 7/28/21 2:30 PM, Artem Panfilov wrote:
We have broken CI builds on your bare-metal CentOS 7 servers with latest master. I think it is good reason to have a support. Our corporate clients have CentOS 7 too.
I thought we solved problems associated with bare builds on ancient OSes by using containerized builds. There's is CI infrastructure based on this on source.denx.de that uses docker. As things change, and we eventually move to GNU TLS, a lot more things might break for old build hosts. I believe it's worth looking at containerized builds.
Given that a solution exists for your problem, I think the argument for this patch quite weak.
There is no nice way to handle openssl difference. You could check other commits related to openssl compatibility. They all looks ugly.
Another solution is to disable CONFIG_TOOLS_LIBCRYPTO by default that broke our builds.
Do you need cryptographic features in mkimage? If not just disable TOOLS_LIBCRYPTO in your builds.
Alex
Best regards, Artem
ср, 28 июл. 2021 г., 22:16 Alex G. <mr.nuke.me@gmail.com mailto:mr.nuke.me@gmail.com>:
On 7/28/21 1:10 PM, Artem Panfilov wrote: > Fix LibreSSL compilation for versions before v2.7.0. > > Fix following compilation issue when CONFIG_TOOLS_LIBCRYPTO is enabled: > tools/lib/ecdsa/ecdsa-libcrypto.o: In function `prepare_ctx': > ecdsa-libcrypto.c:(.text+0x94): undefined reference to > `OPENSSL_init_ssl' > ecdsa-libcrypto.c:(.text+0x148): undefined reference to > `EC_GROUP_order_bits' > tools/lib/ecdsa/ecdsa-libcrypto.o: In function > `ecdsa_check_signature.isra.0': > ecdsa-libcrypto.c:(.text+0x32c): undefined reference to `ECDSA_SIG_set0' > tools/lib/ecdsa/ecdsa-libcrypto.o: In function `ecdsa_sign': > ecdsa-libcrypto.c:(.text+0x42c): undefined reference to `ECDSA_SIG_get0' > ecdsa-libcrypto.c:(.text+0x443): undefined reference to `BN_bn2binpad' > ecdsa-libcrypto.c:(.text+0x455): undefined reference to `BN_bn2binpad' > tools/lib/ecdsa/ecdsa-libcrypto.o: In function `ecdsa_add_verify_data': > ecdsa-libcrypto.c:(.text+0x5fa): undefined reference to > `EC_GROUP_order_bits' > ecdsa-libcrypto.c:(.text+0x642): undefined reference to > `EC_POINT_get_affine_coordinates' > > Signed-off-by: Artem Panfilov <panfilov.artyom@gmail.com <mailto:panfilov.artyom@gmail.com>> > --- > lib/ecdsa/ecdsa-libcrypto.c | 80 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 79 insertions(+), 1 deletion(-) > > diff --git a/lib/ecdsa/ecdsa-libcrypto.c b/lib/ecdsa/ecdsa-libcrypto.c > index 1757a14562..50aa093acd 100644 > --- a/lib/ecdsa/ecdsa-libcrypto.c > +++ b/lib/ecdsa/ecdsa-libcrypto.c > @@ -24,6 +24,70 @@ > #include <openssl/ec.h> > #include <openssl/bn.h> > > +#if OPENSSL_VERSION_NUMBER < 0x10100000L || \ > + (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER < 0x02070000fL) Is there a reasonable use case for supporting an external library that is more than three years old at this point? Otherwise NAK, as such #ifdefs don't really help with readability. I think Simon will agree here. There's also the issue of deciding what version we have at compile time, which ignores the dynamic linking nature of .so libs. This leads into soname versioning territory. Let's not go there. Alex > +#include <openssl/err.h> > + > +static int EC_GROUP_order_bits(const EC_GROUP *group) > +{ > + int ret = 0; > + BIGNUM *order; > + > + if (!group) > + return ret; > + > + order = BN_new(); > + > + if (!order) { > + ERR_clear_error(); > + return ret; > + } > + > + if (!EC_GROUP_get_order(group, order, NULL)) { > + ERR_clear_error(); > + BN_free(order); > + return ret; > + } > + > + ret = BN_num_bits(order); > + BN_free(order); > + return ret; > +} > + > +static void ECDSA_SIG_get0(const ECDSA_SIG *sig, const BIGNUM **pr, const BIGNUM **ps) > +{ > + if (pr != NULL) > + *pr = sig->r; > + if (ps != NULL) > + *ps = sig->s; > +} > + > +static int ECDSA_SIG_set0(ECDSA_SIG *sig, BIGNUM *r, BIGNUM *s) > +{ > + if (r == NULL || s == NULL) > + return 0; > + BN_clear_free(sig->r); > + BN_clear_free(sig->s); > + sig->r = r; > + sig->s = s; > + return 1; > +} > + > +int BN_bn2binpad(const BIGNUM *a, unsigned char *to, int tolen) > +{ > + int n = BN_num_bytes(a); > + > + if (n < 0 || n > tolen) > + return -1; > + > + memset(to, 0, tolen - n); > + if (BN_bn2bin(a, to + tolen - n) < 0) > + return -1; > + > + return tolen; > +}tures in mkimage? If not just disable TOOLS_LIBCRYPTO in your builds. > +#endif > + > /* Image signing context for openssl-libcrypto */ > struct signer { > EVP_PKEY *evp_key; /* Pointer to EVP_PKEY object */ > @@ -34,9 +98,18 @@ struct signer { > > static int alloc_ctx(struct signer *ctx, const struct image_sign_info *info) > { > + int ret = 0; > + > memset(ctx, 0, sizeof(*ctx)); > > - if (!OPENSSL_init_ssl(0, NULL)) { > +#if OPENSSL_VERSION_NUMBER < 0x10100000L || \ > +(defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER < 0x02070000fL) > + ret = SSL_library_init(); > +#else > + ret = OPENSSL_init_ssl(0, NULL); > +#endif > + > + if (!ret) { > fprintf(stderr, "Failure to init SSL library\n"); > return -1; > } > @@ -285,7 +358,12 @@ static int do_add(struct signer *ctx, void *fdt, const char *key_node_name) > x = BN_new(); > y = BN_new(); > point = EC_KEY_get0_public_key(ctx->ecdsa_key); > +#if OPENSSL_VERSION_NUMBER < 0x10100000L || \ > +(defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER < 0x02070000fL) > + EC_POINT_get_affine_coordinates_GFp(group, point, x, y, NULL); > +#else > EC_POINT_get_affine_coordinates(group, point, x, y, NULL); > +#endif > > ret = fdt_setprop_string(fdt, key_node, "ecdsa,curve", curve_name); > if (ret < 0) >
On Wed, Jul 28, 2021 at 03:00:44PM -0500, Alex G. wrote:
Hi Artem,
I'm re-adding the u-boot mailing list to the CC field, as I see your email contains no sensitive information.
On 7/28/21 2:30 PM, Artem Panfilov wrote:
We have broken CI builds on your bare-metal CentOS 7 servers with latest master. I think it is good reason to have a support. Our corporate clients have CentOS 7 too.
I thought we solved problems associated with bare builds on ancient OSes by using containerized builds. There's is CI infrastructure based on this on source.denx.de that uses docker. As things change, and we eventually move to GNU TLS, a lot more things might break for old build hosts. I believe it's worth looking at containerized builds.
Given that a solution exists for your problem, I think the argument for this patch quite weak.
There is a fine line at least that I'm willing to walk in terms of supporting ancient OSes directly and also not making things overly complicated in our own tree. That said, openssl tends to be one of the ones where it does get hard to support old versions. LibreSSL 2.7.5 was released December 15th, 2018 and is the end of the 2.7.x line it seems.
I'm interested to hear what the case is where the right call is the say you're building modern software for real world use against such old libraries.
On 28.07.2021 23:07, Tom Rini wrote:
There is a fine line at least that I'm willing to walk in terms of supporting ancient OSes directly and also not making things overly complicated in our own tree. That said, openssl tends to be one of the ones where it does get hard to support old versions. LibreSSL 2.7.5 was released December 15th, 2018 and is the end of the 2.7.x line it seems.
I'm interested to hear what the case is where the right call is the say you're building modern software for real world use against such old libraries.
Hi Tom, I have a specific test case where I test if it's still possible to build upstream master u-boot on our infrastructure (progression testing). I also test runtime on our boards.
I understand that nowadays everyone uses docker container, but we have limited docker nodes right now on our site.
If you don't want to support OpenSSL < 1.1.0 and do not test it, then I suggest dropping it all over the tree because it doesn't make sense and looks misleading with such a partial solution.
Best regards, Artem
On Thu, Jul 29, 2021 at 01:29:35AM +0300, Artem Panfilov wrote:
On 28.07.2021 23:07, Tom Rini wrote:
There is a fine line at least that I'm willing to walk in terms of supporting ancient OSes directly and also not making things overly complicated in our own tree. That said, openssl tends to be one of the ones where it does get hard to support old versions. LibreSSL 2.7.5 was released December 15th, 2018 and is the end of the 2.7.x line it seems.
I'm interested to hear what the case is where the right call is the say you're building modern software for real world use against such old libraries.
Hi Tom, I have a specific test case where I test if it's still possible to build upstream master u-boot on our infrastructure (progression testing). I also test runtime on our boards.
I understand that nowadays everyone uses docker container, but we have limited docker nodes right now on our site.
If you don't want to support OpenSSL < 1.1.0 and do not test it, then I suggest dropping it all over the tree because it doesn't make sense and looks misleading with such a partial solution.
Part of the question is then, were you enabling the SSL-related parts before this change? Or did the way the code is now being enabled/disabled trigger this now being enabled when it wasn't before?
On 29.07.2021 01:56, Tom Rini wrote:
Part of the question is then, were you enabling the SSL-related parts before this change? Or did the way the code is now being enabled/disabled trigger this now being enabled when it wasn't before?
This commit broke the build: https://gitlab.com/u-boot/u-boot/-/commit/cb9faa6f98ae56d70d59505dad290dd3d3...
Our testing board defconfig: https://gitlab.com/u-boot/u-boot/-/blob/master/configs/hsdk_defconfig
I also found this patch that was committed 4 months ago. It includes OpenSSL compatibility for old versions: https://gitlab.com/u-boot/u-boot/-/commit/fbc777429fa35312a9ea5f106692172d31...
--- Artem
On Thu, Jul 29, 2021 at 02:37:10AM +0300, Artem Panfilov wrote:
On 29.07.2021 01:56, Tom Rini wrote:
Part of the question is then, were you enabling the SSL-related parts before this change? Or did the way the code is now being enabled/disabled trigger this now being enabled when it wasn't before?
This commit broke the build: https://gitlab.com/u-boot/u-boot/-/commit/cb9faa6f98ae56d70d59505dad290dd3d3...
OK. How? Or rather, what part of that is causing previously unbuilt code to now be built?
Our testing board defconfig: https://gitlab.com/u-boot/u-boot/-/blob/master/configs/hsdk_defconfig
OK.
I also found this patch that was committed 4 months ago. It includes OpenSSL compatibility for old versions: https://gitlab.com/u-boot/u-boot/-/commit/fbc777429fa35312a9ea5f106692172d31...
Yes, true. And that's two 1-line if/else. That's a reasonable to me level of effort to keep supporting older hosts. Your patch is adding in 60 lines. I really do want to dig a bit more here.
And honestly, part of my concerns also go around "who is going to maintain / test this area?". We don't have these older versions in CI (or we would have seen the problem before merging). Are you volunteering to support the relevant code areas here but on older openssl/libressl ?
On 29.07.2021 02:43, Tom Rini wrote:
Yes, true. And that's two 1-line if/else. That's a reasonable to me level of effort to keep supporting older hosts. Your patch is adding in 60 lines. I really do want to dig a bit more here.
For me, it doesn't matter how many lines of code were added if I can't build host tools with older OpenSSL versions. So what's the point of keeping OpenSSL backward compatibility?
And honestly, part of my concerns also go around "who is going to maintain / test this area?". We don't have these older versions in CI (or we would have seen the problem before merging). Are you volunteering to support the relevant code areas here but on older openssl/libressl ?
We already have a nightly Jenkins CI job that tracks u-boot master and sends internal reports.
The best way would be to testing in the upstream azure pipeline.
You could add the following steps in your trini/u-boot-gitlab-ci-runner docker image: wget -O - https://www.openssl.org/source/old/1.0.2/openssl-1.0.2k.tar.gz | tar -C /opt -xz && \ cd /opt/openssl-1.0.2k/ && ./config shared && \ make && \ make install
In azure pipeline add new job for testing with old OpenSSL: make tools-only_config tools-only NO_SDL=1 \ HOSTLDFLAGS="-ldl -L/usr/local/ssl/lib" \ HOSTCFLAGS="-I/usr/local/ssl/include"
--- Artem
On Thu, Jul 29, 2021 at 01:40:41PM +0300, Artem Panfilov wrote:
On 29.07.2021 02:43, Tom Rini wrote:
Yes, true. And that's two 1-line if/else. That's a reasonable to me level of effort to keep supporting older hosts. Your patch is adding in 60 lines. I really do want to dig a bit more here.
For me, it doesn't matter how many lines of code were added if I can't build host tools with older OpenSSL versions. So what's the point of keeping OpenSSL backward compatibility?
Well yes, this is part of the question now, is there enough interest in the old version to bother with? The other part of the question is what's being built now that wasn't being built before, and is that a bug or a feature (a less CONFIG-dependent set of tools is good for generic distributions).
And honestly, part of my concerns also go around "who is going to maintain / test this area?". We don't have these older versions in CI (or we would have seen the problem before merging). Are you volunteering to support the relevant code areas here but on older openssl/libressl ?
We already have a nightly Jenkins CI job that tracks u-boot master and sends internal reports.
The best way would be to testing in the upstream azure pipeline.
You could add the following steps in your trini/u-boot-gitlab-ci-runner docker image: wget -O - https://www.openssl.org/source/old/1.0.2/openssl-1.0.2k.tar.gz | tar -C /opt -xz && \ cd /opt/openssl-1.0.2k/ && ./config shared && \ make && \ make install
In azure pipeline add new job for testing with old OpenSSL: make tools-only_config tools-only NO_SDL=1 \ HOSTLDFLAGS="-ldl -L/usr/local/ssl/lib" \ HOSTCFLAGS="-I/usr/local/ssl/include"
And the next time something breaks, are you going to be available to fix it?
On 29.07.2021 15:59, Tom Rini wrote:
Well yes, this is part of the question now, is there enough interest in the old version to bother with? The other part of the question is what's being built now that wasn't being built before, and is that a bug or a feature (a less CONFIG-dependent set of tools is good for generic distributions).
OK, if someone else will report the same issue after u-boot release, then it should be fixed. Currently, I am okay with my local fix by disabling the CONFIG_TOOLS_LIBCRYPTO option.
--- Artem
Hi Artem
On 7/29/21 9:52 AM, Artem Panfilov wrote:
On 29.07.2021 15:59, Tom Rini wrote:
Well yes, this is part of the question now, is there enough interest in the old version to bother with? The other part of the question is what's being built now that wasn't being built before, and is that a bug or a feature (a less CONFIG-dependent set of tools is good for generic distributions).
OK, if someone else will report the same issue after u-boot release, then it should be fixed. Currently, I am okay with my local fix by disabling the CONFIG_TOOLS_LIBCRYPTO option.
ECDSA signing was not verified against a libcrypto that old. Given that signatures are non-deterministic, I doubt we could have a CI test that says old-libcrypto, known block must equal known signature.
When we added ECDSA, there was not a need to consider old libcrypto versions, but I also did not pay attention to the #ifdefs in the much older RSA path. I'm sorry that you had to go through the frustrations of getting a patch rejected which does something the codebase already does.
I am going to take a look at cleaning up the RSA path. There's no point in maintaining backwards compatibility if we're not doing it across the board.
Alex
participants (3)
-
Alex G. -
Artem Panfilov -
Tom Rini