Opened 2 years ago

Closed 5 months ago

Last modified 4 months ago

#20874 closed defect (fixed)

ReachableAddresses adds an extra reject *:* on every SAVECONF

Reported by: teor Owned by: neel
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-client, torrc, saveconf, configuration, annoyance, 035-triaged-in-20180711
Cc: neel@…, atagar Actual Points:
Parent ID: Points: 0.5
Reviewer: mikeperry Sponsor:

Description

To fix this issue, we can do two things:

1) We should add the reject *:* to a copy of the list after it has been parsed in parse_reachable_addresses() using append_exit_policy_string(), rather than adding it to the option itself in options_validate().

2) We might also want to call exit_policy_remove_redundancies() on the parsed policy, so that long policies with redundancies are handled more efficiently. This is only likely to ever matter on busy hidden services.

Child Tickets

Change History (17)

comment:1 Changed 2 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:2 Changed 20 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:3 Changed 19 months ago by nickm

Keywords: torrc saveconf configuration annoyance added

comment:4 Changed 8 months ago by neel

Cc: neel@… added
Owner: set to neel
Status: newassigned

comment:5 Changed 8 months ago by neel

My GitHub branch is here: https://github.com/neelchauhan/tor/tree/b20874a

It seems to pass all CI tests except for Build 37.9 which fails on a changed apt-get package name (https://travis-ci.org/neelchauhan/tor/jobs/385463554). This explains why Travis CI marks this as "failed" (https://travis-ci.org/neelchauhan/tor/builds/385463545?utm_source=github_status&utm_medium=notification). I don't keep track of Debian, sorry (I use and develop on FreeBSD).

comment:6 Changed 8 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.5.x-final
Status: assignedneeds_review

This is a bugfix, but it's not essential for 0.3.4, and may have unexpected consequences. So I've assigned it to 0.3.5.

I would like to make sure this patch is tested with stem - I think atagar (or a nyx user?) reported this bug originally.

comment:7 Changed 7 months ago by dgoulet

Reviewer: mikeperry

comment:8 Changed 6 months ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:9 Changed 6 months ago by mikeperry

Cc: atagar added
Status: needs_reviewneeds_revision

This change does not appear to append the reject line if the user uses ReachableORAddresses?

It looks like the code in parse_reachable_addresses() has the property that ReachableAddresses can set either/both OR and DIR addresses if those options are not set, but if they are set, they take precidence over ReachableAddresses. I'm guessing we want this append logic to follow the same behavior?

Also maybe this means it should be part of parse_reachable_addresses() and not policies_parse_from_options() for that reason?

comment:10 Changed 6 months ago by neel

Status: needs_revisionneeds_review

Good catch. My updated PR is here: https://github.com/torproject/tor/pull/245

comment:11 Changed 5 months ago by mikeperry

Status: needs_reviewmerge_ready

I am sorry for the delay here.

Technically, you should not be appending the reject to the or and dir policy if the user only set one of those options and not the other.

However, I think this is OK because it is simple and extra rejects at the end can't hurt in this case (unlike the situation we had before, where they were piling up in the actual config vars). Better simple than we miss an edge case, I think.

Setting this to merge ready!

comment:12 Changed 5 months ago by nickm

Before I merge -- do we have any tests for this code?

comment:13 Changed 5 months ago by neel

There are no tests right now, but I can write them if you want it.

comment:14 Changed 5 months ago by nickm

I'd like a test to make sure that the line gets added at the appropriate time.

Also somebody should test manually to make sure that the "reject" really happens -- that is, if you list a bunch of 'accept' reachable addresses, then other addresses should not be accepted.

comment:15 Changed 5 months ago by neel

I have written tests and pushed them. Same PR as last one. Everything passes.

comment:16 Changed 5 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged!

comment:17 Changed 4 months ago by teor

This is a bugfix on commit 249b72f53e in 0.1.1.5-alpha.

Note: See TracTickets for help on using tickets.