Opened 8 years ago

Last modified 3 months ago

#7193 needs_revision enhancement

Tor's sybil protection doesn't consider IPv6

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ipv6, intro, tor-dirauth, security, sybil, network-health, outreachy-ipv6, network-team-roadmap-2020Q1, 044-must
Cc: tyseom, gk Actual Points:
Parent ID: #24403 Points: 1
Reviewer: nickm Sponsor: Sponsor55-can

Description (last modified by teor)

Some bugs:

get_possible_sybil_list() doesn't consider IPv6 addresses at all.

clear_status_flags_on_sybil() doesn't clear ipv6_addr (and maybe more flags). Obsoleted by consensus method 24, because it requires the Running flag for a router to be in the consensus.

Also, maybe we could add a log_notice or log_info to mention if and which relays were found to be part of a Sybil attack.

Finally (and this is a minor bug), in get_possible_sybil_list() we assume that max_with_same_addr < max_with_same_addr_on_authority, which is true in the current tor network, but maybe it shouldn't be an inherent property of the source code. Obsoleted by #20960: max_with_same_addr_on_authority has been removed.

Child Tickets

Change History (46)

comment:1 Changed 8 years ago by ln5

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

The IPv6 equivalent of an IPv4 /32 is a /64. We could have auths
reject multiple (more than AuthDirMaxServersPerAddr) IPv6 OR ports
with matching upper 64 bits. While the general abundance of IPv6
addresses might make this a bit toothless it shouldn't hurt.

Note that clients are not using IPv6 OR ports by default yet.

Moving to 0.2.4.x-final -- 0.2.3 dir auths don't handle IPv6 OR ports.

comment:2 Changed 7 years ago by nickm

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

Since clients aren't handling IPv4 by default, and since you can't be a public server without an IPv4 address, I *think* this is not urgent for for 0.2.4.

comment:3 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.???

comment:4 Changed 5 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.8.x-final

comment:5 Changed 5 years ago by nickm

Points: small

comment:6 Changed 5 years ago by teor

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final
Severity: Normal

This could be in 0.2.9.

comment:7 Changed 5 years ago by teor

We should probably do this in 0.2.9 if the IPv6 client bootstrap in #17840 gets merged in 0.2.8.

comment:8 Changed 5 years ago by nickm

Keywords: intro added

comment:9 Changed 5 years ago by tyseom

Cc: tyseom added

comment:10 Changed 4 years ago by isabela

Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

tickets market to be removed from milestone 029

comment:11 Changed 4 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:12 Changed 4 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:13 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:14 Changed 3 years ago by dgoulet

Keywords: tor-dirauth added; tor-auth removed

Turns out that tor-auth is for directory authority so make it clearer with tor-dirauth

comment:15 Changed 3 years ago by nickm

Keywords: security sybil added

comment:16 Changed 3 years ago by teor

We need to do #24393 before we do this.

comment:17 Changed 2 years ago by teor

Parent ID: #24403

comment:18 Changed 7 months ago by teor

Description: modified (diff)

We've made some progress on this issue, via other tickets that obsolete some of the bugs listed here.

comment:19 Changed 7 months ago by gk

Keywords: network-health added

comment:20 Changed 6 months ago by teor

Points: small1
Sponsor: Sponsor55-can

There's a good description of how to implement this change in Proposal 312, section 3.5.7:
https://gitweb.torproject.org/torspec.git/tree/proposals/312-relay-auto-ipv6-addr.txt#n1111

comment:21 Changed 5 months ago by gaba

Keywords: outreachy-ipv6 added

Adding this tickets to possible things an outreachy intern could work on.

comment:22 Changed 5 months ago by maurice_pibouin

I am writing tests for my modified implementation of "compare_routerinfo_by_ip_and_bw_", and I have trouble using the MOCK_DECL and MOCK_IMPL macros. I am trying to MOCK the function "routerdigest_is_trusted_dir_type", so I put a MOCK_DECL statement in dirlist.h, and a MOCK_IMPL statement in dirlist.c . On compilation, it doesn't work : I get "redefinition of ‘router_digest_is_trusted_dir_type’". I also tried a bunch of different location for both statements.
See my branch : https://github.com/vnepveu/tor/tree/ticket7193_exp

comment:23 Changed 5 months ago by nickm

You need to read the warnings carefully:

In file included from ./src/lib/container/map.h:15,
                 from ./src/core/or/or.h:27,
                 from src/feature/client/bridges.c:16:
