Opened 3 years ago

Closed 3 years ago

#14314 closed defect (fixed)

ExitPolicy.can_exit_to() returns incorrect results when address is omitted

Reported by: nskinkel Owned by: atagar
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Note: this is with Stem version 1.3.0

When using ExitPolicy.can_exit_to(port=443, strict=False), an incorrect result is returned. Ex:

>>> policy = ExitPolicy('reject 1.0.0.0/8:*', 'accept *:*')
>>> policy.can_exit_to(port=443, strict=False)
>>> False

If an address is included, however, the results are as expected:

>>> policy = ExitPolicy('reject 1.0.0.0/8:*', 'accept *:*')
>>> policy.can_exit_to(address='8.8.8.8', port=443, strict=False)
>>> True

According to Stem's documentation for can_exit_to() for the 'strict' parameter:

strict (bool) -- if the address or port is excluded then check if we can exit to all instances of the defined address or port

I interpret this to mean:

If 'strict' is True, then can_exit_to() should return True iff exits to *all* possible IPs are allowed on the specified port when 'address' is omitted. If 'strict' is False, then can_exit_to() should return True if *any* exits to the specified port are allowed when 'address' is omitted.

Is this just a misunderstanding of the documentation on my part? Or is this actually a bug?

Attached: a script to reproduce the issue.

Child Tickets

Attachments (1)

exit_policy_test.py (688 bytes) - added by nskinkel 3 years ago.
Small script to reproduce issue.

Download all attachments as: .zip

Change History (5)

Changed 3 years ago by nskinkel

Attachment: exit_policy_test.py added

Small script to reproduce issue.

comment:1 Changed 3 years ago by atagar

Hi Nik, something definitely seems inverted here...

>>> policy = ExitPolicy('reject 1.0.0.0/8:*', 'accept *:*')
>>> policy.can_exit_to(port=443, strict=False)
False
>>> policy.can_exit_to(port=443, strict=True)
True

Thanks for the catch! I'm surprised this has remained uncaught for so long (especially since this module has particularly good test coverage). Oh well.

I'm presently finishing up a feature branch but I should be able to get this sorted out this weekend or soon after.

comment:2 Changed 3 years ago by atagar

Oh wait! Think I know what's going on. Presently strict means "if the address or port is omitted, then don't take that into account". So what's happening is as follows...

  • With strict=False
    • We check the first rule (reject 1.0.0.0/8:*) and ignore the address component since we only provided a port so it's equivalent to 'reject *:*'. That matches so we return false.
  • With strict=True
    • We check the first rule (reject 1.0.0.0/8:*) and don't match because the address component doesn't match.
    • We check the second rule (accept *:*). That matches so we return true.

Agreed, this doesn't match what the documentation suggests.

comment:3 Changed 3 years ago by nskinkel

Oh ok. Thanks for getting to this so quickly. This 'strict' behavior makes sense then.

I thought I might just be misunderstanding how to use can_exit_to() properly.

Maybe it would help to just clarify the docstring a bit and/or add an example of using the 'strict' parameter correctly?

comment:4 Changed 3 years ago by atagar

Resolution: fixed
Status: newclosed

Thanks for the catch! Fix pushed. Hopefully I got it right (this made my head hurt...).

https://gitweb.torproject.org/stem.git/commit/?id=db0518c

Note: See TracTickets for help on using tickets.