Opened 10 years ago

Closed 8 years ago

Last modified 6 years ago

#988 closed enhancement (fixed)

Different TLS certs for incoming vs outgoing

Reported by: arma Owned by:
Priority: High Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version: 0.2.0.34
Severity: Keywords:
Cc: arma, nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by nickm)

We should learn to present different TLS certs for incoming connections
vs outgoing connections.

The motivating example is bridges. They want to show the same identity
to people who connect, yet behave like clients when they connect to other
relays (e.g. change keys when they change IP addresses).

(Of course, this change would provide a new way to test for bridges: if a
Tor connects to you, connect back and see if the cert is different. But at
least that's an active test that requires the bridge to connect to you
first. But then, the attack I describe above only kicks in when the bridge
connects to you. Hm.)

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Change History (33)

comment:1 Changed 10 years ago by nickm

This shouldn't be hard, but it probably wants a proposal.

comment:2 Changed 8 years ago by rransom

See bug988 in git://repo.or.cz/tor/rransom.git .

That branch is built on my bug1994-v3 branch (see #1994), so that I could see correct documentation for a few functions before I mutilated them.

comment:3 Changed 8 years ago by rransom

See bug988-v2 ( git://repo.or.cz/tor/rransom.git bug988-v2 ) for another try.

comment:4 Changed 8 years ago by rransom

See bug988-v3 ( git://repo.or.cz/tor/rransom.git bug988-v3 ).

comment:5 Changed 8 years ago by nickm

Description: modified (diff)
Status: newneeds_review

comment:6 Changed 8 years ago by rransom

See bug988-v4 ( git://repo.or.cz/tor/rransom.git bug988-v4 ) (rebased on current bug1994-v3; no other changes).

comment:7 Changed 8 years ago by rransom

I'm reworking my fixes at the moment; bug988-v4 considered broken.

comment:8 Changed 8 years ago by rransom

See bug988-v5 ( git://repo.or.cz/tor/rransom.git bug988-v5 ) for a (hopefully working) set of fixes.

comment:9 in reply to:  8 Changed 8 years ago by rransom

See bug988-v6 ( git://repo.or.cz/tor/rransom.git bug988-v6 ) for a (hopefully working) set of fixes. This time, for sure!

comment:10 Changed 8 years ago by nickm

This looks pretty good, modulo a couple of places I'd like to add more documentation. How have you tested this? What more tests are needed?

comment:11 Changed 8 years ago by nickm

One thing in particular we need to think about is whether anything here breaks if the Tor process turns on or off server mode while it's running.

comment:12 in reply to:  10 Changed 8 years ago by rransom

Replying to nickm:

This looks pretty good, modulo a couple of places I'd like to add more documentation. How have you tested this? What more tests are needed?

I've been using it as my regular Tor client. I toggled BridgeRelay on without stopping Tor, then off, then on again several days later, and had no unexpected issues. I have since restarted Tor with BridgeRelay on, and it has been running (and receiving and handling bridge users' connections properly) without trouble since then.

I have not yet tested:

  1. running this Tor in public server mode
  2. toggling to and from public server mode without stopping Tor
  3. whether the certs Tor sends on incoming and outgoing connections do, in fact, have different public keys in bridge mode

comment:13 Changed 8 years ago by rransom

Turning BridgeRelay off segfaulted it.

comment:14 Changed 8 years ago by nickm

Got time to debug that, or want some help? In the latter case, please ask on #tor-dev; I bet somebody will be interested. :)

comment:15 Changed 8 years ago by Sebastian

I can't reproduce any crash. As far as review, I think public_server_mode() is an unfortunate function name. To me it immediately implied that we'd be dealing with the PublishServerDescriptor options, instead of a simple bridge check. I generally wonder if it is necessary to make this distinction at all, because I can't see how it would hurt to maintain separate client and server certs for public relays. Maybe I'm missing something here?

The functions want better documentation wrt accepted parameters and return values, I think. Also a changes file is missing.

comment:16 in reply to:  15 Changed 8 years ago by rransom

Replying to Sebastian:

As far as review, I think public_server_mode() is an unfortunate function name.

If anyone can suggest a less bad term for a non-bridge relay than ‘public server’, I will happily rename public_server_mode. I couldn't come up with one.

To me it immediately implied that we'd be dealing with the PublishServerDescriptor options, instead of a simple bridge check.

That test is done in functions whose names contain publishable_server. ‘Publishable server’ is probably a bad term for that, as the documentation states that a Tor controller could publish descriptors for a Tor server with PublishServerDescriptor off, thus making the Tor server publishable.

I generally wonder if it is necessary to make this distinction at all, because I can't see how it would hurt to maintain separate client and server certs for public relays. Maybe I'm missing something here?

See Tor spec §5.3.1. If an OR X opens a TLS connection to an OR Y, and a client asks Y to extend a circuit to X, that circuit should generally go over the existing TLS connection between X and Y, even though X was the client (and used its client certificates) for that connection.

The functions want better documentation wrt accepted parameters and return values, I think.

Yes. Also, the tor_tls_context_t reference manipulation should be refactored out into utility functions, because I can no longer tell at a glance whether tor_tls_context_t references are referenced and freed properly.

comment:17 in reply to:  13 Changed 8 years ago by rransom

Replying to rransom:

Turning BridgeRelay off segfaulted it.

I reproduced the crash, and nickm diagnosed it as an unrelated bug.

comment:18 Changed 8 years ago by nickm

The observed crash is now bug2097. With my bug2097 fix, does switching options on and off in a running Tor now work for you?

comment:19 in reply to:  18 Changed 8 years ago by rransom

Replying to nickm:

The observed crash is now bug2097. With my bug2097 fix, does switching options on and off in a running Tor now work for you?

Toggling BridgeRelay didn't crash Tor most of the times that I tried it, and Sebastian tested it multiple times and couldn't reproduce the crash. bug988-v6 doesn't seem to introduce any crashes.

comment:20 Changed 8 years ago by nickm

Milestone: post 0.2.1.xTor: 0.2.1.x-final

Merged. We should also maybe consider a backport to 0.2.1.x, so instead of closing this ticket, I'm reassigning it to 0.2.1.x-final.

comment:21 Changed 8 years ago by arma

Apparently this one is missing a changes file, even though it's already merged into the upcoming 0.2.2.18? Is there a changes file somewhere, or should somebody write one?

comment:22 Changed 8 years ago by nickm

See email; subject line starting with [attn arma]

comment:23 Changed 8 years ago by nickm

Priority: minormajor

comment:24 Changed 8 years ago by nickm

Backported as bug988_021. Needs careful review.

comment:25 Changed 8 years ago by Sebastian

Not sure if we care, the comment in my commit message for the refcounting fix about fixing up a2bb0bf is bogus because this was cherry-picked.

Otherwise the backport seems OK to me.

comment:26 in reply to:  24 Changed 8 years ago by rransom

Replying to nickm:

Backported as bug988_021. Needs careful review.

It looks fine to me. I also grepped for any references to get_identity_key and get_client_identity_key that escaped being replaced with get_tlsclient_identity_key or get_server_identity_key, and didn't find any.

comment:27 Changed 8 years ago by arma

I'm nervous about backporting this one to 0.2.1, since it touches a lot of things and it was tough to get right. Leaving 0.2.1 as-is (and working on making it obsolete) is probably the more robust plan.

comment:28 Changed 8 years ago by Sebastian

So, how do we proceed on this now? nick?

comment:29 Changed 8 years ago by nickm

We should decide whether to backport it, or no. I personally think it might be a better idea spending our effort trying to get 0.2.2.x stable. This is enough like a new feature that leaving it out of 0.2.1.x is fine by me.

rransom (or anybody) -- want to argue for a backport?

comment:30 Changed 8 years ago by nickm

Milestone: Tor: 0.2.1.x-finalTor: 0.2.2.x-final
Resolution: Nonefixed
Status: needs_reviewclosed

Without objections, calling this done.

comment:31 Changed 7 years ago by rransom

For the record:

  • This change did break things on 0.2.2.x and later. In particular, it seems to have introduced #2433, which introduced #2572, although I can't quite see how that happened now.
  • This change was backported to 0.2.1.x anyway in late October 2011, as a prerequisite for the fix for a major anonymity issue.

comment:32 in reply to:  31 Changed 7 years ago by rransom

Replying to rransom:

  • This change did break things on 0.2.2.x and later. In particular, it seems to have introduced #2433, which introduced #2572, although I can't quite see how that happened now.

Er, it introduced #2433, which seems to have introduced #2572.

comment:33 Changed 6 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.