Opened 2 weeks ago

Last modified 3 days ago

#32604 needs_revision enhancement

Add HiddenServiceExportRendPoint and HiddenServiceExportInstanceID directive

Reported by: moonsikpark Owned by: moonsikpark
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs tor-dos extra-review needs-proposal
Cc: asn Actual Points:
Parent ID: #32511 Points:
Reviewer: dgoulet, ahf, teor Sponsor: Sponsor27-can

Description

Add HiddenServiceExportRendPoint and HiddenServiceExportInstanceID directive to export rendezvous point information and instance ID along with circuit ID

Child Tickets

Change History (28)

comment:2 Changed 2 weeks ago by asn

Keywords: tor-hs tor-dos added
Milestone: Tor: 0.4.3.x-final
Sponsor: Sponsor27-can
Status: newneeds_review

comment:3 Changed 13 days ago by dgoulet

Cc: dgoulet removed
Reviewer: dgoulet

comment:4 Changed 8 days ago by dgoulet

Status: needs_reviewneeds_revision

comment:5 Changed 8 days ago by moonsikpark

Status: needs_revisionneeds_review

comment:6 Changed 8 days ago by dgoulet

Keywords: extra-review added
Reviewer: dgouletdgoulet, ahf
Status: needs_reviewneeds_revision

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!

Last edited 8 days ago by dgoulet (previous) (diff)

comment:7 Changed 8 days ago by moonsikpark

Status: needs_revisionneeds_review

OK, CI should be passing, ready for review.

comment:8 Changed 8 days ago by dgoulet

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

comment:9 Changed 8 days ago by ahf

Status: needs_reviewneeds_information

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.

comment:10 in reply to:  9 Changed 8 days ago by teor

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.)

Last edited 8 days ago by teor (previous) (diff)

comment:11 Changed 8 days ago by teor

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.)

comment:12 Changed 8 days ago by teor

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.

comment:13 Changed 7 days ago by moonsikpark

Status: needs_informationneeds_review

comment:14 Changed 7 days ago by moonsikpark

Reviewer: dgoulet, ahfdgoulet, ahf, teor

comment:15 Changed 7 days ago by teor

Status: needs_reviewneeds_revision

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 :-)

comment:16 Changed 7 days ago by moonsikpark

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

comment:17 Changed 7 days ago by teor

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

comment:18 Changed 7 days ago by teor

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.

comment:19 Changed 7 days ago by teor

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

Here are the remaining issues:

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

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

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

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

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

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

comment:20 Changed 7 days ago by moonsikpark

Build still failing due to tests.

comment:21 Changed 6 days ago by 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?

Last edited 6 days ago by moonsikpark (previous) (diff)

comment:22 in reply to:  21 ; Changed 6 days ago by teor

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:

  1. We decide what to do about the existing broken fields: https://trac.torproject.org/projects/tor/ticket/32604?replyto=21#comment:19
  2. We do a design for the final set of fields.
  3. We land this patch with the current fields, in the positions they will have in the final design.
  4. 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.

comment:23 in reply to:  22 Changed 6 days ago by moonsikpark

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:

  1. We decide what to do about the existing broken fields: https://trac.torproject.org/projects/tor/ticket/32604?replyto=21#comment:19
  2. We do a design for the final set of fields.
  3. We land this patch with the current fields, in the positions they will have in the final design.
  4. 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?

Last edited 5 days ago by moonsikpark (previous) (diff)

comment:24 Changed 5 days ago by moonsikpark

Status: needs_revisionneeds_review

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

comment:25 Changed 5 days ago by moonsikpark

Owner: set to moonsikpark
Status: needs_reviewaccepted

comment:26 Changed 5 days ago by moonsikpark

Status: acceptedneeds_review

comment:28 Changed 3 days ago by teor

Keywords: needs-proposal added
Status: needs_reviewneeds_revision
Note: See TracTickets for help on using tickets.