SOCKS handling should accept (and ignore) password auth.

[nickm]

Right now, our SOCKS handling code doesn't accept attempts to include a password authenticator in the handshake, as supported by SOCKS5. We could implement this without too much effort.

The rationale for doing this was to handle SOCKS clients that don't know how _not_ to include authentication information.

[mwenge]

wip at ​https://gitweb.torproject.org/hoganrobert/tor.git/shortlog/refs/heads/bug1666

[nickm]

This is either a prereq for implementing proposal "171-separate-streams-by-port-or-host.txt", or obsoleted by that proposal. When somebody goes to implement that, they should remember that this code is there.

[nickm]

Also, sadly, this code is going to need some light refactoring to work now that we've merged the bufferevents branch. It's worth doing, since we want to also port those &@# unit tests.

It looks like a config_register_addressmaps change leaked into this patch; see config.h and config.c.

Also, it looks like the new parsing code doesn't actually make sure that the username/password stuff appears as the *second* thing the client says. We should make sure we check for that.

Also, is it really disallowed for the client to start writing data before the socks handshake is done? What's the harm in allowing extra data?

[mwenge]

Replying to nickm:

Also, sadly, this code is going to need some light refactoring to work now that we've merged the bufferevents branch. It's worth doing, since we want to also port those &@# unit tests.

Done.

It looks like a config_register_addressmaps change leaked into this patch; see config.h and config.c.

I need to expose config_register_addressmaps for the unit tests.

Also, it looks like the new parsing code doesn't actually make sure that the username/password stuff appears as the *second* thing the client says. We should make sure we check for that.

Done.

Also, is it really disallowed for the client to start writing data before the socks handshake is done? What's the harm in allowing extra data?

You mean after the method has been negotiated we just clobber the rest of the packet?

        /* remove packet from buf. also remove any other extraneous
         * bytes, to support broken socks clients. */
        *drain_out = -1;

[nickm]

You mean after the method has been negotiated we just clobber the rest of the packet?

No, I was talking about the part that said,

+      if (buf->datalen > 2u + usernamelen + 1u + passlen) {
+        log_warn(LD_APP,
+                 "socks5: Malformed username/password. Rejecting.");
+        return -1;
+      }

I meant to ask whether, after we're done accepting the username and password, we shouldn't allow the buffer to still ahve more data that we leave on the buffer? Is the client not allowed to send the connection request until the server answers the authentication?

[nickm]

The part of this that works with proposal 171 (whatever that turns out to be) should indeed go in 0.2.3.x. Proposal 171 turns this into a non-minor priority too, since now this information is actually good for something. :)

[mwenge]

Replying to nickm:

You mean after the method has been negotiated we just clobber the rest of the packet?

No, I was talking about the part that said,

+      if (buf->datalen > 2u + usernamelen + 1u + passlen) {
+        log_warn(LD_APP,
+                 "socks5: Malformed username/password. Rejecting.");
+        return -1;
+      }

I meant to ask whether, after we're done accepting the username and password, we shouldn't allow the buffer to still ahve more data that we leave on the buffer? Is the client not allowed to send the connection request until the server answers the authentication?

Good point - fixed this to permit it.

[nickm]

Looks good to me. There are a couple of whitespace things (like "}else") that we'll need to deal with, but otherwise I'm good to merge this after one last look. I'm going to hold off till I'm back from vacation, though, since I won't be around to try to merge fixes if I'm wrong and this breaks.

Also, instead of just draining the authentication info, we will eventually need to store it. We might as well add that code while we're adding this patch, since it'll be fresh in our minds.

[ioerror]

What else needs to be done on this bug? I think we should accept and store username/password data for SOCKS5 connections. Additionally, I think prop 171 is pretty important, so this would be quite helpful for that bug.

[nickm]

Let's get this merged!

For changes please see my new branch "bug1666" in my public repository, based on Robert's branch above.

I think that the want_length_out computations in parse_socks() are suspicious. I've fixed these (I think) in 1ed615ded7db0765e.

The big list of tests makes me quite happy. I refactored them into a test suite of their own. Previously, some of the subtests were only passing because of state that was already set in the socks_request_t; I fixed that.

I think that the current code is incorrect in how it getting a message that starts with 0x01. Right now, if we have negotiated socks5, then it accepts username/password auth. But it does this potentially infinitely many times, and it also does it if we have not negotiated username/password auth! I think that we should be more strict here. My aec396d might fix that.

In 2e6604f4 I added code to record the username/password; we'll need that.

In 05c424 I changed the calling convention for fetch_from_buf_socks to more closely match fetch_from_evbuffer socks.

This probably needs another review now, and maybe a little more testing.

[nickm]

I've got a bug1666_spec branch on my desktop that makes the appropriate changes in the torspec repo; once I have a torspec repo of my own, I should push it there.

[ln5]

I have two minor docu fixes in my nickm-bug1666.

[ln5]

Do we risk running out of data in fetch_from_evbuffer_socks()?

evbuffer_get_length() ==> left in buffer, total
datalen = inspect_evbuffer() ==> available bytes to read atm

The while loop continues as long as there's more data than what was
available earlier.

What if datalen == 1 or even 0?

1. datalen = v.iov_len and then the buffer is drained with as much as parse_socks() says it consumed

2. if evbuffer_get_length() returns something >1 the while condition is true

3. can inspect_evbuffer() return 0 or 1?

      - 0: if want_length == 0 -- if first parse_socks() call set
        *want_length_out=0 -- won't happen
      - 0: if evbuffer_get_length() returns 0 -- won't happen since (2)
      - v.iov_len of the last chunk
      - something >= n -- never < 2
   

Can we have v.iov_len == 0 or 1?

[ln5]

My brain is clearly not built for grokking logic like the one in parse_socks(). :-(

What with situations like req->socks_version == 5 && req->got_auth? Can it happen and what can data point at? Or rather, what values are possible for req->socks_version and req->got_auth when data points at anything which is _not_ the start of a socks message?

[nickm]

I merged your doc fix onto my bug1666 branch, and cherry-picked the spec fix onto my branch on the torspec repository.

Trying to understand your other comments now.

[nickm]

On the looping issue -- I think it's probably easier to clean up the code than to convince ourselves that the existing code is right. Probably the easiest fix is to just add the check for what we _want_ to have happen: that every time we go through the loop, we had better make progress: we should either drain something, or pass more data to parse_socks than we did the last time, or we may be in an infinite loop.

On parse_socks(): Yeah, that logic was convoluted. It's been yucky for years. Perhaps it's time to refactor once this stuff is merged. It seems that req->socks_version gets set when we are done parsing the first part of the socks request. If that's socks4, the request is all of one piece, and we're done. If it's socks5 and we're using anything other than SOCKS_NO_AUTH then we need to parse the authentication. Once that's done, we set got_auth.

So, once req->socks_version == 5 && req->got_auth, we have negotiated socks v5 with username/password authentication and parsed the authentication, but (if we're not done yet) we haven't parsed the actual command part of the request yet.

[nickm]

Added another commit to try to make this a little more bulletproof.

[ln5]

Replying to nickm:

Added another commit to try to make this a little more bulletproof.

#tor-dev 2011-07-12 17:36 CEST:
<ln5> nickm: i find the logic in the loop of fetch_from_evbuffer_socks()

totally understandable in your last commit. i really can't tell if it
works or not but i feel much better.

Nick opened #3569.

[nickm]

Merged 1666.