Tor 0.3.5.x mishandles empty socks5 auth

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 legacy/trac#29050 (moved).

I asked Josh Glazebrook, the author of the socks library in legacy/trac#29050 (moved), 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.