Opened 3 months ago

Last modified 2 weeks ago

#31107 merge_ready defect

channel: channel_tls_handle_cell() CELL_VERSIONS code reached

Reported by: dgoulet Owned by: nickm
Priority: High Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.4-alpha
Severity: Normal Keywords: tor-relay, tor-channel 042-must consider-backport-after-0423 029-backport 035-backport 040-backport 041-backport BugSmashFund
Cc: Actual Points: .1
Parent ID: Points:
Reviewer: teor Sponsor:

Description

Relay operator reported this 2 days ago:

https://lists.torproject.org/pipermail/tor-relays/2019-July/017486.html

This code has been reached in channel_tls_handle_cell():

    case CELL_VERSIONS:
      tor_fragile_assert();
      break;

Child Tickets

TicketStatusOwnerSummaryComponent
#31136closedtor_bug_occurred_() channel_tls_handle_cell: This line should not have been reached.Core Tor/Tor
#31221closedLine unexpectedly reached at channel_tls_handle_cell at ../src/core/or/channeltls.c:1111Core Tor/Tor

Change History (20)

comment:1 Changed 3 months ago by jk

Experienced same error - details below:

16:06:35 [NYX_NOTICE] BUG: Unexpected exception from ConnectionTracker: 'getpwnam(): name not found: _tor-instance1' [999 duplicates hidden]
15:53:43 [WARN] Bug: /usr/bin/tor(+0x55ec4) [0x4e2ec4] (on Tor 0.3.5.8 )
15:53:43 [WARN] Bug: /usr/bin/tor(connection_handle_read+0x960) [0x4dc564] (on Tor 0.3.5.8 )
15:53:43 [WARN] Bug: /usr/bin/tor(+0x8c554) [0x519554] (on Tor 0.3.5.8 )
15:53:43 [WARN] Bug: /usr/bin/tor(channel_tls_handle_cell+0x4a8) [0x4f1b1c] (on Tor 0.3.5.8 )
15:53:43 [WARN] Bug: /usr/bin/tor(tor_bug_occurred_+0xb4) [0x687a78] (on Tor 0.3.5.8 )
15:53:43 [WARN] Bug: /usr/bin/tor(log_backtrace_impl+0x4c) [0x68c8c4] (on Tor 0.3.5.8 )
15:53:43 [WARN] Bug: Line unexpectedly reached at channel_tls_handle_cell at ../src/core/or/channeltls.c:1111. Stack trace: (on Tor 0.3.5.8)
15:53:43 [WARN] tor_bug_occurred_(): Bug: ../src/core/or/channeltls.c:1111: channel_tls_handle_cell: This line should not have been reached. (Future instances of this warning will be silenced.) (on Tor 0.3.5.8 )

Last edited 3 months ago by jk (previous) (diff)

comment:2 Changed 6 weeks ago by nickm

Keywords: security crash added

comment:3 Changed 6 weeks ago by nickm

See also #31221 and #31136

comment:4 Changed 5 weeks ago by nickm

Keywords: 042-must added

comment:5 Changed 4 weeks ago by nickm

Priority: MediumVery High

Make all 042-must objects "Very High" priority.

comment:6 Changed 4 weeks ago by nickm

Owner: set to nickm
Status: newaccepted

comment:7 Changed 4 weeks ago by nickm

Keywords: crash removed

Okay. Fortunately, this isn't a crash: tor_fragile_assert() just logs a stack trace like this.

comment:8 Changed 4 weeks ago by nickm

Keywords: 029-backport? 035-backport 040-backport 041-backport BugSmashFund added; security removed
Priority: Very HighHigh

Okay, this is a bug, and an old one. It looks like our logic in connection_or_process_cells_from_inbuf() is wrong in the way that it handles variable-length cells.

Basically, what it is doing right now it this:

try to fetch var_cell_t from buffer.
if (we got a var_cell_t) {
   give it to the channel layer.
   return
}
see whether we have more than 512/514 bytes on the buffer
if (we do) {
   package it as a cell_t
   give it to the channel layer
   return;
}
wait for more data

See the problem? If we have a pending incomplete variable-length cell of more than 512/514 bytes, it will get mis-packaged as a regular fixed-length cell.

What fun~~

Last edited 4 weeks ago by nickm (previous) (diff)

comment:9 Changed 4 weeks ago by nickm

Ohhh, wait. It doesn't actually work that way. I need to reconsider.

comment:10 Changed 4 weeks ago by nickm

Okay, here is a better diagnosis.

The problem is that cell_command_is_var_length() looks at the link protocol version when deciding whether a cell is variable-length. That's cool, but it does mean that CELL_VERSIONS is not necessarily a variable-length cell. So if somebody sends a VERSIONS cell on a v1 connection, we'll hit this warning.

comment:11 Changed 4 weeks ago by nickm

Actual Points: .1

comment:12 Changed 4 weeks ago by nickm

The right solution here is to make this tor_fragile_assert() into a protocol warning, since it happens when somebody else is violating the protocol. It is not a security issue or a crash after all. (Hooray!)

0.2.9 fix: bug31107_029. PR at https://github.com/torproject/tor/pull/1330

The merge forward is clean; these branches are for CI.

0.3.5 fix: bug31107_035. PR at https://github.com/torproject/tor/pull/1331
0.4.0 fix: bug31107_040. PR at https://github.com/torproject/tor/pull/1332
0.4.1 fix: bug31107_041. PR at https://github.com/torproject/tor/pull/1333
0.4.2 fix: bug31107_master. PR at https://github.com/torproject/tor/pull/1334

I'll put this in needs_review once CI is passing.

comment:13 Changed 4 weeks ago by nickm

Status: acceptedneeds_review

CI has passed

comment:14 Changed 4 weeks ago by nickm

Status: needs_reviewneeds_revision

(oops. There is a practracker failure on master.)

comment:15 Changed 4 weeks ago by nickm

Status: needs_revisionneeds_review

(practracker failure fixed)

comment:16 Changed 3 weeks ago by asn

Reviewer: teor

comment:17 Changed 3 weeks ago by teor

Keywords: consider-backport-after-0433 added
Status: needs_reviewmerge_ready
Version: Tor: 0.2.4.4-alpha

Looks fine to me, and looks like a low-risk backport

comment:18 Changed 3 weeks ago by nickm

Keywords: asn-merge added

comment:19 Changed 3 weeks ago by asn

Keywords: asn-merge removed
Milestone: Tor: 0.4.2.x-finalTor: 0.4.1.x-final

Merged. Leaving open for backports.

comment:20 Changed 2 weeks ago by teor

Keywords: consider-backport-after-0423 029-backport added; consider-backport-after-0433 029-backport? removed
Note: See TracTickets for help on using tickets.