Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#4361 closed defect (fixed)

Shouldn't the v3 client process the certs cell before sending her netinfo cell?

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: spec-conformance tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The tor-spec used to say:

   As soon as it gets the CERTS cell, the initiator knows
   whether the responder is correctly authenticated.  At this point the
   initiator may send a NETINFO cell if it does not wish to
   authenticate, or a CERTS cell, an AUTHENTICATE cell (4.4), and a NETINFO
   cell if it does.

I changed it to:

   The initiator can use the CERTS cell to confirm whether
   the responder is correctly authenticated. If the initiator does not wish
   to authenticate, it can send a NETINFO cell once it has received the
   VERSIONS cell from the responder. If the initiator does wish to
   authenticate, it waits until it gets the AUTH_CHALLENGE cell, and then
   sends a CERTS cell, an AUTHENTICATE cell (4.4), and a NETINFO
   cell.

since that's what the code does.

But troll_un points out that we should probably change the code so the client checks the CERTS cell and either hangs up then, or sends her NETINFO comfortable in the knowledge that she knows who she's sending the NETINFO cell to.

If we do change the code, we'd then want to revert (and probably still clean up a bit more) the spec change.

Child Tickets

Change History (10)

comment:1 Changed 9 years ago by nickm

Hrm. That change seems fine. It doesn't actually lose us a round-trip, since we shouldn't be making any circuits as a client until we have received a CERTS cell.

comment:2 Changed 9 years ago by arma

Frosty provides the following suggested patch:

commit d1e64818d24a6b73505fd2ce5daa6bc1c229220e
Author: frosty <frosty@rootedker.nl>
Date:   Tue Nov 1 04:49:20 2011 +0100

    Conforming spec

diff --git a/src/or/command.c b/src/or/command.c
index c4cc3a9..868a743 100644
--- a/src/or/command.c
+++ b/src/or/command.c
@@ -673,8 +673,7 @@ command_process_versions_cell(var_cell_t *cell, or_connectio
     const int send_chall = !started_here && public_server_mode(get_options());
     /* If our certs cell will authenticate us, or if we have no intention of
      * authenticating, send a netinfo cell right now. */
-    const int send_netinfo =
-      !(started_here && public_server_mode(get_options()));
+    const int send_netinfo = !started_here;
     const int send_any =
       send_versions || send_certs || send_chall || send_netinfo;
     tor_assert(conn->link_proto >= 3);
@@ -1006,6 +1005,13 @@ command_process_cert_cell(var_cell_t *cell, or_connection
 
     conn->handshake_state->id_cert = id_cert;
     id_cert = NULL;
+    if (!public_server_mode(get_options())) {
+      if (connection_or_send_netinfo(conn) < 0) {
+        log_warn(LD_OR, "Couldn't send netinfo cell");
+        connection_mark_for_close(TO_CONN(conn));
+        goto err;
+      }
+    }
   } else {
     if (! (id_cert && auth_cert))
       ERR("The certs we wanted were missing");

comment:3 Changed 9 years ago by nickm

Status: newneeds_review

That looks like it ought to work, though I am a little worried about the long-term viability of calling connection_or_send_netinfo() before we update received_cert_cell -- perhaps at some point in the future that will matter.

See branch "bug4361" in my public repository for an extra commit on top of frosty's original patch: it adds some comments and moves the send_netinfo() call around a little. This one needs testing.

comment:4 Changed 9 years ago by arma

We should be aware that there's a race condition here where if the user changes from being a server or not at just the right time, it could cause either zero netinfo cells or double netinfo cells. I'm not worried, unless there are worse implications to that behavior than I realize.

comment:5 Changed 9 years ago by arma

branch looks fine to me.

we should be sure to muck with the spec in some suitable way after we merge.

What testing did you have in mind (beyond just merging it into master and finding out if somebody complains?)

comment:6 in reply to:  5 Changed 9 years ago by nickm

Replying to arma:

What testing did you have in mind (beyond just merging it into master and finding out if somebody complains?)

Running it in chutney and making sure that it interoperates with 0.2.2 , and with 0.2.3.8-alpha

comment:7 Changed 9 years ago by nickm

Keywords: spec-conformance added

comment:8 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

It seems to work for me. Merging. Changing the spec too.

comment:9 Changed 8 years ago by nickm

Keywords: tor-client added

comment:10 Changed 8 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.