Opened 6 years ago

Closed 6 years ago

#12023 closed defect (fixed)

Should we update timestamp_last_added_nonpadding when we send a destroy cell?

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client 025-triaged
Cc: Actual Points:
Parent ID: #6799 Points:
Reviewer: Sponsor:

Description

As near as I can tell based on my work on #6799, our fix for #7912 introduces a "fun" property: DESTROY cells no longer count as "non-padding" cells for the purpose of killing an idle connection. Here's a debug log (with the #6799 fixes in place):

May 15 22:54:07.000 [debug] circuit_expire_old_circuits_clientside(): Closing ci
rcuit that has been unused for 3626990 msec.
May 15 22:54:07.000 [debug] circuit_get_by_circid_channel_impl(): circuit_get_by
_circid_channel_impl() returning circuit 0x7f0846e25ff0 for circ_id 2927002286, 
channel ID 19 (0x7f0846e65b20)
May 15 22:54:07.000 [debug] circuit_get_by_circid_channel_impl(): circuit_get_by
_circid_channel_impl() returning circuit 0x7f0846e25ff0 for circ_id 2927002286, 
channel ID 19 (0x7f0846e65b20)
May 15 22:54:07.000 [debug] circuitmux_append_destroy_cell(): Cmux at 0x7f0846e4
b2b0 queued a destroy for circ 2927002286, cmux counter is now 1, global counter
 is now 1
May 15 22:54:07.000 [debug] circuitmux_append_destroy_cell(): Primed a buffer.
May 15 22:54:07.000 [debug] channel_write_packed_cell(): Writing packed_cell_t 0
x7f0846e65d38 to channel 0x7f0846e65b20 with global ID 19
May 15 22:54:07.000 [debug] circuit_get_by_circid_channel_impl(): circuit_get_by
_circid_channel_impl() returning circuit 0x7f0846e25ff0 for circ_id 2927002286, 
channel ID 19 (0x7f0846e65b20)
May 15 22:54:07.000 [debug] circuitmux_notify_xmit_destroy(): Cmux at 0x7f0846e4b2b0 sent a destroy, cmux counter is now 0, global counter is now 0
May 15 22:54:07.000 [debug] channel_send_destroy(): Sending destroy (circID 2927002286) on channel 0x7f0846e65b20 (global ID 19)
May 15 22:54:07.000 [notice] Expiring non-used OR connection 0x7f0846e28060 to fd 5 (127.0.0.1:5004) [idle 3627, timeout 1274, canonical=1].

Note that the channel counts as having been "idle for an hour", even though the DESTROY cell was just sent immediately before.

Do we care about this? Is it actually wrong?

Child Tickets

Change History (12)

comment:1 Changed 6 years ago by cypherpunks

Yes, a DESTROY cell is obviously not a padding cell. Variables that appear to measure one thing but measure another make future bugs more likely.

comment:2 Changed 6 years ago by nickm

Sure, I agree that if we keep the current behavior, we should change the name to last_added_nonidle_cell or something. My question is whether a DESTROY cell should reset the timer for closing a connection because it's idle.

comment:3 Changed 6 years ago by cypherpunks

Yes, because otherwise what type of cell was sent is more visible to outside observers, and probably other attacks are made worse too.

comment:4 Changed 6 years ago by nickm

In that case we should probably rethink the #6799 fix, since a late destroy from the client will often kill the circuit anyway. (Since the connection will be canonical on the client's side but non-canonical on the relay's side, the relay will often be past its idle timeout when the client kills the last circuit.)

Last edited 6 years ago by nickm (previous) (diff)

comment:5 Changed 6 years ago by cypherpunks

So fix for #7912 introduces a "fun" property for client side of client-to-relay connection, but actually no change anything. Relay will close connection from client after last long-before-nothing-sent circuit was destroyed for both cases with and without #7912.
While #6799 initially was about relay-to-relay connections. But if we want to fix client-to-relay connections expire time-out then yes need to rethink #6799.

comment:6 Changed 6 years ago by cypherpunks

My question is whether a DESTROY cell should reset the timer for closing a connection because it's idle.

Yes. DESTROY cell belongs to circuit, last but yet alive.

comment:7 Changed 6 years ago by nickm

Status: newneeds_review

I think that this will be fixed by the change in the latest branch for #6799, "ticket6799_024_v2".

comment:8 Changed 6 years ago by arma

Branch looks plausible. I didn't try running it or anything though.

comment:9 Changed 6 years ago by nickm

Keywords: 025-triaged added

Calling this 0.2.5-triaged since it's included in #6799 .

comment:10 Changed 6 years ago by andrea

See code review for the ticket6799_024_v2 branch at #6799

comment:11 Changed 6 years ago by nickm

Parent ID: #6799

comment:12 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

#6799 is merged into master, so this is fixed. (This wasn't an issue with 0.2.4, so not marking this one for backport.)

Note: See TracTickets for help on using tickets.