commit 5faf54970de4bc3c80a268b5b139c713413562fd
Author: Taylor Yu <catalyst(a)torproject.org>
Date: Tue Jun 25 10:34:53 2019 -0500
Fix some onion helpers
Fix add_onion_helper_clientauth() and add_onion_helper_keyarg() to
explicitly call the appropriate control reply abstractions instead of
allocating a string to pass to their callers.
Part of ticket 30889.
---
src/feature/control/control_cmd.c | 75 ++++++++++++---------------
src/feature/control/control_cmd.h | 5 +-
src/test/test_controller.c | 106 +++++++++++++++++++++++---------------
3 files changed, 100 insertions(+), 86 deletions(-)
diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c
index 051dd12ba..ad4a4ef0a 100644
--- a/src/feature/control/control_cmd.c
+++ b/src/feature/control/control_cmd.c
@@ -1743,16 +1743,10 @@ handle_control_add_onion(control_connection_t *conn,
goto out;
} else if (!strcasecmp(arg->key, "ClientAuth")) {
- char *err_msg = NULL;
int created = 0;
rend_authorized_client_t *client =
- add_onion_helper_clientauth(arg->value,
- &created, &err_msg);
+ add_onion_helper_clientauth(arg->value, &created, conn);
if (!client) {
- if (err_msg) {
- connection_write_str_to_buf(err_msg, conn);
- tor_free(err_msg);
- }
goto out;
}
@@ -1817,19 +1811,13 @@ handle_control_add_onion(control_connection_t *conn,
add_onion_secret_key_t pk = { NULL };
const char *key_new_alg = NULL;
char *key_new_blob = NULL;
- char *err_msg = NULL;
const char *onionkey = smartlist_get(args->args, 0);
if (add_onion_helper_keyarg(onionkey, discard_pk,
&key_new_alg, &key_new_blob, &pk, &hs_version,
- &err_msg) < 0) {
- if (err_msg) {
- connection_write_str_to_buf(err_msg, conn);
- tor_free(err_msg);
- }
+ conn) < 0) {
goto out;
}
- tor_assert(!err_msg);
/* Hidden service version 3 don't have client authentication support so if
* ClientAuth was given, send back an error. */
@@ -1929,27 +1917,30 @@ handle_control_add_onion(control_connection_t *conn,
* ADD_ONION command. Return a new crypto_pk_t and if a new key was generated
* and the private key not discarded, the algorithm and serialized private key,
* or NULL and an optional control protocol error message on failure. The
- * caller is responsible for freeing the returned key_new_blob and err_msg.
+ * caller is responsible for freeing the returned key_new_blob.
*
* Note: The error messages returned are deliberately vague to avoid echoing
* key material.
+ *
+ * Note: conn is only used for writing control replies. For testing
+ * purposes, it can be NULL if control_write_reply() is appropriately
+ * mocked.
*/
STATIC int
add_onion_helper_keyarg(const char *arg, int discard_pk,
const char **key_new_alg_out, char **key_new_blob_out,
add_onion_secret_key_t *decoded_key, int *hs_version,
- char **err_msg_out)
+ control_connection_t *conn)
{
smartlist_t *key_args = smartlist_new();
crypto_pk_t *pk = NULL;
const char *key_new_alg = NULL;
char *key_new_blob = NULL;
- char *err_msg = NULL;
int ret = -1;
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");
+ control_write_endreply(conn, 512, "Invalid key type/blob");
goto err;
}
@@ -1966,12 +1957,12 @@ add_onion_helper_keyarg(const char *arg, int discard_pk,
/* "RSA:<Base64 Blob>" - Loading a pre-existing RSA1024 key. */
pk = crypto_pk_base64_decode_private(key_blob, strlen(key_blob));
if (!pk) {
- err_msg = tor_strdup("512 Failed to decode RSA key\r\n");
+ control_write_endreply(conn, 512, "Failed to decode RSA key");
goto err;
}
if (crypto_pk_num_bits(pk) != PK_BYTES*8) {
crypto_pk_free(pk);
- err_msg = tor_strdup("512 Invalid RSA key size\r\n");
+ control_write_endreply(conn, 512, "Invalid RSA key size");
goto err;
}
decoded_key->v2 = pk;
@@ -1982,7 +1973,7 @@ add_onion_helper_keyarg(const char *arg, int discard_pk,
if (base64_decode((char *) sk->seckey, sizeof(sk->seckey), key_blob,
strlen(key_blob)) != sizeof(sk->seckey)) {
tor_free(sk);
- err_msg = tor_strdup("512 Failed to decode ED25519-V3 key\r\n");
+ control_write_endreply(conn, 512, "Failed to decode ED25519-V3 key");
goto err;
}
decoded_key->v3 = sk;
@@ -1994,15 +1985,15 @@ add_onion_helper_keyarg(const char *arg, int discard_pk,
/* "RSA1024", RSA 1024 bit, also currently "BEST" by default. */
pk = crypto_pk_new();
if (crypto_pk_generate_key(pk)) {
- tor_asprintf(&err_msg, "551 Failed to generate %s key\r\n",
- key_type_rsa1024);
+ control_printf_endreply(conn, 551, "Failed to generate %s key",
+ key_type_rsa1024);
goto err;
}
if (!discard_pk) {
if (crypto_pk_base64_encode_private(pk, &key_new_blob)) {
crypto_pk_free(pk);
- tor_asprintf(&err_msg, "551 Failed to encode %s key\r\n",
- key_type_rsa1024);
+ control_printf_endreply(conn, 551, "Failed to encode %s key",
+ key_type_rsa1024);
goto err;
}
key_new_alg = key_type_rsa1024;
@@ -2013,8 +2004,8 @@ add_onion_helper_keyarg(const char *arg, int discard_pk,
ed25519_secret_key_t *sk = tor_malloc_zero(sizeof(*sk));
if (ed25519_secret_key_generate(sk, 1) < 0) {
tor_free(sk);
- tor_asprintf(&err_msg, "551 Failed to generate %s key\r\n",
- key_type_ed25519_v3);
+ control_printf_endreply(conn, 551, "Failed to generate %s key",
+ key_type_ed25519_v3);
goto err;
}
if (!discard_pk) {
@@ -2024,8 +2015,8 @@ add_onion_helper_keyarg(const char *arg, int discard_pk,
sizeof(sk->seckey), 0) != (len - 1)) {
tor_free(sk);
tor_free(key_new_blob);
- tor_asprintf(&err_msg, "551 Failed to encode %s key\r\n",
- key_type_ed25519_v3);
+ control_printf_endreply(conn, 551, "Failed to encode %s key",
+ key_type_ed25519_v3);
goto err;
}
key_new_alg = key_type_ed25519_v3;
@@ -2033,11 +2024,11 @@ add_onion_helper_keyarg(const char *arg, int discard_pk,
decoded_key->v3 = sk;
*hs_version = HS_VERSION_THREE;
} else {
- err_msg = tor_strdup("513 Invalid key type\r\n");
+ control_write_endreply(conn, 513, "Invalid key type");
goto err;
}
} else {
- err_msg = tor_strdup("513 Invalid key type\r\n");
+ control_write_endreply(conn, 513, "Invalid key type");
goto err;
}
@@ -2051,11 +2042,6 @@ add_onion_helper_keyarg(const char *arg, int discard_pk,
});
smartlist_free(key_args);
- if (err_msg_out) {
- *err_msg_out = err_msg;
- } else {
- tor_free(err_msg);
- }
*key_new_alg_out = key_new_alg;
*key_new_blob_out = key_new_blob;
@@ -2065,27 +2051,30 @@ add_onion_helper_keyarg(const char *arg, int discard_pk,
/** Helper function to handle parsing a ClientAuth argument to the
* ADD_ONION command. Return a new rend_authorized_client_t, or NULL
* and an optional control protocol error message on failure. The
- * caller is responsible for freeing the returned auth_client and err_msg.
+ * caller is responsible for freeing the returned auth_client.
*
* If 'created' is specified, it will be set to 1 when a new cookie has
* been generated.
+ *
+ * Note: conn is only used for writing control replies. For testing
+ * purposes, it can be NULL if control_write_reply() is appropriately
+ * mocked.
*/
STATIC rend_authorized_client_t *
-add_onion_helper_clientauth(const char *arg, int *created, char **err_msg)
+add_onion_helper_clientauth(const char *arg, int *created,
+ control_connection_t *conn)
{
int ok = 0;
tor_assert(arg);
tor_assert(created);
- tor_assert(err_msg);
- *err_msg = NULL;
smartlist_t *auth_args = smartlist_new();
rend_authorized_client_t *client =
tor_malloc_zero(sizeof(rend_authorized_client_t));
smartlist_split_string(auth_args, arg, ":", 0, 0);
if (smartlist_len(auth_args) < 1 || smartlist_len(auth_args) > 2) {
- *err_msg = tor_strdup("512 Invalid ClientAuth syntax\r\n");
+ control_write_endreply(conn, 512, "Invalid ClientAuth syntax");
goto err;
}
client->client_name = tor_strdup(smartlist_get(auth_args, 0));
@@ -2095,7 +2084,7 @@ add_onion_helper_clientauth(const char *arg, int *created, char **err_msg)
client->descriptor_cookie,
NULL, &decode_err_msg) < 0) {
tor_assert(decode_err_msg);
- tor_asprintf(err_msg, "512 %s\r\n", decode_err_msg);
+ control_write_endreply(conn, 512, decode_err_msg);
tor_free(decode_err_msg);
goto err;
}
@@ -2106,7 +2095,7 @@ add_onion_helper_clientauth(const char *arg, int *created, char **err_msg)
}
if (!rend_valid_client_name(client->client_name)) {
- *err_msg = tor_strdup("512 Invalid name in ClientAuth\r\n");
+ control_write_endreply(conn, 512, "Invalid name in ClientAuth");
goto err;
}
diff --git a/src/feature/control/control_cmd.h b/src/feature/control/control_cmd.h
index 5c3d1a1ce..4b6d54abe 100644
--- a/src/feature/control/control_cmd.h
+++ b/src/feature/control/control_cmd.h
@@ -91,10 +91,11 @@ STATIC int add_onion_helper_keyarg(const char *arg, int discard_pk,
const char **key_new_alg_out,
char **key_new_blob_out,
add_onion_secret_key_t *decoded_key,
- int *hs_version, char **err_msg_out);
+ int *hs_version,
+ control_connection_t *conn);
STATIC rend_authorized_client_t *add_onion_helper_clientauth(const char *arg,
- int *created, char **err_msg_out);
+ int *created, control_connection_t *conn);
STATIC control_cmd_args_t *control_cmd_parse_args(
const char *command,
diff --git a/src/test/test_controller.c b/src/test/test_controller.c
index ee48d656b..b9cbe0a14 100644
--- a/src/test/test_controller.c
+++ b/src/test/test_controller.c
@@ -9,6 +9,7 @@
#include "feature/control/control.h"
#include "feature/control/control_cmd.h"
#include "feature/control/control_getinfo.h"
+#include "feature/control/control_proto.h"
#include "feature/client/entrynodes.h"
#include "feature/hs/hs_common.h"
#include "feature/nodelist/networkstatus.h"
@@ -201,42 +202,58 @@ static const control_cmd_syntax_t one_arg_kwargs_syntax = {
static const parse_test_params_t parse_one_arg_kwargs_params =
TESTPARAMS( one_arg_kwargs_syntax, one_arg_kwargs_tests );
+static char *reply_str = NULL;
+/* Mock for control_write_reply that copies the string for inspection
+ * by tests */
+static void
+mock_control_write_reply(control_connection_t *conn, int code, int c,
+ const char *s)
+{
+ (void)conn;
+ (void)code;
+ (void)c;
+ tor_free(reply_str);
+ reply_str = tor_strdup(s);
+}
+
static void
test_add_onion_helper_keyarg_v3(void *arg)
{
int ret, hs_version;
add_onion_secret_key_t pk;
char *key_new_blob = NULL;
- char *err_msg = NULL;
const char *key_new_alg = NULL;
(void) arg;
+ MOCK(control_write_reply, mock_control_write_reply);
memset(&pk, 0, sizeof(pk));
/* Test explicit ED25519-V3 key generation. */
+ tor_free(reply_str);
ret = add_onion_helper_keyarg("NEW:ED25519-V3", 0, &key_new_alg,
&key_new_blob, &pk, &hs_version,
- &err_msg);
+ NULL);
tt_int_op(ret, OP_EQ, 0);
tt_int_op(hs_version, OP_EQ, HS_VERSION_THREE);
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);
+ tt_ptr_op(reply_str, OP_EQ, NULL);
tor_free(pk.v3); pk.v3 = NULL;
tor_free(key_new_blob);
/* Test discarding the private key. */
+ tor_free(reply_str);
ret = add_onion_helper_keyarg("NEW:ED25519-V3", 1, &key_new_alg,
&key_new_blob, &pk, &hs_version,
- &err_msg);
+ NULL);
tt_int_op(ret, OP_EQ, 0);
tt_int_op(hs_version, OP_EQ, HS_VERSION_THREE);
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);
+ tt_ptr_op(reply_str, OP_EQ, NULL);
tor_free(pk.v3); pk.v3 = NULL;
tor_free(key_new_blob);
@@ -256,9 +273,10 @@ test_add_onion_helper_keyarg_v3(void *arg)
tor_asprintf(&key_blob, "ED25519-V3:%s", base64_sk);
tt_assert(key_blob);
+ tor_free(reply_str);
ret = add_onion_helper_keyarg(key_blob, 1, &key_new_alg,
&key_new_blob, &pk, &hs_version,
- &err_msg);
+ NULL);
tor_free(key_blob);
tt_int_op(ret, OP_EQ, 0);
tt_int_op(hs_version, OP_EQ, HS_VERSION_THREE);
@@ -266,7 +284,7 @@ test_add_onion_helper_keyarg_v3(void *arg)
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);
+ tt_ptr_op(reply_str, OP_EQ, NULL);
tor_free(pk.v3); pk.v3 = NULL;
tor_free(key_new_blob);
}
@@ -274,7 +292,8 @@ test_add_onion_helper_keyarg_v3(void *arg)
done:
tor_free(pk.v3);
tor_free(key_new_blob);
- tor_free(err_msg);
+ tor_free(reply_str);
+ UNMOCK(control_write_reply);
}
static void
@@ -285,72 +304,73 @@ test_add_onion_helper_keyarg_v2(void *arg)
crypto_pk_t *pk1 = NULL;
const char *key_new_alg = NULL;
char *key_new_blob = NULL;
- char *err_msg = NULL;
char *encoded = NULL;
char *arg_str = NULL;
(void) arg;
+ MOCK(control_write_reply, mock_control_write_reply);
memset(&pk, 0, sizeof(pk));
/* Test explicit RSA1024 key generation. */
+ tor_free(reply_str);
ret = add_onion_helper_keyarg("NEW:RSA1024", 0, &key_new_alg, &key_new_blob,
- &pk, &hs_version, &err_msg);
+ &pk, &hs_version, NULL);
tt_int_op(ret, OP_EQ, 0);
tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO);
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);
+ tt_ptr_op(reply_str, OP_EQ, NULL);
/* Test "BEST" key generation (Assumes BEST = RSA1024). */
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, &hs_version, &err_msg);
+ &pk, &hs_version, NULL);
tt_int_op(ret, OP_EQ, 0);
tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO);
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);
+ tt_ptr_op(reply_str, OP_EQ, NULL);
/* Test discarding the private key. */
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, &hs_version, &err_msg);
+ &pk, &hs_version, NULL);
tt_int_op(ret, OP_EQ, 0);
tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO);
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_ptr_op(reply_str, OP_EQ, NULL);
/* Test generating a invalid key type. */
crypto_pk_free(pk.v2); pk.v2 = NULL;
ret = add_onion_helper_keyarg("NEW:RSA512", 0, &key_new_alg, &key_new_blob,
- &pk, &hs_version, &err_msg);
+ &pk, &hs_version, NULL);
tt_int_op(ret, OP_EQ, -1);
tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO);
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);
+ tt_assert(reply_str);
/* Test loading a RSA1024 key. */
- tor_free(err_msg);
+ tor_free(reply_str);
pk1 = pk_generate(0);
tt_int_op(0, OP_EQ, crypto_pk_base64_encode_private(pk1, &encoded));
tor_asprintf(&arg_str, "RSA1024:%s", encoded);
ret = add_onion_helper_keyarg(arg_str, 0, &key_new_alg, &key_new_blob,
- &pk, &hs_version, &err_msg);
+ &pk, &hs_version, NULL);
tt_int_op(ret, OP_EQ, 0);
tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO);
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_ptr_op(reply_str, OP_EQ, NULL);
tt_int_op(crypto_pk_cmp_keys(pk1, pk.v2), OP_EQ, 0);
/* Test loading a invalid key type. */
@@ -359,36 +379,37 @@ test_add_onion_helper_keyarg_v2(void *arg)
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, &hs_version, &err_msg);
+ &pk, &hs_version, NULL);
tt_int_op(ret, OP_EQ, -1);
tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO);
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);
+ tt_assert(reply_str);
/* Test loading a invalid key. */
tor_free(arg_str);
crypto_pk_free(pk.v2); pk.v2 = NULL;
- tor_free(err_msg);
+ tor_free(reply_str);
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, &hs_version, &err_msg);
+ &pk, &hs_version, NULL);
tt_int_op(ret, OP_EQ, -1);
tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO);
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);
+ tt_assert(reply_str);
done:
crypto_pk_free(pk1);
crypto_pk_free(pk.v2);
tor_free(key_new_blob);
- tor_free(err_msg);
+ tor_free(reply_str);
tor_free(encoded);
tor_free(arg_str);
+ UNMOCK(control_write_reply);
}
static void
@@ -542,49 +563,52 @@ static void
test_add_onion_helper_clientauth(void *arg)
{
rend_authorized_client_t *client = NULL;
- char *err_msg = NULL;
int created = 0;
(void)arg;
+ MOCK(control_write_reply, mock_control_write_reply);
/* Test "ClientName" only. */
- client = add_onion_helper_clientauth("alice", &created, &err_msg);
+ tor_free(reply_str);
+ client = add_onion_helper_clientauth("alice", &created, NULL);
tt_assert(client);
tt_assert(created);
- tt_ptr_op(err_msg, OP_EQ, NULL);
+ tt_ptr_op(reply_str, OP_EQ, NULL);
rend_authorized_client_free(client);
/* Test "ClientName:Blob" */
+ tor_free(reply_str);
client = add_onion_helper_clientauth("alice:475hGBHPlq7Mc0cRZitK/B",
- &created, &err_msg);
+ &created, NULL);
tt_assert(client);
tt_assert(!created);
- tt_ptr_op(err_msg, OP_EQ, NULL);
+ tt_ptr_op(reply_str, OP_EQ, NULL);
rend_authorized_client_free(client);
/* Test invalid client names */
+ tor_free(reply_str);
client = add_onion_helper_clientauth("no*asterisks*allowed", &created,
- &err_msg);
+ NULL);
tt_ptr_op(client, OP_EQ, NULL);
- tt_assert(err_msg);
- tor_free(err_msg);
+ tt_assert(reply_str);
/* Test invalid auth cookie */
- client = add_onion_helper_clientauth("alice:12345", &created, &err_msg);
+ tor_free(reply_str);
+ client = add_onion_helper_clientauth("alice:12345", &created, NULL);
tt_ptr_op(client, OP_EQ, NULL);
- tt_assert(err_msg);
- tor_free(err_msg);
+ tt_assert(reply_str);
/* Test invalid syntax */
+ tor_free(reply_str);
client = add_onion_helper_clientauth(":475hGBHPlq7Mc0cRZitK/B", &created,
- &err_msg);
+ NULL);
tt_ptr_op(client, OP_EQ, NULL);
- tt_assert(err_msg);
- tor_free(err_msg);
+ tt_assert(reply_str);
done:
rend_authorized_client_free(client);
- tor_free(err_msg);
+ tor_free(reply_str);
+ UNMOCK(control_write_reply);
}
/* Mocks and data/variables used for GETINFO download status tests */
