[PATCH 0/2] Align return values across crypto backends

[Storm, Christian]

This series aligns and documents the expected return values across
all crypto backends. It further fixes a bug due to inconsistent
caller and callee semantics. Please review CAREFULLY!

Best regards,
Felix Moessbauer
Siemens AG

[ Sent by Christian Storm <christi...@siemens.com> ]

Felix Moessbauer (2):
fix(openssl): correctly handle failure of EVP_DigestFinal
refactor(mbedtls): align HASH_final return values across
implementations

crypto/swupdate_HASH_mbedtls.c | 2 +-
crypto/swupdate_HASH_openssl.c | 4 +++-
include/swupdate_crypto.h | 4 ++++
3 files changed, 8 insertions(+), 2 deletions(-)

--
2.53.0

[Storm, Christian]

From: Felix Moessbauer <felix.mo...@siemens.com>

The EVP_DigestFinal_ex function returns 1 on success, 0 on failure.
However, the caller expects < 0 as failure, success otherwise. By that,
failures in the HASH_final function are silently ignored.

This currently cannot be exploited, as the md_len != SHA256_HASH_LENGTH
in cpio_utils.c catches this (the md_len stays at the initial value of
0). We fix it by explicitly comparing the result of EVP_DigestFinal_ex
against the expected values.

Fixes: d38d5359 ("Prepare to use multiple crypto engines")
Signed-off-by: Felix Moessbauer <felix.mo...@siemens.com>
---
crypto/swupdate_HASH_openssl.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/crypto/swupdate_HASH_openssl.c b/crypto/swupdate_HASH_openssl.c
index 9820b9c5..32fe8047 100644
--- a/crypto/swupdate_HASH_openssl.c
+++ b/crypto/swupdate_HASH_openssl.c
@@ -87,8 +87,10 @@ static int openssl_HASH_final(void *ctx, unsigned char *md_value,
if (!dgst)
return -EFAULT;

- return EVP_DigestFinal_ex (dgst->ctx, md_value, md_len);
+ if (EVP_DigestFinal_ex (dgst->ctx, md_value, md_len) != 1)
+ return -EIO;

+ return 0;
}

static void openssl_HASH_cleanup(void *ctx)
--
2.53.0

[Storm, Christian]

From: Felix Moessbauer <felix.mo...@siemens.com>

The HASH_final implementations are expected to return 0 on success and a
negative value on error. While the mbedtls_HASH_final correctly
implements this interface, it still is better to align the return codes
across the backends - what we do in this commit.

While doing so, we also document the expected return values of the
crypto backends.

Signed-off-by: Felix Moessbauer <felix.mo...@siemens.com>
---

crypto/swupdate_HASH_mbedtls.c | 2 +-
include/swupdate_crypto.h | 4 ++++
2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/crypto/swupdate_HASH_mbedtls.c b/crypto/swupdate_HASH_mbedtls.c
index 4165b940..9005256b 100644
--- a/crypto/swupdate_HASH_mbedtls.c
+++ b/crypto/swupdate_HASH_mbedtls.c
@@ -99,7 +99,7 @@ static int mbedtls_HASH_final(void *ctx, unsigned char *md_value,
*md_len = mbedtls_md_get_size(dgst->mbedtls_md_context.md_info);
#endif
}
- return 1;
+ return 0;

}

diff --git a/include/swupdate_crypto.h b/include/swupdate_crypto.h
index aa9da964..0e579dfb 100644
--- a/include/swupdate_crypto.h
+++ b/include/swupdate_crypto.h
@@ -46,6 +46,10 @@ typedef struct {
void (*DECRYPT_cleanup)(void *ctx);
} swupdate_decrypt_lib;

