Stop advertising an IPv6 exit policy when DNS is broken for IPv6

When dns_seems_to_be_broken_for_ipv6(), exits should stop advertising an IPv6 exit policy.

Here's a rough design:

• when dns_seems_to_be_broken_for_ipv6() is first set to 1, mark the relay descriptor dirty
• when rebuilding the descriptor, check dns_seems_to_be_broken_for_ipv6() before including an IPv6 exit policy
• reset dns_seems_to_be_broken_for_ipv6() periodically, maybe every 1-3 days?

changed milestone to %Tor: 0.4.4.x-final

Trac:
Parent Ticket: #24833 (moved)

added 044-must component::core tor/tor extra-review ipv6 milestone::Tor: 0.4.4.x-final needs-proposal no-backport owner::neel parent::24833 priority::medium security-review-dos-risk severity::normal status::needs-information tor-dns tor-exit type::defect version::tor 0.2.9.14 labels

Trac:
Owner: N/A to neel
Status: new to assigned

PR: https://github.com/torproject/tor/pull/1771

There is no test, but I didn't find tests for the existing "is DNS broken" code either.

Trac:
Status: assigned to needs_review

We have a policy that new code needs tests, even if old code doesn't have them.

Trac:
Status: needs_review to needs_revision

Historically, exit code has been risky, and had bugs that we didn't find until much later,

So I'm marking this ticket for extra review, and ai don't think we should backport.

Trac:
Keywords: N/A deleted, no-backport, extra-review added
Milestone: Tor: unspecified to Tor: 0.4.4.x-final
Reviewer: N/A to teor

(Sorry for multiple comments, I kept thinking about this ticket today.)

Can you describe the design of this new feature?

The code doesn't match the rough design in the ticket description. That's ok, but without a design, I can't tell the difference between a bug and a feature. In particular, I wonder why we need separate "was_disabled" and "is_disabled" variables.

This IPv6 DNS code is currently unused, so it has never been tested. So I want to make sure we have the design right.

Here are some issues I noticed when reading the code:

• the code only counts DNS errors on timeout, but there are actually 11 different DNS errors. We should consider which errors we want to track, and which ones we want to ignore. See http://www.wangafu.net/~nickm/libevent-2.1/doxygen/html/dns_8h.html
• the minimum number of queries before failure is 10. But that could happen by chance, on server startup. Let's make the minimum something more reasonable. We can make it at least 1000. But maybe we should set it to 1 when TestingTorNetwork is set. That way, broken IPv6 exits will fail quickly in chutney.

We should find out which DNS errors can be triggered by tor clients, and ignore them. Otherwise, a client that floods an exit with bad DNS queries could disable IPv6 exiting on that relay. I think Nick might be able to help here.

I think it's ok to fail thousands of client circuits, before an IPv6 exit disables IPv6. Because getting the new descriptor to clients can take an hour or two. There's also a tradeoff here: we want quiet exits to disable IPv6 eventually. But we want busy exits to survive a momentary glitch.

Trac:
Keywords: tor-client deleted, security-review-dos-risk added
Cc: cypherpunks to cypherpunks, nickm

The design is as follows:

• When dns_is_broken_for_ipv6 is set to 1, we launch a timer for 24 hours to reset this to 0

• While dns_is_broken_for_ipv6 = 1, the relay will not advertise an exit policy.

• After 24 hours, the timer will reset to 0

While my PR is not ready for review yet (need tests), I have increased the minimum number of queries to 1000, and on TestingTorNetwork decreased it to 1.

I added unit tests. However, this still only counts DNS errors on timeout.

Setting as needs review, feel free to put as needs revision if more DNS errors are necessary.

Trac:
Status: needs_revision to needs_review

Trac:
Status: needs_review to needs_revision

There is one problem with unit tests: The code this adds relies on code that calls the relay module, and some compilations don't include this, so that's why the tests fail.

Trac:
Status: needs_revision to assigned
Owner: neel to N/A

Trac:
Status: assigned to needs_revision

The relay module should always be built when running with the unit tests, though. Are you sure that's the issue?

Trac:
Owner: N/A to neel
Status: needs_revision to assigned

Trac:
Status: assigned to needs_revision
Cc: cypherpunks, nickm to cypherpunks, nickm, neel

Trac:
Status: needs_revision to needs_review

I don't have time to keep on reviewing this patch right now. I'm really busy with google summer of code and outreachy. So I'm going to pass it to another reviewer.

Here are some things for the reviewer to check:

Replying to teor:

This IPv6 DNS code is currently unused, so it has never been tested. So I want to make sure we have the design right.

Here are some issues I noticed when reading the code:

• the code only counts DNS errors on timeout, but there are actually 11 different DNS errors. We should consider which errors we want to track, and which ones we want to ignore. See http://www.wangafu.net/~nickm/libevent-2.1/doxygen/html/dns_8h.html

Which errors should we turn off IPv6 DNS for? All of them? Only the ones that clients can't trigger?

• the minimum number of queries before failure is 10. But that could happen by chance, on server startup. Let's make the minimum something more reasonable. We can make it at least 1000. But maybe we should set it to 1 when TestingTorNetwork is set. That way, broken IPv6 exits will fail quickly in chutney.

The last version of the PR I reviewed changed the wrong "10". Please check that the new PR changes this code: https://github.com/torproject/tor/pull/1771/files#diff-ed2a85a7ec36e73dc681fe94a7dcf524L1556

We should find out which DNS errors can be triggered by tor clients, and ignore them. Otherwise, a client that floods an exit with bad DNS queries could disable IPv6 exiting on that relay. I think Nick might be able to help here.

We also need to think about the risk of DNS-based attacks.

I think it's ok to fail thousands of client circuits, before an IPv6 exit disables IPv6. Because getting the new descriptor to clients can take an hour or two. There's also a tradeoff here: we want quiet exits to disable IPv6 eventually. But we want busy exits to survive a momentary glitch.

Overall, I wonder if this patch is the best way to solve this issue. Perhaps we should manually apply the BadExit flag through the network health team. Perhaps we should set the limits much, much higher.

Do we know how many queries a busy exit processes? And how many timeouts they have? It's really hard to make a good design without good data.

Trac:
Reviewer: teor to N/A

Trac:
Status: needs_review to needs_revision