Opened 4 years ago

Closed 4 years ago

#17183 closed enhancement (implemented)

Add exit-policy/reject-private so stem can discover ExitPolicyRejectPrivate rules

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: stem, 028-triaged, TorCoreTeam201511
Cc: atagar Actual Points:
Parent ID: Points: small
Reviewer: Sponsor:

Description

Add controller getinfo exit-policy/reject-private for the reject rules added by ExitPolicyRejectPrivate. This makes it easier for stem to display exit policies. Add unit tests for getinfo exit-policy/*.

Child Tickets

TicketTypeStatusOwnerSummary
#17611enhancementclosedControl spec patch for exit-policy/reject-private/*

Attachments (2)

0001-Fix-memory-leak-in-policies-test.patch (736 bytes) - added by cypherpunks 4 years ago.
0001-Fix-a-memory-leak-in-the-exit-policy-parsing-code.patch (1.2 KB) - added by cypherpunks 4 years ago.

Download all attachments as: .zip

Change History (39)

comment:1 Changed 4 years ago by teor

Status: newneeds_review

See my branch getinfo-private-exitpolicy at https://github.com/teor2345/tor.git

comment:2 Changed 4 years ago by nickm

Keywords: 028-triaged added

comment:3 Changed 4 years ago by nickm

Points: small

comment:4 Changed 4 years ago by nickm

Status: needs_reviewneeds_revision

Quick review:

  • Looks mostly good; glad to see tests.
  • we never use strcat in Tor: too many linkers and security tools shout about it. Instead I'd just suggest you use smartlist_add_asprintf() in the inner loop. It's waaay less error-prone.
  • There needs to be a corresponding control-spec.txt patch.

comment:5 Changed 4 years ago by teor

I wonder if we should add the addresses from #17027 while doing this. Possibly, they belong under another getinfo: exit-policy/reject-relay ????

comment:6 Changed 4 years ago by teor

Keywords: TorCoreTeam201512 added
Severity: Normal

comment:7 Changed 4 years ago by teor

I think a good design is:

  • exit-policy/reject-private - all ExitPolicyRejectPrivate addresses
  • exit-policy/reject-private/networks - ExitPolicyRejectPrivate private network addresses excluded by default
  • exit-policy/reject-private/mine - ExitPolicyRejectPrivate private network addresses excluded because they are:
    • configured as addresses on the relay's tor instance
    • configured on the local network interfaces on the relay's OS

comment:8 Changed 4 years ago by teor

Status: needs_revisionneeds_review

I think I've done what atagar wants for stem now.

See my branch getinfo-private-exitpolicy-v2 in https://github.com/teor2345/tor.git

I've updated the code based on feedback, added new GETINFO questions, and updated the unit tests.

I've added:

  • exit-policy/reject-private/default - private network addresses rejected by default (compiled-in)
  • exit-policy/reject-private/relay - private network addresses rejected because they are:
    • published as addresses in the relay's descriptor
    • configured as outbound connection bind addresses
    • configured as ports (for example, ORPort, DirPort)
    • configured on the local network interfaces on the relay's OS

Which replace:

  • exit-policy/reject-private

The existing items are:

  • exit-policy/ipv4 - IPv4 exit policy
  • exit-policy/ipv6 - IPv4 exit policy
  • exit-policy/full - IPv4 and IPv6 exit policies
  • exit-policy/default - default exit policy (compiled-in)

I can easily split exit-policy/reject-private/default and exit-policy/reject-private/relay into ipv4, ipv6, and full if needed. I could also combine them into exit-policy/reject-private ipv4, ipv6, and full, which would be a little more difficult. (This depends on exactly what atagar wants for stem.)

I've also tested these over stem's tor-prompt. (They block a temporary relay's autodiscovered IP address, which is exactly what I want to happen.)

I'll do the control-spec changes tomorrow.

comment:9 Changed 4 years ago by atagar

Sounds great, thanks teor!

comment:10 Changed 4 years ago by teor

The control-spec changes are in #17611.

I've created a new branch getinfo-private-exitpolicy-v3 which cherry-picks the latest fix to #17027, and then passes a list of configured addresses to policies_parse_exit_policy_reject_private.

comment:11 Changed 4 years ago by nickm

re 236d74869ff68fd88553ce170f4784b3a9d761a7 : it looks like this logic is duplicated with the code that actually builds the list of addresses rejected in the exit policies. Should it be unified?

comment:12 in reply to:  11 Changed 4 years ago by teor

Replying to nickm:

re 236d74869ff68fd88553ce170f4784b3a9d761a7 : it looks like this logic is duplicated with the code that actually builds the list of addresses rejected in the exit policies. Should it be unified?

I thought they were different enough that I should leave them alone.
But I think they could at least benefit from a utility "check this address before adding it" function. I'll look into this tomorrow.

comment:13 in reply to:  11 Changed 4 years ago by teor

Replying to nickm:

re 236d74869ff68fd88553ce170f4784b3a9d761a7 : it looks like this logic is duplicated with the code that actually builds the list of addresses rejected in the exit policies. Should it be unified?

Please see my branch getinfo-private-exitpolicy-v4.

I squashed the branch (including the squash in bug17027-reject-private-027-v6), and left a standard fixup implementing these changes as the final commit. (If you've reviewed the v3 branch, you only need review the final commit.)

It creates 3 utility functions for adding addresses from a tor_addr_t*, uint32 ipv4h, and or_options (outbound bind addresses). Naturally, these functions call each other as appropriate.

policies_parse_exit_policy_from_options and getinfo_helper_policies then call these functions as appropriate to add addresses to the smartlist.

comment:14 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

great; squashing and merging!

comment:15 Changed 4 years ago by teor

Resolution: implemented
Status: closedreopened

This branch needed two fixes. I have added fixups to getinfo-private-exitpolicy-v4 for these issues, which also affect #17027.

policies_parse_exit_policy_reject_private Unit Tests

The unit tests for policies_parse_exit_policy_reject_private assumed too much about the interface addresses on the local system. This meant that tests failed on some machines.

I modified the unit tests to assume less, and used a mocked version of get_interface_address6_list to provide a range of addresses for testing.

getinfo exit-policy/reject-private/relay Implementation & Unit Tests

The implementation was ignoring ExitPolicyRejectPrivate and had a memory leak. The unit tests didn't set ExitPolicyRejectPrivate, and were missing a static on a mock function declaration.

I fixed these issues.

comment:16 Changed 4 years ago by teor

Status: reopenedneeds_review

comment:17 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

I cherry-picked one of these to master as a09e7cd31a16244793c0848321c25e9cf6d8046f. The other had already been applied? Please let me know if I missed a commit though.

comment:18 Changed 4 years ago by teor

Resolution: fixed
Status: closedreopened

I made two fixups to the same commit - should I have based the second fixup on the first fixup instead?

The second fixup was 755f22d9d7b1e59e9259b635e51697aeade43bfd, it wasn't merged to master.

comment:19 Changed 4 years ago by teor

Status: reopenedneeds_review

comment:20 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

ah, okay. merged that one then too. Thanks!

(In the future, there's no need to make "fixup!" commits on already-merged commits, since we can't go back and edit the already-merged ones.)

comment:21 Changed 4 years ago by cypherpunks

Resolution: fixed
Status: closedreopened

The getinfo_helper_policies test fails on my machine with the error

policy/getinfo_helper_policies: *** Error in `./src/test/test': munmap_chunk(): invalid pointer ***
Aborted

comment:22 Changed 4 years ago by teor

Are you running the latest Tor master? (This branch was only merged 10 hours ago.)
Can you help us identify the line of code or memory allocation causing this issue?

comment:23 Changed 4 years ago by cypherpunks

I am running the latest Tor master (commit e5754c42d124549b3fd8e8d7c11d4dde3b5acec1)
The backtrace i got from GDB is

#0  __kernel_vsyscall ()
#1  __GI_raise (sig=sig@entry=6) at raise.c:56
#2  __GI_abort () at abort.c:89
#3  __libc_message (do_abort=do_abort@entry=1, fmt=fmt@entry="*** Error in `%s': %s: 0x%s ***\n") at libc_fatal.c:175
#4  malloc_printerr (action=<optimized out>, str="munmap_chunk(): invalid pointer", ptr=) at malloc.c:4996
#5  munmap_chunk (p=<optimized out>) at malloc.c:2816
#6  addr_policy_free (p=) at policies.c:2225
#7  addr_policy_list_free (lst=) at policies.c:2204
#8  getinfo_helper_policies (conn=, question="exit-policy/reject-private/relay", answer=, errmsg=) at policies.c:2171
#9  test_policies_getinfo_helper_policies (arg=) at test_policy.c:1001
#10 testcase_run_bare_ (testcase=<policy_tests+40>) at tinytest.c:105
#11 testcase_run_one (group=<testgroups+256>, testcase=<policy_tests+40>) at tinytest.c:252
#12 tinytest_main (c=2, v=, groups=<testgroups>) at tinytest.c:434
#13 main (c=2, v=) at testing_common.c:293

comment:24 Changed 4 years ago by teor

Keywords: TorCoreTeam201511 added; TorCoreTeam201512 removed
Status: reopenedneeds_review

Thanks, I used addr_policy_list_free where I should have used smartlist_free.

Please see my commit 53aae488e5dbdad6548671735d8b270e08613841 on getinfo-private-exitpolicy-v4 on https://github.com/teor2345/tor.git

comment:25 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged that!

comment:26 Changed 4 years ago by cypherpunks

That fixed it.

Changed 4 years ago by cypherpunks

comment:27 Changed 4 years ago by cypherpunks

Resolution: fixed
Status: closedreopened

Sorry to reopen this ticket again, but i found two other issues after running it through Valgrind.

The first issue is a memory leak in the getinfo_helper_policies test. The attached patch solves this.

The second issue are uninitialized value warnings such as;

Conditional jump or move depends on uninitialised value(s)
   tor_addr_is_null (address.c:869)
   tor_addr_is_public_for_reject (policies.c:910)
   addr_policy_append_reject_addr_filter (policies.c:928)
   addr_policy_append_reject_addr_list_filter (policies.c:964)
   policies_parse_exit_policy_reject_private (policies.c:1081)
   getinfo_helper_policies (policies.c:2163)
   test_policies_getinfo_helper_policies (test_policy.c:1001)
   testcase_run_bare_ (tinytest.c:105)
   testcase_run_one (tinytest.c:252)
   tinytest_main (tinytest.c:434)
   main (testing_common.c:293)

Conditional jump or move depends on uninitialised value(s)
   tor_addr_is_internal_ (address.c:364)
   tor_addr_is_public_for_reject (policies.c:910)
   addr_policy_append_reject_addr_filter (policies.c:928)
   addr_policy_append_reject_addr_list_filter (policies.c:964)
   policies_parse_exit_policy_reject_private (policies.c:1081)
   getinfo_helper_policies (policies.c:2163)
   test_policies_getinfo_helper_policies (test_policy.c:1001)
   testcase_run_bare_ (tinytest.c:105)
   testcase_run_one (tinytest.c:252)
   tinytest_main (tinytest.c:434)
   main (testing_common.c:293)

Research into these warnings lead me to the policies_add_ipv4h_to_smartlist function. The function allocates the ipv4_tor_addr on the stack and adds a pointer to it to addr_list. This means that memory held by ipv4_tor_addr is freed once the function returns and the pointer is then pointing to uninitialized memory. I do not have a patch for this issue because i am unsure how to allocate the ipv4_tor_addr variable on the heap without creating a memory leak.

comment:28 Changed 4 years ago by teor

Status: reopenedneeds_review

Thank you for catching both of these issues.

My branch fix-policies-memory contains fixes for both issues mentioned in the previous comment:

  • copy the addresses before adding them to the smartlist, then have the caller free them when the smartlist is freed (and update the function names and comments to make this explicit)
  • fix a memory leak in the getinfo_helper_policies test: the attached patch applied using 'git am'

I am sorry that this patch has needed so many fixes - I introduced the use-after-stack-return issue when I refactored the functions in getinfo-private-exitpolicy-v4 in comment 13.

comment:29 Changed 4 years ago by teor

I need to cherry-pick half of the first commit to #17027.
So that they'll apply cleanly if we backport #17027, I've split the first commit in two.
(#17610 is not affected.)

Please review and merge fix-policies-memory-v2 (it has the same changes, just split into 3 commits).

comment:30 Changed 4 years ago by cypherpunks

The commits in fix-policies-memory-v2 fixes all of the issues i found. However, i have two comments on the code;

  1. the malloc call on policies.c:1282 should be tor_malloc
  2. the cast to a pointer to void on policies.c:1300 is unnecessary.

comment:31 in reply to:  30 Changed 4 years ago by teor

Replying to cypherpunks:

The commits in fix-policies-memory-v2 fixes all of the issues i found. However, i have two comments on the code;

  1. the malloc call on policies.c:1282 should be tor_malloc
  2. the cast to a pointer to void on policies.c:1300 is unnecessary.

Thanks for catching these, please see the fixup I just added to fix-policies-memory-v2.

comment:32 Changed 4 years ago by cypherpunks

LGTM, i have no further comments.

comment:33 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

LGTM too; merged!

comment:34 Changed 4 years ago by cypherpunks

Resolution: fixed
Status: closedreopened

Valgrind found another memory leak. The attached patch fixes it.

comment:35 Changed 4 years ago by cypherpunks

Status: reopenedneeds_review

comment:36 Changed 4 years ago by teor

Thanks for the patch, cypherpunks! (And the extensive testing using valgrind.)

Please see my branch fix-exitpolicy-leak, which includes the attached patch, and also a commit which initialises configured_addresses to NULL.

Let's get this merged.

comment:37 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

seems fine; merged!

Note: See TracTickets for help on using tickets.