Opened 4 months ago

Closed 5 weeks ago

#30454 closed defect (fixed)

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.3.5.x-final
Component: Core Tor/Tor Version: Tor: 0.3.0.1-alpha
Severity: Normal Keywords: tor-hs, 035-backport, 040-backport, nickm-merge, network-team-roadmap-2019-Q1Q2, 0411-alpha
Cc: Actual Points:
Parent ID: Points: 1
Reviewer: asn Sponsor: Sponsor27-must

Description

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
  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 0.3.0.1-alpha.

Child Tickets

Change History (25)

comment:1 Changed 4 months ago by dgoulet

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

PR: https://github.com/torproject/tor/pull/1011
Branch: ticket30454_041_01

Backports on 034 to merge forward: https://github.com/torproject/tor/pull/1012

comment:2 Changed 4 months ago by asn

Reviewer: asn

comment:3 Changed 4 months 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 4 months ago by dgoulet

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

comment:5 Changed 4 months ago by dgoulet

Keywords: nickm-merge added
Status: needs_revisionmerge_ready

comment:6 Changed 4 months ago by nickm

Status: merge_readyneeds_revision

I left a question on the 1012 PR.

comment:7 Changed 4 months ago by gaba

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

comment:8 Changed 4 months 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 4 months ago by nickm

Keywords: 0411-alpha added

comment:10 Changed 4 months ago by asn

Status: needs_reviewmerge_ready

Looks good to me.

comment:11 Changed 4 months ago by nickm

Squashed as ticket30454_034_01_squashed with new PR at https://github.com/torproject/tor/pull/1036 .

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 months ago by teor

Owner: set to dgoulet
Status: merge_readyassigned

comment:13 Changed 4 months ago by teor

Status: assignedmerge_ready

comment:14 Changed 4 months ago by dgoulet

Status: merge_readyneeds_review

Branch: ticket30454_035_01
PR: https://github.com/torproject/tor/pull/1039

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

comment:15 Changed 4 months 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.

comment:16 Changed 4 months ago by nickm

Keywords: 041-must removed

Since this is merged in 041, it is no longer 041-must.

comment:17 Changed 4 months ago by nickm

Milestone: Tor: 0.4.0.x-finalTor: 0.3.5.x-final

Backported to 0.4.0, marking for 0.3.5

comment:18 Changed 3 months ago by nickm

Keywords: 034-backport removed

Removing 034-backport from all open tickets: 034 has reached EOL.

comment:19 Changed 3 months ago by gaba

Owner: dgoulet deleted
Status: merge_readyassigned

dgoulet will assign himself to the ones he is working on right now.

comment:20 in reply to:  19 Changed 3 months ago by teor

Replying to gaba:

dgoulet will assign himself to the ones he is working on right now.

Gaba, this ticket was in merge_ready, but it changed to assigned when you changed the owner.
Please don't change the owner on tickets that are in needs_review, merge_ready, or closed.
And please be careful of needs_revision tickets.

Can you please fix all the tickets that were in needs_review, needs_revision, merge_ready, and closed?

comment:21 Changed 3 months ago by teor

Status: assignedmerge_ready

comment:22 Changed 3 months ago by gaba

Owner: set to dgoulet
Status: merge_readyassigned

Right. This ticket should not change. Thanks for pointing it out.

comment:23 Changed 3 months ago by gaba

Status: assignedmerge_ready

comment:24 Changed 6 weeks ago by teor

This PR was merged into 0.4.0, so it can be merged now.

comment:25 Changed 5 weeks ago by teor

Resolution: fixed
Status: merge_readyclosed

Backported to 0.3.5.
Merged with the other 0.3.5 and 0.4.0 backports on 2019-08-12.

Note: See TracTickets for help on using tickets.