Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#2330 closed defect (fixed)

SOCKS client cannot handle responses longer than 128 bytes

Reported by: rransom Owned by:
Priority: Low Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: easy
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

doors points out that if a SOCKS proxy sends a reply longer than 128 bytes, Tor may never parse the entire reply. (This can happen, for example, if a SOCKS proxy sends an absurdly long DOMAINNAME in the BND.ADDR field.)

Child Tickets

Change History (11)

comment:1 Changed 9 years ago by rransom

Keywords: easy added

comment:2 Changed 9 years ago by rransom

For the 'changes' file: This bug was introduced when the SOCKS client code was added, in commit 75472c19c3fdcda913eb8117c917ddfd445b2b77 (first released in version 0.2.2.1-alpha).

The patch should be based on maint-0.2.2, but a second patch will be needed on master (on top of the fix to #2327) to fix the bufferevents SOCKS client code.

The most annoying part of fixing this will be calculating the maximum SOCKS reply length. See IETF RFC 1928 for the SOCKS 5 spec; I don't know where to find the SOCKS 4 spec.

comment:3 Changed 9 years ago by arma

http://ss5.sourceforge.net/socks4.protocol.txt looks like a plausible version of the socks4 spec.

The original url, http://archive.socks.permeo.com/protocol/socks4.protocol, as specified in our doc/spec/socks-spec.txt, looks to be dead. :(

comment:4 Changed 9 years ago by nickm

Given that the reply format is:

        +----+-----+-------+------+----------+----------+
        |VER | REP |  RSV  | ATYP | BND.ADDR | BND.PORT |
        +----+-----+-------+------+----------+----------+
        | 1  |  1  | X'00' |  1   | Variable |    2     |
        +----+-----+-------+------+----------+----------+

and the longest possible length for BND.ADDR is 256 (one length byte, 255 hostname bytes), I think we're looking at 232 as the longest possible length for a socks reply. Let's round up to doing a pullup(512).

comment:5 Changed 9 years ago by nickm

(From what I can tell, socks4 provides no actual limit on the length of the userid portion of the request.)

comment:6 Changed 9 years ago by nickm

Resolution: fixed
Status: newclosed

Merged a fix to 0.2.2 and 0.2.3.

comment:7 Changed 9 years ago by cypherpunks

What about bufferevent case, now it's broken.

comment:8 in reply to:  7 ; Changed 9 years ago by rransom

Replying to cypherpunks:

What about bufferevent case, now it's broken.

He fixed that code in the merge commit itself, so it didn't show up in anything sent to or-cvs.

comment:9 Changed 9 years ago by cypherpunks

Thanks. Seems or-cvs not more usefull for audit of changes. Guess it never was, yeah.

comment:10 in reply to:  8 Changed 9 years ago by cypherpunks

Replying to rransom:

it didn't show up in anything sent to or-cvs.

Why such mail list exist at all? It's not only useless, it's hides real changes. Thats a wrong way.

Developers should anounce such behavior of or-cvs, so no one tried to read it for practical use.

comment:11 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.