Opened 2 years ago

Closed 2 years ago

#25627 closed defect (fixed)

tsocks_gethostbyaddr_r scribbles garbage over data->hostname and then relies on it

Reported by: exarkun Owned by: dgoulet
Priority: Medium Milestone:
Component: Core Tor/Torsocks Version:
Severity: Blocker Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


Here's an edited version of the implementation with extraneous portions removed in an effort to make the mistake more clear:

        struct data {
                char *hostname;
                char *addr_list[2];
                char padding[];
        } *data;
        // ...
        data = (struct data *) buf;
        // ...
                ret_str = inet_ntop(type, addr, buf, buflen);
        // ...
        if (data->hostname) {

Specifically, notice that data is an alias for buf and therefore the underlying memory is given to inet_ntop to write on as a char* but then tsocks_gethostbyaddr_r tries to interpret that same memory through the data struct to retrieve the hostname field. The result is garbage which provokes a crash shortly afterwards:

        he->h_length = strlen(he->h_name);     

This case is triggered by an error response to the resolve_ptr request (\5\1\0\0) when the IP address is valid (so that inet_ntop writes the garbage to buf).

I can imagine a couple fixes. The simplest overall would seem to be to stack allocated a new buffer for inet_ntop. This makes calling tsocks_gethostbyaddr_r a little more expensive (in terms of stack space) but greatly simplifies the code be removing the surprising aliasing.

Child Tickets

Attachments (1)

gethostbyaddr_r.patch (2.1 KB) - added by exarkun 2 years ago.
Patch with test and fix

Download all attachments as: .zip

Change History (6)

comment:1 Changed 2 years ago by exarkun

I left one line out (and don't have description edit permissions I guess). In the last code snippet, he->h_name is an alias for data->hostname.

Changed 2 years ago by exarkun

Attachment: gethostbyaddr_r.patch added

Patch with test and fix

comment:2 Changed 2 years ago by arma

Status: newneeds_review

comment:3 Changed 2 years ago by exarkun

Note that the attached patch is near code modified by another recent patch and so I generated it stacked on top of those changes to avoid conflicts. But this does mean should be dealt with first.

comment:4 Changed 2 years ago by asn

Hello exarkun and thanks for the code and the ticket! We are not the best at maintaining torsocks right nowbecause of various other stuff going on, but I let the maintainer (dgoulet) know about this patch and he said he will try to merge all the stuff soon! We hope to have news for you soon! : )

comment:5 Changed 2 years ago by dgoulet

Resolution: fixed
Status: needs_reviewclosed

Merged in commit 8013dfb1ebf6cb1d.


Note: See TracTickets for help on using tickets.