hs-v3: INTRODUCE1 trunnel code doensn't handle HS_INTRO_ACK_STATUS_CANT_RELAY
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.
-
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
-
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. -
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.
-
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 (moved) branch which re-used this error code and during testing, the intro point relay exploded.
All this got into 0.3.0.1-alpha.