Opened 7 years ago

Closed 6 years ago

#9386 closed enhancement (fixed)

Prop 221: Use CREATE_FAST less, or not at all

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client, prop221, 024-backport, arma-backport-02422
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Supposing something goes wrong with TLS, we'll wish we had never implemented CREATE_FAST.

Supposing that we're talking to a node with ntor support but without ECDHE support in TLS, we'll wish we weren't using CREATE_FAST.

I propose that in 0.2.5, we should stop using CREATE_FAST with nodes that support the ntor handshake.

Child Tickets

Change History (24)

comment:1 Changed 7 years ago by nickm

Keywords: prop221 added; needs-proposal removed

comment:2 Changed 7 years ago by nickm

Status: newneeds_review
Summary: Use CREATE_FAST less, or not at allProp 221: Use CREATE_FAST less, or not at all

See branch "prop221" in my public repository. See also branch "prop221" in my public torspec repository.

comment:3 Changed 7 years ago by arma

"conesensus"

comment:4 Changed 7 years ago by arma

The change in eed326f0 still makes us lose a feature we were using: if your network has gone away, but the kernel hasn't told your tcp connection to fail, then we'd notice when we timed out waiting for a response to our create_fast cell. Now we no longer do.

This is one of those weird edge cases that e.g. made Nathan extra happy with Tor's network state detection on Android.

I wonder if we want to keep the "mark it down" behavior when we're closing the circuit not due to a destroy cell.

comment:5 Changed 7 years ago by nickm

Okay, I've updated the branch. Any better now?

And, with your preferred fix, can a guard now be useless but still get kept indefinitely by just answering every CREATE with DESTROY?

comment:6 Changed 7 years ago by arma

Looks better yes. I wonder if there are other edge cases.

As for the attack you describe, yes. I think that used to be possible too by answering with a created-fast and then just dropping any extend or begindir cells?

I guess another option here is what you were heading toward originally: we dump the guard if it ever can't process one of our create requests. That seems a poor plan though, especially in light of the current circuit create load?

comment:7 in reply to:  6 Changed 7 years ago by nickm

Replying to arma:

Looks better yes. I wonder if there are other edge cases.

There likely are. Can you think of any likely way to find them better than "read the code hard" or "merge and see what happens"?

As for the attack you describe, yes. I think that used to be possible too by answering with a created-fast and then just dropping any extend or begindir cells?

That's also true.

I guess another option here is what you were heading toward originally: we dump the guard if it ever can't process one of our create requests. That seems a poor plan though, especially in light of the current circuit create load?

Probably so.

Shall we merge, or think more?

comment:8 Changed 7 years ago by arma

Go ahead and merge, I say. And since it's a consensus param, you can merge into 0.2.4 too if you want. (If we later discover a bug, we can throw away this consensus param and try again with another one.)

comment:9 Changed 7 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.4.x-final

I'm a tiny bit nervous putting this into 0.2.4 right away. If 0.2.4 client uptake goes faster than server uptake, it might make more load than we want. (Also, a lot of my work to make ntor go faster is targeted for 0.2.5, which will have uptake even more slowly.)

Do you think this matters? If so, let's hold off on an 0.2.4 merge.

In either case, I've merged the branch (now called "prop221_squashed_024" into master). I'm leaving this ticket in 0.2.4 for possible merge to the maint-0.2.4 branch.

comment:10 Changed 6 years ago by arma

Step one, I've set usecreatefast=0 in moria1's consensus params, and asked other authority operators to set it too.

If things don't explode, we should backport it in time for e.g. 0.2.4.22.

comment:11 Changed 6 years ago by nickm

Recommendation: backport, especially since 0.2.3 and 0.2.4 TLS ciphersuites exist.

comment:12 Changed 6 years ago by nickm

Keywords: nickm-backport-02422 added

Adding a tag for tickets I think we should backport into 0.2.4.22. Omitting ones where I said "unsure"

comment:13 Changed 6 years ago by arma

Ok.

comment:14 Changed 6 years ago by nickm

Keywords: arma-backport-02422 added

comment:15 Changed 6 years ago by nickm

Looking at the prop221 branch: I think this means that we should backport fec12af65366e0789bfdd4affef0b7ff6ca1f921 and ad71d42f4b10b6f06355f44f5c82d6c7859a08da ("Implement proposal 221: Stop sending CREATE_FAST" and a fixup commit.) but not eed326f074634d00cc8455d3b9d854550f8a2955 and 8911ee01b5679ccfa76a5353859d21daf964e780 ("circuit_build_failed: distinguish "first hop chan failed", "CREATE failed"" and a squash commit.)

Roger, do you agree?

comment:16 Changed 6 years ago by nickm

Or hm. From note above, perhaps we should backport the whole thing; I'd forgotten the impact of the second part of this.

comment:17 Changed 6 years ago by nickm

I've backported the whole cleaned-up patch series as prop221_024

comment:18 Changed 6 years ago by nickm

On examination, I think "yes, backport both" is probably right.

comment:19 Changed 6 years ago by nickm

Current status: I think that maybe this is a bit too big to backport for 0.2.4.22. Merging the TLS ciphersuite fixes should improve forward secrecy for 0.2.4.22 users in a less invasive way.

comment:20 Changed 6 years ago by nickm

Keywords: nickm-backport-02422 removed

comment:21 Changed 6 years ago by nickm

I think aiming this one for 0.2.4.23 would not be crazy, if we think 0.2.4 clients will exist for a long time.

comment:22 Changed 6 years ago by arma

What is the remaining proposed patch for 0.2.4.x? It looks like some stuff got merged for 0.2.4.22 and some didn't?

comment:23 Changed 6 years ago by nickm

The remaining proposed patch AFAIK is "prop221_024" in my public repository.

comment:24 Changed 6 years ago by arma

Resolution: fixed
Status: needs_reviewclosed

Merged. Hope this was a good idea.

Note: See TracTickets for help on using tickets.