From 09468cc58b52132af1232e2cd3925c273382bba6 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 24 Oct 2019 11:08:25 -0400 Subject: [PATCH 1/3] dir: Look if circuit is closed in connection_dir_is_anonymous() Before inspecting the p_chan, we must check if the circuit is marked for close because if it is the case, the channels are nullified from the circuit. Several valid cases can mark the circuit for close of the directory connection. Fixes #31958 Signed-off-by: David Goulet --- changes/ticket31958 | 5 +++++ src/feature/dircommon/directory.c | 12 +++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 changes/ticket31958 diff --git a/changes/ticket31958 b/changes/ticket31958 new file mode 100644 index 0000000000..8206064dfe --- /dev/null +++ b/changes/ticket31958 @@ -0,0 +1,5 @@ + o Minor bugfixes (directory): + - When checking if a directory connection is anonymous, test if the circuit + was marked for close before looking at its channel. This avoids a BUG() + stacktrace in case it was previously closed. Fixes bug 31958; bugfix on + 0.4.2.1-alpha. diff --git a/src/feature/dircommon/directory.c b/src/feature/dircommon/directory.c index b3db0aa108..1ac35dd8b5 100644 --- a/src/feature/dircommon/directory.c +++ b/src/feature/dircommon/directory.c @@ -225,7 +225,17 @@ connection_dir_is_anonymous(const dir_connection_t *dir_conn) return false; } - /* Get the previous channel to learn if it is a client or relay link. */ + /* It is possible that the circuit was closed because one of the channel was + * closed or a DESTROY cell was received. Either way, this connection can + * not continue so return that it is not anonymous since we can not know for + * sure if it is. */ + if (circ->marked_for_close) { + return false; + } + + /* Get the previous channel to learn if it is a client or relay link. We + * BUG() because if the circuit is not mark for close, we ought to have a + * p_chan else we have a code flow issue. */ if (BUG(CONST_TO_OR_CIRCUIT(circ)->p_chan == NULL)) { log_info(LD_DIR, "Rejected HSDir request: no p_chan"); return false; From 985717675cf2c613ca88bae694695c5ee7db05d4 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 24 Oct 2019 11:23:31 -0400 Subject: [PATCH 2/3] dir: Remove connection_dir_is_anonymous() logging First, remove the HSDir mention which should not be in that generic function. Second, move them to debug() level since they are possible error case. Part of #31958 Signed-off-by: David Goulet --- src/feature/dircommon/directory.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/feature/dircommon/directory.c b/src/feature/dircommon/directory.c index 1ac35dd8b5..8e5b413326 100644 --- a/src/feature/dircommon/directory.c +++ b/src/feature/dircommon/directory.c @@ -212,7 +212,8 @@ connection_dir_is_anonymous(const dir_connection_t *dir_conn) * be closed or marked for close. */ if (linked_conn == NULL || linked_conn->magic != EDGE_CONNECTION_MAGIC || conn->linked_conn_is_closed || conn->linked_conn->marked_for_close) { - log_info(LD_DIR, "Rejected HSDir request: not linked to edge"); + log_debug(LD_DIR, "Directory connection is not anonymous: " + "not linked to edge"); return false; } @@ -221,7 +222,8 @@ connection_dir_is_anonymous(const dir_connection_t *dir_conn) /* Can't be a circuit we initiated and without a circuit, no channel. */ if (circ == NULL || CIRCUIT_IS_ORIGIN(circ)) { - log_info(LD_DIR, "Rejected HSDir request: not on OR circuit"); + log_debug(LD_DIR, "Directory connection is not anonymous: " + "not on OR circuit"); return false; } @@ -230,6 +232,8 @@ connection_dir_is_anonymous(const dir_connection_t *dir_conn) * not continue so return that it is not anonymous since we can not know for * sure if it is. */ if (circ->marked_for_close) { + log_debug(LD_DIR, "Directory connection is not anonymous: " + "circuit marked for close"); return false; } @@ -237,7 +241,8 @@ connection_dir_is_anonymous(const dir_connection_t *dir_conn) * BUG() because if the circuit is not mark for close, we ought to have a * p_chan else we have a code flow issue. */ if (BUG(CONST_TO_OR_CIRCUIT(circ)->p_chan == NULL)) { - log_info(LD_DIR, "Rejected HSDir request: no p_chan"); + log_debug(LD_DIR, "Directory connection is not anonymous: " + "no p_chan on circuit"); return false; } From 3867ca4925a2e99b7bcc4526ccf4cfe602bef3bf Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 24 Oct 2019 11:25:05 -0400 Subject: [PATCH 3/3] dir: Return 503 code when rejecting single hop request Single hop rejection (POST and GET) for HS v3 descriptor now return a 503 code which is more accurate code from dir-spec.txt and from other rejection case in the code. For instance if you are not a relay and you get a POST request, a 503 code is sent back with a rejection message. Part of #31958 Signed-off-by: David Goulet --- src/feature/dircache/dircache.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/feature/dircache/dircache.c b/src/feature/dircache/dircache.c index 7c6af3582b..d4d0ad9939 100644 --- a/src/feature/dircache/dircache.c +++ b/src/feature/dircache/dircache.c @@ -1393,7 +1393,8 @@ handle_get_hs_descriptor_v3(dir_connection_t *conn, /* Reject non anonymous dir connections (which also tests if encrypted). We * do not allow single hop clients to query an HSDir. */ if (!connection_dir_is_anonymous(conn)) { - write_short_http_response(conn, 404, "Not found"); + write_short_http_response(conn, 503, + "Rejecting single hop HS v3 descriptor request"); goto done; } @@ -1636,7 +1637,12 @@ directory_handle_command_post,(dir_connection_t *conn, const char *headers, /* Handle HS descriptor publish request. We force an anonymous connection * (which also tests for encrypted). We do not allow single-hop client to * post a descriptor onto an HSDir. */ - if (connection_dir_is_anonymous(conn) && !strcmpstart(url, "/tor/hs/")) { + if (!strcmpstart(url, "/tor/hs/")) { + if (!connection_dir_is_anonymous(conn)) { + write_short_http_response(conn, 503, + "Rejecting single hop HS descriptor post"); + goto done; + } const char *msg = "HS descriptor stored successfully."; /* We most probably have a publish request for an HS descriptor. */