Opened 2 years ago

Closed 14 months ago

#27284 closed defect (fixed)

Check IPv6 exit policies on microdescs

Reported by: teor Owned by: neel
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version: Tor: 0.2.3.1-alpha
Severity: Normal Keywords: no-backport, ipv6, 040-deferred-20190220, teor-unreached-2019-03-08, asn-merge
Cc: dgoulet, intrigeri, neel Actual Points:
Parent ID: #27248 Points:
Reviewer: nickm Sponsor:

Description

In node_exit_policy_rejects_all(), we check IPv4 and IPv6 policies on ri, but on md we only check IPv4:

  else if (node->md)
    return node->md->exit_policy == NULL ||
      short_policy_is_reject_star(node->md->exit_policy);

One way to fix this issue is to refactor the existing code to check a new policy_is_reject_star, and then populate policy_is_reject_star when the md is parsed. (Like we already do with the ri.)

Child Tickets

Change History (28)

comment:1 Changed 2 years ago by teor

Parent ID: #27236#27080

Flatten the tree

comment:2 Changed 2 years ago by teor

Keywords: ipv6 added
Milestone: Tor: 0.3.5.x-finalTor: 0.3.6.x-final

Defer to 0.3.6, because it doesn't have any clear benefit in 0.3.5 just before a freeze.

comment:3 Changed 2 years ago by teor

Parent ID: #27080

Un-parenting, it's not required for 0.3.4.

comment:4 Changed 2 years ago by teor

Keywords: 034-backport-maybe removed

comment:5 Changed 2 years ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:6 Changed 22 months ago by teor

Parent ID: #27248

#27248 would also fix this issue, and many similar issues.

comment:7 Changed 20 months ago by nickm

Keywords: 040-deferred-20190220 added
Milestone: Tor: 0.4.0.x-finalTor: unspecified

Deferring 51 tickets from 0.4.0.x-final. Tagging them with 040-deferred-20190220 for visibility. These are the tickets that did not get 040-must, 040-can, or tor-ci.

comment:8 Changed 20 months ago by teor

Keywords: teor-unreached-2019-03-08 added
Owner: teor deleted

I'd like to do these tickets, but not in the next few months.

comment:9 Changed 19 months ago by cypherpunks

if this is the case with md only, is the journey until fix to go with:

UseMicrodescriptors 0

?

comment:10 in reply to:  9 Changed 19 months ago by cypherpunks

Replying to cypherpunks:

if this is the case with md only, is the journey until fix to go with:

UseMicrodescriptors 0

?

This is a question not a fix? But the question is, it is the fix? Who knows?

comment:11 Changed 14 months ago by neel

Owner: set to neel

comment:12 Changed 14 months ago by neel

Cc: neel added

comment:14 Changed 14 months ago by neel

Status: assignedneeds_review

Setting as needs review. PR is in Comment 13.

comment:15 Changed 14 months ago by teor

Keywords: consider-backport-after-042-stable 029-backport 035-backport 040-backport 041-backport 042-backport added
Milestone: Tor: unspecifiedTor: 0.4.2.x-final
Status: needs_reviewneeds_revision
Version: Tor: 0.2.3.1-alpha

This code has bugs, and it has no unit tests.

comment:16 Changed 14 months ago by neel

The statement policy == NULL exists because a pure !policy doesn't work (I believe it's a pointer) and fails the existing tests. It may also be a bug.

I will add tests tomorrow and then set as needs review.

comment:17 Changed 14 months ago by neel

Status: needs_revisionneeds_review

Nevermind, I have added tests as they were trivial. Setting as needs review.

comment:18 in reply to:  16 Changed 14 months ago by teor

Replying to neel:

The statement policy == NULL exists because a pure !policy doesn't work (I believe it's a pointer) and fails the existing tests. It may also be a bug.

I will add tests tomorrow and then set as needs review.

policy == NULL and !policy should be equivalent?

comment:19 Changed 14 months ago by dgoulet

Reviewer: nickm

comment:20 Changed 14 months ago by nickm

Status: needs_reviewneeds_revision

I've left a few comments on the PR; I believe Teor is correct about the C.

comment:21 Changed 14 months ago by neel

Status: needs_revisionneeds_review

I made the necessary changes and pushed it as fixup commits.

comment:22 Changed 14 months ago by nickm

This looks better to me. I'd like to turn the boolean back into a bitfield, but move it to be next to the other bitfields, in order to save memory.

Teor, do you think we will want to backport this? If so I want to rebase it before merging--but I am not sure how necessary a backport is, or how difficult it would be.

comment:23 Changed 14 months ago by neel

I made the boolean back into a bitfield and moved it to the other bitfields in a fixup commit.

comment:24 Changed 14 months ago by nickm

Status: needs_reviewmerge_ready

That change LGTM. Before we merge, let's decide if it's a backport candidate.

comment:25 Changed 14 months ago by teor

I think this is "no backport".
Here's why:

Before this fix, exits that reject IPv4 but accept IPv6 are marked as "rejects all".
After this fix, exits that reject IPv4 but accept IPv6 are marked as "accepts some".

These exits are rare, so the change is not worth backporting.

comment:26 Changed 14 months ago by nickm

Keywords: asn-merge added

Thanks for the info! Your reasoning makes sense to me.

comment:27 Changed 14 months ago by teor

Keywords: no-backport added; consider-backport-after-042-stable 029-backport 035-backport 040-backport 041-backport 042-backport removed

comment:28 Changed 14 months ago by asn

Resolution: fixed
Status: merge_readyclosed

merged!

Note: See TracTickets for help on using tickets.