[tor-bugs] #30454 [Core Tor/Tor]: hs-v3: INTRODUCE1 trunnel code doensn't handle HS_INTRO_ACK_STATUS_CANT_RELAY

[Tor Bug Tracker & Wiki]

#30454: hs-v3: INTRODUCE1 trunnel code doensn't handle
HS_INTRO_ACK_STATUS_CANT_RELAY
--------------------------+------------------------------------------------
     Reporter:  dgoulet   |      Owner:  (none)
         Type:  defect    |     Status:  new
     Priority:  High      |  Milestone:  Tor: 0.4.1.x-final
    Component:  Core      |    Version:  Tor: 0.3.0.1-alpha
  Tor/Tor                 |
     Severity:  Normal    |   Keywords:  tor-hs, 035-backport, 040-backport
Actual Points:            |  Parent ID:
       Points:  1         |   Reviewer:
      Sponsor:            |
  Sponsor27-must          |
--------------------------+------------------------------------------------
 I'm not sure how it happens because this code got in almost at the same
 time but the introduction ACK status code are not synchronized with what
 trunnel can parse which can lead to the intro point hard asserting() if it
 ever tries to send the status code: `HS_INTRO_ACK_STATUS_CANT_RELAY =
 0x0003`.

 See `cell_introduce1.trunnel`, it does not handle status code `0x0003`:

 {{{
   u16 status IN [0x0000, 0x0001, 0x0002];
 }}}

 Fortunately for us, it can NOT happen with the current code. The only call
 site is here:

 {{{
   /* Relay the cell to the service on its intro circuit with an INTRODUCE2
    * cell which is the same exact payload. */
   if (relay_send_command_from_edge(CONTROL_CELL_ID,
 TO_CIRCUIT(service_circ),
                                    RELAY_COMMAND_INTRODUCE2,
                                    (char *) request, request_len, NULL)) {
     log_warn(LD_PROTOCOL, "Unable to send INTRODUCE2 cell to the
 service.");
     /* Inform the client that we can't relay the cell. */
     status = HS_INTRO_ACK_STATUS_CANT_RELAY;
     goto send_ack;
   }
 }}}

 ... and turns out that `relay_send_command_from_edge()` can *NEVER* return
 anything else than 0 (we've audited all the currently supported versions,
 they all behave the same there).

 This prevents tor from asserting in `send_introduce_ack_cell()` with:

 {{{
   /* A wrong status is a very bad code flow error as this value is
 controlled
    * by the code in this file and not an external input. This means we use
 a
    * code that is not known by the trunnel ABI. */
   tor_assert(ret == 0);
 }}}

 There are a series of things to fix here.

 1. The status code should be defined within the trunnel file and ONLY that
 ABI should be used. In short, `hs_intro_ack_status_t` should disappear in
 favor of the one that needs to be defined with trunnel. Side node, same
 must be done for: `hs_intro_auth_key_type_t`

 2. Because clients won't be able to know what
 `HS_INTRO_ACK_STATUS_CANT_RELAY` is, we should remove it at once for now
 since it was/is never used.

 3. We should add a default case to the trunnel definition so if we ever
 extend this later, tor will not explode or simply fail to parse the cell.

 4. We should conduct an audit of the `tor_assert()` in the HS code and
 replace them with non fatal ones if possible.

 We got very lucky here because else it would have been an easy remote
 relay crash so (4) is very important. One lesson is also to *NEVER*
 duplicate any values defined in a trunnel file. Always use the defined ABI
 of that trunnel spec.

 End note, this was found by #15516 branch which re-used this error code
 and during testing, the intro point relay exploded.

 All this got into 0.3.0.1-alpha.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/30454>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online