Scalable HSes by splitting intro/rendezvous

asn suggested creating a ticket before the proposal is committed, so here I go.

Most relevant mail from the thread (contains the proposal): https://lists.torproject.org/pipermail/tor-dev/2015-October/009625.html

changed milestone to %Tor: unspecified

added component::core tor/tor milestone::Tor: unspecified multicore owner::TvdW points::6 priority::medium scaling severity::normal sponsor::27-can status::needs-revision tor-dos tor-hs type::enhancement labels

Had some time, implemented the rest of the proposal as well to check feasibility and updated the proposal with some new insights. Looking forward to all comments on this!

Patch 1, splitting the intro and rendezvous code (basically a no-op): https://github.com/TvdW/tor/commit/0443e38d776a114458e2f56e435f324c38e7a17a

Patch 2, actually implementing the proposal: https://github.com/TvdW/tor/commit/b8d41f66efdb856b7813c4394f8a81c82e1f2e07

Simple controller implementation that proves my proposal works: https://gist.github.com/TvdW/3f720b9c6ffcd71967c1

N.B. Those patches obviously need some documentation, a changes file, and maybe some general sanity.

Github branch instead of separate commits (I will continue updating this branch): https://github.com/TvdW/tor/commits/split-hs-intro-rend

Trac:
Status: new to needs_review
Milestone: N/A to Tor: 0.2.8.x-final

Trac:
Points: N/A to medium
Keywords: N/A deleted, tor-hs added
Sponsor: N/A to SponsorR

Could we have a spec revision (based on tor-dev comments) before I review this? I'd like clarity on what the opaque blobs are supposed to contain so that I know whether they really do.

Trac:
Cc: N/A to teor

Revisions done based on comments sent to me through IRC and mail.

torspec branch : https://github.com/TvdW/torspec/commits/split-hs-intro-rend

tor branch : https://github.com/TvdW/tor/commits/split-hs-intro-rend

Sample controller : https://gist.github.com/TvdW/3f720b9c6ffcd71967c1

The tor branch does not currently contain tests. I will add these as soon as I figure out how to actually test this through unit tests.

Comments on the spec branch:

• I still think "INTRODUCE" is a pretty vague name. How about INTRODUCE_HANDOFF?
• I really don't like sending private keys around like this. Is there any way to avoid it?

Comments on the code's documentation:

• I still believe you should specify the actual contents and format of the blob. rend_service_handoff_introduce has this information, but only sort of.
• Actually, rend_service_handoff_introduce's generation and parsing code is the kind of thing I made Trunnel for. Here is a trunnel specification that would let you get rid of 90% of the code for generating and parsing these:

struct introduction_v0 {
   nulterm tor_version;
   u32 request_len;
   u8 request[request_len];
   u8 rend_pk_digest[20];
   u8 rsa_private_key[..];
 };

Though personally I would suggest something more like this instead:

struct introduction_v0 {
   u32 blob_magic;
   u16 blob_version IN [0];
   u16 request_len;
   u8 request[request_len];
   u8 rend_pk_digest[DIGEST_LEN];
   u8 rsa_privkey_len;
   u8 rsa_privkey[rsa_privkey_len];
};

Trac:
Severity: N/A to Normal

Trac:
Status: needs_review to needs_revision

(Actually, the non-trunnel binary parsing/generating code is broken. It can overflow its buffer on bad inputs, and it checks for overflow only after copying data in some bases, and it has endianness issues. I'd suggest scrapping it for the trunnel-generated version.)

Replying to nickm:

Comments on the spec branch:

• I really don't like sending private keys around like this. Is there any way to avoid it?

I discussed an alternative design with TvdW, where the introduction side decrypts the blob using the introduction key, then hands off the decrypted fields to the rendezvous side. This avoids sending the key, so if the decrypted fields are exposed, only that rendezvous is affected. The tradeoff is increased decryption load on the introduction side.

I think this change is worth it for the security improvement.

(I went and merged the proposal to the proposals repository, so it can be under version control. We should merge the next version of it too.)

Replying to teor:

Replying to nickm:

Comments on the spec branch:

• I really don't like sending private keys around like this. Is there any way to avoid it?

I discussed an alternative design with TvdW, where the introduction side decrypts the blob using the introduction key, then hands off the decrypted fields to the rendezvous side. This avoids sending the key, so if the decrypted fields are exposed, only that rendezvous is affected. The tradeoff is increased decryption load on the introduction side.

I think this change is worth it for the security improvement.

It seems to me that if this new design goes forward, the feature will be almost equivalent to what #16059 (moved) needs for the "rendezvous approver" API.

Trac:
Cc: teor to teor, special

The latest branch for this proposal leaves rend_service_relaunch_rendezvous untouched. It is called if the first rendezvous attempt fails in the middle of building the circuit to the rendezvous point.

Can you please either:

• explain why rend_service_relaunch_rendezvous works as-is with this feature, or
• modify rend_service_relaunch_rendezvous to work correctly with this feature.

Has a decision been taken here on what the final proposal should be doing? Whether the decryption happens on the introducer side or on the rendezvouer side?

Trac:
Priority: Medium to High

Please don't review this yet, a code revision is needed.

I just pushed a new branch, now with the decryption done before the handoff, and the event renamed to INTRODUCE_HANDOFF. The blob logic is now moved to Trunnel.

https://github.com/TvdW/tor/commits/split-hs-intro-rend-v2

Spec branch will follow soon. Code branch still lacks tests (Sorry!)

@teor: I had a look at rend_service_relaunch_rendezvous. Looks like no change is needed, as rend_service_perform_rendezvous sets up the internal structures correctly.