./src/feature/nodelist/dirlist.h:30:16: error: redundant redeclaration of ‘router_digest_is_trusted_dir_type’ [-Werror=redundant-decls]
   30 | MOCK_DECL(int, router_digest_is_trusted_dir_type,
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./src/lib/testsupport/testsupport.h:128:6: note: in definition of macro ‘MOCK_DECL’
  128 |   rv funcname arglist
      |      ^~~~~~~~
In file included from src/feature/client/bridges.c:28:
./src/feature/nodelist/dirlist.h:28:5: note: previous declaration of ‘router_digest_is_trusted_dir_type’ was here
   28 | int router_digest_is_trusted_dir_type(const char *digest,
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This is telling you that the function router_digest_is_trusted_dir_type is getting declared twice: first on line 28, and then on line 30.

If you remove the declaration on line 28, that warning will go away.

There's still another warning in dirlist.c, since you're using MOCK_DECL there: when you are implementing a function, you use MOCK_IMPL instead. There should only be one implementation of the function in dirlist.c, and it should be decorated with MOCK_IMPL.

When you actually want to declare a mock replacement for the function, you do that in the unit tests, and you give it a different name from the original function. Then in the test code, you can use MOCK and UNMOCK to replace the original function.

There's more documentation on mocking, along with examples, in src/lib/testsupport/testsupport.h,
and in doc/HACKING/WritingTests.md

Last edited 5 months ago by nickm (previous) (diff)

comment:24 Changed 5 months ago by maurice_pibouin

Status: newneeds_review

Hi ! Managed to write a refactorization of compare_router_by_ip_and_bw_, aswell as the corresponding unit tests. Before submitting a pull request with the same thing for get_possible_sybil_list, I'd very much appreciate if you could point out any flaws in my current work.
Branch : https://github.com/vnepveu/tor/tree/ticket7193

comment:25 Changed 5 months ago by nickm

Reviewer: nickm

Thanks! I'll have a look on Monday.

comment:26 Changed 5 months ago by gk

Cc: gk added

comment:27 Changed 5 months ago by nickm

Status: needs_reviewneeds_revision

Some notes on the content of the patch:

  • This like is incorrect node_t *node_running = tor_malloc(sizeof(node_t *));. That "sizeof" is taking the size of a pointer, not the size of a node_t. Because of that, you'll only allocate a few bytes, not a whole node_t.
  • It looks like the mock_node_get* functions have a memory leak -- you malloc node_running, but there is nothing that frees it. Please remember that in C, every allocated object needs to have a corresponding free. If you run configure with --enable-fragile-hardening it will turn on extra run-time checking that will catch this kind of problem for you.
  • When using strlcpy or strlcat, it's good style to use the size of the target buffer.
  • For the address comparison, could we use tor_addr_compare() ?
  • I don't understand why we're looking at the family type for the ipv6 address -- if it's an IPv6 address, then the family should always be AF_INET6. (And if the family is AF_INET, then it shouldn't be stored in a variable called ipv6_addr).
  • Most fundamentally: this function seems to be for sorting relays into a list by address and then by bandwidth. But now it is trying to sort by ipv6 address _and_ by ipv4 address at the same time. I don't think that makes sense -- a relay can have both kinds of address, but it wouldn't appear at more than one point in the list necessarily.

It probably makes sense for there to be two different variants of the function -- one that sorts by ipv6 and one that sorts by ipv4. The parts of the two functions that would be shared in common should turn into a third function that they both would call.

comment:28 Changed 5 months ago by maurice_pibouin

Thanks a lot for your comments, I will try to fix everything !

comment:29 Changed 5 months ago by gaba

Keywords: network-team-roadmap-2020Q1 added

Add all the tickets from sponsor 55 that are done and being worked on to the keyword #network-team-roadmap-2020Q1 so I can look at them in the wiki page...

comment:30 in reply to:  20 Changed 5 months ago by maurice_pibouin

Replying to teor:

There's a good description of how to implement this change in Proposal 312, section 3.5.7:
https://gitweb.torproject.org/torspec.git/tree/proposals/312-relay-auto-ipv6-addr.txt#n1111

I am trying to change the get_possible_sybil_list function.
If adding the "family" argument is easy, it seems like it would require changes where the function is called (inside dirserv_generate_networkstatus_vote_obj l4535), so that one call is made for IPv6 routers, and one is made for IPv4 routers. I was thinking of creating two routers list (one IPv4 , one IPv6) but that seems messy.
Wouldn't it be better to filter the "family" directly inside get_possible_sybil_list so as to avoid having to change too many things ?

Last edited 5 months ago by maurice_pibouin (previous) (diff)

comment:31 Changed 4 months ago by teor

I agree with Nick's answer to this question:

It probably makes sense for there to be two different variants of the function -- one that sorts by ipv6 and one that sorts by ipv4. The parts of the two functions that would be shared in common should turn into a third function that they both would call.

I don't mind so much about the interface between get_possible_sybil_list() and dirserv_generate_networkstatus_vote_obj().

It seems like a good idea to me to get separate lists of IPv4 and IPv6 sybils. You could try to merge them before returning to dirserv_generate_networkstatus_vote_obj(), but that might be hard. Or you could just loop through each list separately, and call rep_hist_make_router_pessimal() on them all.

comment:32 Changed 4 months ago by MrSquanchee

Cc: usuraj35@… added

comment:33 Changed 4 months ago by MrSquanchee

Cc: usuraj35@… removed

comment:34 Changed 4 months ago by maurice_pibouin

Status: needs_revisionneeds_review

I refactored everything according to your comments. Please tell me what you think of it and I'll write the tests (there are 5 functions to test now ...) once you are satisfied with my patch.
Branch : ​https://github.com/vnepveu/tor/tree/ticket7193

comment:35 Changed 4 months ago by teor

Milestone: Tor: unspecifiedTor: 0.4.4.x-final

Hmm, maybe we missed the review on this one because it's not in the 0.4.4 milestone.

Also, last weekend was a 3-5 day weekend for a lot of people, so we're still catching up.

comment:36 Changed 4 months ago by nickm

Hi again, and sorry about the delay! The last week has been very intense.

This new version of the code looks more workable than the last. I'd suggest these changes:

  • I'd suggest making a new patch that takes the new parts of dirserv_generate_networkstatus_vote_obj and turns it into a new function, for testing purposes.
  • Also, it seems like the code in dirserv_generate_networkstatus_vote_obj won't compare routers by IPv6 addresses if they have any IPv4 address. That seems wrong.
  • I don't think that removing the comment in that function about not using rl->routers is an improvement.
  • Our code style is not to write "if (a) b;" or "else c;" all on one line. We should always have a new line for the code in the if and else blocks.
  • I think we could have a single "omit_as_sybil" digestmap, and add both sets of routers that we want to remove to it. That would remove the need for duplication in other parts of the code. (Also, it is not correct to call dirserv_compute_performance_thresholds twice in this way: it needs to be called exactly once per vote, with the complete list of sybils to omit.)
  • You don't need to malloc last_ipv6_addr in get_possible_sybil_list; you can just put it on the stack with tor_addr_t last_ipv6_addr;.
  • To copy tor_addr_t, use tor_addr_copy.
  • For the next version of this patch, could you please make a pull request? That way, travis and appveyor will both run tests on the code to see if it's passing.

comment:37 Changed 4 months ago by nickm

Status: needs_reviewneeds_revision

comment:38 Changed 4 months ago by maurice_pibouin

Thank you for the review !

  • I'm not sure what you mean by new patch : a different PR, branch, issue, or just new commits ?
  • I didn't do a PR because I thought it was reserved to "finished" patches (ie that include tests), I will use a PR next time
  • Comment removal was a mistake
  • I didn't see anything in CodingStandards.md about the if-else newline, is it just common good practice ?

comment:39 in reply to:  38 Changed 4 months ago by nickm

Replying to maurice_pibouin:

Thank you for the review !

  • I'm not sure what you mean by new patch : a different PR, branch, issue, or just new commits ?

New commits would be fine.

  • I didn't do a PR because I thought it was reserved to "finished" patches (ie that include tests), I will use a PR next time

It's good to have a PR even if it's not ready. That lets you get CI results, and lets us comment directly on the code.

  • Comment removal was a mistake
  • I didn't see anything in CodingStandards.md about the if-else newline, is it just common good practice ?

It's what we do in the rest of the code, except in a few places where we didn't catch it during initial code review.

comment:40 Changed 3 months ago by teor

Status: needs_revisionneeds_review

I think this ticket needs review, I see a new PR here:
https://github.com/torproject/tor/pull/1871

comment:41 Changed 3 months ago by maurice_pibouin

Status: needs_reviewneeds_revision

Not ready for review yet, the PR was just to see if it passed CI, switched PR to draft.

comment:42 in reply to:  41 Changed 3 months ago by MrSquanchee

Replying to maurice_pibouin:

Not ready for review yet, the PR was just to see if it passed CI, switched PR to draft.

You can enable travis-ci in your forked repository. That way you can submit your PR after completion.
Thanks :)

comment:43 Changed 3 months ago by maurice_pibouin

Status: needs_revisionneeds_review

Finally managed to write the whole patch ! See https://github.com/torproject/tor/pull/1871
I feel like the test file is a bit too long, do you have suggestions on how to factorize it better ?

comment:44 Changed 3 months ago by nickm

Keywords: 044-must added

Add 044-must to all security tickets in 0.4.4

comment:45 Changed 3 months ago by nickm

It looks like you're getting an error in travis CI:

src/test/test_dirvote.c:38:14: error: no previous extern declaration for non-static variable 'router_properties' [-Werror,-Wmissing-variable-declarations]

digestmap_t *router_properties = NULL;

This error means that the compiler wants you either to declare router_properties as static, to make it clear that it can only be used inside of test_dirvote.c, or instead to make an extern declaration for router_properties in some header, to make it clear that other modules can use it too.

Now I'm looking over the patch...

comment:46 Changed 3 months ago by nickm

Status: needs_reviewneeds_revision

The patch looks pretty good; I've noted a couple more places it could use const. Besides that and the CI issue, I think we're ok.

Also I think the test file is probably fine as it is: they're often not too short.

Note: See TracTickets for help on using tickets.