From 1490d0cb8b5ef6cc65b12e2ec82b63d61faa1cfb Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Tue, 28 Apr 2020 15:09:18 +0200 Subject: [PATCH 01/16] Treat OPEN same way as DECRYPT in case of a CipherUpdate/Final error Without this change Decrypt returned INVALID_PARAM, while Open returned INTERNAL in the same case (e.g. wrong key). Change-Id: I8aaf77b4a550303a68834dd0ace9fa5a52130868 --- src/encrypt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/encrypt.c b/src/encrypt.c index dfc2fcb..510e6f8 100644 --- a/src/encrypt.c +++ b/src/encrypt.c @@ -1064,7 +1064,7 @@ int encrypt_update(yaca_context_h ctx, ret = EVP_CipherUpdate(c->cipher_ctx, output, &loutput_len, input, input_len); if (ret != 1 || loutput_len < 0) { - if (mode == EVP_CIPH_CCM_MODE && op_type == OP_DECRYPT) { + if (mode == EVP_CIPH_CCM_MODE && (op_type == OP_DECRYPT || op_type == OP_OPEN)) { /* A non positive return value from EVP_CipherUpdate should be considered as * a failure to authenticate ciphertext and/or AAD. * It does not necessarily indicate a more serious error. @@ -1108,7 +1108,7 @@ int encrypt_finalize(yaca_context_h ctx, if (mode != EVP_CIPH_WRAP_MODE && mode != EVP_CIPH_CCM_MODE) { ret = EVP_CipherFinal(c->cipher_ctx, output, &loutput_len); if (ret != 1 || loutput_len < 0) { - if (mode == EVP_CIPH_GCM_MODE && op_type == OP_DECRYPT) + if (mode == EVP_CIPH_GCM_MODE && (op_type == OP_DECRYPT || op_type == OP_OPEN)) /* A non positive return value from EVP_CipherFinal should be considered as * a failure to authenticate ciphertext and/or AAD. * It does not necessarily indicate a more serious error. -- 2.7.4 From f2705161226e3c67744afd966d752247b1c90f09 Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Tue, 28 Apr 2020 16:33:25 +0200 Subject: [PATCH 02/16] Unify errors treating from OpenSSL functions. Have 3 lines block almost everywhere where we call OpenSSL function and want to handle its error code. Always ERROR_DUMP in such a case. Also some other unification of OpenSSL returns where we don't care about its errors (loading keys where errors are expected due to autodetection). Change-Id: Ie9e2f19bae099cfaddaa9c45a6de985f09b3f97b --- src/crypto.c | 10 ++++++---- src/digest.c | 8 ++++++-- src/encrypt.c | 60 ++++++++++++++++++++++++++++++++++++----------------------- src/key.c | 33 ++++++++++++++++---------------- src/sign.c | 25 +++++++++++++++++-------- 5 files changed, 83 insertions(+), 53 deletions(-) diff --git a/src/crypto.c b/src/crypto.c index 7123ea8..98e941a 100644 --- a/src/crypto.c +++ b/src/crypto.c @@ -331,8 +331,9 @@ API int yaca_malloc(size_t size, void **memory) *memory = OPENSSL_malloc(size); if (*memory == NULL) { - ERROR_DUMP(YACA_ERROR_OUT_OF_MEMORY); - return YACA_ERROR_OUT_OF_MEMORY; + const int ret = YACA_ERROR_OUT_OF_MEMORY; + ERROR_DUMP(ret); + return ret; } return YACA_ERROR_NONE; @@ -356,8 +357,9 @@ API int yaca_realloc(size_t size, void **memory) void *tmp = OPENSSL_realloc(*memory, size); if (tmp == NULL) { - ERROR_DUMP(YACA_ERROR_OUT_OF_MEMORY); - return YACA_ERROR_OUT_OF_MEMORY; + const int ret = YACA_ERROR_OUT_OF_MEMORY; + ERROR_DUMP(ret); + return ret; } *memory = tmp; diff --git a/src/digest.c b/src/digest.c index 9812d6d..9b88442 100644 --- a/src/digest.c +++ b/src/digest.c @@ -93,8 +93,11 @@ static int get_digest_output_length(const yaca_context_h ctx, return YACA_ERROR_INVALID_PARAMETER; int md_size = EVP_MD_CTX_size(c->md_ctx); - if (md_size <= 0) - return YACA_ERROR_INTERNAL; + if (md_size <= 0) { + const int ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + return ret; + } *output_len = md_size; @@ -130,6 +133,7 @@ int digest_get_algorithm(yaca_digest_algorithm_e algo, const EVP_MD **md) if (ret == YACA_ERROR_NONE && *md == NULL) { ret = YACA_ERROR_INTERNAL; ERROR_DUMP(ret); + return ret; } return ret; diff --git a/src/encrypt.c b/src/encrypt.c index 510e6f8..a369e53 100644 --- a/src/encrypt.c +++ b/src/encrypt.c @@ -305,8 +305,9 @@ static int get_encrypt_output_length(const yaca_context_h ctx, size_t input_len, block_size = EVP_CIPHER_CTX_block_size(c->cipher_ctx); if (block_size <= 0) { - ERROR_DUMP(YACA_ERROR_INTERNAL); - return YACA_ERROR_INTERNAL; + const int ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + return ret; } if (input_len > 0) { @@ -504,8 +505,11 @@ static int encrypt_ctx_setup(struct yaca_encrypt_context_s *c, assert(key != YACA_KEY_NULL); const EVP_CIPHER *cipher = EVP_CIPHER_CTX_cipher(c->cipher_ctx); - if (cipher == NULL) - return YACA_ERROR_INTERNAL; + if (cipher == NULL) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + return ret; + } lkey = key_get_simple(key); assert(lkey != NULL); @@ -699,8 +703,9 @@ static int set_encrypt_property(yaca_context_h ctx, return YACA_ERROR_INVALID_PARAMETER; if (EVP_CipherUpdate(c->cipher_ctx, NULL, &len, value, value_len) != 1) { - ERROR_DUMP(YACA_ERROR_INTERNAL); - return YACA_ERROR_INTERNAL; + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + return ret; } c->state = ENC_CTX_AAD_UPDATED; break; @@ -710,8 +715,9 @@ static int set_encrypt_property(yaca_context_h ctx, return YACA_ERROR_INVALID_PARAMETER; if (EVP_CipherUpdate(c->cipher_ctx, NULL, &len, value, value_len) != 1) { - ERROR_DUMP(YACA_ERROR_INTERNAL); - return YACA_ERROR_INTERNAL; + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + return ret; } c->state = ENC_CTX_AAD_UPDATED; break; @@ -722,8 +728,9 @@ static int set_encrypt_property(yaca_context_h ctx, return YACA_ERROR_INVALID_PARAMETER; if (EVP_CIPHER_CTX_ctrl(c->cipher_ctx, EVP_CTRL_GCM_SET_TAG, value_len, (void*)value) != 1) { - ERROR_DUMP(YACA_ERROR_INTERNAL); - return YACA_ERROR_INTERNAL; + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + return ret; } c->state = ENC_CTX_TAG_SET; break; @@ -772,8 +779,9 @@ static int set_encrypt_property(yaca_context_h ctx, int padding = *(yaca_padding_e*)value == YACA_PADDING_NONE ? 0 : 1; if (EVP_CIPHER_CTX_set_padding(c->cipher_ctx, padding) != 1) { - ERROR_DUMP(YACA_ERROR_INTERNAL); - return YACA_ERROR_INTERNAL; + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + return ret; } if (c->backup_ctx != NULL) c->backup_ctx->padding = padding; @@ -825,9 +833,9 @@ static int get_encrypt_property(const yaca_context_h ctx, yaca_property_e proper EVP_CTRL_GCM_GET_TAG, c->tag_len, tag) != 1) { - yaca_free(tag); - ERROR_DUMP(YACA_ERROR_INTERNAL); - return YACA_ERROR_INTERNAL; + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + goto err; } *value = tag; *value_len = c->tag_len; @@ -849,9 +857,9 @@ static int get_encrypt_property(const yaca_context_h ctx, yaca_property_e proper EVP_CTRL_CCM_GET_TAG, c->tag_len, tag) != 1) { - yaca_free(tag); - ERROR_DUMP(YACA_ERROR_INTERNAL); - return YACA_ERROR_INTERNAL; + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + goto err; } *value = tag; *value_len = c->tag_len; @@ -862,6 +870,10 @@ static int get_encrypt_property(const yaca_context_h ctx, yaca_property_e proper } return YACA_ERROR_NONE; + +err: + yaca_free(tag); + return ret; } static int check_key_bit_length_for_algo(yaca_encrypt_algorithm_e algo, size_t key_bit_len) @@ -938,6 +950,7 @@ int encrypt_get_algorithm(yaca_encrypt_algorithm_e algo, if (ret == YACA_ERROR_NONE && *cipher == NULL) { ret = YACA_ERROR_INTERNAL; ERROR_DUMP(ret); + return ret; } return ret; @@ -1070,12 +1083,12 @@ int encrypt_update(yaca_context_h ctx, * It does not necessarily indicate a more serious error. * There is no call to EVP_CipherFinal. */ - ret = YACA_ERROR_INVALID_PARAMETER; + return YACA_ERROR_INVALID_PARAMETER; } else { ret = YACA_ERROR_INTERNAL; ERROR_DUMP(ret); + return ret; } - return ret; } *output_len = loutput_len; @@ -1134,8 +1147,8 @@ API int yaca_encrypt_get_iv_bit_length(yaca_encrypt_algorithm_e algo, size_t key_bit_len, size_t *iv_bit_len) { - const EVP_CIPHER *cipher; int ret; + const EVP_CIPHER *cipher; if (iv_bit_len == NULL || key_bit_len % 8 != 0) return YACA_ERROR_INVALID_PARAMETER; @@ -1146,8 +1159,9 @@ API int yaca_encrypt_get_iv_bit_length(yaca_encrypt_algorithm_e algo, ret = EVP_CIPHER_iv_length(cipher); if (ret < 0) { - ERROR_DUMP(YACA_ERROR_INTERNAL); - return YACA_ERROR_INTERNAL; + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + return ret; } *iv_bit_len = ret * 8; diff --git a/src/key.c b/src/key.c index 15bc937..75e3a57 100644 --- a/src/key.c +++ b/src/key.c @@ -320,18 +320,18 @@ static EVP_PKEY *d2i_DSAparams_bio_helper(BIO *src) dsa = d2i_DSAparams_bio(src, NULL); if (dsa == NULL) - return NULL; + goto err; pkey = EVP_PKEY_new(); if (pkey == NULL) - goto exit; + goto err; if (EVP_PKEY_assign_DSA(pkey, dsa) != 1) - goto exit; + goto err; return pkey; -exit: +err: EVP_PKEY_free(pkey); DSA_free(dsa); return NULL; @@ -346,18 +346,18 @@ static EVP_PKEY *d2i_DHparams_bio_helper(BIO *src) dh = d2i_DHparams_bio(src, NULL); if (dh == NULL) - return NULL; + goto err; pkey = EVP_PKEY_new(); if (pkey == NULL) - goto exit; + goto err; if (EVP_PKEY_assign_DH(pkey, dh) != 1) - goto exit; + goto err; return pkey; -exit: +err: EVP_PKEY_free(pkey); DH_free(dh); return NULL; @@ -373,28 +373,28 @@ static EVP_PKEY *d2i_ECPKParameters_bio_helper(BIO *src) ecg = d2i_ECPKParameters_bio(src, NULL); if (ecg == NULL) - return NULL; + goto err; eck = EC_KEY_new(); if (eck == NULL) - goto exit; + goto err; if (EC_KEY_set_group(eck, ecg) != 1) - goto exit; + goto err; EC_GROUP_free(ecg); ecg = NULL; pkey = EVP_PKEY_new(); if (pkey == NULL) - goto exit; + goto err; if (EVP_PKEY_assign_EC_KEY(pkey, eck) != 1) - goto exit; + goto err; return pkey; -exit: +err: EVP_PKEY_free(pkey); EC_KEY_free(eck); EC_GROUP_free(ecg); @@ -437,8 +437,9 @@ static int import_evp(yaca_key_h *key, src = BIO_new_mem_buf(data, data_len); if (src == NULL) { - ERROR_DUMP(YACA_ERROR_INTERNAL); - return YACA_ERROR_INTERNAL; + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + return ret; } /* Possible PEM */ diff --git a/src/sign.c b/src/sign.c index 66fd5b2..e276af6 100644 --- a/src/sign.c +++ b/src/sign.c @@ -84,6 +84,7 @@ static int get_sign_output_length(const yaca_context_h ctx, { assert(output_len != NULL); + int ret; EVP_PKEY_CTX *pctx; struct yaca_sign_context_s *c = get_sign_context(ctx); assert(c != NULL); @@ -93,19 +94,24 @@ static int get_sign_output_length(const yaca_context_h ctx, return YACA_ERROR_INVALID_PARAMETER; pctx = EVP_MD_CTX_pkey_ctx(c->md_ctx); - if (pctx == NULL) - return YACA_ERROR_INTERNAL; + if (pctx == NULL) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + return ret; + } EVP_PKEY *pkey = EVP_PKEY_CTX_get0_pkey(pctx); if (pkey == NULL) { - ERROR_DUMP(YACA_ERROR_INTERNAL); - return YACA_ERROR_INTERNAL; + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + return ret; } int len = EVP_PKEY_size(pkey); if (len <= 0) { - ERROR_DUMP(YACA_ERROR_INTERNAL); - return YACA_ERROR_INTERNAL; + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + return ret; } *output_len = len; @@ -139,8 +145,11 @@ int set_sign_property(yaca_context_h ctx, return YACA_ERROR_INVALID_PARAMETER; pctx = EVP_MD_CTX_pkey_ctx(c->md_ctx); - if (pctx == NULL) - return YACA_ERROR_INTERNAL; + if (pctx == NULL) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + return ret; + } /* this function only supports padding */ if (property != YACA_PROPERTY_PADDING || value_len != sizeof(yaca_padding_e)) -- 2.7.4 From a90fee4e125a136cffba66c36bcb3e0d48907318 Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Tue, 28 Apr 2020 17:33:44 +0200 Subject: [PATCH 03/16] Add all debug functions to debug.h Add translate_error that was not available. Move others from internal. Include debug.h in internal.h. The reason for that is to make it easier to test debug functions by only including debug.h. internal.h is not includable by C++ code. Both those headers are internal and nothing changes in terms of public API. Change-Id: Ica6886c9253d45a5f131a36b457044132daee14a --- src/debug.c | 1 + src/debug.h | 29 ++++++++++++++++++++++++++++- src/internal.h | 24 +++++------------------- 3 files changed, 34 insertions(+), 20 deletions(-) diff --git a/src/debug.c b/src/debug.c index e6e2d06..e23214b 100644 --- a/src/debug.c +++ b/src/debug.c @@ -34,6 +34,7 @@ #include "internal.h" #include "debug.h" + // TODO any better idea than to use __thread? static __thread yaca_error_cb error_cb = NULL; static bool error_strings_loaded = false; diff --git a/src/debug.h b/src/debug.h index e6b5902..8a268fd 100644 --- a/src/debug.h +++ b/src/debug.h @@ -24,18 +24,45 @@ #ifndef YACA_DEBUG_H #define YACA_DEBUG_H + +#include + +#include + + #ifdef __cplusplus extern "C" { #endif typedef void (*yaca_error_cb)(const char*); - void yaca_debug_set_error_cb(yaca_error_cb cb); +const char *yaca_debug_translate_error(yaca_error_e err); + +#define ERROR_CLEAR() ERR_clear_error() + +void error_dump(const char *file, int line, const char *function, int code); +#define ERROR_DUMP(code) error_dump(__FILE__, __LINE__, __func__, (code)) + +/** + * Function responsible for translating the openssl error to yaca error and + * clearing/dumping the openssl error queue. Use only after openssl function + * failure. + * + * The function checks only first error in the queue. If the function doesn't + * find any error in openssl queue or is not able to translate it, it will + * return YACA_ERROR_INTERNAL and dump openssl errors if any. If the + * translation succeeds the function will clear the error queue and return the + * result of translation. + */ +int error_handle(const char *file, int line, const char *function); +#define ERROR_HANDLE() error_handle(__FILE__, __LINE__, __func__) + #ifdef __cplusplus } /* extern */ #endif + #endif /* YACA_DEBUG_H */ diff --git a/src/internal.h b/src/internal.h index c6b933c..a020d03 100644 --- a/src/internal.h +++ b/src/internal.h @@ -24,17 +24,20 @@ #ifndef YACA_INTERNAL_H #define YACA_INTERNAL_H + #include #include #include -#include #include #include #include #include +#include "debug.h" + + #define API __attribute__ ((visibility("default"))) #define UNUSED __attribute__((unused)) @@ -161,24 +164,7 @@ struct yaca_key_evp_s *key_get_evp(const yaca_key_h key); yaca_key_h key_copy(const yaca_key_h key); -void error_dump(const char *file, int line, const char *function, int code); -#define ERROR_DUMP(code) error_dump(__FILE__, __LINE__, __func__, (code)) -#define ERROR_CLEAR() ERR_clear_error() - -/** - * Function responsible for translating the openssl error to yaca error and - * clearing/dumping the openssl error queue. Use only after openssl function - * failure. - * - * The function checks only first error in the queue. If the function doesn't - * find any error in openssl queue or is not able to translate it, it will - * return YACA_ERROR_INTERNAL and dump openssl errors if any. If the - * translation succeeds the function will clear the error queue and return the - * result of translation. - */ -int error_handle(const char *file, int line, const char *function); -#define ERROR_HANDLE() error_handle(__FILE__, __LINE__, __func__) - int rsa_padding2openssl(yaca_padding_e padding); + #endif /* YACA_INTERNAL_H */ -- 2.7.4 From 6eda8faaa32ddc289d71923de6f00392de8a639e Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Wed, 29 Apr 2020 18:29:53 +0200 Subject: [PATCH 04/16] Clarify the code that caused many sleepless engineers We have to stop this madness. Change-Id: I7407efe46ac02c0c2427966eb4bd52db180522fc --- src/debug.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/debug.c b/src/debug.c index e23214b..1e6035d 100644 --- a/src/debug.c +++ b/src/debug.c @@ -104,13 +104,16 @@ void error_dump(const char *file, int line, const char *function, int code) } } + /* In case the while broke early due to the BUF_SIZE, write + * ellipsis and clear remaining errors that might have not been + * read by ERR_get_error() */ if (written >= BUF_SIZE - 1) { strncpy(buf + BUF_SIZE - ELLIPSIS_SIZE, ELLIPSIS, ELLIPSIS_SIZE); written = BUF_SIZE - 1; ERR_clear_error(); } - buf[written] = '\0'; + buf[written] = '\0'; (*error_cb)(buf); } -- 2.7.4 From 65e226c1dbf8713553168ede7d40709ea8292f3d Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Mon, 4 May 2020 17:16:00 +0200 Subject: [PATCH 05/16] Handle errors properly in various key.c functions BIO_flush() BIO_read() BIO_write() BIO_reset() EVP_aes_256_cbc() EVP_PKEY_up_ref() Change-Id: Id74d0710ce8a12f982d0011b83d46880fe2b6116 --- src/key.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 97 insertions(+), 17 deletions(-) diff --git a/src/key.c b/src/key.c index 75e3a57..dab5873 100644 --- a/src/key.c +++ b/src/key.c @@ -200,6 +200,8 @@ static int base64_decode(const char *data, size_t data_len, BIO **output) /* Try to decode */ for (;;) { + int read = 0; + ret = BIO_read(b64, tmpbuf, TMP_BUF_LEN); if (ret < 0) { ret = YACA_ERROR_INTERNAL; @@ -207,17 +209,24 @@ static int base64_decode(const char *data, size_t data_len, BIO **output) goto exit; } - if (ret == YACA_ERROR_NONE) + if (ret == 0) break; + read = ret; - if (BIO_write(dst, tmpbuf, ret) != ret) { + ret = BIO_write(dst, tmpbuf, ret); + if (ret != read) { ret = YACA_ERROR_INTERNAL; ERROR_DUMP(ret); goto exit; } } - BIO_flush(dst); + ret = BIO_flush(dst); + if (ret <= 0) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + goto exit; + } /* Check wether the length of the decoded data is what we expected */ out_len = BIO_get_mem_data(dst, &out); @@ -445,7 +454,12 @@ static int import_evp(yaca_key_h *key, /* Possible PEM */ if (strncmp("----", data, 4) == 0) { if (pkey == NULL) { - BIO_reset(src); + ret = BIO_reset(src); + if (ret <= 0) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + goto exit; + } pkey = PEM_read_bio_PrivateKey(src, NULL, cb, (void*)&cb_data); if (ERROR_HANDLE() == YACA_ERROR_INVALID_PASSWORD) { ret = YACA_ERROR_INVALID_PASSWORD; @@ -456,7 +470,12 @@ static int import_evp(yaca_key_h *key, } if (pkey == NULL) { - BIO_reset(src); + ret = BIO_reset(src); + if (ret <= 0) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + goto exit; + } pkey = PEM_read_bio_PUBKEY(src, NULL, cb, NULL); ERROR_CLEAR(); imported_key_category = IMPORTED_KEY_CATEGORY_PUBLIC; @@ -464,7 +483,12 @@ static int import_evp(yaca_key_h *key, } if (pkey == NULL) { - BIO_reset(src); + ret = BIO_reset(src); + if (ret <= 0) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + goto exit; + } pkey = PEM_read_bio_Parameters(src, NULL); ERROR_CLEAR(); imported_key_category = IMPORTED_KEY_CATEGORY_PARAMETERS; @@ -472,7 +496,12 @@ static int import_evp(yaca_key_h *key, } if (pkey == NULL) { - BIO_reset(src); + ret = BIO_reset(src); + if (ret <= 0) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + goto exit; + } X509 *x509 = PEM_read_bio_X509(src, NULL, cb, NULL); if (x509 != NULL) { pkey = X509_get_pubkey(x509); @@ -486,7 +515,12 @@ static int import_evp(yaca_key_h *key, /* Possible DER */ else { if (pkey == NULL) { - BIO_reset(src); + ret = BIO_reset(src); + if (ret <= 0) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + goto exit; + } pkey = d2i_PKCS8PrivateKey_bio(src, NULL, cb, (void*)&cb_data); if (ERROR_HANDLE() == YACA_ERROR_INVALID_PASSWORD) { ret = YACA_ERROR_INVALID_PASSWORD; @@ -497,7 +531,12 @@ static int import_evp(yaca_key_h *key, } if (pkey == NULL) { - BIO_reset(src); + ret = BIO_reset(src); + if (ret <= 0) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + goto exit; + } pkey = d2i_PrivateKey_bio(src, NULL); ERROR_CLEAR(); imported_key_category = IMPORTED_KEY_CATEGORY_PRIVATE; @@ -505,7 +544,12 @@ static int import_evp(yaca_key_h *key, } if (pkey == NULL) { - BIO_reset(src); + ret = BIO_reset(src); + if (ret <= 0) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + goto exit; + } pkey = d2i_PUBKEY_bio(src, NULL); ERROR_CLEAR(); imported_key_category = IMPORTED_KEY_CATEGORY_PUBLIC; @@ -513,7 +557,12 @@ static int import_evp(yaca_key_h *key, } if (pkey == NULL) { - BIO_reset(src); + ret = BIO_reset(src); + if (ret <= 0) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + goto exit; + } pkey = d2i_DSAparams_bio_helper(src); ERROR_CLEAR(); imported_key_category = IMPORTED_KEY_CATEGORY_PARAMETERS; @@ -521,7 +570,12 @@ static int import_evp(yaca_key_h *key, } if (pkey == NULL) { - BIO_reset(src); + ret = BIO_reset(src); + if (ret <= 0) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + goto exit; + } pkey = d2i_DHparams_bio_helper(src); ERROR_CLEAR(); imported_key_category = IMPORTED_KEY_CATEGORY_PARAMETERS; @@ -529,7 +583,12 @@ static int import_evp(yaca_key_h *key, } if (pkey == NULL) { - BIO_reset(src); + ret = BIO_reset(src); + if (ret <= 0) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + goto exit; + } pkey = d2i_ECPKParameters_bio_helper(src); ERROR_CLEAR(); imported_key_category = IMPORTED_KEY_CATEGORY_PARAMETERS; @@ -537,7 +596,12 @@ static int import_evp(yaca_key_h *key, } if (pkey == NULL) { - BIO_reset(src); + ret = BIO_reset(src); + if (ret <= 0) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + goto exit; + } X509 *x509 = d2i_X509_bio(src, NULL); if (x509 != NULL) { pkey = X509_get_pubkey(x509); @@ -714,8 +778,14 @@ static int export_evp_default_bio(struct yaca_key_evp_s *evp_key, int ret; const EVP_CIPHER *enc = NULL; - if (password != NULL) + if (password != NULL) { enc = EVP_aes_256_cbc(); + if (enc == NULL) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + return ret; + } + } switch (key_file_fmt) { @@ -818,7 +888,12 @@ static int export_evp_pkcs8_bio(struct yaca_key_evp_s *evp_key, assert(mem != NULL); int ret; - const EVP_CIPHER *enc = EVP_aes_256_cbc();; + const EVP_CIPHER *enc = EVP_aes_256_cbc(); + if (enc == NULL) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + return ret; + } /* PKCS8 export requires a password */ if (password == NULL) @@ -1166,7 +1241,12 @@ static int generate_evp_pkey_key(int evp_id, size_t key_bit_len, EVP_PKEY *param if (ret != YACA_ERROR_NONE) return ret; } else { - EVP_PKEY_up_ref(params); + ret = EVP_PKEY_up_ref(params); + if (ret <= 0) { + ret = YACA_ERROR_INTERNAL; + ERROR_DUMP(ret); + return ret; + } } kctx = EVP_PKEY_CTX_new(params, NULL); -- 2.7.4 From 17ad71c95f51bc8cf202233f54bc95c05d642744 Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Wed, 6 May 2020 18:16:31 +0200 Subject: [PATCH 06/16] Change key_copy API, it's only used in one place Only simple keys are copied and only in one place. Simplify it to a specialized function and put it as static where it's needed. Change-Id: I4d83ab4b3290ad9758315045345450f7d5cf2d3b --- src/encrypt.c | 38 ++++++++++++++++++++++++++++++++++++-- src/internal.h | 2 -- src/key.c | 47 ----------------------------------------------- 3 files changed, 36 insertions(+), 51 deletions(-) diff --git a/src/encrypt.c b/src/encrypt.c index a369e53..459eca8 100644 --- a/src/encrypt.c +++ b/src/encrypt.c @@ -25,6 +25,7 @@ #include #include #include +#include #include @@ -545,6 +546,27 @@ static int encrypt_ctx_setup(struct yaca_encrypt_context_s *c, return YACA_ERROR_NONE; } +static int key_copy_simple(const yaca_key_h key, yaca_key_h *out) +{ + assert(key != YACA_KEY_NULL); + assert(out != NULL); + + int ret; + struct yaca_key_simple_s *simple = key_get_simple(key); + assert(simple != NULL); + + struct yaca_key_simple_s *copy; + size_t size = sizeof(struct yaca_key_simple_s) + simple->bit_len / 8; + + ret = yaca_zalloc(size, (void**)©); + if (ret != YACA_ERROR_NONE) + return ret; + + memcpy(copy, key, size); + *out = (yaca_key_h)copy; + return YACA_ERROR_NONE; +} + static int encrypt_ctx_backup(struct yaca_encrypt_context_s *c, const EVP_CIPHER *cipher, const yaca_key_h sym_key, @@ -562,14 +584,26 @@ static int encrypt_ctx_backup(struct yaca_encrypt_context_s *c, if (ret != YACA_ERROR_NONE) return ret; + ret = key_copy_simple(sym_key, &bc->sym_key); + if (ret != YACA_ERROR_NONE) + goto err; + if (iv != YACA_KEY_NULL) { + ret = key_copy_simple(iv, &bc->iv); + if (ret != YACA_ERROR_NONE) + goto err; + } bc->cipher = cipher; - bc->sym_key = key_copy(sym_key); - bc->iv = key_copy(iv); bc->padding = YACA_PADDING_PKCS7; c->backup_ctx = bc; return YACA_ERROR_NONE; + +err: + yaca_key_destroy(bc->iv); + yaca_key_destroy(bc->sym_key); + yaca_free(bc); + return ret; } static int encrypt_ctx_restore(struct yaca_encrypt_context_s *c) diff --git a/src/internal.h b/src/internal.h index a020d03..7cc5641 100644 --- a/src/internal.h +++ b/src/internal.h @@ -162,8 +162,6 @@ int encrypt_finalize(yaca_context_h ctx, struct yaca_key_simple_s *key_get_simple(const yaca_key_h key); struct yaca_key_evp_s *key_get_evp(const yaca_key_h key); -yaca_key_h key_copy(const yaca_key_h key); - int rsa_padding2openssl(yaca_padding_e padding); diff --git a/src/key.c b/src/key.c index dab5873..256806b 100644 --- a/src/key.c +++ b/src/key.c @@ -1413,53 +1413,6 @@ struct yaca_key_evp_s *key_get_evp(const yaca_key_h key) } } -static yaca_key_h key_copy_simple(const struct yaca_key_simple_s *key) -{ - int ret; - assert(key != NULL); - - struct yaca_key_simple_s *copy; - size_t size = sizeof(struct yaca_key_simple_s) + key->bit_len / 8; - - ret = yaca_zalloc(size, (void**)©); - if (ret != YACA_ERROR_NONE) - return YACA_KEY_NULL; - - memcpy(copy, key, size); - return (yaca_key_h)copy; -} - -static yaca_key_h key_copy_evp(const struct yaca_key_evp_s *key) -{ - int ret; - assert(key != NULL); - - struct yaca_key_evp_s *copy = NULL; - ret = yaca_zalloc(sizeof(struct yaca_key_evp_s), (void**)©); - if (ret != YACA_ERROR_NONE) - return YACA_KEY_NULL; - - /* raise the refcount */ - EVP_PKEY_up_ref(key->evp); - - copy->key.type = key->key.type; - copy->evp = key->evp; - return (yaca_key_h)copy; -} - -yaca_key_h key_copy(const yaca_key_h key) -{ - struct yaca_key_simple_s *simple = key_get_simple(key); - struct yaca_key_evp_s *evp = key_get_evp(key); - - if (simple != NULL) - return key_copy_simple(simple); - else if (evp != NULL) - return key_copy_evp(evp); - - return YACA_KEY_NULL; -} - API int yaca_key_get_type(const yaca_key_h key, yaca_key_type_e *key_type) { const struct yaca_key_s *lkey = (const struct yaca_key_s *)key; -- 2.7.4 From 5229a9c1aba388d85bdb018bd381567694c90ee0 Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Thu, 7 May 2020 13:36:35 +0200 Subject: [PATCH 07/16] Add yaca's include deps to examples Change-Id: I3dbaf43351efe3bec426503e5181d549415dadd3 --- examples/CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/examples/CMakeLists.txt b/examples/CMakeLists.txt index a71dc0c..ad96bee 100644 --- a/examples/CMakeLists.txt +++ b/examples/CMakeLists.txt @@ -1,5 +1,5 @@ # -# Copyright (c) 2016 Samsung Electronics Co., Ltd All Rights Reserved +# Copyright (c) 2016-2020 Samsung Electronics Co., Ltd All Rights Reserved # # Contact: Krzysztof Jackiewicz # @@ -21,6 +21,7 @@ # INCLUDE_DIRECTORIES(${API_FOLDER}) +INCLUDE_DIRECTORIES(SYSTEM ${YACA_DEPS_INCLUDE_DIRS}) SET(COMMON_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/misc.c) -- 2.7.4 From 83386931220dbe26f558e7607fec16c20723e64b Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Fri, 8 May 2020 10:38:43 +0200 Subject: [PATCH 08/16] Better error handling in encrypt_ctx_init() and encrypt_ctx_setup_iv() Some calls to encrypt_ctx_init() and encrypt_ctx_setup_iv() asserts their return code that it cannot be EINVAL. But the OpenSSL functions there (namely EVP_CIPHER_CTX_set_key_length() and EVP_CIPHER_CTX_ctrl) can fail because of other reasons. Handle this properly. Side effect of this change is that while setting wrong IVLEN for CCM it's impossible to distinguish error codes, as OpenSSL does not set them in most cases. Handle this internally. Change-Id: Ib82871c8f4bf348c9ff4b90467886edcc19f6f9e --- src/debug.c | 2 ++ src/encrypt.c | 23 +++++++++++++++-------- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/debug.c b/src/debug.c index 1e6035d..fbb4dc2 100644 --- a/src/debug.c +++ b/src/debug.c @@ -149,6 +149,8 @@ int error_handle(const char *file, int line, const char *function) case ERR_PACK(ERR_LIB_EVP, EVP_F_EVP_PKEY_DERIVE_SET_PEER, EVP_R_DIFFERENT_PARAMETERS): case ERR_PACK(ERR_LIB_EC, EC_F_PKEY_EC_CTRL, EC_R_INVALID_DIGEST_TYPE): case ERR_PACK(ERR_LIB_DSA, DSA_F_PKEY_DSA_CTRL, DSA_R_INVALID_DIGEST_TYPE): + case ERR_PACK(ERR_LIB_EVP, EVP_F_EVP_CIPHER_CTX_SET_KEY_LENGTH, EVP_R_INVALID_KEY_LENGTH): + case ERR_PACK(ERR_LIB_EVP, EVP_F_EVP_CIPHER_CTX_CTRL, EVP_R_CTRL_OPERATION_NOT_IMPLEMENTED): ret = YACA_ERROR_INVALID_PARAMETER; break; case ERR_PACK(ERR_LIB_ASN1, ASN1_F_ASN1_GET_OBJECT, ASN1_R_TOO_LONG): diff --git a/src/encrypt.c b/src/encrypt.c index 459eca8..51ca1a9 100644 --- a/src/encrypt.c +++ b/src/encrypt.c @@ -431,11 +431,8 @@ static int encrypt_ctx_init(struct yaca_encrypt_context_s *c, /* Handling of algorithms with variable key length */ ret = EVP_CIPHER_CTX_set_key_length(c->cipher_ctx, key_bit_len / 8); - if (ret != 1) { - ret = YACA_ERROR_INVALID_PARAMETER; - ERROR_CLEAR(); - return ret; - } + if (ret != 1) + return ERROR_HANDLE(); return YACA_ERROR_NONE; } @@ -473,20 +470,28 @@ static int encrypt_ctx_setup_iv(struct yaca_encrypt_context_s *c, /* IV length doesn't match cipher (GCM & CCM supports variable IV length) */ if (default_iv_bit_len != iv->bit_len) { + size_t iv_len = iv->bit_len / 8; int mode = EVP_CIPHER_CTX_mode(c->cipher_ctx); if (mode == EVP_CIPH_GCM_MODE) { ret = EVP_CIPHER_CTX_ctrl(c->cipher_ctx, EVP_CTRL_GCM_SET_IVLEN, - iv->bit_len / 8, NULL); + iv_len, NULL); } else if (mode == EVP_CIPH_CCM_MODE) { + /* OpenSSL does not return a specific error code when + * wrong IVLEN is passed. It just returns 0. So there + * is no way to distinguish this error from ENOMEM for + * example. Handle this in our code then. + */ + if (iv_len < 7 || iv_len > 13) + return YACA_ERROR_INVALID_PARAMETER; ret = EVP_CIPHER_CTX_ctrl(c->cipher_ctx, EVP_CTRL_CCM_SET_IVLEN, - iv->bit_len / 8, NULL); + iv_len, NULL); } else { return YACA_ERROR_INVALID_PARAMETER; } if (ret != 1) - return YACA_ERROR_INVALID_PARAMETER; + return ERROR_HANDLE(); } } @@ -626,6 +631,8 @@ static int encrypt_ctx_restore(struct yaca_encrypt_context_s *c) ret = encrypt_ctx_init(c, c->backup_ctx->cipher, key->bit_len); assert(ret != YACA_ERROR_INVALID_PARAMETER); + if (ret != YACA_ERROR_NONE) + return ret; if (c->backup_ctx->padding == YACA_PADDING_NONE && EVP_CIPHER_CTX_set_padding(c->cipher_ctx, 0) != 1) { -- 2.7.4 From 3097b1e32a57a5101259281ddb8eeed61939b26e Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Mon, 11 May 2020 18:26:16 +0200 Subject: [PATCH 09/16] Remove support for OpenSSL 1.0.x, it's EOL Change-Id: If860fb8c5f3ea3fc128d52860e923e0cff582cd2 --- src/crypto.c | 103 ++------------------------------------------------------- src/debug.c | 6 ---- src/encrypt.c | 23 ------------- src/internal.h | 25 -------------- 4 files changed, 2 insertions(+), 155 deletions(-) diff --git a/src/crypto.c b/src/crypto.c index 98e941a..585114a 100644 --- a/src/crypto.c +++ b/src/crypto.c @@ -44,10 +44,6 @@ #include "internal.h" -#if OPENSSL_VERSION_NUMBER < 0x10100000L -static pthread_mutex_t *mutexes = NULL; -#endif /* OPENSSL_VERSION_NUMBER < 0x10100000L */ - static __thread bool current_thread_initialized = false; static size_t threads_cnt = 0; static pthread_mutex_t init_mutex = PTHREAD_MUTEX_INITIALIZER; @@ -82,8 +78,6 @@ static int getrandom_wrapper(unsigned char *buf, int num) return 1; } -#if OPENSSL_VERSION_NUMBER >= 0x10100000L - static int RAND_METHOD_seed(UNUSED const void *buf, UNUSED int num) { return 1; @@ -94,18 +88,6 @@ static int RAND_METHOD_add(UNUSED const void *buf, UNUSED int num, UNUSED double return 1; } -#else /* OPENSSL_VERSION_NUMBER >= 0x10100000L */ - -static void RAND_METHOD_seed(UNUSED const void *buf, UNUSED int num) -{ -} - -static void RAND_METHOD_add(UNUSED const void *buf, UNUSED int num, UNUSED double entropy) -{ -} - -#endif /* OPENSSL_VERSION_NUMBER >= 0x10100000L */ - static int RAND_METHOD_bytes(unsigned char *buf, int num) { return getrandom_wrapper(buf, num); @@ -141,41 +123,6 @@ static const RAND_METHOD new_rand_method = { RAND_METHOD_status, }; -#if OPENSSL_VERSION_NUMBER < 0x10100000L - -static void locking_callback(int mode, int type, UNUSED const char *file, UNUSED int line) -{ - /* Ignore NULL mutexes and lock/unlock error codes as we can't do anything - * about them. */ - - if (mutexes == NULL) - return; - - if (mode & CRYPTO_LOCK) - pthread_mutex_lock(&mutexes[type]); - else if (mode & CRYPTO_UNLOCK) - pthread_mutex_unlock(&mutexes[type]); -} - -static unsigned long thread_id_callback() -{ - return pthread_self(); -} - -static void destroy_mutexes(int count) -{ - if (mutexes != NULL) { - for (int i = 0; i < count; i++) { - /* Ignore returned value as we can't do anything about it */ - pthread_mutex_destroy(&mutexes[i]); - } - yaca_free(mutexes); - mutexes = NULL; - } -} - -#endif /* OPENSSL_VERSION_NUMBER < 0x10100000L */ - API int yaca_initialize(void) { int ret = YACA_ERROR_NONE; @@ -225,42 +172,6 @@ API int yaca_initialize(void) OpenSSL_add_all_digests(); OpenSSL_add_all_ciphers(); -#if OPENSSL_VERSION_NUMBER < 0x10100000L - /* enable threads support */ - assert(mutexes == NULL); - - if (CRYPTO_num_locks() > 0) { - ret = yaca_malloc(CRYPTO_num_locks() * sizeof(pthread_mutex_t), - (void**)&mutexes); - - if (ret != YACA_ERROR_NONE) - goto exit; - - for (int i = 0; i < CRYPTO_num_locks(); i++) { - if (pthread_mutex_init(&mutexes[i], NULL) != 0) { - ret = YACA_ERROR_NONE; - switch (errno) { - case ENOMEM: - ret = YACA_ERROR_OUT_OF_MEMORY; - break; - case EAGAIN: - case EPERM: - case EBUSY: - case EINVAL: - default: - ret = YACA_ERROR_INTERNAL; - } - destroy_mutexes(i); - - goto exit; - } - } - - CRYPTO_set_id_callback(thread_id_callback); - CRYPTO_set_locking_callback(locking_callback); - } -#endif /* OPENSSL_VERSION_NUMBER < 0x10100000L */ - /* * TODO: * - We should also decide on OpenSSL config. @@ -272,9 +183,9 @@ API int yaca_initialize(void) current_thread_initialized = true; } -#if OPENSSL_VERSION_NUMBER < 0x10100000L || !defined SYS_getrandom +#if !defined SYS_getrandom exit: -#endif /* OPENSSL_VERSION_NUMBER < 0x10100000L || !defined SYS_getrandom */ +#endif /* !defined SYS_getrandom */ pthread_mutex_unlock(&init_mutex); @@ -288,9 +199,6 @@ API void yaca_cleanup(void) return; /* per thread cleanup */ -#if OPENSSL_VERSION_NUMBER < 0x10100000L - ERR_remove_thread_state(NULL); -#endif /* OPENSSL_VERSION_NUMBER < 0x10100000L */ CRYPTO_cleanup_all_ex_data(); pthread_mutex_lock(&init_mutex); @@ -307,13 +215,6 @@ API void yaca_cleanup(void) urandom_fd = -2; #endif /* SYS_getrandom */ -#if OPENSSL_VERSION_NUMBER < 0x10100000L - /* threads support cleanup */ - CRYPTO_set_id_callback(NULL); - CRYPTO_set_locking_callback(NULL); - - destroy_mutexes(CRYPTO_num_locks()); -#endif /* OPENSSL_VERSION_NUMBER < 0x10100000L */ } assert(threads_cnt > 0); diff --git a/src/debug.c b/src/debug.c index fbb4dc2..486180e 100644 --- a/src/debug.c +++ b/src/debug.c @@ -127,18 +127,12 @@ int error_handle(const char *file, int line, const char *function) /* known errors */ switch (err) { -#if OPENSSL_VERSION_NUMBER > 0x10100000L case ERR_PACK(ERR_LIB_RSA, RSA_F_RSA_OSSL_PRIVATE_DECRYPT, RSA_R_DATA_GREATER_THAN_MOD_LEN): case ERR_PACK(ERR_LIB_RSA, RSA_F_RSA_OSSL_PUBLIC_DECRYPT, RSA_R_DATA_GREATER_THAN_MOD_LEN): case ERR_PACK(ERR_LIB_RSA, RSA_F_RSA_OSSL_PRIVATE_DECRYPT, RSA_R_DATA_TOO_LARGE_FOR_MODULUS): case ERR_PACK(ERR_LIB_RSA, RSA_F_RSA_OSSL_PUBLIC_DECRYPT, RSA_R_DATA_TOO_LARGE_FOR_MODULUS): case ERR_PACK(ERR_LIB_PEM, PEM_F_GET_NAME, PEM_R_NO_START_LINE): case ERR_PACK(ERR_LIB_PEM, PEM_F_GET_HEADER_AND_DATA, PEM_R_BAD_END_LINE): -#else /* OPENSSL_VERSION_NUMBER > 0x10100000L */ - case ERR_PACK(ERR_LIB_RSA, RSA_F_PKEY_RSA_CTRL, RSA_R_INVALID_KEYBITS): - case ERR_PACK(ERR_LIB_RSA, RSA_F_RSA_EAY_PRIVATE_DECRYPT, RSA_R_DATA_GREATER_THAN_MOD_LEN): - case ERR_PACK(ERR_LIB_RSA, RSA_F_RSA_EAY_PUBLIC_DECRYPT, RSA_R_DATA_GREATER_THAN_MOD_LEN): -#endif /* OPENSSL_VERSION_NUMBER > 0x10100000L */ case ERR_PACK(ERR_LIB_RSA, RSA_F_PKEY_RSA_CTRL, RSA_R_KEY_SIZE_TOO_SMALL): case ERR_PACK(ERR_LIB_EVP, EVP_F_EVP_PKEY_CTX_CTRL, EVP_R_COMMAND_NOT_SUPPORTED): case ERR_PACK(ERR_LIB_PEM, PEM_F_PEM_READ_BIO, PEM_R_NO_START_LINE): diff --git a/src/encrypt.c b/src/encrypt.c index 51ca1a9..82f5d77 100644 --- a/src/encrypt.c +++ b/src/encrypt.c @@ -526,13 +526,6 @@ static int encrypt_ctx_setup(struct yaca_encrypt_context_s *c, if (ret != YACA_ERROR_NONE) return ret; -#if OPENSSL_VERSION_NUMBER < 0x10100000L - /* Fix for OpenSSL error in 3DES CFB1 */ - int nid = EVP_CIPHER_CTX_nid(c->cipher_ctx); - if (nid == NID_des_ede3_cfb1) - EVP_CIPHER_CTX_set_flags(c->cipher_ctx, EVP_CIPH_FLAG_LENGTH_BITS); -#endif - if (liv != NULL) iv_data = (unsigned char*)liv->d; @@ -1109,13 +1102,6 @@ int encrypt_update(yaca_context_h ctx, } } - /* Fix for OpenSSL error in 3DES CFB1 */ - if (EVP_CIPHER_CTX_test_flags(c->cipher_ctx, EVP_CIPH_FLAG_LENGTH_BITS) != 0) { - if (input_len > INT_MAX / 8) - return YACA_ERROR_INVALID_PARAMETER; - input_len *= 8; - } - ret = EVP_CipherUpdate(c->cipher_ctx, output, &loutput_len, input, input_len); if (ret != 1 || loutput_len < 0) { if (mode == EVP_CIPH_CCM_MODE && (op_type == OP_DECRYPT || op_type == OP_OPEN)) { @@ -1135,11 +1121,6 @@ int encrypt_update(yaca_context_h ctx, *output_len = loutput_len; c->state = target_state; - - /* Fix for OpenSSL error in 3DES CFB1 */ - if (EVP_CIPHER_CTX_test_flags(c->cipher_ctx, EVP_CIPH_FLAG_LENGTH_BITS) != 0) - *output_len /= 8; - return YACA_ERROR_NONE; } @@ -1175,10 +1156,6 @@ int encrypt_finalize(yaca_context_h ctx, *output_len = loutput_len; - /* Fix for OpenSSL error in 3DES CFB1 */ - if (EVP_CIPHER_CTX_test_flags(c->cipher_ctx, EVP_CIPH_FLAG_LENGTH_BITS) != 0) - *output_len /= 8; - c->state = ENC_CTX_FINALIZED; return YACA_ERROR_NONE; } diff --git a/src/internal.h b/src/internal.h index 7cc5641..b395b66 100644 --- a/src/internal.h +++ b/src/internal.h @@ -41,31 +41,6 @@ #define API __attribute__ ((visibility("default"))) #define UNUSED __attribute__((unused)) -/* Functions that handle the hidden nature of internal - * OpenSSL structures that don't exist in OpenSSL < 1.1.0 - */ -#if OPENSSL_VERSION_NUMBER < 0x10100000L - -static inline EVP_PKEY_CTX *EVP_MD_CTX_pkey_ctx(const EVP_MD_CTX *ctx) -{ - return ctx->pctx; -} - -static inline int EVP_PKEY_up_ref(EVP_PKEY *pkey) -{ - if (CRYPTO_add(&pkey->references, 1, CRYPTO_LOCK_EVP_PKEY) <= 0) - return 0; - return 1; -} - -static inline RSA *EVP_PKEY_get0_RSA(EVP_PKEY *pkey) -{ - if (pkey->type != EVP_PKEY_RSA) - return NULL; - return pkey->pkey.rsa; -} - -#endif /* OPENSSL_VERSION_NUMBER < 0x10100000L */ enum yaca_context_type_e { YACA_CONTEXT_INVALID = 0, -- 2.7.4 From 71cd0b9a8afeed0ce309d8d4fcee48f2ced0fe34 Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Tue, 12 May 2020 14:57:29 +0200 Subject: [PATCH 10/16] Remove unused OpenSSL RAND methods According to docs we can pass NULL to the functions we don't want/need in RAND_METHOD struct. As we don't use them, drop those unneeded. RAND_pseudo_bytes() was deprecated in OpenSSL 1.1.0. Change-Id: Id28795119d6efdd11664d1d81be0524d87e987cf --- src/crypto.c | 27 ++++----------------------- 1 file changed, 4 insertions(+), 23 deletions(-) diff --git a/src/crypto.c b/src/crypto.c index 585114a..b798196 100644 --- a/src/crypto.c +++ b/src/crypto.c @@ -78,30 +78,11 @@ static int getrandom_wrapper(unsigned char *buf, int num) return 1; } -static int RAND_METHOD_seed(UNUSED const void *buf, UNUSED int num) -{ - return 1; -} - -static int RAND_METHOD_add(UNUSED const void *buf, UNUSED int num, UNUSED double entropy) -{ - return 1; -} - static int RAND_METHOD_bytes(unsigned char *buf, int num) { return getrandom_wrapper(buf, num); } -static void RAND_METHOD_cleanup(void) -{ -} - -static int RAND_METHOD_pseudorand(UNUSED unsigned char *buf, UNUSED int num) -{ - return getrandom_wrapper(buf, num); -} - static int RAND_METHOD_status(void) { #ifdef SYS_getrandom @@ -115,11 +96,11 @@ static int RAND_METHOD_status(void) } static const RAND_METHOD new_rand_method = { - RAND_METHOD_seed, + NULL, RAND_METHOD_bytes, - RAND_METHOD_cleanup, - RAND_METHOD_add, - RAND_METHOD_pseudorand, + NULL, + NULL, + NULL, RAND_METHOD_status, }; -- 2.7.4 From 04eb57b0dd89e74feace9dec903006b50208a04f Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Wed, 17 Jun 2020 18:19:54 +0200 Subject: [PATCH 11/16] Handle errors from EVP_PKEY_CTX_set_* Not every error from EVP_PKEY_CTX_set_* is INTERNAL. Some should be handled lightly like trying to set DH key with bit_len < 256. Change-Id: I5993c8d04600ae1e5b0851d924087704c58c0f9c --- src/key.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/key.c b/src/key.c index 256806b..17afc7c 100644 --- a/src/key.c +++ b/src/key.c @@ -1201,8 +1201,7 @@ static int generate_evp_pkey_params(int evp_id, size_t key_bit_len, EVP_PKEY **p break; } if (ret != 1) { - ret = YACA_ERROR_INTERNAL; - ERROR_DUMP(ret); + ret = ERROR_HANDLE(); goto exit; } -- 2.7.4 From a8929ac54c843e43def74155a2986c63c7831353 Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Thu, 18 Jun 2020 17:53:32 +0200 Subject: [PATCH 12/16] Padding has to be set before update in case of decryption When doing encrypt/seal padding can be set before finalize as was before. But it appears that decrypt behaves differently. In that case padding has to be set before update or the decryption will be incorrect. Change-Id: I86ede38d0d79d401329c25c656e5c6b4c92e02cb --- api/yaca/yaca_types.h | 8 ++++++-- src/encrypt.c | 3 ++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/api/yaca/yaca_types.h b/api/yaca/yaca_types.h index a4ae309..65b758a 100644 --- a/api/yaca/yaca_types.h +++ b/api/yaca/yaca_types.h @@ -456,7 +456,9 @@ typedef enum { * Padding can be disabled using yaca_context_set_property() and * #YACA_PROPERTY_PADDING,#YACA_PADDING_NONE, * then the total length of data passed until *_finalize() MUST be a multiple of block size. - * #YACA_PROPERTY_PADDING can be set at the latest before the *_finalize() call. + * In case of encrypt/seal #YACA_PROPERTY_PADDING can be set at the + * latest before the *_finalize() call. In case of decrypt/open + * it can be set at the latest before the *_update() call. */ YACA_BCM_ECB, @@ -475,7 +477,9 @@ typedef enum { * Padding can be disabled using yaca_context_set_property() and * #YACA_PROPERTY_PADDING, #YACA_PADDING_NONE, * then the total length of data passed until *_finalize() MUST be a multiple of block size. - * #YACA_PROPERTY_PADDING can be set at the latest before the *_finalize() call. + * In case of encrypt/seal #YACA_PROPERTY_PADDING can be set at the + * latest before the *_finalize() call. In case of decrypt/open + * it can be set at the latest before the *_update() call. */ YACA_BCM_CBC, diff --git a/src/encrypt.c b/src/encrypt.c index 82f5d77..9c6a1a2 100644 --- a/src/encrypt.c +++ b/src/encrypt.c @@ -808,7 +808,8 @@ static int set_encrypt_property(yaca_context_h ctx, value_len != sizeof(yaca_padding_e) || (*(yaca_padding_e*)value != YACA_PADDING_NONE && *(yaca_padding_e*)value != YACA_PADDING_PKCS7) || - c->state == ENC_CTX_FINALIZED) + ((is_encryption_op(c->op_type)) && c->state == ENC_CTX_FINALIZED) || + (!(is_encryption_op(c->op_type)) && c->state != ENC_CTX_INITIALIZED)) return YACA_ERROR_INVALID_PARAMETER; int padding = *(yaca_padding_e*)value == YACA_PADDING_NONE ? 0 : 1; -- 2.7.4 From a97af8d086e04e29435dcaf836a13b40df655ec1 Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Fri, 19 Jun 2020 15:25:52 +0200 Subject: [PATCH 13/16] Distinguish different cases with the same OpenSSL error code When importing a key with a wrong password and decrypting data with wrong key/bcm or simply broken data OpenSSL can return exactly the same error code (ERR_LIB_EVP, EVP_F_EVP_DECRYPTFINAL_EX, EVP_R_BAD_DECRYPT). As we need to distinguish INVALID_PARAM and INVALID_PASS in import_key, but decryption cannot return INVALID_PASS handle this manually in the decryption. Change-Id: Iba2b5fccfb1660c20b76a345bc799a0b145d700c --- src/encrypt.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/encrypt.c b/src/encrypt.c index 9c6a1a2..27632f8 100644 --- a/src/encrypt.c +++ b/src/encrypt.c @@ -1144,14 +1144,23 @@ int encrypt_finalize(yaca_context_h ctx, if (mode != EVP_CIPH_WRAP_MODE && mode != EVP_CIPH_CCM_MODE) { ret = EVP_CipherFinal(c->cipher_ctx, output, &loutput_len); if (ret != 1 || loutput_len < 0) { - if (mode == EVP_CIPH_GCM_MODE && (op_type == OP_DECRYPT || op_type == OP_OPEN)) - /* A non positive return value from EVP_CipherFinal should be considered as - * a failure to authenticate ciphertext and/or AAD. - * It does not necessarily indicate a more serious error. - */ + if (mode == EVP_CIPH_GCM_MODE && (op_type == OP_DECRYPT || op_type == OP_OPEN)) { + /* A non positive return value from EVP_CipherFinal should be + * considered as a failure to authenticate ciphertext and/or + * AAD. It does not necessarily indicate a more serious error. + */ return YACA_ERROR_INVALID_PARAMETER; - else - return ERROR_HANDLE(); + } else { + /* The same error code is used if trying to import a key with a + * wrong password and in case of a decrypt error due to wrong + * BCM or a key. Finalize cannot return INVALID_PASS so handle + * this here. + */ + ret = ERROR_HANDLE(); + if (ret == YACA_ERROR_INVALID_PASSWORD) + ret = YACA_ERROR_INVALID_PARAMETER; + return ret; + } } } -- 2.7.4 From e565760f2d59be2dd4b871410ecbf63718c5da41 Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Fri, 19 Jun 2020 15:35:07 +0200 Subject: [PATCH 14/16] Clarify possible AAD length Change-Id: I86f83db0c144508fbca593be27bb9c558a69a195 --- api/yaca/yaca_types.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/api/yaca/yaca_types.h b/api/yaca/yaca_types.h index 65b758a..ea602bf 100644 --- a/api/yaca/yaca_types.h +++ b/api/yaca/yaca_types.h @@ -499,6 +499,7 @@ typedef enum { * Set after yaca_decrypt_update() / yaca_open_update() and before * yaca_decrypt_finalize() / yaca_open_finalize() in decryption / open operation.\n * - #YACA_PROPERTY_GCM_AAD = additional authentication data (optional)\n + * AAD length can have any positive value.\n * Set after yaca_encrypt_initialize() / yaca_seal_initialize() and before * yaca_encrypt_update() / yaca_seal_update() in encryption / seal operation.\n * Set after yaca_decrypt_initialize() / yaca_open_initialize() and before @@ -553,6 +554,7 @@ typedef enum { * Set after yaca_decrypt_initialize() / yaca_open_initialize() and before * yaca_decrypt_update() / yaca_open_update() in decryption / open operation.\n * - #YACA_PROPERTY_CCM_AAD = additional authentication data (optional)\n + * AAD length can have any positive value.\n * The total plaintext length must be passed to yaca_encrypt_update() / yaca_seal_update() * if AAD is used.\n * Set after yaca_encrypt_initialize() / yaca_seal_initialize() and before -- 2.7.4 From 8dcd66e4d5c1bf43c71a98eb9e2f3c74503e8977 Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Fri, 19 Jun 2020 15:48:48 +0200 Subject: [PATCH 15/16] Clarify calling update only once with CCM When using BCM_CCM yaca update function can be called only once for the plaintext or ciphertext regardless of using AAD. Clarify that in the docs. Change-Id: I350404dd0be10dd7c70d565e60a73497b6601de7 --- api/yaca/yaca_types.h | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/api/yaca/yaca_types.h b/api/yaca/yaca_types.h index ea602bf..ec32158 100644 --- a/api/yaca/yaca_types.h +++ b/api/yaca/yaca_types.h @@ -504,8 +504,9 @@ typedef enum { * yaca_encrypt_update() / yaca_seal_update() in encryption / seal operation.\n * Set after yaca_decrypt_initialize() / yaca_open_initialize() and before * yaca_decrypt_update() / yaca_open_update() in decryption / open operation.\n - * @see yaca_context_set_property() - * @see yaca_context_get_property() + * . + * @see yaca_context_set_property() + * @see yaca_context_get_property() */ YACA_BCM_GCM, @@ -559,16 +560,17 @@ typedef enum { * if AAD is used.\n * Set after yaca_encrypt_initialize() / yaca_seal_initialize() and before * yaca_encrypt_update() / yaca_seal_update() in encryption / seal operation.\n - * You can only call yaca_encrypt_update() / yaca_seal_update() once for AAD - * and once for the plaintext.\n * The total encrypted text length must be passed to yaca_decrypt_update() / * yaca_open_update() if AAD is used.\n * Set after yaca_decrypt_initialize() / yaca_open_initialize() and before * yaca_decrypt_update() / yaca_open_update() in decryption / open operation.\n - * You can only call yaca_decrypt_update() / yaca_open_update() once for AAD - * and once for the encrypted text.\n - * @see yaca_context_set_property() - * @see yaca_context_get_property() + * . + * You can only call yaca_encrypt_update() / yaca_seal_update() once for AAD (if used) + * and once for the plaintext.\n + * You can only call yaca_decrypt_update() / yaca_open_update() once for AAD (if used) + * and once for the encrypted text.\n + * @see yaca_context_set_property() + * @see yaca_context_get_property() */ YACA_BCM_CCM, -- 2.7.4 From 1098d368d387b25034a6d8e2ba850dd656d67f63 Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Fri, 19 Jun 2020 17:32:18 +0200 Subject: [PATCH 16/16] Clarify bit_length for yaca_key_generate Add info about symmetric keys and fix DH prime length that has to be >= 256 (OpenSSL requires that). Change-Id: Ic5704d88a103a30dd5c8742a87f4e08e2f54c5f7 --- api/yaca/yaca_key.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api/yaca/yaca_key.h b/api/yaca/yaca_key.h index 7ac0642..8abf4d1 100644 --- a/api/yaca/yaca_key.h +++ b/api/yaca/yaca_key.h @@ -185,11 +185,13 @@ int yaca_key_export(const yaca_key_h key, yaca_key_format_e key_fmt, yaca_key_fi * @remarks This function is used to generate symmetric keys, private asymmetric keys * or key generation parameters for key types that support them (DSA, DH and EC). * @remarks Supported key lengths: + * - SYMMETRIC/IV: >= 8bits + * - DES: 64, 128 or 192bits * - RSA: length >= 512bits * - DSA: length >= 512bits, multiple of 64 * - DH: a value taken from #yaca_key_bit_length_dh_rfc_e or * (YACA_KEY_LENGTH_DH_GENERATOR_* | prime_length_in_bits), - * where prime_length_in_bits can be any positive number + * where prime_length_in_bits has to be >= 256 * - EC: a value taken from #yaca_key_bit_length_ec_e * @remarks The @a key should be released using yaca_key_destroy(). * @param[in] key_type Type of the key to be generated -- 2.7.4