Conflux circuits ignore exit family and same subnet restrictions when choosing guards

Summary

First of all, apologies if this was intentional. I know that vanguard circuits also ignore families. I'm not sure if this is supposed to be the case for conflux circuits as well.

I was looking into #40935 and reading up on the circuit construction and node family code for that, and I noticed that I was able to build circuits through a guard and an exit in the same family. Looking closer, the restriction for conflux circuits uses an exclude list that only checks guards against items in that list. Exits are added to the list, but the exit family is not checked.

I wrote a failing unit test to demonstrate what I think should happen: 0001-Add-unit-test-for-selecting-guards-with-exit-restric.patch

[PATCH] Add unit test for selecting guards with exit restrictions

From 375591b9b1a83752591b51843bb35d19b4ee6282 Mon Sep 17 00:00:00 2001
From: Cecylia Bocovich <cohosh@torproject.org>
Date: Tue, 24 Sep 2024 16:32:41 -0400
Subject: [PATCH] Add unit test for selecting guards with exit restrictions

This test checks to see whether guard selection for circuits honor exit
family restrictions.
---
 src/test/test_entrynodes.c | 70 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c
index 118b66dfa7..6106bf335d 100644
--- a/src/test/test_entrynodes.c
+++ b/src/test/test_entrynodes.c
@@ -24,6 +24,7 @@
 #include "feature/dirclient/dirclient.h"
 #include "feature/client/entrynodes.h"
 #include "feature/nodelist/nodelist.h"
+#include "feature/nodelist/nodefamily.h"
 #include "feature/nodelist/networkstatus.h"
 #include "core/or/policies.h"
 #include "feature/nodelist/routerlist.h"
@@ -2322,6 +2323,74 @@ test_entry_guard_select_for_circuit_highlevel_primary_retry(void *arg)
   circuit_guard_state_free(guard2);
 }
 
+static void
+test_entry_guard_select_for_circuit_exit_family_restriction(void *arg)
+{
+  (void) arg;
+  entry_guard_restriction_t *rst = NULL;
+  guard_selection_t *gs = guard_selection_new("default", GS_TYPE_NORMAL);
+  int retval;
+  unsigned state = 9999;
+
+  /* Create our circuit */
+  circuit_t *circ = dummy_origin_circuit_new(30);
+  origin_circuit_t *oc = TO_ORIGIN_CIRCUIT(circ);
+  oc->build_state = tor_malloc_zero(sizeof(cpath_build_state_t));
+
+  /* First pick the exit and pin it on the build_state */
+  retval = onion_pick_cpath_exit(oc, NULL, 0);
+  tt_int_op(retval, OP_EQ, 0);
+
+  /* Then pick a guard */
+  entry_guards_update_primary(gs);
+  entry_guard_t *g = select_entry_guard_for_circuit(gs, GUARD_USAGE_TRAFFIC,
+                                                    NULL, &state);
+
+  tt_assert(g);
+  tt_assert(g->is_primary);
+  tt_uint_op(state, OP_EQ, GUARD_CIRC_STATE_USABLE_ON_COMPLETION);
+  tt_i64_op(g->last_tried_to_connect, OP_EQ, approx_time());
+
+  /* Add the guard and the exit to each others' families */
+  get_options_mutable()->EnforceDistinctSubnets = 0;
+  const char* exit_id =
+      (const char*)build_state_get_exit_rsa_id(oc->build_state);
+  const node_t *exit = node_get_by_id(exit_id);
+  const node_t *guard = node_get_by_id(g->identity);
+  exit->md->family = nodefamily_parse((const char*)g->nickname,
+          node_get_rsa_id_digest(exit),0);
+  guard->md->family =
+      nodefamily_parse((const char*)exit->rs->nickname,
+              node_get_rsa_id_digest(guard),0);
+  tt_assert(nodefamily_contains_nickname(exit->md->family,
+              (const char*)g->nickname));
+
+  /* We should get a different guard, after adding the exit restriction */
+  rst = guard_create_exit_restriction((const uint8_t*)exit_id);
+  entry_guard_t *g2 =
+      select_entry_guard_for_circuit(gs, GUARD_USAGE_TRAFFIC, rst, &state);
+  tt_assert(g2);
+  tt_assert(g2->is_primary);
+  tt_uint_op(state, OP_EQ, GUARD_CIRC_STATE_USABLE_ON_COMPLETION);
+  tt_i64_op(g2->last_tried_to_connect, OP_EQ, approx_time());
+  tt_ptr_op(g2, OP_NE, g);
+
+  /* Now check that conflux circuits satisfy the exit family restriction */
+  rst = guard_create_conflux_restriction(oc);
+  smartlist_add(rst->excluded, tor_memdup(exit_id, DIGEST_LEN));
+  g2 = select_entry_guard_for_circuit(gs, GUARD_USAGE_TRAFFIC, rst, &state);
+  tt_assert(g2);
+  tt_assert(g2->is_primary);
+  tt_uint_op(state, OP_EQ, GUARD_CIRC_STATE_USABLE_ON_COMPLETION);
+  tt_i64_op(g2->last_tried_to_connect, OP_EQ, approx_time());
+  tt_ptr_op(g2, OP_NE, g);
+
+ done:
+  circuit_free_(circ);
+  guard_selection_free(gs);
+  entry_guard_restriction_free(rst);
+}
+
 static void
 test_entry_guard_select_and_cancel(void *arg)
 {
@@ -3199,6 +3268,7 @@ struct testcase_t entrynodes_tests[] = {
   BFN_TEST(select_for_circuit_highlevel_primary),
   BFN_TEST(select_for_circuit_highlevel_confirm_other),
   BFN_TEST(select_for_circuit_highlevel_primary_retry),
+  BFN_TEST(select_for_circuit_exit_family_restriction),
   BFN_TEST(select_and_cancel),
   BFN_TEST(drop_guards),
   BFN_TEST(outdated_dirserver_exclusion),
-- 
2.45.2

If I'm not mistaken, this will also not check to see whether guards and exits are in the same subnet because that only happens in the guard_in_node_family function.

Possible fixes

It should be possible to modify guard_create_conflux_restriction to use exclude_id to set the exit node ID or something similar and use that to check against the exit family with guard_obeys_exit_restriction called for RST_EXCL_LIST restrictions as well.