channel: channel_tls_handle_cell() CELL_VERSIONS code reached

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;

changed milestone to %Tor: 0.2.9.x-final

Trac:
Child Ticket(s): #31136 (moved), #31221 (moved)

added 029-backport 035-backport 040-backport 041-backport 042-must BugSmashFund actualpoints::.1 component::core tor/tor consider-backport-after-0423 milestone::Tor: 0.2.9.x-final owner::nickm priority::high resolution::fixed reviewer::teor severity::normal status::closed tor-channel tor-relay type::defect version::tor 0.2.4.4-alpha labels

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 )

Trac:
Username: jk

Trac:
Keywords: tor-channel deleted, tor-channel security crash added

See also #31221 (moved) and #31136 (moved)

Trac:
Keywords: tor-channel security crash deleted, tor-channel security crash 042-must added

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

Trac:
Priority: Medium to Very High

Trac:
Owner: N/A to nickm
Status: new to accepted

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

Trac:
Keywords: tor-channel security crash 042-must deleted, tor-channel security 042-must added

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!

Trac:
Priority: Very High to High
Keywords: tor-channel security 042-must deleted, tor-channel 042-must 029-backport? 035-backport 040-backport 041-backport BugSmashFund added

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

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.

Trac:
Actualpoints: N/A to .1

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.

CI has passed

Trac:
Status: accepted to needs_review

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

Trac:
Status: needs_review to needs_revision

(practracker failure fixed)

Trac:
Status: needs_revision to needs_review

Trac:
Reviewer: N/A to teor

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

Trac:
Status: needs_review to merge_ready
Keywords: tor-channel 042-must 029-backport? 035-backport 040-backport 041-backport BugSmashFund deleted, tor-channel 042-must consider-backport-after-0433 029-backport? 035-backport 040-backport 041-backport BugSmashFund added
Version: N/A to Tor: 0.2.4.4-alpha

Trac:
Keywords: tor-channel 042-must consider-backport-after-0433 029-backport? 035-backport 040-backport 041-backport BugSmashFund deleted, tor-channel 042-must consider-backport-after-0433 029-backport? 035-backport 040-backport 041-backport BugSmashFund asn-merge added

Merged. Leaving open for backports.

Trac:
Keywords: tor-channel 042-must consider-backport-after-0433 029-backport? 035-backport 040-backport 041-backport BugSmashFund asn-merge deleted, tor-channel 042-must consider-backport-after-0433 029-backport? 035-backport 040-backport 041-backport BugSmashFund added
Milestone: Tor: 0.4.2.x-final to Tor: 0.4.1.x-final

Trac:
Keywords: tor-channel 042-must consider-backport-after-0433 029-backport? 035-backport 040-backport 041-backport BugSmashFund deleted, tor-channel 042-must consider-backport-after-0423 029-backport 035-backport 040-backport 041-backport BugSmashFund added

Backported to 0.4.1.

Trac:
Milestone: Tor: 0.4.1.x-final to Tor: 0.4.0.x-final

Merged to 0.2.9 and later. Merged #31107 (moved), #31466 (moved), #30916 (moved), #31408 (moved), #31837 (moved), and #31897 (moved) together.

Trac:
Resolution: N/A to fixed
Milestone: Tor: 0.4.0.x-final to Tor: 0.2.9.x-final
Status: merge_ready to closed

closed

added 48m of time spent

mentioned in issue #31136 (moved)

mentioned in issue #31221 (moved)

mentioned in issue #31408 (moved)

mentioned in issue #31466 (moved)

mentioned in issue #31837 (moved)

mentioned in issue #31897 (moved)

moved to tpo/core/tor#31107 (closed)