Opened 8 months ago

Closed 7 months ago

#29175 closed defect (fixed)

Tor 0.3.5.x mishandles empty socks5 auth

Reported by: arma Owned by: rl1987
Priority: High Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: regression, 035-backport
Cc: rl1987, bugiguiman, jakett2 Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by arma)

There is a regression in Tor 0.3.5.x with handling socks5 handshakes that offer username/password auth but then present empty username and password.

It came in during commit 9068ac3c (which went into 0.3.5.1-alpha).

The symptom is that when you do your socks5 handshake with an empty username and password (e.g. like pidgin does it), you get log lines like

Jan 24 19:40:17.000 [warn] Fetching socks handshake failed. Closing.
Jan 24 19:40:17.000 [warn] socks5: parsing failed - invalid user/pass authentication message.

Bug reported by a person on #tor, and also separately by #29050.

I asked Josh Glazebrook, the author of the socks library in #29050, for some actual traces of correct behavior / incorrect behavior, and he sent:

socks v2.2.3
tor v0.3.5.7
client announces it only supports noauth (00)
server chooses noauth (00)


send <Buffer 05 01 00> [00 = noauth]
recv <Buffer 05 00> [server chose noauth]
send <Buffer 05 01 00 03 09 69 70 69 6e 66 6f 2e 69 6f 00 50> [connect command - ipinfo.io]
recv <Buffer 05 00 00 01 00> [success]




socks v2.2.2
tor v0.3.5.7
client announces it supports noauth (00) and userpass (02)
server chooses userpass (02)
connection is unsuccessful - server returns general socks server failure, tor logs indicate invalid user/pass.
Jan 24 14:47:15.000 [warn] Fetching socks handshake failed. Closing.
Jan 24 14:47:56.000 [warn] socks5: parsing failed - invalid user/pass authentication message.


send <Buffer 05 02 00 02> [00 = noauth, 02 = userpass]
recv <Buffer 05 02> [server chose userpass]
send <Buffer 01 00 00> [00 = zero length username, 00 = zero length password]
recv <Buffer 01 00> [00 = server indicates auth success]
send <Buffer 05 01 00 03 09 69 70 69 6e 66 6f 2e 69 6f 00 50> [connect command - ipinfo.io]
recv <Buffer 05 01 00 01 00> [01- general SOCKS server failure] (in tor's log output it's saying auth is invalid at this point)



socks v.2.2.2
tor via torbrowser 8.0.4 (unsure of which tor version, but likely older than 0.3.5.7)
client announces it supports noauth (00) and userpass (02)
server chooses userpass (02)
connection is successful

send <Buffer 05 02 00 02> [00 = noauth, 02 = userpass]
recv <Buffer 05 02> [server chose userpass]
send <Buffer 01 00 00> [00 = zero length username, 00 = zero length password]
recv <Buffer 01 00> [00 = server indicates auth success]
send <Buffer 05 01 00 03 09 69 70 69 6e 66 6f 2e 69 6f 00 50> [connect command - ipinfo.io]
recv <Buffer 05 00 00 01 00> [00 = success]

Josh further gives us this hint:

It appears there is indeed something that changed between older versions of     
Tor and newer versions. Looking at the actual data being exchanged, it's        
exactly the same.                                                               
                                                                                
The only odd thing is tor v0.3.5.7 is indicating in the auth reply that the     
authentication was a success, and only when the connect command is sent,        
it's returning a "general socks server failure" code, but in the actual tor     
log it's logging invalid user/pass.

I went spelunking and found this code newly in 0.3.5.x:

  if (usernamelen && username) {
    tor_free(req->username);
    req->username = tor_memdup_nulterm(username, usernamelen);
    req->usernamelen = usernamelen;

    req->got_auth = 1;
  }

  if (passwordlen && password) {
    tor_free(req->password);
    req->password = tor_memdup_nulterm(password, passwordlen);
    req->passwordlen = passwordlen;

    req->got_auth = 1;
  }

Compare to the code in 0.3.4.x:

      if (usernamelen) {
        req->username = tor_memdup(data+2u, usernamelen);
        req->usernamelen = usernamelen;
      }
      if (passlen) {
        req->password = tor_memdup(data+3u+usernamelen, passlen);
        req->passwordlen = passlen;
      }
      *drain_out = 2u + usernamelen + 1u + passlen;
      req->got_auth = 1;

So in 0.3.4.x when we got a type 0x02 handshake but empty username and empty password, we would still set got_auth to 1. In 0.3.5.x when that happens, we leave got_auth at 0.

The result is that the socks5 handshake itself still goes identically, but when we get the empty username and password, we don't record that we received it, even though we send back a "handshake was successful" response. And then when application data shows up after that, we try to treat it as a socks5 username and password, because we're still waiting for one. That's where things go bad.

Child Tickets

Change History (11)

comment:1 Changed 8 months ago by arma

My proposed fix would be:

diff --git a/src/core/proto/proto_socks.c b/src/core/proto/proto_socks.c
index 86c656e..8e3cf4a 100644
--- a/src/core/proto/proto_socks.c
+++ b/src/core/proto/proto_socks.c
@@ -451,7 +451,6 @@ parse_socks5_userpass_auth(const uint8_t *raw_data, socks_re
     req->username = tor_memdup_nulterm(username, usernamelen);
     req->usernamelen = usernamelen;
 
-    req->got_auth = 1;
   }
 
   if (passwordlen && password) {
@@ -459,9 +458,10 @@ parse_socks5_userpass_auth(const uint8_t *raw_data, socks_r
     req->password = tor_memdup_nulterm(password, passwordlen);
     req->passwordlen = passwordlen;
 
-    req->got_auth = 1;
   }
 
+  req->got_auth = 1;
+
   end:
   socks5_client_userpass_auth_free(trunnel_req);
   return res;

But I don't know if that is a *sufficient* fix. Somebody should test it. :)

comment:2 Changed 8 months ago by yawning

Rejecting malformed username/password authentication attempts is the correct behavior.

send <Buffer 01 00 00> [00 = zero length username, 00 = zero length password]

Both UNAME and PASSWD are explicitly specified as 1 to 255 octets long. Fix the client library.

See:

comment:3 Changed 8 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

Will look some more into this tomorrow.

comment:4 Changed 8 months ago by nickm

Keywords: 035-backport added; backport-035 removed
Priority: MediumHigh

comment:5 Changed 8 months ago by arma

Description: modified (diff)

(fixing mis-paste in josh's text)

comment:6 Changed 8 months ago by rl1987

Status: acceptedneeds_review

https://github.com/torproject/tor/pull/670

I would side with keeping the interoperability we previously had, even if we technically violate the protocol spec. This will make some somewhat broken SOCKS5 clients work with tor again.

comment:7 Changed 8 months ago by arma

Status: needs_reviewmerge_ready

Looks good to me! Let's get this in, and also into a new 0,3.5.x as soon as is practical.

Version 0, edited 8 months ago by arma (next)

comment:8 Changed 8 months ago by boklm

Cc: bugiguiman added

#29236 is a duplicate.

comment:10 Changed 7 months ago by jakett2

Cc: jakett2 added

comment:11 Changed 7 months ago by nickm

Milestone: Tor: 0.4.0.x-finalTor: 0.3.5.x-final
Resolution: fixed
Status: merge_readyclosed

Merged to maint-0.3.5 and forward.

Note: See TracTickets for help on using tickets.