From b271f359f56b722ca9d9cafe7a43510d31d87991 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Mon, 30 Mar 2020 16:09:52 +0300 Subject: [PATCH 1/3] hs-v3: Don't allow registration of an all-zeroes client auth key. The client auth protocol allows attacker-controlled x25519 private keys being passed around, which allows an attacker to potentially trigger the all-zeroes assert for client_auth_sk in hs_descriptor.c:decrypt_descriptor_cookie(). We fixed that by making sure that an all-zeroes client auth key will not be used. There are no guidelines for validating x25519 private keys, and the assert was there as a sanity check for code flow issues (we don't want to enter that function with an unitialized key if client auth is being used). To avoid such crashes in the future, we also changed the assert to a BUG-and-err. --- changes/bug33545 | 4 ++++ src/feature/control/control_hs.c | 9 ++++++++- src/feature/hs/hs_client.h | 2 +- src/test/test_hs_control.c | 14 ++++++++++++++ 4 files changed, 27 insertions(+), 2 deletions(-) create mode 100644 changes/bug33545 diff --git a/changes/bug33545 b/changes/bug33545 new file mode 100644 index 00000000000..c051b016057 --- /dev/null +++ b/changes/bug33545 @@ -0,0 +1,4 @@ + o Minor bugfixes (hidden services): + - Block a client-side assert by disallowing the registration of an x25519 + client auth key that's all zeroes. Fixes bug 33545; bugfix on + 0.4.3.1-alpha. Patch based on patch from "cypherpunks". \ No newline at end of file diff --git a/src/feature/control/control_hs.c b/src/feature/control/control_hs.c index d3cd363f635..8deceb64120 100644 --- a/src/feature/control/control_hs.c +++ b/src/feature/control/control_hs.c @@ -50,11 +50,18 @@ parse_private_key_from_control_port(const char *client_privkey_str, if (base64_decode((char*)privkey->secret_key, sizeof(privkey->secret_key), key_blob, - strlen(key_blob)) != sizeof(privkey->secret_key)) { + strlen(key_blob)) != sizeof(privkey->secret_key)) { control_printf_endreply(conn, 512, "Failed to decode x25519 private key"); goto err; } + if (fast_mem_is_zero((const char*)&privkey->secret_key, + sizeof(privkey->secret_key))) { + control_printf_endreply(conn, 553, + "Invalid private key \"%s\"", key_blob); + goto err; + } + retval = 0; err: diff --git a/src/feature/hs/hs_client.h b/src/feature/hs/hs_client.h index 3660bfa96cc..d0a3a7015f5 100644 --- a/src/feature/hs/hs_client.h +++ b/src/feature/hs/hs_client.h @@ -45,7 +45,7 @@ typedef enum { REGISTER_SUCCESS_AND_DECRYPTED, /* We failed to register these credentials, because of a bad HS address. */ REGISTER_FAIL_BAD_ADDRESS, - /* We failed to register these credentials, because of a bad HS address. */ + /* We failed to store these credentials in a persistent file on disk. */ REGISTER_FAIL_PERMANENT_STORAGE, } hs_client_register_auth_status_t; diff --git a/src/test/test_hs_control.c b/src/test/test_hs_control.c index 9277711d2aa..8ba9f1173c7 100644 --- a/src/test/test_hs_control.c +++ b/src/test/test_hs_control.c @@ -467,6 +467,20 @@ test_hs_control_bad_onion_client_auth_add(void *arg) cp1 = buf_get_contents(TO_CONN(&conn)->outbuf, &sz); tt_str_op(cp1, OP_EQ, "512 Failed to decode x25519 private key\r\n"); + tor_free(cp1); + tor_free(args); + + /* Register with an all zero client key */ + args = tor_strdup("jt4grrjwzyz3pjkylwfau5xnjaj23vxmhskqaeyfhrfylelw4hvxcuyd " + "x25519:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="); + retval = handle_control_command(&conn, (uint32_t) strlen(args), args); + tt_int_op(retval, OP_EQ, 0); + + /* Check contents */ + cp1 = buf_get_contents(TO_CONN(&conn)->outbuf, &sz); + tt_str_op(cp1, OP_EQ, "553 Invalid private key \"AAAAAAAAAAAAAAAAAAAA" + "AAAAAAAAAAAAAAAAAAAAAAA=\"\r\n"); + client_auths = get_hs_client_auths_map(); tt_assert(!client_auths); From a97e4a1b17426923f2a312e9510336a83d95e0a1 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Mon, 30 Mar 2020 16:33:30 +0300 Subject: [PATCH 2/3] hs-v3: Change all-zeroes hard-assert to a BUG-and-err. And also disallow all-zeroes keys from the filesystem; add a test for it too. --- src/feature/hs/hs_client.c | 7 +++++++ src/feature/hs/hs_descriptor.c | 8 ++++++-- src/test/test_hs_client.c | 4 ++++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c index af8cb0b4108..da1202b642f 100644 --- a/src/feature/hs/hs_client.c +++ b/src/feature/hs/hs_client.c @@ -2132,6 +2132,13 @@ parse_auth_file_content(const char *client_key_str) "can't be decoded: %s", seckey_b32); goto err; } + + if (fast_mem_is_zero((const char*)auth->enc_seckey.secret_key, + sizeof(auth->enc_seckey.secret_key))) { + log_warn(LD_REND, "Client authorization private key can't be all-zeroes"); + goto err; + } + strncpy(auth->onion_address, onion_address, HS_SERVICE_ADDR_LEN_BASE32); /* We are reading this from the disk, so set the permanent flag anyway. */ diff --git a/src/feature/hs/hs_descriptor.c b/src/feature/hs/hs_descriptor.c index 65d6c7a581d..27823aa7961 100644 --- a/src/feature/hs/hs_descriptor.c +++ b/src/feature/hs/hs_descriptor.c @@ -1424,10 +1424,14 @@ decrypt_descriptor_cookie(const hs_descriptor_t *desc, tor_assert(!fast_mem_is_zero( (char *) &desc->superencrypted_data.auth_ephemeral_pubkey, sizeof(desc->superencrypted_data.auth_ephemeral_pubkey))); - tor_assert(!fast_mem_is_zero((char *) client_auth_sk, - sizeof(*client_auth_sk))); tor_assert(!fast_mem_is_zero((char *) desc->subcredential, DIGEST256_LEN)); + /* Catch potential code-flow cases of an unitialized private key sneaking + * into this function. */ + if (BUG(fast_mem_is_zero((char *)client_auth_sk, sizeof(*client_auth_sk)))) { + goto done; + } + /* Get the KEYS component to derive the CLIENT-ID and COOKIE-KEY. */ keystream_length = build_descriptor_cookie_keys(desc->subcredential, DIGEST256_LEN, diff --git a/src/test/test_hs_client.c b/src/test/test_hs_client.c index 5f7fe9c404e..4d938e46375 100644 --- a/src/test/test_hs_client.c +++ b/src/test/test_hs_client.c @@ -732,6 +732,10 @@ test_parse_auth_file_content(void *arg) /* Bigger key than it should be */ tt_assert(!parse_auth_file_content("xx:descriptor:x25519:" "vjqea4jbhwwc4hto7ekyvqfbeodghbaq6nxi45hz4wr3qvhqv3yqa")); + /* All-zeroes key */ + tt_assert(!parse_auth_file_content("xx:descriptor:x25519:" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")); + done: tor_free(auth); } From 7c49c7088028fc98119c972c4edd14a4b8c975a9 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Mon, 13 Apr 2020 14:08:55 +0300 Subject: [PATCH 3/3] fixup! hs-v3: Don't allow registration of an all-zeroes client auth key. --- src/feature/control/control_hs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/feature/control/control_hs.c b/src/feature/control/control_hs.c index 8deceb64120..f5b331de9ac 100644 --- a/src/feature/control/control_hs.c +++ b/src/feature/control/control_hs.c @@ -55,7 +55,7 @@ parse_private_key_from_control_port(const char *client_privkey_str, goto err; } - if (fast_mem_is_zero((const char*)&privkey->secret_key, + if (fast_mem_is_zero((const char*)privkey->secret_key, sizeof(privkey->secret_key))) { control_printf_endreply(conn, 553, "Invalid private key \"%s\"", key_blob);