From 5a49b6b013e5624a0cc05daad5eb636c750f8ae3 Mon Sep 17 00:00:00 2001 From: Duc Nguyen Date: Sun, 22 Oct 2023 15:13:14 -0400 Subject: [PATCH] Fix memory leaks --- src/sig_stfl/sig_stfl.h | 11 +++---- src/sig_stfl/xmss/sig_stfl_xmss.h | 2 +- .../xmss/sig_stfl_xmss_secret_key_functions.c | 9 ++---- tests/test_sig_stfl.c | 30 +++++++++---------- 4 files changed, 24 insertions(+), 28 deletions(-) diff --git a/src/sig_stfl/sig_stfl.h b/src/sig_stfl/sig_stfl.h index 1052b24f21..df46e7a6bb 100644 --- a/src/sig_stfl/sig_stfl.h +++ b/src/sig_stfl/sig_stfl.h @@ -262,7 +262,7 @@ typedef struct OQS_SIG_STFL_SECRET_KEY { /* The (maximum) length, in bytes, of secret keys for this signature scheme. */ size_t length_secret_key; - /* The variant specific secret key data */ + /* The variant specific secret key data, must be allocated at the initialization. */ void *secret_key_data; /* mutual exclusion struct */ @@ -281,7 +281,7 @@ typedef struct OQS_SIG_STFL_SECRET_KEY { * @param[out] sk_len length of private key as a byte stream * @param[out] sk_buf_ptr pointer to private key data as a byte stream * @returns length of key material data available - * Caller deletes the buffer if memory was allocated. + * Caller is responsible to **unallocate** the pointer to buffer `sk_buf_ptr`. */ OQS_STATUS (*serialize_key)(OQS_SIG_STFL_SECRET_KEY *sk, size_t *sk_len, uint8_t **sk_buf_ptr); @@ -292,8 +292,8 @@ typedef struct OQS_SIG_STFL_SECRET_KEY { * @param[in] key_len length of the returned byte string * @param[in] sk_buf The secret key data to populate key object * @param[in] context application specific data - * @returns status of the operation populated with key material none-zero length. Caller - * deletes the buffer. if sk_buf is NULL the function returns the length + * @returns status of the operation populated with key material none-zero length. + * Caller is responsible to **unallocate** the buffer `sk_buf`. */ OQS_STATUS (*deserialize_key)(OQS_SIG_STFL_SECRET_KEY *sk, const size_t sk_len, const uint8_t *sk_buf, void *context); @@ -511,9 +511,10 @@ OQS_STATUS OQS_SIG_STFL_SECRET_KEY_unlock(OQS_SIG_STFL_SECRET_KEY *sk); */ void OQS_SIG_STFL_SECRET_KEY_SET_store_cb(OQS_SIG_STFL_SECRET_KEY *sk, secure_store_sk store_cb, void *context); +/* Serialize stateful secret key data into a byte string, return an allocated buffer. Users is responsible to unallocate the buffer `sk_buf`. */ OQS_API OQS_STATUS OQS_SECRET_KEY_STFL_serialize_key(OQS_SIG_STFL_SECRET_KEY *sk, size_t *sk_len, uint8_t **sk_buf); -/* Insert lms byte string in an LMS secret key object */ +/* Insert stateful byte string into an secret key object. User is responsible to unallocate buffer `sk_buf`. */ OQS_API OQS_STATUS OQS_SECRET_KEY_STFL_deserialize_key(OQS_SIG_STFL_SECRET_KEY *sk, size_t key_len, const uint8_t *sk_buf, void *context); #if defined(__cplusplus) diff --git a/src/sig_stfl/xmss/sig_stfl_xmss.h b/src/sig_stfl/xmss/sig_stfl_xmss.h index 361224c3fe..0254e695c0 100644 --- a/src/sig_stfl/xmss/sig_stfl_xmss.h +++ b/src/sig_stfl/xmss/sig_stfl_xmss.h @@ -566,7 +566,7 @@ OQS_API OQS_STATUS OQS_SIG_STFL_alg_xmssmt_sigs_total(unsigned long long *total, /* Generic XMSS SECRET_KEY object initialization */ OQS_SIG_STFL_SECRET_KEY *OQS_SECRET_KEY_XMSS_new(size_t length_secret_key); -/* Serialize XMSS secret key data into a byte string */ +/* Serialize XMSS secret key data into a byte string, return an allocated buffer. Users have to unallocated the buffer. */ OQS_STATUS OQS_SECRET_KEY_XMSS_serialize_key(OQS_SIG_STFL_SECRET_KEY *sk, size_t *sk_len, uint8_t **sk_buf_ptr); /* Deserialize XMSS byte string into an XMSS secret key data */ diff --git a/src/sig_stfl/xmss/sig_stfl_xmss_secret_key_functions.c b/src/sig_stfl/xmss/sig_stfl_xmss_secret_key_functions.c index 0dcf1d6731..141b2a7da3 100644 --- a/src/sig_stfl/xmss/sig_stfl_xmss_secret_key_functions.c +++ b/src/sig_stfl/xmss/sig_stfl_xmss_secret_key_functions.c @@ -67,7 +67,7 @@ OQS_SIG_STFL_SECRET_KEY *OQS_SECRET_KEY_XMSS_new(size_t length_secret_key) { return sk; } -/* Serialize XMSS secret key data into a byte string. */ +/* Serialize XMSS secret key data into a byte string, return an allocated buffer. Users have to unallocated the buffer. */ OQS_STATUS OQS_SECRET_KEY_XMSS_serialize_key(OQS_SIG_STFL_SECRET_KEY *sk, size_t *sk_len, uint8_t **sk_buf_ptr) { if (sk == NULL || sk_len == NULL || sk_buf_ptr == NULL) { return OQS_ERROR; @@ -99,12 +99,7 @@ OQS_STATUS OQS_SECRET_KEY_XMSS_deserialize_key(OQS_SIG_STFL_SECRET_KEY *sk, cons return OQS_ERROR; } - sk->secret_key_data = malloc(sk_len); - if (sk->secret_key_data == NULL) { - return OQS_ERROR; - } - - memcpy(sk->secret_key_data, sk_buf, sk_len); + memcpy(sk->secret_key_data, sk_buf, sk->length_secret_key); sk->context = context; return OQS_SUCCESS; diff --git a/tests/test_sig_stfl.c b/tests/test_sig_stfl.c index e5abc27cd7..744816718a 100644 --- a/tests/test_sig_stfl.c +++ b/tests/test_sig_stfl.c @@ -52,7 +52,7 @@ static uint8_t message_2[] = "The quick brown fox jumped from the tree."; /* * Write stateful secret keys to disk. */ -static OQS_STATUS test_save_secret_key(uint8_t *key_buf, size_t buf_len, void *context) { +static OQS_STATUS save_secret_key(uint8_t *key_buf, size_t buf_len, void *context) { if (key_buf == NULL || buf_len == 0 || context == NULL) { return OQS_ERROR; } @@ -420,7 +420,7 @@ static OQS_STATUS sig_stfl_test_correctness(const char *method_name, const char /* set context and secure store callback */ context = strdup(((file_store))); - OQS_SIG_STFL_SECRET_KEY_SET_store_cb(secret_key, test_save_secret_key, (void *)context); + OQS_SIG_STFL_SECRET_KEY_SET_store_cb(secret_key, save_secret_key, (void *)context); #if OQS_USE_PTHREADS_IN_TESTS sk_lock = (pthread_mutex_t *)malloc(sizeof(pthread_mutex_t)); @@ -571,13 +571,13 @@ static OQS_STATUS sig_stfl_test_correctness(const char *method_name, const char static OQS_STATUS sig_stfl_test_secret_key(const char *method_name, const char *katfile) { OQS_STATUS rc = OQS_SUCCESS; OQS_SIG_STFL_SECRET_KEY *sk = NULL; - OQS_SIG_STFL_SECRET_KEY *sk_frm_file = NULL; + OQS_SIG_STFL_SECRET_KEY *sk_from_file = NULL; unsigned long long num_sig_left = 0, max_num_sigs = 0; OQS_SIG_STFL *sig_obj = NULL; uint8_t *public_key = NULL; - uint8_t *frm_file_sk_buf = NULL; + uint8_t *from_file_sk_buf = NULL; uint8_t *to_file_sk_buf = NULL; - size_t frm_file_sk_len = 0; + size_t from_file_sk_len = 0; size_t to_file_sk_len = 0; char *context = NULL; char *context_2 = NULL; @@ -654,27 +654,27 @@ static OQS_STATUS sig_stfl_test_secret_key(const char *method_name, const char * /* set context and secure store callback */ if (sk->set_scrt_key_store_cb) { context = strdup(file_store_name); - sk->set_scrt_key_store_cb(sk, test_save_secret_key, (void *)context); + sk->set_scrt_key_store_cb(sk, save_secret_key, (void *)context); } /* read secret key from disk */ - frm_file_sk_buf = malloc(to_file_sk_len); - if (oqs_fload("sk", file_store_name, frm_file_sk_buf, to_file_sk_len, &frm_file_sk_len) != OQS_SUCCESS) { + from_file_sk_buf = malloc(to_file_sk_len); + if (oqs_fload("sk", file_store_name, from_file_sk_buf, to_file_sk_len, &from_file_sk_len) != OQS_SUCCESS) { goto err; } - if (to_file_sk_len != frm_file_sk_len) { + if (to_file_sk_len != from_file_sk_len) { fprintf(stderr, "ERROR: OQS_SECRET_KEY_new stored length not equal read length\n"); goto err; } - sk_frm_file = OQS_SIG_STFL_SECRET_KEY_new(method_name); - if (sk_frm_file == NULL) { + sk_from_file = OQS_SIG_STFL_SECRET_KEY_new(method_name); + if (sk_from_file == NULL) { fprintf(stderr, "ERROR: 2nd OQS_SECRET_KEY_new failed\n"); goto err; } context_2 = strdup(file_store_name); - rc = OQS_SECRET_KEY_STFL_deserialize_key(sk_frm_file, frm_file_sk_len, frm_file_sk_buf, (void *)context_2); + rc = OQS_SECRET_KEY_STFL_deserialize_key(sk_from_file, from_file_sk_len, from_file_sk_buf, (void *)context_2); if (rc != OQS_SUCCESS) { fprintf(stderr, "OQS restore %s from file failed.\n", method_name); @@ -693,7 +693,7 @@ static OQS_STATUS sig_stfl_test_secret_key(const char *method_name, const char * OQS_MEM_insecure_free(public_key); OQS_MEM_secure_free(to_file_sk_buf, to_file_sk_len); - OQS_MEM_secure_free(frm_file_sk_buf, frm_file_sk_len); + OQS_MEM_secure_free(from_file_sk_buf, from_file_sk_len); OQS_SIG_STFL_free(sig_obj); OQS_MEM_insecure_free(context); OQS_MEM_insecure_free(context_2); @@ -774,7 +774,7 @@ static OQS_STATUS sig_stfl_test_sig_gen(const char *method_name) { key_store_name = convert_method_name_to_file_name(method_name); /* set context and secure store callback */ context = strdup(((key_store_name))); - OQS_SIG_STFL_SECRET_KEY_SET_store_cb(lock_test_sk, test_save_secret_key, (void *)context); + OQS_SIG_STFL_SECRET_KEY_SET_store_cb(lock_test_sk, save_secret_key, (void *)context); /* * Get max num signature and the amount remaining @@ -933,7 +933,7 @@ static OQS_STATUS sig_stfl_test_secret_key_lock(const char *method_name, const c /* set context and secure store callback */ if (lock_test_sk->set_scrt_key_store_cb) { lock_test_context = convert_method_name_to_file_name(method_name); - lock_test_sk->set_scrt_key_store_cb(lock_test_sk, test_save_secret_key, (void *)lock_test_context); + lock_test_sk->set_scrt_key_store_cb(lock_test_sk, save_secret_key, (void *)lock_test_context); } return OQS_SUCCESS;