Opened 2 weeks ago

Last modified 3 days ago

#30454 merge_ready defect

hs-v3: INTRODUCE1 trunnel code doensn't handle HS_INTRO_ACK_STATUS_CANT_RELAY

Reported by: dgoulet Owned by: dgoulet
Priority: High Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version: Tor:
Severity: Normal Keywords: tor-hs, 034-backport, 035-backport, 040-backport, 041-must, nickm-merge, network-team-roadmap-2019-Q1Q2, 0411-alpha
Cc: Actual Points:
Parent ID: Points: 1
Reviewer: asn 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),
                                   (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. */
    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
  1. 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.
  1. 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.
  1. 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

Child Tickets

Change History (15)

comment:1 Changed 2 weeks ago by dgoulet

Keywords: 034-backport 041-must added
Status: newneeds_review

Branch: ticket30454_041_01

Backports on 034 to merge forward:

comment:2 Changed 2 weeks ago by asn

Reviewer: asn

comment:3 Changed 11 days ago by asn

Status: needs_reviewneeds_revision

Hm, the backport branch CI seems to be broken.

There is also a broken job on the master appveyor CI. Any ideas on how to restart that?

After that, we can merge_ready.

comment:4 Changed 11 days ago by dgoulet

Fixed the 034 issue... Putting in merge_ready but should only be merged when CI passes.

comment:5 Changed 11 days ago by dgoulet

Keywords: nickm-merge added
Status: needs_revisionmerge_ready

comment:6 Changed 11 days ago by nickm

Status: merge_readyneeds_revision

I left a question on the 1012 PR.

comment:7 Changed 11 days ago by gaba

Keywords: network-team-roadmap-2019-Q1Q2 added

comment:8 Changed 10 days ago by dgoulet

Status: needs_revisionneeds_review

I've pushed the requested fix which I agree. I've also pushed a new commit that unifies the onion key type ABI there to the trunnel one.

comment:9 Changed 9 days ago by nickm

Keywords: 0411-alpha added

comment:10 Changed 5 days ago by asn

Status: needs_reviewmerge_ready

Looks good to me.

comment:11 Changed 5 days ago by nickm

Squashed as ticket30454_034_01_squashed with new PR at .

Could I please have a branch that merges this to 0.3.5? It doesn't merge cleanly I am not 100% sure what to do with the conflict.

comment:12 Changed 4 days ago by teor

Owner: set to dgoulet
Status: merge_readyassigned

comment:13 Changed 4 days ago by teor

Status: assignedmerge_ready

comment:14 Changed 3 days ago by dgoulet

Status: merge_readyneeds_review

Branch: ticket30454_035_01

(I've basically merged nickm/ticket30454_034_01_squashed in an 035 branch and fixed the conflict.

comment:15 Changed 3 days ago by nickm

Milestone: Tor: 0.4.1.x-finalTor: 0.4.0.x-final
Status: needs_reviewmerge_ready

Merged into 0.4.1; marking for possible backport.

Note: See TracTickets for help on using tickets.