From 536b5c8059bc3356edb8687c423c5966a2729b6d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 1 Mar 2022 11:22:34 -0500 Subject: [PATCH 1/5] connection_or_set_identity_digest(): Make errors nonfatal. Previously we were using tor_assert() to enforce the documented invariant here; this commit changes it to use BUG() instead. It will protect us from crashes if the next commit (on #40563) turns out to expose a bug somewhere. --- src/core/or/connection_or.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/core/or/connection_or.c b/src/core/or/connection_or.c index e3e81ed9cb..a6f73d328a 100644 --- a/src/core/or/connection_or.c +++ b/src/core/or/connection_or.c @@ -182,8 +182,10 @@ connection_or_set_identity_digest(or_connection_t *conn, const int ed_changed = ed_id_was_set && (!ed_id || !ed25519_pubkey_eq(ed_id, &chan->ed25519_identity)); - tor_assert(!rsa_changed || !rsa_id_was_set); - tor_assert(!ed_changed || !ed_id_was_set); + if (BUG(rsa_changed && rsa_id_was_set)) + return; + if (BUG(ed_changed && ed_id_was_set)) + return; if (!rsa_changed && !ed_changed) return; From a79046f40a515473ece5eb74aa72f82511571fe0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 1 Mar 2022 11:53:10 -0500 Subject: [PATCH 2/5] Fix logic for whether a channel's Ed25519 ID is changing The previous code would notice if we were changing from one identity to another, but not if we were changing from no identity to having an identity. This problem caused a bug (spotted by cypherpunks in ticket #40563) where if we created a channel for a circuit request that doesn't include an Ed25519 identity, we won't be able to use that channel later for requests that _do_ list Ed25519. Fix for 40563; bugfix on 0.3.0.1-alpha. --- src/core/or/connection_or.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/core/or/connection_or.c b/src/core/or/connection_or.c index a6f73d328a..069ee1d571 100644 --- a/src/core/or/connection_or.c +++ b/src/core/or/connection_or.c @@ -179,8 +179,9 @@ connection_or_set_identity_digest(or_connection_t *conn, chan && !ed25519_public_key_is_zero(&chan->ed25519_identity); const int rsa_changed = tor_memneq(conn->identity_digest, rsa_digest, DIGEST_LEN); - const int ed_changed = ed_id_was_set && - (!ed_id || !ed25519_pubkey_eq(ed_id, &chan->ed25519_identity)); + const int ed_changed = (!ed_id_was_set && ed_id) || + (ed_id_was_set && ed_id && chan && + !ed25519_pubkey_eq(ed_id, &chan->ed25519_identity)); if (BUG(rsa_changed && rsa_id_was_set)) return; From 93625da29ed603dad14305ef767c61ca6e4ad5e1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 1 Mar 2022 11:55:40 -0500 Subject: [PATCH 3/5] connection_or_set_identity_digest: more defensive programming We expect ed_id == NULL here to indicate "no ed id", but other parts of Tor sometimes use an all-0 ed_id. Here we detect that input and replace it with what's expected. --- src/core/or/connection_or.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/core/or/connection_or.c b/src/core/or/connection_or.c index 069ee1d571..b3b5c389d5 100644 --- a/src/core/or/connection_or.c +++ b/src/core/or/connection_or.c @@ -165,6 +165,9 @@ connection_or_set_identity_digest(or_connection_t *conn, if (conn->chan) chan = TLS_CHAN_TO_BASE(conn->chan); + if (BUG(ed_id && ed25519_public_key_is_zero(ed_id))) + ed_id = NULL; + log_info(LD_HANDSHAKE, "Set identity digest for %s at %p: %s %s.", connection_describe(TO_CONN(conn)), conn, From ecbab95998e2f0e5c80bcd7d67633f33e96595bd Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 2 Mar 2022 09:38:45 -0500 Subject: [PATCH 4/5] Add a changes file for 40563. --- changes/bug40563 | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changes/bug40563 diff --git a/changes/bug40563 b/changes/bug40563 new file mode 100644 index 0000000000..e7a3deec6d --- /dev/null +++ b/changes/bug40563 @@ -0,0 +1,8 @@ + o Major bugfixes (relay): + - When opening a channel because of a circuit request that did not + include an Ed25519 identity, record the Ed25519 identity that we + actually received, so that we can use the channel for other circuit + requests that _do_ list an Ed25519 identity. + (Previously we had code to record this identity, but a logic bug + caused it to be disabled.) Fixes bug 40563; bugfix on 0.3.0.1-alpha. + Patch from "cypherpunks". From 33bb1c5fcac82dad438d398155f5b45ae549e21a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 2 Mar 2022 09:38:58 -0500 Subject: [PATCH 5/5] connection_or_set_identity_digest(): handle zero ed_id better It looks like our code actually assumes (by dereferencing it in a log call) that ed_id will _not_ be NULL, but rather will be a bunch of zero bytes. Refactor the code accordingly, and stop using NULL tests on ed_id. --- src/core/or/connection_or.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/core/or/connection_or.c b/src/core/or/connection_or.c index b3b5c389d5..54fbdf7d33 100644 --- a/src/core/or/connection_or.c +++ b/src/core/or/connection_or.c @@ -165,9 +165,6 @@ connection_or_set_identity_digest(or_connection_t *conn, if (conn->chan) chan = TLS_CHAN_TO_BASE(conn->chan); - if (BUG(ed_id && ed25519_public_key_is_zero(ed_id))) - ed_id = NULL; - log_info(LD_HANDSHAKE, "Set identity digest for %s at %p: %s %s.", connection_describe(TO_CONN(conn)), conn, @@ -180,10 +177,12 @@ connection_or_set_identity_digest(or_connection_t *conn, const int rsa_id_was_set = ! tor_digest_is_zero(conn->identity_digest); const int ed_id_was_set = chan && !ed25519_public_key_is_zero(&chan->ed25519_identity); + const int new_ed_id_is_set = + (ed_id && !ed25519_public_key_is_zero(ed_id)); const int rsa_changed = tor_memneq(conn->identity_digest, rsa_digest, DIGEST_LEN); - const int ed_changed = (!ed_id_was_set && ed_id) || - (ed_id_was_set && ed_id && chan && + const int ed_changed = bool_neq(ed_id_was_set, new_ed_id_is_set) || + (ed_id_was_set && new_ed_id_is_set && chan && !ed25519_pubkey_eq(ed_id, &chan->ed25519_identity)); if (BUG(rsa_changed && rsa_id_was_set)) @@ -204,8 +203,7 @@ connection_or_set_identity_digest(or_connection_t *conn, memcpy(conn->identity_digest, rsa_digest, DIGEST_LEN); /* If we're initializing the IDs to zero, don't add a mapping yet. */ - if (tor_digest_is_zero(rsa_digest) && - (!ed_id || ed25519_public_key_is_zero(ed_id))) + if (tor_digest_is_zero(rsa_digest) && !new_ed_id_is_set) return; /* Deal with channels */