Add HiddenServiceExportRendPoint and HiddenServiceExportInstanceID directive

Trac:
Parent Ticket: #32511 (moved)

added 043-deferred component::core tor/tor extra-review milestone::Tor: unspecified needs-proposal owner::moonsikpark parent::32511 priority::medium reviewer::ahf reviewer::dgoulet reviewer::teor severity::normal sponsor::27-can status::needs-revision tor-dos tor-hs type::enhancement + 1 deleted label

PR here. https://github.com/torproject/tor/pull/1562

Trac:
Username: moonsikpark

Trac:
Keywords: N/A deleted, tor-hs tor-dos added
Status: new to needs_review
Milestone: N/A to Tor: 0.4.3.x-final
Sponsor: N/A to Sponsor27-can

Trac:
Cc: asn, dgoulet to asn
Reviewer: N/A to dgoulet

Trac:
Status: needs_review to needs_revision

Trac:
Username: moonsikpark
Status: needs_revision to needs_review

One tiny thing remains. We can remove code from your fixes :). We should be ready to merge after that.

Oh and CI is failing.

I will still ask ahf for a second review on this since he worked on the original feature.

Thanks!

Trac:
Status: needs_review to needs_revision
Reviewer: dgoulet to dgoulet, ahf
Keywords: tor-hs tor-dos deleted, tor-hs tor-dos extra-review added

OK, CI should be passing, ready for review.

Trac:
Username: moonsikpark
Status: needs_revision to needs_review

ACK for me. I'll let ahf merge_ready it with his review.

I left some comments there. I think my biggest question is what is the IPv6 story in this? Encoding a 32-bit IPv4 address in an IPv6 address is smart, but encoding an IPv6 address in an IPv6 address whilst still leaving space for other data is going to be difficult.

Trac:
Status: needs_review to needs_information

Replying to ahf:

I left some comments there. I think my biggest question is what is the IPv6 story in this? Encoding a 32-bit IPv4 address in an IPv6 address is smart, but encoding an IPv6 address in an IPv6 address whilst still leaving space for other data is going to be difficult.

The PROXY protocol has 2 address fields and 2 port fields: PROXY TCP6 (SOURCE_IPV6) (DEST_IPV6) (SOURCE_PORT) (DEST_PORT)

In the current Tor network, Tor relays must have one IPv4 address and port, and can optionally have an IPv6 address and port.

So here's the information we might want to capture:

REND_IPV4 (4 bytes)
REND_IPV4_PORT (2 bytes)
REND_IPV6 (16 bytes)
REND_IPV6_PORT (2 bytes)
INSTANCE_ID (2 bytes)
CIRCUIT_ID (4 bytes)

And here's how this patch does that: PROXY TCP6 (RESERVED_4_BYTES|REND_IPV4|REND_IPV4_PORT|INSTANCE_ID|CIRCUIT_ID) (::1) (1) (0)

If we ever need to capture the IPv6 address and port:

If we can use DEST_IPV6 and DEST_PORT:

Use DEST_IPV6 for REND_IPV6
Use DEST_PORT for REND_IPV6_PORT

If we can't, we only have 4 bytes left to store 18 bytes, so we hash the IPv6 and port, and use the first 4 bytes:

RESERVED_4_BYTES = H(REND_IPV6|REND_IPV6_PORT)[:4]

I think we can make this decision later?

But we should definitely document that IPv6 is not supported, and that the address is the canonical IPv4 address of the rend point. (And not guaranteed to be the actual address that the circuit is connecting through.)

Actually, we can't use RESERVED_4_BYTES, because it's set to "fc00". We'd have to use one of the other fields instead. (Or replace the IPv4 address with the hash.)

I also did a review of the code: we're missing some unit tests for the new options.

There's also a few things we could do to make the design and code more maintainable in future.

Trac:
Username: moonsikpark
Status: needs_information to needs_review

Trac:
Username: moonsikpark
Reviewer: dgoulet, ahf to dgoulet, ahf, teor

I found another undocumented field, and a few minor issues in the changes file and the code,

Thanks for your patience, we're almost there :-)

Trac:
Status: needs_review to needs_revision

teor, can you have a look at my tests? It’s giving me uaf errors, probably my mistake but can’t figure out.

Trac:
Username: moonsikpark

On Windows, I see:

