| Message ID | 20210728181023.25067-1-panfilov.artyom@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/1] lib/ecdsa: Fix LibreSSL before v2.7.0 | expand |
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 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/cb9faa6f98ae56d70d59505dad290dd3d381cb7b 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/fbc777429fa35312a9ea5f106692172d3153e659 --- 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/cb9faa6f98ae56d70d59505dad290dd3d381cb7b 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/fbc777429fa35312a9ea5f106692172d3153e659 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
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) +#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; +} +#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)
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> --- lib/ecdsa/ecdsa-libcrypto.c | 80 ++++++++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 1 deletion(-)