+/*
+ * Return:
+ * 0 on success, < 0 on error
+ */
typedef struct {
void *(*HASH_init)(const char *SHAlength);
int (*HASH_update)(void *ctx, const unsigned char *buf, size_t len);
--
2.53.0

[Storm, Christian]

[Storm, Christian]

From: Felix Moessbauer <felix.mo...@siemens.com>

The EVP_DigestFinal_ex function returns 1 on success, 0 on failure.
However, the caller expects < 0 as failure, success otherwise. By that,
failures in the HASH_final function are silently ignored.

This currently cannot be exploited, as the md_len != SHA256_HASH_LENGTH
in cpio_utils.c catches this (the md_len stays at the initial value of
0). We fix it by explicitly comparing the result of EVP_DigestFinal_ex
against the expected values.

Fixes: d38d5359 ("Prepare to use multiple crypto engines")

Signed-off-by: Felix Moessbauer <felix.mo...@siemens.com>
---

[Storm, Christian]

From: Felix Moessbauer <felix.mo...@siemens.com>

The HASH_final implementations are expected to return 0 on success and a
negative value on error. While the mbedtls_HASH_final correctly
implements this interface, it still is better to align the return codes
across the backends - what we do in this commit.

While doing so, we also document the expected return values of the
crypto backends.

Signed-off-by: Felix Moessbauer <felix.mo...@siemens.com>
---

[Stefano Babic]

Hi Felix,

This is the digest implementation for openSSL, and then I look at the
EVP_DigestFinal_ex function
(https://github.com/openssl/openssl/blob/master/crypto/evp/digest.c),
this returns 0 in case of error - so ok, I see that an error is ignored.
But why do we have to compare with "1" ? I do not see this in openSSL code.

Stefano

> + return 0;
> }
>
> static void openssl_HASH_cleanup(void *ctx)

--

_______________________________________________________________________
Nabla Software Engineering GmbH
Hirschstr. 111A | 86156 Augsburg | Tel: +49 821 45592596
Geschäftsführer : Stefano Babic | HRB 40522 Augsburg
E-Mail: sba...@nabladev.com

[Stefano Babic]

Reviewed-by: Stefano Babic <stefan...@swupdate.org>

[Stefano Babic]

Applied, but patch resulted malformed (probabkly due the mailer) and I
had to change myself the code.

Best regards,
Stefano

[Stefano Babic]

Same here, applied but it resulted malformed.

[Stefano Babic]

Hi Felix,

On 5/26/26 13:45, MOESSBAUER, Felix wrote:

> Thanks, but that's strange. On my side the patch mail applies cleanly,
> and I'm sending patches to MLs on a daily basis without any complaints
> (yet).
>
> Well... the patch was re-sent by Christian as my mails are not accepted
> on this ML.

You need to be registered to ML to be able to send patches. This was
introduced after the ML was over-flooded when ML was open to everybody.

However, ML is hosted by Google, and Google requires to have a gmail
account or a gmail account to associate. So nothing is free.

I was also thinking about to host myself the ML and I have already
prepared the server, but if users are happy with current setup, I won't
change.

> Maybe that's the culprit.

Yes, it is, all patches sent by Christian are malformed.

Best regards,
Stefano

>
> Felix

[Storm, Christian]

Hi,

>>> Applied, but patch resulted malformed (probabkly due the mailer) and I
>>> had to change myself the code.
>> Thanks, but that's strange. On my side the patch mail applies cleanly,
>> and I'm sending patches to MLs on a daily basis without any complaints
>> (yet).
>> Well... the patch was re-sent by Christian as my mails are not accepted
>> on this ML.
>
> You need to be registered to ML to be able to send patches. This was introduced after the ML was over-flooded when ML was open to everybody.
>
> However, ML is hosted by Google, and Google requires to have a gmail account or a gmail account to associate. So nothing is free.
>
> I was also thinking about to host myself the ML and I have already prepared the server, but if users are happy with current setup, I won't change.
>
>> Maybe that's the culprit.
>
> Yes, it is, all patches sent by Christian are malformed.

I'm sorry for the inconvenience! We seem to have some serious issues with our Mailer since (some) patches internally passed to me do also not apply cleanly, seemingly randomly...

Again, sorry for the inconvenience and efforts caused on your side!


Kind regards,
Christian


--
Dr. Christian Storm
Siemens AG, FT RPD CED
Friedrich-Ludwig-Bauer-Str. 3, 85748 Garching, Germany