TROVE-2022-002: The SafeSocks option for SOCKS4(a) is inverted leading to SOCKS4 going through

This is a report from hackerone: https://hackerone.com/bugs?subject=torproject&report_id=1784589

Below is the content of the report which has detailed information.

We have classified this as medium considering that tor was not defending in-depth for dangerous SOCKS request and so any user relying on SafeSocks 1 to make sure they don't link DNS leak and their Tor traffic wasn't safe afterall for SOCKS4(a).

Tor Browser doesn't use SafeSocks 1 and SOCKS4 so at least the likely vast majority of users are not affected.

H1 Report

Summary:

This bug appears to be a typo; src/core/proto/proto_socks.c, process_socks4_request() contains the line:
if (is_socks4a && !addressmap_have_mapping(req->address, 0)) {

Which presumably should be !is_socks4a. The following logic appears correct only if that change is made; as it exists now, the check is inverted and only warns on and rejects safe SOCKS4a requests.

Steps To Reproduce:

1. Configure Tor with SafeSocks set to 1 (listening on localhost, with default port 9050)
2. Perform an unsafe SOCKS4 request; e.g., with socat: echo -en "HEAD / HTTP/1.1\r\nHost: duckduckgo.com\r\n\r\n" | socat -v STDIO SOCKS4:127.0.0.1:duckduckgo.com:80,socksport=9050,shut-none
   Note that the locally-performed DNS request is leaked, and only the server IP is sent to Tor, but Tor does not block the request as it should have. The request reaches the server, and Tor does not log that a DNS leak may have occurred.
3. Perform a safe SOCKS4a request: echo -en "HEAD / HTTP/1.1\r\nHost: duckduckgo.com\r\n\r\n" | socat -v STDIO SOCKS4a:127.0.0.1:duckduckgo.com:80,socksport=9050,shut-none
   Note that the domain is being sent to Tor, but Tor incorrectly blocks the request, and logs an incorrect "Your application (using socks4 to port 80) is giving Tor only an IP address." error.

The check is inverted; when using SOCKS4/SOCKS4a, SafeSocks 1 does the exact opposite of what it should, ensuring the user is making only unsafe requests. I have not tested SOCKS5, though it presumably works correctly given TorBrowser uses it.

Additional info:

I tested this on Tor versions 0.4.7.10 and 0.4.5.10; git history suggests this has been broken all the way back to 0.3.5, seems to have been introduced in a rewrite, merge commit 75569335, see https://gitweb.torproject.org/user/nickm/tor.git/commit/src/or/proto_socks.c?h=socks_trunnel4_squashed_merged&id=9155e08450fe7a609f8223202e8aa7dfbca20a6d

The commands above were run using Bash and socat, and this bug had been discovered while using a custom SOCKS4a application.

Impact

Impact is a DNS leak that occurs under a configuration intended to guard against such a possibility. That configuration is not under attacker control, but users who believe they need this protection may be more likely to turn on the broken config option.

2 scenarios are plausible, that impact user anonymity by leaking DNS traffic with SafeSocks 1:

1. Users configuring SOCKS4 apps that don't support SOCKS4a will be led to believe their traffic is fully secured, when in fact DNS requests are still being leaked.
2. Users configuring apps that do support SOCKS4a could be misled into selecting SOCKS4 instead, as Tor will error out if they try to use the safe configuration; this could conceivably create a DNS leak where one need not exist.

It should also be noted that the incorrect warning does still get printed even with SafeSocks 0, but it does not cause any otherwise-visible problems.

Note that users will only be impacted if they connect using SOCKS4, and rely on SafeSocks to protect against DNS leaks. SafeSocks 1 is recommended in a number of online guides to securing Tor. The application I was working on when I discovered this also relied on it as a defense-in-depth measure. I nevertheless suspect SOCKS4a may be a rare configuration, given this bug seems to have existed for about 4 years without detection.