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 <dgoulet@torproject.org>

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:<Base64 Blob>" - 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:<Algorithm>" - 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:

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 *

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