Opened 7 years ago

Closed 6 years ago

#9650 closed defect (fixed)

create_managed_proxy_environment: lack of checks for values returned by get_first_listener_addrport_string

Reported by: cypherpunks Owned by: asn
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-pt 024-backport 025-triaged
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

get_first_listener_addrport_string() can return NULL according to it's documentation.
No checks in create_managed_proxy_environment() so NULL can be passed to smartlist_add_asprintf()

If you sure NULL never returns for such case then it needs proper assert check at least.

Child Tickets

Change History (9)

comment:1 Changed 7 years ago by nickm

Keywords: tor-pt added
Owner: set to asn
Status: newassigned

comment:2 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

comment:3 Changed 6 years ago by nickm

Keywords: 024-backport added
Status: assignedneeds_review

For review: branch bug9650 in my public repository.

If we think this could happen somehow IRL, we should do a partial backport (which would be pretty easy). IMO it's easier to do the backport than to prove that the backport isn't needed.

comment:4 Changed 6 years ago by andrea

Keywords: 025-triaged added

comment:5 Changed 6 years ago by asn

Looks good to me.

I don't think this can happen IRL, since in config.c we make sure that we have an ORPort torrc option before launching any server-side PT proxies (see options_act). Also, we do

    if (options->ExtORPort_lines) {
      char *ext_or_addrport_tmp =
        get_first_listener_addrport_string(CONN_TYPE_EXT_OR_LISTENER);

which means that that function will only be called if there is an ExtORPort torrc option too.

I assume that an ORPort/ExtORPort torrc line will eventually become a listener which can be found by get_first_listener_addrport_string(). I think this is the case, since Tor errors out if it can't set up a listener.

In any case, the proposed branch looks defensive and good to me.

comment:6 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.4.x-final

Well, I wonder if it's possible the orport line would specify only an advertised address and not a listener or something like that.

Either way the clear answer is "merge this". So, merged. Also, marked as possible backport. (To backport, we would cherry-pick 753a246a14a021dc3e4bc26c40d6fe7c2d60bb1b)

comment:7 Changed 6 years ago by nickm

Recommendation: no backport, not actually hurting anything the way tor 0.2.4 is written

comment:8 Changed 6 years ago by arma

"no backport" sounds fine to me

comment:9 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final
Resolution: fixed
Status: needs_reviewclosed

Not a bug in 0.2.4; not a risk of becoming a bug in 0.2.4 unless we backport some other major PT thing that destabilizes the PT code. Closing as fixed in 0.2.5.

Note: See TracTickets for help on using tickets.