SOCKS handling should accept (and ignore) password auth.
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.
Activity
changed milestone to %Tor: 0.2.3.x-final
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?
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;-
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?
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.
-
-
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.
-
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.
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 atmThe while loop continues as long as there's more data than what was available earlier.
What if datalen == 1 or even 0?
-
datalen = v.iov_len and then the buffer is drained with as much as parse_socks() says it consumed
-
if evbuffer_get_length() returns something >1 the while condition is true
-
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 < 2Can we have v.iov_len == 0 or 1?
-
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?