Opened 7 months ago

Closed 7 months ago

#24952 closed defect (fixed)

channel: channel_tls_get_remote_addr_method() should return the "real_addr" of the connection

Reported by: dgoulet Owned by:
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-channel, review-group-31
Cc: ffmancera@… Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

The accurate address of a connection is real_addr, not the addr member.

Lets make channel_tls_get_remote_addr_method() return the content of real_addr instead.

Child Tickets

Change History (8)

comment:1 Changed 7 months ago by ffmancera

Cc: ffmancera@… added

comment:2 Changed 7 months ago by nickm

If we do this and merge it, let's remember to look at every user of this method to make sure that nothing depended on the old behavior.

comment:3 Changed 7 months ago by ffmancera

Status: newneeds_review

If we do this and merge it, let's remember to look at every user of this method to make sure > that nothing depended on the old behavior.

channel_tls_get_remote_addr_method() is used only by chan->get_remote_addr which is used by channel_get_addr_if_possible() in channel.c. So if I am not wrong nothing depends on the old behaviour.

Here is the patch, check my github branch bug24952. I hope everything is fine :-)

Version 0, edited 7 months ago by ffmancera (next)

comment:4 Changed 7 months ago by nickm

But, who calls channel_get_addr_if_possible()? Will they still work if they get real_addr instead of addr?

comment:5 Changed 7 months ago by nickm

Keywords: review-group-31 added

comment:6 Changed 7 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewmerge_ready

There seems to be 3 callsites:

  1. connection_exit_begin_conn()

The address of the or_circ->p_chan is returned which I think in this case is good to use real_addr because it is used to actually connect to the directory.

  1. channel_do_open_actions()

That is actually the part where this ticket is important because it is where we note down the TCP connection client address into the geoip cache.

  1. channel_dump_statistics()

The address is used to print a log with the remote connected address so again, the real_addr is what we probably want.

So I think we are good. Patch lgtm;

comment:7 Changed 7 months ago by dgoulet

After an IRC discussion with nickm, I've cherry-picked the commit onto ticket24902_029_05 so it can be merged forward in master.

The reason is that this fix is needed by the DoS subsystem (#24902) for which we'll backport to 029.

comment:8 Changed 7 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged that to master. It'll wind up backported to the various older branches. Thanks!

Note: See TracTickets for help on using tickets.