Opened 14 months ago

Closed 6 weeks 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 14 months ago by teor

Parent ID: #27236#27080

Flatten the tree

comment:2 Changed 14 months 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 14 months ago by teor

Parent ID: #27080

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

comment:4 Changed 12 months ago by teor

Keywords: 034-backport-maybe removed

comment:5 Changed 11 months 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 10 months ago by teor

Parent ID: #27248

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

comment:7 Changed 8 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 7 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 6 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 6 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 7 weeks ago by neel

Owner: set to neel

comment:12 Changed 7 weeks ago by neel

Cc: neel added

comment:14 Changed 7 weeks ago by neel

Status: assignedneeds_review

Setting as needs review. PR is in Comment 13.

comment:15 Changed 7 weeks 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 7 weeks 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 7 weeks 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 7 weeks 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 7 weeks ago by dgoulet

Reviewer: nickm

comment:20 Changed 7 weeks 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 7 weeks ago by neel

Status: needs_revisionneeds_review

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

comment:22 Changed 7 weeks 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 7 weeks 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 7 weeks 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 7 weeks 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 7 weeks ago by nickm

Keywords: asn-merge added

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

comment:27 Changed 7 weeks 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 6 weeks ago by asn

Resolution: fixed
Status: merge_readyclosed

merged!

Note: See TracTickets for help on using tickets.