2697bash.exe : In file included from ../src/test/test_hs_service.c:38:
2698At line:2 char:5
2699+     & $commandPath $args 2>&1
2700+     ~~~~~~~~~~~~~~~~~~~~~~~~~
2701    + CategoryInfo          : NotSpecified: (In file include...s_service.c:38::String) [], RemoteException
2702    + FullyQualifiedErrorId : NativeCommandError
2703 
2704../src/test/test_hs_service.c: In function 'test_export_client_circuit_id':
2705
2706../src/core/or/circuitbuild.h:61:32: error: 'chosen_exit' may be used uninitialized in this function [-Werror=maybe-uninitialized]
2707   61 |   FREE_AND_NULL(extend_info_t, extend_info_free_, (info))
2708      |                                ^~~~~~~~~~~~~~~~~
2709
2710../src/test/test_hs_service.c:2209:18: note: 'chosen_exit' was declared here
2711 2209 |   extend_info_t *chosen_exit = NULL;
2712      |                  ^~~~~~~~~~~
2713
2714cc1.exe: all warnings being treated as errors
2715
2716make[1]: *** [Makefile:19844: src/test/test-test_hs_service.o] Error 1
2717
2718

https://ci.appveyor.com/project/torproject/tor/builds/29309624/job/ul63snoa8hv5m334

On macOS, I see:

5505hs_service/export_client_circuit_id: [forking] =================================================================
5506==36432==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e000107338 at pc 0x000100781651 bp 0x7ffeef985f60 sp 0x7ffeef985f58
5507READ of size 8 at 0x60e000107338 thread T0
5508    #0 0x100781650 in extend_info_free_ circuitbuild.c:2798
5509    #1 0x10078c55e in circuit_free_ circuitlist.c:1159
5510    #2 0x1004fd8e5 in test_export_client_circuit_id test_hs_service.c:2288
5511    #3 0x1006f39b3 in testcase_run_one tinytest.c:107
5512    #4 0x1006f4282 in tinytest_main tinytest.c:454
5513    #5 0x1006f1ec7 in main testing_common.c:350
5514    #6 0x7fff7964e3d4 in start (libdyld.dylib:x86_64+0x163d4)
5515
55160x60e000107338 is located 120 bytes inside of 160-byte region [0x60e0001072c0,0x60e000107360)
5517freed by thread T0 here:
5518    #0 0x101d53bad in wrap_free (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x5ebad)
5519    #1 0x1004fd8bb in test_export_client_circuit_id test_hs_service.c:2286
5520    #2 0x1006f39b3 in testcase_run_one tinytest.c:107
5521    #3 0x1006f4282 in tinytest_main tinytest.c:454
5522    #4 0x1006f1ec7 in main testing_common.c:350
5523    #5 0x7fff7964e3d4 in start (libdyld.dylib:x86_64+0x163d4)
5524
5525previously allocated by thread T0 here:
5526    #0 0x101d539f3 in wrap_malloc (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x5e9f3)
5527    #1 0x100ad6bef in tor_malloc_zero_ malloc.c:45
5528    #2 0x100782e3d in extend_info_new circuitbuild.c:2690
5529    #3 0x1004fe98c in test_export_client_circuit_id test_hs_service.c:2272
5530    #4 0x1006f39b3 in testcase_run_one tinytest.c:107
5531    #5 0x1006f4282 in tinytest_main tinytest.c:454
5532    #6 0x1006f1ec7 in main testing_common.c:350
5533    #7 0x7fff7964e3d4 in start (libdyld.dylib:x86_64+0x163d4)

https://travis-ci.org/torproject/tor/jobs/620648132?utm_medium=notification&utm_source=github_status

It looks like you're freeing something twice, these logs have the line numbers. I'll have a look tomorrow when I'm at my computer.

Here's a pull request that fixes the double-free, documents the current behaviour, and fixes the changes file:

master: https://github.com/torproject/tor/pull/1585

Here are the remaining issues:

The fc00::/8 block is undefined, we should be using fd00::/8, see https://en.wikipedia.org/wiki/Unique_local_address
The byte order of global_identifier and instance_id depends on the host byte order
The source port duplicates a part of global_identifier
There are a spare 3 bytes in the IPv6 address, after fc, they are currently set to 00:0000
Adding these new features is already a breaking change, because the previously constant bytes dead:beef:4dad now vary
We're missing tests for destination port

Here's what I suggest we do, if we want to commit to a breaking change:

Use fd00::/8 and document that it is the unique local address prefix
Use network byte order
Put part of the fingerprint in the source port
Use the spare address bytes for part of the fingerprint
Document the breaking change in the man page and changes file
Test all the old and new features

If we only want to commit to a small breaking change:

Document that fc00::/8 is undefined
Use network byte order for new fields, and document host byte order for the old field
Don't change the source port
Use some spare address bytes for part of the fingerprint, but preserve fc00
Document the newly varying fields in the man page and changes file, as a breaking change
Test all the old and new features

dgoulet, ahf, what do you think we should do here?

Build still failing due to tests.

Trac:
Username: moonsikpark

OK, made CI happy by letting circuit_free_() free or_circ->build_state->chosen_exit.

