tor-resolve: Do not truncate too long hostnames

If a hostname is supplied to tor-resolve which is too long, it will be silently truncated, resulting in a different hostname lookup:

$ tor-resolve $(python -c 'print("google.com" + "m" * 256)')

If tor-resolve uses SOCKS5, the length is stored in an unsigned char, which overflows in this case and leads to the hostname "google.com". As this one is a valid hostname, it returns an address instead of giving an error due to the invalid supplied hostname.

Trac:
Username: junglefowl

changed milestone to %Tor: 0.2.9.x-final

added 029-backport component::core tor/tor milestone::Tor: 0.2.9.x-final points::0.5 priority::medium reporter::junglefowl resolution::fixed security-review severity::normal status::closed tor-resolve type::defect version::tor 0.2.9.8 labels

I suggest using UINT8_MAX instead of a magic number.

Replying to cypherpunks:

I suggest using UINT8_MAX instead of a magic number.

256 is UINT8_MAX + 1.

Putting this in 0.3.0, because truncating inputs and substituting part of a string for a port number is problematic.

This could be a security issue if tor-resolve is used on untrusted inputs.

This might be worth doing an 0.2.9 backport, but I'm not sure if we want to do one to 0.2.8.

Trac:
Keywords: N/A deleted, 029-backport, security-review added
Status: new to needs_revision
Points: N/A to 0.5
Milestone: N/A to Tor: 0.3.0.x-final

This should be clamped at 255 or 253. The former is the SOCKS5 limit, the latter is the DNS wire protocol limit.

Edit: I'd probably go with 255.

addrlen is already increased by 1 to contain the ending \0 character in the string. When its value is assigned to the unsigned char in the data packet, it is subtracted by one:

(*out)[ 4 ] = (char)(uint8_t)(addrlen - 1);

Due to this, i chose to cap at addrlen > 256 because 256-1=255 can still be properly put into the length byte. The ending \0 is not needed in the protocol, otherwise google.commm[...] would have been parsed by the remote peer.

I could reduce the constant to 255. As yawning pointed out, the DNS lookup of the remote peer would fail anyway, but I prefer to support as much as the SOCKS5 protocol offers.

Trac:
Username: junglefowl

Replying to junglefowl:

addrlen is already increased by 1 to contain the ending \0 character in the string. When its value is assigned to the unsigned char in the data packet, it is subtracted by one:

(*out)[ 4 ] = (char)(uint8_t)(addrlen - 1);

Due to this, i chose to cap at addrlen > 256 because 256-1=255 can still be properly put into the length byte. The ending \0 is not needed in the protocol, otherwise google.commm[...] would have been parsed by the remote peer.

I could reduce the constant to 255. As yawning pointed out, the DNS lookup of the remote peer would fail anyway, but I prefer to support as much as the SOCKS5 protocol offers.

Let's reduce it to UINT8_MAX then.

Trac:
Username: junglefowl

tor-resolve.2.patch

Trac:
Username: junglefowl

Trac:
Username: junglefowl

tor-resolve.patch

Trac:
Username: junglefowl

I have adjusted the patch as requested. By accident I uploaded it twice, but the files are identical. I am sorry for that.

Trac:
Username: junglefowl

Trac:
Username: junglefowl
Status: needs_revision to needs_review

Looks good to me, let's get it merged at some point.

Trac:
Status: needs_review to merge_ready

Merged!

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

Cherry-picked to 0.2.9

Trac:
Milestone: Tor: 0.3.0.x-final to Tor: 0.2.9.x-final

closed

changed time estimate to 4h

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