[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>