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 4 commits
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
3 changes: 3 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 @@ -20,8 +20,11 @@

#define DEFAULT_SERVERS "localhost:4433"

/* must be treated as an opaque object by users */
struct ra_tls_ctx {
void* ssl;
void* net;
void* conf;
};

typedef int (*verify_measurements_cb_t)(const char* mrenclave, const char* mrsigner,
Expand Down
171 changes: 106 additions & 65 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 @@ -33,39 +33,58 @@
#include "secret_prov.h"
#include "util.h"

/* these are globals because the user may continue using the SSL session even after invoking
* secret_provision_start() (in the user-supplied callback) */
static mbedtls_ctr_drbg_context g_ctr_drbg;
static mbedtls_entropy_context g_entropy;
static mbedtls_ssl_config g_conf;
static mbedtls_x509_crt g_verifier_ca_chain;
static mbedtls_net_context g_verifier_fd;
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;

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;
}

extern int common_write(mbedtls_ssl_context* ssl, const uint8_t* buf, size_t size);
extern int common_read(mbedtls_ssl_context* ssl, uint8_t* buf, size_t size);
extern int common_close(mbedtls_ssl_context* ssl);

int secret_provision_write(struct ra_tls_ctx* ctx, const uint8_t* buf, size_t size) {
mbedtls_ssl_context* ssl = (mbedtls_ssl_context*)ctx->ssl;
return common_write(ssl, buf, size);
}

int secret_provision_read(struct ra_tls_ctx* ctx, uint8_t* buf, size_t size) {
mbedtls_ssl_context* ssl = (mbedtls_ssl_context*)ctx->ssl;
return common_read(ssl, buf, size);
}

int secret_provision_close(struct ra_tls_ctx* ctx) {
mbedtls_ssl_context* ssl = (mbedtls_ssl_context*)ctx->ssl;
Copy link
Contributor

Choose a reason for hiding this comment

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

secret_provision_close may be called twice by user. Consider to add parameter check here? e,g,
if(!ctx)
if(!ssl)

mbedtls_ssl_config* conf = (mbedtls_ssl_config*)ctx->conf;
mbedtls_net_context* net = (mbedtls_net_context*)ctx->net;

int ret = common_close(ssl);

mbedtls_ssl_free(ssl);
mbedtls_ssl_config_free(conf);
mbedtls_net_free(net);
free(ssl);
free(conf);
free(net);
return ret;
Copy link
Contributor

Choose a reason for hiding this comment

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

consider to zero the variables before return.
ctx->net = 0;
ctx->ssl = 0;
...

}

