Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Pal/Linux-SGX] tools: Fix resource leak in Secret Prov lib #647

Closed
wants to merge 5 commits into from
Closed

[Pal/Linux-SGX] tools: Fix resource leak in Secret Prov lib #647

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
91b764b
[Pal/Linux-SGX] tools: Fix resource leak in Secret Prov lib
dimakuv Jun 10, 2022
73a14b5
fixup! [Pal/Linux-SGX] tools: Fix resource leak in Secret Prov lib
dimakuv Jun 10, 2022
fc7f1d2
fixup! [Pal/Linux-SGX] tools: Fix resource leak in Secret Prov lib
dimakuv Jun 13, 2022
3649d7a
fixup! [Pal/Linux-SGX] tools: Fix resource leak in Secret Prov lib
dimakuv Jun 15, 2022
48f963e
fixup! [Pal/Linux-SGX] tools: Fix resource leak in Secret Prov lib
dimakuv Jun 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ typedef int (*verify_measurements_cb_t)(const char* mrenclave, const char* mrsig

typedef int (*secret_provision_cb_t)(struct ra_tls_ctx* ctx);

/* internally used functions, not exported */
__attribute__ ((visibility("hidden")))
void secret_provision_free_resources(void);

/*!
* \brief Write arbitrary data in an established RA-TLS session.
*
Expand Down
57 changes: 31 additions & 26 deletions Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_attest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,28 +44,39 @@ static mbedtls_ssl_context g_ssl;
static mbedtls_pk_context g_my_ratls_key;
static mbedtls_x509_crt g_my_ratls_cert;

static uint8_t* provisioned_secret = NULL;
static size_t provisioned_secret_size = 0;
static uint8_t* g_provisioned_secret = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

global variables means cannot be used in multi-threads environment.

static size_t g_provisioned_secret_size = 0;

void secret_provision_free_resources(void) {
mbedtls_x509_crt_free(&g_my_ratls_cert);
mbedtls_pk_free(&g_my_ratls_key);
mbedtls_net_free(&g_verifier_fd);
mbedtls_ssl_free(&g_ssl);
mbedtls_ssl_config_free(&g_conf);
mbedtls_x509_crt_free(&g_verifier_ca_chain);
mbedtls_ctr_drbg_free(&g_ctr_drbg);
mbedtls_entropy_free(&g_entropy);
}

int secret_provision_get(uint8_t** out_secret, size_t* out_secret_size) {
if (!out_secret || !out_secret_size)
return -EINVAL;

*out_secret = provisioned_secret;
*out_secret_size = provisioned_secret_size;
*out_secret = g_provisioned_secret;
*out_secret_size = g_provisioned_secret_size;
return 0;
}

void secret_provision_destroy(void) {
if (provisioned_secret && provisioned_secret_size)
if (g_provisioned_secret && g_provisioned_secret_size)
#ifdef __STDC_LIB_EXT1__
memset_s(provisioned_secret, 0, provisioned_secret_size);
memset_s(g_provisioned_secret, 0, g_provisioned_secret_size);
#else
memset(provisioned_secret, 0, provisioned_secret_size);
memset(g_provisioned_secret, 0, g_provisioned_secret_size);
#endif
free(provisioned_secret);
provisioned_secret = NULL;
provisioned_secret_size = 0;
free(g_provisioned_secret);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems need to add"if (g_provisioned_secret)", otherwise free() may lead to exception when calling secret_provision_destroy() twice

g_provisioned_secret = NULL;
g_provisioned_secret_size = 0;
}

int secret_provision_start(const char* in_servers, const char* in_ca_chain_path,
Expand Down Expand Up @@ -248,14 +259,17 @@ int secret_provision_start(const char* in_servers, const char* in_ca_chain_path,
goto out;
}

provisioned_secret_size = received_secret_size;
provisioned_secret = malloc(provisioned_secret_size);
if (!provisioned_secret) {
/* destroy a previously provisioned secret, if any */
secret_provision_destroy();

g_provisioned_secret_size = received_secret_size;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems this line should be moved to line 271 where malloc have succeeded.

g_provisioned_secret = malloc(g_provisioned_secret_size);
if (!g_provisioned_secret) {
ret = -ENOMEM;
goto out;
}

ret = secret_provision_read(&ctx, provisioned_secret, provisioned_secret_size);
ret = secret_provision_read(&ctx, g_provisioned_secret, g_provisioned_secret_size);
if (ret < 0) {
goto out;
}
Expand All @@ -270,17 +284,7 @@ int secret_provision_start(const char* in_servers, const char* in_ca_chain_path,
out:
if (ret < 0) {
secret_provision_destroy();
}

if (ret < 0 || !out_ctx) {
mbedtls_x509_crt_free(&g_my_ratls_cert);
mbedtls_pk_free(&g_my_ratls_key);
mbedtls_net_free(&g_verifier_fd);
mbedtls_ssl_free(&g_ssl);
mbedtls_ssl_config_free(&g_conf);
mbedtls_x509_crt_free(&g_verifier_ca_chain);
mbedtls_ctr_drbg_free(&g_ctr_drbg);
mbedtls_entropy_free(&g_entropy);
secret_provision_free_resources();
}

free(servers);
Expand Down Expand Up @@ -348,7 +352,8 @@ __attribute__((constructor)) static void secret_provision_constructor(void) {

size_t total_written = 0;
while (total_written < sizeof(keydata)) {
ssize_t written = write(fd, keydata + total_written, sizeof(keydata) - total_written);
ssize_t written = write(fd, keydata + total_written,
sizeof(keydata) - total_written);
if (written > 0) {
total_written += written;
} else if (written == 0) {
Expand Down
18 changes: 13 additions & 5 deletions Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,21 +66,29 @@ int secret_provision_read(struct ra_tls_ctx* ctx, uint8_t* buf, size_t size) {
}

int secret_provision_close(struct ra_tls_ctx* ctx) {
if (!ctx || !ctx->ssl)
return 0;
int ret;
if (!ctx || !ctx->ssl) {
ret = 0;
goto out;
}

mbedtls_ssl_context* _ssl = (mbedtls_ssl_context*)ctx->ssl;

int ret = -1;
ret = -1;
while (ret < 0) {
ret = mbedtls_ssl_close_notify(_ssl);
if (ret == MBEDTLS_ERR_SSL_WANT_READ || ret == MBEDTLS_ERR_SSL_WANT_WRITE) {
continue;
}
if (ret < 0) {
/* use well-known error code for a typical case when remote party closes connection */
return ret == MBEDTLS_ERR_SSL_PEER_CLOSE_NOTIFY ? -ECONNRESET : ret;
if (ret == MBEDTLS_ERR_SSL_PEER_CLOSE_NOTIFY)
ret = -ECONNRESET;
goto out;
}
}
return 0;
ret = 0;
out:
secret_provision_free_resources();
return ret;
}
4 changes: 4 additions & 0 deletions Pal/src/host/Linux-SGX/tools/ra-tls/secret_prov_verify.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ struct thread_info {
/* SSL/TLS + RA-TLS handshake is not thread-safe, use coarse-grained lock */
static pthread_mutex_t g_handshake_lock;

void secret_provision_free_resources(void) {
/* nothing to be freed */
}

static void* client_connection(void* data) {
int ret;
struct thread_info* ti = (struct thread_info*)data;
Expand Down