Opened 7 months ago

Closed 7 months ago

Last modified 6 months ago

#21280 closed defect (fixed)

tor-resolve: Do not truncate too long hostnames

Reported by: junglefowl Owned by:
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor: 0.2.9.8
Severity: Normal Keywords: tor-resolve, security-review, 029-backport
Cc: Actual Points:
Parent ID: Points: 0.5
Reviewer: Sponsor:

Description

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.

Child Tickets

Attachments (2)

tor-resolve.2.patch (1.3 KB) - added by junglefowl 7 months ago.
tor-resolve.patch (1.3 KB) - added by junglefowl 7 months ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 7 months ago by cypherpunks

I suggest using UINT8_MAX instead of a magic number.

comment:2 in reply to:  1 Changed 7 months ago by teor

Keywords: security-review 029-backport added
Milestone: Tor: 0.3.0.x-final
Points: 0.5
Status: newneeds_revision

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.

comment:3 Changed 7 months ago by yawning

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.

Last edited 7 months ago by yawning (previous) (diff)

comment:4 Changed 7 months ago by 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.

comment:5 in reply to:  4 Changed 7 months ago by teor

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.

Changed 7 months ago by junglefowl

Attachment: tor-resolve.2.patch added

Changed 7 months ago by junglefowl

Attachment: tor-resolve.patch added

comment:6 Changed 7 months ago by junglefowl

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

comment:7 Changed 7 months ago by junglefowl

Status: needs_revisionneeds_review

comment:8 Changed 7 months ago by teor

Status: needs_reviewmerge_ready

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

comment:9 Changed 7 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged!

comment:10 Changed 6 months ago by nickm

Milestone: Tor: 0.3.0.x-finalTor: 0.2.9.x-final

Cherry-picked to 0.2.9

Note: See TracTickets for help on using tickets.