Before extending the exposure range of rp fingerprint, I think there might be other metrics we can export that can help onion services. Potentially circ->rend_data->nr_streams or circ->hs_ident->intro_auth_pk or circ->hs_ident->rendezvous_cookie. If we can't export all 20 bytes, exporting part of it and exporting other things like which intro point did the client choose? or what was the rendezvous cookie? or how many streams is the client opening now? might help.

I think INSTANCE_ID don't have to be that long, we can change it to 1 byte and move it to the front(fdAA where AA=INSTANCE_ID).

And I'm curious about dst_ipv6 fixed to ::1. real_addr of rend_service_port_config_t is not always localhost. Does backends ignore the destination address? Does it mean we can cram data in dst_ipv6 too?

Trac:
Username: moonsikpark

Replying to moonsikpark:

OK, made CI happy by letting circuit_free_() free or_circ->build_state->chosen_exit.

Thanks!

Before extending the exposure range of rp fingerprint, I think there might be other metrics we can export that can help onion services. Potentially circ->rend_data->nr_streams or circ->hs_ident->intro_auth_pk

Sure that seems fine.

or circ->hs_ident->rendezvous_cookie.

No, I don't think we should export raw cryptographic material. If you really want this info, you should export siphash(circ->hs_ident->rendezvous_cookie). (The cookie should not be common across instances, unless the client is doing a replay attack across different instances. If you want to be able to detect relay attacks, you should do H(CONSTANT_PREFIX|circ->hs_ident->rendezvous_cookie).)

I think INSTANCE_ID don't have to be that long, we can change it to 1 byte and move it to the front(fdAA where AA=INSTANCE_ID).

Sure.

And I'm curious about dst_ipv6 fixed to ::1. real_addr of rend_service_port_config_t is not always localhost. Does backends ignore the destination address?

I don't know who is actually using this feature right now. Maybe ahf does?

Does it mean we can cram data in dst_ipv6 too?

That's 16 bytes, so you could get the whole fingerprint if you wanted it. But maybe there are more valuable things.

I'm going to suggest a way forward:

We decide what to do about the existing broken fields: https://trac.torproject.org/projects/tor/ticket/32604?replyto=21#comment:19
We do a design for the final set of fields.
We land this patch with the current fields, in the positions they will have in the final design.
We open a new ticket for any new fields.

This feature is getting complex enough that we might need a proposal, a spec, or a very well-designed manual page entry.

Replying to teor:

No, I don't think we should export raw cryptographic material. If you really want this info, you should export siphash(circ->hs_ident->rendezvous_cookie). (The cookie should not be common across instances, unless the client is doing a replay attack across different instances. If you want to be able to detect relay attacks, you should do H(CONSTANT_PREFIX|circ->hs_ident->rendezvous_cookie).) I agree. Should we have a new variable that stores H(CONSTANT_PREFIX|circ->hs_ident->rendezvous_cookie) in rend_data_t? I'm going to suggest a way forward:

We decide what to do about the existing broken fields: https://trac.torproject.org/projects/tor/ticket/32604?replyto=21#comment:19

We do a design for the final set of fields.

We land this patch with the current fields, in the positions they will have in the final design.

We open a new ticket for any new fields.

This feature is getting complex enough that we might need a proposal, a spec, or a very well-designed manual page entry. Edit: I agree. Let's get this over with first.

I think writing a proposal or a spec is the way to go considering the complexity. How do I write a proposal or a spec?

Trac:
Username: moonsikpark

OK, I think I did all I can do within the ticket's scope.

Trac:
Username: moonsikpark
Status: needs_revision to needs_review

Trac:
Username: moonsikpark
Owner: N/A to moonsikpark
Status: needs_review to accepted

Trac:
Username: moonsikpark
Status: accepted to needs_review

You can write a proposal using this process: https://gitweb.torproject.org/torspec.git/tree/proposals/001-process.txt#n39

Here are two good example proposals:

Specifying data formats: https://gitweb.torproject.org/torspec.git/tree/proposals/284-hsv3-control-port.txt

Adding a new option: https://gitweb.torproject.org/torspec.git/tree/proposals/309-optimistic-socks-in-tor.txt

Trac:
Status: needs_review to needs_revision
Keywords: tor-hs tor-dos extra-review deleted, tor-hs tor-dos extra-review needs-proposal added

All 0.4.3.x tickets without 043-must, 043-should, or 043-can are about to be deferred.

Trac:
Keywords: tor-hs tor-dos extra-review needs-proposal deleted, 043-deferred, tor-dos, tor-hs, needs-proposal, extra-review added

Trac:
Milestone: Tor: 0.4.3.x-final to Tor: unspecified

mentioned in issue #32511 (moved)

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

Add HiddenServiceExportRendPoint and HiddenServiceExportInstanceID directive

Child items 0

Activity