Opened 7 weeks ago

Closed 4 weeks ago

#30721 closed defect (fixed)

tor_addr_port_lookup() is overly permissive

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: technical-debt, tor-addr, refactor, practracker-improvement
Cc: Actual Points: 1.5
Parent ID: Points: 0.5
Reviewer: catalyst Sponsor: Sponsor31-can

Description

Like tor_addr_parse() in #23082, tor_addr_port_lookup() also allows square brackets around IPv4 addresses.

But it's slightly less permissive: it requires a terminating ].

For example, this command line should be rejected, but it is not:

tor ORPort [127.0.0.1]:9001

Child Tickets

Change History (11)

comment:1 Changed 7 weeks ago by teor

Actual Points: 0.20.5
Keywords: tor-addr added; operator-visible-change removed
Points: 0.20.5
Status: assignedneeds_review
Version: Tor: unspecified

This bug was introduced in in 0.2.1.5-alpha, when tor_addr_lookup() was called tor_addr_port_parse().

The first commit fixes the bug, the next two commits refactor the code so the logic is clearer. I split tor_addr_lookup() into 3 separate functions as part of the refactor, the split gets rid of a practracker exception.

See my pull request https://github.com/torproject/tor/pull/1068

This change will break some rare, invalid tor configs, so we can't backport it.

comment:2 Changed 7 weeks ago by teor

Keywords: refactor practracker-improvement added

comment:3 Changed 7 weeks ago by asn

Reviewer: catalyst

comment:4 in reply to:  1 ; Changed 7 weeks ago by catalyst

Status: needs_reviewneeds_revision

Replying to teor:

This bug was introduced in in 0.2.1.5-alpha, when tor_addr_lookup() was called tor_addr_port_parse().

The first commit fixes the bug, the next two commits refactor the code so the logic is clearer. I split tor_addr_lookup() into 3 separate functions as part of the refactor, the split gets rid of a practracker exception.

See my pull request https://github.com/torproject/tor/pull/1068

This change will break some rare, invalid tor configs, so we can't backport it.

Thanks! This looks good by visual inspection. The commit structure is helpful. The first commit could use a few minor changes:

  • Add a changes file
  • Maybe add unit tests to ensure that IPv4 addresses with square brackets get rejected?

comment:5 in reply to:  4 ; Changed 6 weeks ago by teor

Actual Points: 0.51.0

Replying to catalyst:

Replying to teor:

This bug was introduced in in 0.2.1.5-alpha, when tor_addr_lookup() was called tor_addr_port_parse().

The first commit fixes the bug, the next two commits refactor the code so the logic is clearer. I split tor_addr_lookup() into 3 separate functions as part of the refactor, the split gets rid of a practracker exception.

See my pull request https://github.com/torproject/tor/pull/1068

This change will break some rare, invalid tor configs, so we can't backport it.

Thanks! This looks good by visual inspection. The commit structure is helpful. The first commit could use a few minor changes:

  • Add a changes file

The PR already has changes/bug30721:
https://github.com/torproject/tor/pull/1068/files#diff-82ae46251bd81539f5fb75c1d7e7a82b

  • Maybe add unit tests to ensure that IPv4 addresses with square brackets get rejected?

Hmm yeah the unit tests are not in a great state. I'm working on them.

comment:6 in reply to:  5 Changed 6 weeks ago by teor

Actual Points: 1.01.5
Status: needs_revisionneeds_review

Replying to teor:

Replying to catalyst:

  • Maybe add unit tests to ensure that IPv4 addresses with square brackets get rejected?

Hmm yeah the unit tests are not in a great state. I'm working on them.

So I found two bugs while improving the unit tests:

  • ambiguous IPv6:port without brackets were allowed, now they are banned in the address-port parsing functions only (1b39bde)
  • sometimes the port was set on failure, now it is always zero (679cce7)

I rewrote the tests and added a lot of tests cases, to make sure we covered all these changes.

Some of the ambiguous test cases may fail on Linux or Windows (I only tested on macOS), so I'll check CI in an hour or so to make sure.

comment:7 Changed 4 weeks ago by catalyst

Status: needs_reviewneeds_information

Thanks! The new tests look reasonable. I wonder if it's necessary to make all those multi-statement macro definitions, instead of helper functions (and maybe much smaller macros that call those functions)? I think helper function would be a little cleaner. Please let me know what you think.

comment:8 in reply to:  7 ; Changed 4 weeks ago by teor

Status: needs_informationneeds_review

Replying to catalyst:

Thanks! The new tests look reasonable. I wonder if it's necessary to make all those multi-statement macro definitions, instead of helper functions (and maybe much smaller macros that call those functions)? I think helper function would be a little cleaner. Please let me know what you think.

The problem with helper functions is that failures are attributed to the test assertions in the helper function, without any context. So it's very hard to tell which test data caused the failure.

I'm not sure if there is some way of providing context as part of the test macros. I suggest that we merge this code as-is, because it's functional, adds coverage, and makes sure we won't introduce future bugs. Then we can open a ticket for follow-up.

comment:9 in reply to:  8 ; Changed 4 weeks ago by catalyst

Status: needs_reviewmerge_ready

Replying to teor:

Replying to catalyst:

Thanks! The new tests look reasonable. I wonder if it's necessary to make all those multi-statement macro definitions, instead of helper functions (and maybe much smaller macros that call those functions)? I think helper function would be a little cleaner. Please let me know what you think.

The problem with helper functions is that failures are attributed to the test assertions in the helper function, without any context. So it's very hard to tell which test data caused the failure.

I'm not sure if there is some way of providing context as part of the test macros. I suggest that we merge this code as-is, because it's functional, adds coverage, and makes sure we won't introduce future bugs. Then we can open a ticket for follow-up.

I see. I agree we should open a new ticket to follow up, in that case.

I think I see that the problem is buried in the TT_DECLARE() macro in tinytest_macros.h. I think it's possible to work around it, but it might be nontrivial. (Rough sketch: redefine TT_DECLARE() in helper functions to read file and line info from function parameters, and make file and line numbers explicit parameters to helper functions.)

comment:10 in reply to:  9 Changed 4 weeks ago by teor

Replying to catalyst:

Replying to teor:

Replying to catalyst:

Thanks! The new tests look reasonable. I wonder if it's necessary to make all those multi-statement macro definitions, instead of helper functions (and maybe much smaller macros that call those functions)? I think helper function would be a little cleaner. Please let me know what you think.

The problem with helper functions is that failures are attributed to the test assertions in the helper function, without any context. So it's very hard to tell which test data caused the failure.

I'm not sure if there is some way of providing context as part of the test macros. I suggest that we merge this code as-is, because it's functional, adds coverage, and makes sure we won't introduce future bugs. Then we can open a ticket for follow-up.

I see. I agree we should open a new ticket to follow up, in that case.

I think I see that the problem is buried in the TT_DECLARE() macro in tinytest_macros.h. I think it's possible to work around it, but it might be nontrivial. (Rough sketch: redefine TT_DECLARE() in helper functions to read file and line info from function parameters, and make file and line numbers explicit parameters to helper functions.)

I opened #30968 with your suggestion, and my alternate suggestion (a format string that allows us to print the data being tested).

comment:11 Changed 4 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

Squashed and merged!

Note: See TracTickets for help on using tickets.