SOCKS client cannot handle responses longer than 128 bytes

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.)

changed milestone to %Tor: 0.2.2.x-final

added component::core tor/tor easy milestone::Tor: 0.2.2.x-final priority::low resolution::fixed status::closed type::defect labels

Trac:
Keywords: N/A deleted, easy added

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 (moved)) 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.

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. :(

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).

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

Merged a fix to 0.2.2 and 0.2.3.

Trac:
Resolution: N/A to fixed
Status: new to closed

What about bufferevent case, now it's broken.

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.

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

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.

Trac:
Component: Tor Client to Tor

closed

moved to tpo/core/tor#2330 (closed)