Opened 5 years ago

Closed 5 years ago

#14259 closed defect (fixed)

memleak in connection_ap_handshake_rewrite_and_attach()

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: #7555 Points:
Reviewer: Sponsor:

Description

I think there is a memleak in connection_ap_handshake_rewrite_and_attach(). Specifically, in this block of code:

  if (socks->command == SOCKS_COMMAND_RESOLVE_PTR) {
    unsigned rewrite_flags = 0;
    if (conn->use_cached_ipv4_answers)
      rewrite_flags |= AMR_FLAG_USE_IPV4_DNS;
    if (conn->use_cached_ipv6_answers)
      rewrite_flags |= AMR_FLAG_USE_IPV6_DNS;

    if (addressmap_rewrite_reverse(socks->address, sizeof(socks->address),
                                   rewrite_flags, &map_expires)) {
      char *result = tor_strdup(socks->address);
      /* remember _what_ is supposed to have been resolved. */
      tor_snprintf(socks->address, sizeof(socks->address), "REVERSE[%s]",
                  orig_address);
      connection_ap_handshake_socks_resolved(conn, RESOLVED_TYPE_HOSTNAME,
                                             strlen(result), (uint8_t*)result,
                                             -1,
                                             map_expires);
      connection_mark_unattached_ap(conn,
                                END_STREAM_REASON_DONE |
                                END_STREAM_REASON_FLAG_ALREADY_SOCKS_REPLIED);
      return 0;
    }

The result string is strdupped and passed to that function without it ever being freed.

I'm not sure if this code can be reached by - say - a malicious website.

Child Tickets

Change History (4)

comment:1 Changed 5 years ago by nickm

Keywords: tor-client added
Parent ID: #7555

I don't think this is too worrisome, since it only happens when you're doing reverse DNS, *and* when you have client-side DNS cacheing enabled (NOT RECOMMEDED). I'll do a fix here as part of my #7555 branch.

comment:2 Changed 5 years ago by nickm

Status: newneeds_review

My bug7555_v2 branch has a fix for this as 54e4aaf52c882626fe2cce0ba704d9661269ab99

comment:3 Changed 5 years ago by asn

Looks good to me.

Maybe a better alternative would have been to never tor_strdup() that string at all, and just pass socks->address directly? Careful with this approach though because the tor_snprintf() garbles that string before the call to connection_ap_hondshake_socks_resolved(), some reordering would be required.

Last edited 5 years ago by asn (previous) (diff)

comment:4 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged as part of 7555; I think that the garbling means I don't want to touch this right now. But feel free to submit another tested patch. :)

Note: See TracTickets for help on using tickets.