From 8c02fc15ae8391d800926c0c6df7fb258139ce79 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 5 Dec 2017 14:24:00 -0500 Subject: [PATCH] control: Don't use void pointer for ADD_ONION secret key Make this a bit more safe with at least type checking of the pointers depending on the version. Signed-off-by: David Goulet --- src/or/control.c | 30 +++++++------- src/or/control.h | 13 +++++- src/test/test_controller.c | 81 ++++++++++++++++++++------------------ 3 files changed, 69 insertions(+), 55 deletions(-) diff --git a/src/or/control.c b/src/or/control.c index dc045c39a4..d84ce7d639 100644 --- a/src/or/control.c +++ b/src/or/control.c @@ -4458,10 +4458,11 @@ handle_control_hspost(control_connection_t *conn, * containing the onion address without the .onion part. On error, address_out * is untouched. */ static hs_service_add_ephemeral_status_t -add_onion_helper_add_service(int hs_version, void *pk, smartlist_t *port_cfgs, - int max_streams, int max_streams_close_circuit, - int auth_type, smartlist_t *auth_clients, - char **address_out) +add_onion_helper_add_service(int hs_version, + add_onion_secret_key_t *pk, + smartlist_t *port_cfgs, int max_streams, + int max_streams_close_circuit, int auth_type, + smartlist_t *auth_clients, char **address_out) { hs_service_add_ephemeral_status_t ret; @@ -4471,12 +4472,12 @@ add_onion_helper_add_service(int hs_version, void *pk, smartlist_t *port_cfgs, switch (hs_version) { case HS_VERSION_TWO: - ret = rend_service_add_ephemeral(pk, port_cfgs, max_streams, + ret = rend_service_add_ephemeral(pk->v2, port_cfgs, max_streams, max_streams_close_circuit, auth_type, auth_clients, address_out); break; case HS_VERSION_THREE: - ret = hs_service_add_ephemeral(pk, port_cfgs, max_streams, + ret = hs_service_add_ephemeral(pk->v3, port_cfgs, max_streams, max_streams_close_circuit, address_out); break; default: @@ -4668,7 +4669,7 @@ handle_control_add_onion(control_connection_t *conn, /* Parse the "keytype:keyblob" argument. */ int hs_version = 0; - void *pk = NULL; + add_onion_secret_key_t pk; const char *key_new_alg = NULL; char *key_new_blob = NULL; char *err_msg = NULL; @@ -4697,7 +4698,7 @@ handle_control_add_onion(control_connection_t *conn, * regardless of success/failure. */ char *service_id = NULL; - int ret = add_onion_helper_add_service(hs_version, pk, port_cfgs, + int ret = add_onion_helper_add_service(hs_version, &pk, port_cfgs, max_streams, max_streams_close_circuit, auth_type, auth_clients, &service_id); @@ -4796,7 +4797,7 @@ handle_control_add_onion(control_connection_t *conn, STATIC int add_onion_helper_keyarg(const char *arg, int discard_pk, const char **key_new_alg_out, char **key_new_blob_out, - void **decoded_key, int *hs_version, + add_onion_secret_key_t *decoded_key, int *hs_version, char **err_msg_out) { smartlist_t *key_args = smartlist_new(); @@ -4806,8 +4807,6 @@ add_onion_helper_keyarg(const char *arg, int discard_pk, char *err_msg = NULL; int ret = -1; - *decoded_key = NULL; - smartlist_split_string(key_args, arg, ":", SPLIT_IGNORE_BLANK, 0); if (smartlist_len(key_args) != 2) { err_msg = tor_strdup("512 Invalid key type/blob\r\n"); @@ -4835,7 +4834,7 @@ add_onion_helper_keyarg(const char *arg, int discard_pk, err_msg = tor_strdup("512 Invalid RSA key size\r\n"); goto err; } - *decoded_key = pk; + decoded_key->v2 = pk; *hs_version = HS_VERSION_TWO; } else if (!strcasecmp(key_type_ed25519_v3, key_type)) { /* "ED25519-V3:" - Loading a pre-existing ed25519 key. */ @@ -4846,7 +4845,7 @@ add_onion_helper_keyarg(const char *arg, int discard_pk, err_msg = tor_strdup("512 Failed to decode ED25519-V3 key\r\n"); goto err; } - *decoded_key = sk; + decoded_key->v3 = sk; *hs_version = HS_VERSION_THREE; } else if (!strcasecmp(key_type_new, key_type)) { /* "NEW:" - Generating a new key, blob as algorithm. */ @@ -4868,7 +4867,7 @@ add_onion_helper_keyarg(const char *arg, int discard_pk, } key_new_alg = key_type_rsa1024; } - *decoded_key = pk; + decoded_key->v2 = pk; *hs_version = HS_VERSION_TWO; } else if (!strcasecmp(key_type_ed25519_v3, key_blob)) { ed25519_secret_key_t *sk = tor_malloc_zero(sizeof(*sk)); @@ -4891,7 +4890,7 @@ add_onion_helper_keyarg(const char *arg, int discard_pk, } key_new_alg = key_type_ed25519_v3; } - *decoded_key = sk; + decoded_key->v3 = sk; *hs_version = HS_VERSION_THREE; } else { err_msg = tor_strdup("513 Invalid key type\r\n"); @@ -4903,7 +4902,6 @@ add_onion_helper_keyarg(const char *arg, int discard_pk, } /* Succeded in loading or generating a private key. */ - tor_assert(*decoded_key); ret = 0; err: diff --git a/src/or/control.h b/src/or/control.h index 23f6eed8e5..28ffeaed86 100644 --- a/src/or/control.h +++ b/src/or/control.h @@ -263,9 +263,20 @@ void format_cell_stats(char **event_string, circuit_t *circ, cell_stats_t *cell_stats); STATIC char *get_bw_samples(void); +/* ADD_ONION secret key to create an ephemeral service. The command supports + * multiple versions so this union stores the key and passes it to the HS + * subsystem depending on the requested version. */ +typedef union add_onion_secret_key_t { + /* Hidden service v2 secret key. */ + crypto_pk_t *v2; + /* Hidden service v3 secret key. */ + ed25519_secret_key_t *v3; +} add_onion_secret_key_t; + STATIC int add_onion_helper_keyarg(const char *arg, int discard_pk, const char **key_new_alg_out, - char **key_new_blob_out, void **decoded_key, + char **key_new_blob_out, + add_onion_secret_key_t *decoded_key, int *hs_version, char **err_msg_out); STATIC rend_authorized_client_t * diff --git a/src/test/test_controller.c b/src/test/test_controller.c index a5132bd4c9..af19f63f6c 100644 --- a/src/test/test_controller.c +++ b/src/test/test_controller.c @@ -17,37 +17,39 @@ static void test_add_onion_helper_keyarg_v3(void *arg) { int ret, hs_version; - void *pk_ptr = NULL; + add_onion_secret_key_t pk; char *key_new_blob = NULL; char *err_msg = NULL; const char *key_new_alg = NULL; (void) arg; + memset(&pk, 0, sizeof(pk)); + /* Test explicit ED25519-V3 key generation. */ ret = add_onion_helper_keyarg("NEW:ED25519-V3", 0, &key_new_alg, - &key_new_blob, &pk_ptr, &hs_version, + &key_new_blob, &pk, &hs_version, &err_msg); tt_int_op(ret, OP_EQ, 0); tt_int_op(hs_version, OP_EQ, HS_VERSION_THREE); - tt_assert(pk_ptr); + tt_assert(pk.v3); tt_str_op(key_new_alg, OP_EQ, "ED25519-V3"); tt_assert(key_new_blob); tt_ptr_op(err_msg, OP_EQ, NULL); - tor_free(pk_ptr); pk_ptr = NULL; + tor_free(pk.v3); pk.v3 = NULL; tor_free(key_new_blob); /* Test discarding the private key. */ ret = add_onion_helper_keyarg("NEW:ED25519-V3", 1, &key_new_alg, - &key_new_blob, &pk_ptr, &hs_version, + &key_new_blob, &pk, &hs_version, &err_msg); tt_int_op(ret, OP_EQ, 0); tt_int_op(hs_version, OP_EQ, HS_VERSION_THREE); - tt_assert(pk_ptr); + tt_assert(pk.v3); tt_ptr_op(key_new_alg, OP_EQ, NULL); tt_ptr_op(key_new_blob, OP_EQ, NULL); tt_ptr_op(err_msg, OP_EQ, NULL); - tor_free(pk_ptr); pk_ptr = NULL; + tor_free(pk.v3); pk.v3 = NULL; tor_free(key_new_blob); /* Test passing a key blob. */ @@ -67,22 +69,22 @@ test_add_onion_helper_keyarg_v3(void *arg) tor_asprintf(&key_blob, "ED25519-V3:%s", base64_sk); tt_assert(key_blob); ret = add_onion_helper_keyarg(key_blob, 1, &key_new_alg, - &key_new_blob, &pk_ptr, &hs_version, + &key_new_blob, &pk, &hs_version, &err_msg); tor_free(key_blob); tt_int_op(ret, OP_EQ, 0); tt_int_op(hs_version, OP_EQ, HS_VERSION_THREE); - tt_assert(pk_ptr); - tt_mem_op(pk_ptr, OP_EQ, hex_sk, 64); + tt_assert(pk.v3); + tt_mem_op(pk.v3, OP_EQ, hex_sk, 64); tt_ptr_op(key_new_alg, OP_EQ, NULL); tt_ptr_op(key_new_blob, OP_EQ, NULL); tt_ptr_op(err_msg, OP_EQ, NULL); - tor_free(pk_ptr); pk_ptr = NULL; + tor_free(pk.v3); pk.v3 = NULL; tor_free(key_new_blob); } done: - tor_free(pk_ptr); + tor_free(pk.v3); tor_free(key_new_blob); tor_free(err_msg); } @@ -91,8 +93,8 @@ static void test_add_onion_helper_keyarg_v2(void *arg) { int ret, hs_version; - void *pk_ptr = NULL; - crypto_pk_t *pk = NULL; + add_onion_secret_key_t pk; + crypto_pk_t *pk1 = NULL; const char *key_new_alg = NULL; char *key_new_blob = NULL; char *err_msg = NULL; @@ -101,97 +103,100 @@ test_add_onion_helper_keyarg_v2(void *arg) (void) arg; + memset(&pk, 0, sizeof(pk)); + /* Test explicit RSA1024 key generation. */ ret = add_onion_helper_keyarg("NEW:RSA1024", 0, &key_new_alg, &key_new_blob, - &pk_ptr, &hs_version, &err_msg); + &pk, &hs_version, &err_msg); tt_int_op(ret, OP_EQ, 0); tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO); - tt_assert(pk_ptr); + tt_assert(pk.v2); tt_str_op(key_new_alg, OP_EQ, "RSA1024"); tt_assert(key_new_blob); tt_ptr_op(err_msg, OP_EQ, NULL); /* Test "BEST" key generation (Assumes BEST = RSA1024). */ - crypto_pk_free(pk_ptr); pk_ptr = NULL; + crypto_pk_free(pk.v2); pk.v2 = NULL; tor_free(key_new_blob); ret = add_onion_helper_keyarg("NEW:BEST", 0, &key_new_alg, &key_new_blob, - &pk_ptr, &hs_version, &err_msg); + &pk, &hs_version, &err_msg); tt_int_op(ret, OP_EQ, 0); tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO); - tt_assert(pk_ptr); + tt_assert(pk.v2); tt_str_op(key_new_alg, OP_EQ, "RSA1024"); tt_assert(key_new_blob); tt_ptr_op(err_msg, OP_EQ, NULL); /* Test discarding the private key. */ - crypto_pk_free(pk_ptr); pk_ptr = NULL; + crypto_pk_free(pk.v2); pk.v2 = NULL; tor_free(key_new_blob); ret = add_onion_helper_keyarg("NEW:BEST", 1, &key_new_alg, &key_new_blob, - &pk_ptr, &hs_version, &err_msg); + &pk, &hs_version, &err_msg); tt_int_op(ret, OP_EQ, 0); tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO); - tt_assert(pk_ptr); + tt_assert(pk.v2); tt_ptr_op(key_new_alg, OP_EQ, NULL); tt_ptr_op(key_new_blob, OP_EQ, NULL); tt_ptr_op(err_msg, OP_EQ, NULL); /* Test generating a invalid key type. */ - crypto_pk_free(pk_ptr); pk_ptr = NULL; + crypto_pk_free(pk.v2); pk.v2 = NULL; ret = add_onion_helper_keyarg("NEW:RSA512", 0, &key_new_alg, &key_new_blob, - &pk_ptr, &hs_version, &err_msg); + &pk, &hs_version, &err_msg); tt_int_op(ret, OP_EQ, -1); tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO); - tt_ptr_op(pk_ptr, OP_EQ, NULL); + tt_assert(!pk.v2); tt_ptr_op(key_new_alg, OP_EQ, NULL); tt_ptr_op(key_new_blob, OP_EQ, NULL); tt_assert(err_msg); /* Test loading a RSA1024 key. */ tor_free(err_msg); - pk = pk_generate(0); - tt_int_op(0, OP_EQ, crypto_pk_base64_encode(pk, &encoded)); + pk1 = pk_generate(0); + tt_int_op(0, OP_EQ, crypto_pk_base64_encode(pk1, &encoded)); tor_asprintf(&arg_str, "RSA1024:%s", encoded); ret = add_onion_helper_keyarg(arg_str, 0, &key_new_alg, &key_new_blob, - &pk_ptr, &hs_version, &err_msg); + &pk, &hs_version, &err_msg); tt_int_op(ret, OP_EQ, 0); tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO); - tt_assert(pk_ptr); + tt_assert(pk.v2); tt_ptr_op(key_new_alg, OP_EQ, NULL); tt_ptr_op(key_new_blob, OP_EQ, NULL); tt_ptr_op(err_msg, OP_EQ, NULL); - tt_int_op(crypto_pk_cmp_keys(pk, pk_ptr), OP_EQ, 0); + tt_int_op(crypto_pk_cmp_keys(pk1, pk.v2), OP_EQ, 0); /* Test loading a invalid key type. */ tor_free(arg_str); - crypto_pk_free(pk); pk = NULL; - crypto_pk_free(pk_ptr); pk_ptr = NULL; + crypto_pk_free(pk1); pk1 = NULL; + crypto_pk_free(pk.v2); pk.v2 = NULL; tor_asprintf(&arg_str, "RSA512:%s", encoded); ret = add_onion_helper_keyarg(arg_str, 0, &key_new_alg, &key_new_blob, - &pk_ptr, &hs_version, &err_msg); + &pk, &hs_version, &err_msg); tt_int_op(ret, OP_EQ, -1); tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO); - tt_ptr_op(pk_ptr, OP_EQ, NULL); + tt_assert(!pk.v2); tt_ptr_op(key_new_alg, OP_EQ, NULL); tt_ptr_op(key_new_blob, OP_EQ, NULL); tt_assert(err_msg); /* Test loading a invalid key. */ tor_free(arg_str); - crypto_pk_free(pk_ptr); pk_ptr = NULL; + crypto_pk_free(pk.v2); pk.v2 = NULL; tor_free(err_msg); encoded[strlen(encoded)/2] = '\0'; tor_asprintf(&arg_str, "RSA1024:%s", encoded); ret = add_onion_helper_keyarg(arg_str, 0, &key_new_alg, &key_new_blob, - &pk_ptr, &hs_version, &err_msg); + &pk, &hs_version, &err_msg); tt_int_op(ret, OP_EQ, -1); tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO); - tt_ptr_op(pk_ptr, OP_EQ, NULL); + tt_assert(!pk.v2); tt_ptr_op(key_new_alg, OP_EQ, NULL); tt_ptr_op(key_new_blob, OP_EQ, NULL); tt_assert(err_msg); done: - crypto_pk_free(pk_ptr); + crypto_pk_free(pk1); + crypto_pk_free(pk.v2); tor_free(key_new_blob); tor_free(err_msg); tor_free(encoded);