int secret_provision_start(const char* in_servers, const char* in_ca_chain_path,
Expand All @@ -78,19 +97,31 @@ int secret_provision_start(const char* in_servers, const char* in_ca_chain_path,
char* connected_addr = NULL;
char* connected_port = NULL;

mbedtls_ctr_drbg_init(&g_ctr_drbg);
mbedtls_entropy_init(&g_entropy);
mbedtls_x509_crt_init(&g_verifier_ca_chain);
mbedtls_ctr_drbg_context ctr_drbg;
mbedtls_entropy_context entropy;
mbedtls_x509_crt verifier_ca_chain;
mbedtls_pk_context my_ratls_key;
mbedtls_x509_crt my_ratls_cert;

mbedtls_net_context* verifier_fd = malloc(sizeof(*verifier_fd));
mbedtls_ssl_config* conf = malloc(sizeof(*conf));
mbedtls_ssl_context* ssl = malloc(sizeof(*ssl));
if (!verifier_fd || !conf || !ssl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if verifier_fd was allocated successfully, but ssl failed to be allocated, then return -ENOMEM here will lead to memory leak.

return -ENOMEM;
}

mbedtls_pk_init(&g_my_ratls_key);
mbedtls_x509_crt_init(&g_my_ratls_cert);
mbedtls_ctr_drbg_init(&ctr_drbg);
mbedtls_entropy_init(&entropy);
mbedtls_x509_crt_init(&verifier_ca_chain);
mbedtls_pk_init(&my_ratls_key);
mbedtls_x509_crt_init(&my_ratls_cert);

mbedtls_net_init(&g_verifier_fd);
mbedtls_ssl_config_init(&g_conf);
mbedtls_ssl_init(&g_ssl);
mbedtls_net_init(verifier_fd);
mbedtls_ssl_config_init(conf);
mbedtls_ssl_init(ssl);

const char* pers = "secret-provisioning";
ret = mbedtls_ctr_drbg_seed(&g_ctr_drbg, mbedtls_entropy_func, &g_entropy,
ret = mbedtls_ctr_drbg_seed(&ctr_drbg, mbedtls_entropy_func, &entropy,
(const uint8_t*)pers, strlen(pers));
if (ret < 0) {
goto out;
Expand Down Expand Up @@ -137,7 +168,7 @@ int secret_provision_start(const char* in_servers, const char* in_ca_chain_path,
if (!connected_port)
continue;

ret = mbedtls_net_connect(&g_verifier_fd, connected_addr, connected_port,
ret = mbedtls_net_connect(verifier_fd, connected_addr, connected_port,
MBEDTLS_NET_PROTO_TCP);
if (!ret)
break;
Expand All @@ -147,53 +178,53 @@ int secret_provision_start(const char* in_servers, const char* in_ca_chain_path,
goto out;
}

ret = mbedtls_ssl_config_defaults(&g_conf, MBEDTLS_SSL_IS_CLIENT, MBEDTLS_SSL_TRANSPORT_STREAM,
ret = mbedtls_ssl_config_defaults(conf, MBEDTLS_SSL_IS_CLIENT, MBEDTLS_SSL_TRANSPORT_STREAM,
MBEDTLS_SSL_PRESET_DEFAULT);
if (ret < 0) {
goto out;
}

ret = mbedtls_x509_crt_parse_file(&g_verifier_ca_chain, ca_chain_path);
ret = mbedtls_x509_crt_parse_file(&verifier_ca_chain, ca_chain_path);
if (ret != 0) {
goto out;
}

char crt_issuer[256];
ret = mbedtls_x509_dn_gets(crt_issuer, sizeof(crt_issuer), &g_verifier_ca_chain.issuer);
ret = mbedtls_x509_dn_gets(crt_issuer, sizeof(crt_issuer), &verifier_ca_chain.issuer);
if (ret < 0) {
goto out;
}

mbedtls_ssl_conf_authmode(&g_conf, MBEDTLS_SSL_VERIFY_REQUIRED);
mbedtls_ssl_conf_ca_chain(&g_conf, &g_verifier_ca_chain, NULL);
mbedtls_ssl_conf_authmode(conf, MBEDTLS_SSL_VERIFY_REQUIRED);
mbedtls_ssl_conf_ca_chain(conf, &verifier_ca_chain, NULL);

ret = ra_tls_create_key_and_crt(&g_my_ratls_key, &g_my_ratls_cert);
ret = ra_tls_create_key_and_crt(&my_ratls_key, &my_ratls_cert);
if (ret < 0) {
goto out;
}

mbedtls_ssl_conf_rng(&g_conf, mbedtls_ctr_drbg_random, &g_ctr_drbg);
mbedtls_ssl_conf_rng(conf, mbedtls_ctr_drbg_random, &ctr_drbg);

ret = mbedtls_ssl_conf_own_cert(&g_conf, &g_my_ratls_cert, &g_my_ratls_key);
ret = mbedtls_ssl_conf_own_cert(conf, &my_ratls_cert, &my_ratls_key);
if (ret < 0) {
goto out;
}

ret = mbedtls_ssl_setup(&g_ssl, &g_conf);
ret = mbedtls_ssl_setup(ssl, conf);
if (ret < 0) {
goto out;
}

ret = mbedtls_ssl_set_hostname(&g_ssl, connected_addr);
ret = mbedtls_ssl_set_hostname(ssl, connected_addr);
if (ret < 0) {
goto out;
}

mbedtls_ssl_set_bio(&g_ssl, &g_verifier_fd, mbedtls_net_send, mbedtls_net_recv, NULL);
mbedtls_ssl_set_bio(ssl, verifier_fd, mbedtls_net_send, mbedtls_net_recv, NULL);

ret = -1;
while (ret < 0) {
ret = mbedtls_ssl_handshake(&g_ssl);
ret = mbedtls_ssl_handshake(ssl);
if (ret == MBEDTLS_ERR_SSL_WANT_READ || ret == MBEDTLS_ERR_SSL_WANT_WRITE) {
continue;
}
Expand All @@ -202,13 +233,12 @@ int secret_provision_start(const char* in_servers, const char* in_ca_chain_path,
}
}

uint32_t flags = mbedtls_ssl_get_verify_result(&g_ssl);
uint32_t flags = mbedtls_ssl_get_verify_result(ssl);
if (flags != 0) {
ret = MBEDTLS_ERR_X509_CERT_VERIFY_FAILED;
goto out;
}

struct ra_tls_ctx ctx = {.ssl = &g_ssl};
uint8_t buf[128] = {0};
size_t size;

Expand All @@ -217,7 +247,7 @@ int secret_provision_start(const char* in_servers, const char* in_ca_chain_path,
size = sprintf((char*)buf, SECRET_PROVISION_REQUEST);
size += 1; /* include null byte */

ret = secret_provision_write(&ctx, buf, size);
ret = common_write(ssl, buf, size);
if (ret < 0) {
goto out;
}
Expand All @@ -228,8 +258,7 @@ int secret_provision_start(const char* in_servers, const char* in_ca_chain_path,
"buffer must be sufficiently large to hold SECRET_PROVISION_RESPONSE + int32");

memset(buf, 0, sizeof(buf));
ret = secret_provision_read(&ctx, buf,
sizeof(SECRET_PROVISION_RESPONSE) + sizeof(received_secret_size));
ret = common_read(ssl, buf, sizeof(SECRET_PROVISION_RESPONSE) + sizeof(received_secret_size));
if (ret < 0) {
goto out;
}
Expand All @@ -248,40 +277,51 @@ 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 = malloc(received_secret_size);
if (!g_provisioned_secret) {
ret = -ENOMEM;
goto out;
}
g_provisioned_secret_size = received_secret_size;

ret = secret_provision_read(&ctx, provisioned_secret, provisioned_secret_size);
ret = common_read(ssl, g_provisioned_secret, g_provisioned_secret_size);
if (ret < 0) {
goto out;
}

if (out_ctx) {
out_ctx->ssl = ctx.ssl;
/* pass ownership of SSL session to the caller; it is caller's responsibility to gracefuly
* terminate the session using secret_provision_close() */
out_ctx->ssl = ssl;
out_ctx->conf = conf;
out_ctx->net = verifier_fd;
} else {
secret_provision_close(&ctx);
common_close(ssl);
}

ret = 0;
out:
if (!out_ctx) {
mbedtls_ssl_free(ssl);
mbedtls_ssl_config_free(conf);
mbedtls_net_free(verifier_fd);
free(ssl);
free(conf);
free(verifier_fd);
}

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);
}
mbedtls_x509_crt_free(&my_ratls_cert);
mbedtls_pk_free(&my_ratls_key);
mbedtls_x509_crt_free(&verifier_ca_chain);
mbedtls_ctr_drbg_free(&ctr_drbg);
mbedtls_entropy_free(&entropy);

free(servers);
free(ca_chain_path);
Expand Down Expand Up @@ -348,7 +388,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
30 changes: 14 additions & 16 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 @@ -16,17 +16,19 @@

#include "secret_prov.h"

int secret_provision_write(struct ra_tls_ctx* ctx, const uint8_t* buf, size_t size) {
int common_write(mbedtls_ssl_context* ssl, const uint8_t* buf, size_t size);
Copy link
Contributor

Choose a reason for hiding this comment

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

it's too common to use prefix common_ , may harm other function definition in Gramine modules
actually it's tls_

int common_read(mbedtls_ssl_context* ssl, uint8_t* buf, size_t size);
int common_close(mbedtls_ssl_context* ssl);

int common_write(mbedtls_ssl_context* ssl, const uint8_t* buf, size_t size) {
int ret;

if (!ctx || !ctx->ssl || size > INT_MAX)
if (!ssl || size > INT_MAX)
return -EINVAL;

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

size_t written = 0;
while (written < size) {
ret = mbedtls_ssl_write(_ssl, buf + written, size - written);
ret = mbedtls_ssl_write(ssl, buf + written, size - written);
if (ret == MBEDTLS_ERR_SSL_WANT_READ || ret == MBEDTLS_ERR_SSL_WANT_WRITE)
continue;
if (ret < 0) {
Expand All @@ -39,17 +41,15 @@ int secret_provision_write(struct ra_tls_ctx* ctx, const uint8_t* buf, size_t si
return (int)written;
}

int secret_provision_read(struct ra_tls_ctx* ctx, uint8_t* buf, size_t size) {
int common_read(mbedtls_ssl_context* ssl, uint8_t* buf, size_t size) {
int ret;

if (!ctx || !ctx->ssl || size > INT_MAX)
if (!ssl || size > INT_MAX)
return -EINVAL;

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

size_t read = 0;
while (read < size) {
ret = mbedtls_ssl_read(_ssl, buf + read, size - read);
ret = mbedtls_ssl_read(ssl, buf + read, size - read);
if (!ret)
return -ECONNRESET;
if (ret == MBEDTLS_ERR_SSL_WANT_READ || ret == MBEDTLS_ERR_SSL_WANT_WRITE)
Expand All @@ -65,21 +65,19 @@ int secret_provision_read(struct ra_tls_ctx* ctx, uint8_t* buf, size_t size) {
return (int)read;
}

int secret_provision_close(struct ra_tls_ctx* ctx) {
if (!ctx || !ctx->ssl)
int common_close(mbedtls_ssl_context* ssl) {
if (!ssl)
return 0;

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

int ret = -1;
while (ret < 0) {
ret = mbedtls_ssl_close_notify(_ssl);
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;
return ret == MBEDTLS_ERR_SSL_PEER_CLOSE_NOTIFY ? -ECONNRESET : -EPERM;
}
}
return 0;
Expand Down
Loading