Invalid SOCKS5 response to UDP associate request

The attached script sends UDP ASSOCIATE request to tor socks5 server. It gets two bytes in response: $ ./testcase.py

05 03 00 01 08 08 08 08 00 35 << 05 00

As per RFC 1928, this response is invalid. Only 2 bytes out of 10 expected bytes are in response (see Section 6: Replies). Subsection "UDP ASSOCIATE" there says "In the reply to a UDP ASSOCIATE request, the BND.PORT and BND.ADDR fields indicate the port number/address where the client MUST send UDP request messages to be relayed."

Tor sends back only two fields: 05 (version), 00 (Reply field=succeeded), and nothing else. It should either send ip/port in a full reply, or (maybe) 05 01 (error response).

changed milestone to %Tor: 0.2.6.x-final

added component::core tor/tor easy milestone::Tor: 0.2.6.x-final owner::rl1987 priority::medium resolution::fixed status::closed tor-client type::defect labels

Trac:
testcase1.py

Test case

tor-0.2.4.23

I think, expected behavior for DNS port is to open the temporary DNSPort for the duration of this association, and for other ports is to return a failure code.

I think, expected behavior for DNS port is to open the temporary DNSPort for the duration of this association, and for other ports is to return a failure code.

This sounds worth doing to me

Trac:
Keywords: N/A deleted, tor-client added
Component: - Select a component to Tor
Milestone: N/A to Tor: 0.2.???

FWIW we do not support UDP ASSOCIATE at all, though this is still a bug. The relevant location to change would be in connection_edge.c:connection_mark_unattached_ap_() I believe.

Per RFC 1928: X'07' Command not supported

The right thing to do here would be to send back:

05 07 00 01 00 00 00 00 00 00

Our socks code in general is a huge mess, so maybe this should be done as part of #11138 (moved).

Replying to yawning:

FWIW we do not support UDP ASSOCIATE at all, though this is still a bug. The relevant location to change would be in connection_edge.c:connection_mark_unattached_ap_() I believe.

I'd try to do it in parse_socks() when we say

        /* not a connect or resolve or a resolve_ptr? we don't support it. */
        log_warn(LD_APP,"socks5: command %d not recognized. Rejecting.",
                 req->command);
        return -1;

Set req->reply and req->replylen and then connection_ap_handshake_process_socks() will send it for you.

Replying to arma:

Replying to yawning:

FWIW we do not support UDP ASSOCIATE at all, though this is still a bug. The relevant location to change would be in connection_edge.c:connection_mark_unattached_ap_() I believe.

I'd try to do it in parse_socks() when we say {{{ /* not a connect or resolve or a resolve_ptr? we don't support it. */ log_warn(LD_APP,"socks5: command %d not recognized. Rejecting.", req->command); return -1; }}}

Set req->reply and req->replylen and then connection_ap_handshake_process_socks() will send it for you.

Indeed that is a better place to do this. From looking through parse_socks(), it appears that our error handling has identical issues in other cases where we reject the request.

If we don't decide to fix this as part of the refactor (which will happen $deity knows when):

• Add static void make_socks_error(socks_request_t *req, uint8_t reason);
• Call with the appropriate error codes before returning out of parse_socks()

would be trivial to implement.

Trac:
Keywords: N/A deleted, easy added

Trac:
Owner: N/A to rl1987
Status: new to accepted

Trac:
testcase1_v2.py

The above Python script was testing this behavior in slightly incorrect way, since it would send the UDP ASSOCIATE request right after establishing TCP connection to Tor. This is wrong way to do it, since SOCKS5 server expects to get version identifier/method selection message from client right after the TCP connection is established. If we send the UDP ASSOCIATE request when Socks listener is still in expecting-to-get-version-id/method-selection-message-from-client state, it rightfully responds with METHOD selection message 05 00 (see section "3. Procedure for TCP-based clients" in RFC 1928).

I have fixed this fault in Python script and made quick-and-dirty fix for SOCKS5 code as well. See https://github.com/rl1987/tor/tree/bug12971

Testing my changes with fixed script:

>> 05 01 00
<< 05 00
>> 05 03 00 01 08 08 08 08 00 35
<< 05 07 00 01 00 00 00 00 00 00

Trac:
Status: accepted to needs_review

Still, even if the script was sending the messages out of sequence, the original response was invalid as it was.

A few thoughts:

• A better fix would be to check if CMD is any of the commands we actually support, instead of explicitly checking if it is UDP ASSOCIATE. I could edit the test case to issue a BIND (also unsupported), and it wouldn't send the correct response. Note that this doesn't mean, also check BIND, what would happen if I send a request with CMD=0x23?
• The blurb to generate the response should be a function. static int send_socks5_error(socks_request_t *req, socks5_reply_status_t reason); would be what I would use.
• 0x07 -> SOCKS5_COMMAND_NOT_SUPPORTED (If you use the prototype I suggested, this should be obvious).
• Adding a check for UDP_ASSOCIATE to the if is incorrect, execution will never reach there, since you're examining the command earlier in the function, and returning.
• Line 1996 (your copy), gratuitous whitespace change.

It would be nice after carving out the error generation if you looked at other places where we reject requests and sent back sensible errors. For example:

          log_warn(LD_APP,"socks5: unsupported address type %d. Rejecting.",
                   (int) *(data+3));
          return send_socks5_error(req, SOCKS5_ADDRESS_TYPE_NOT_SUPPORTED);
          /* NB: I assume send_socks5_error always returns -1, could also make it a void and have a separate return, but this is more concise. */

For cases where none of the more specific error reasons are applicable, SOCKS5_GENERAL_ERROR should be used.

Trac:
Status: needs_review to needs_revision

https://github.com/rl1987/tor/tree/bug12971_take2

Trac:
Status: needs_revision to needs_review

Trac:
Milestone: Tor: 0.2.??? to Tor: 0.2.6.x-final

Looks much better now. Some minor thoughts.

• I would personally prefer if the function name included the socks version, as the code supports both socks 4 and socks 5, so it's immediately obvious when skimming the code.
• The socks code has unit tests. It would be nice if the test was updated for this change. test_socks.c:test_socks_5_unsupported_commands() would be the function to edit, and even has helpful comments reflecting the old broken behavior.

Apart from those things, looks good to me.

Trac:
Status: needs_review to needs_revision

Made some changes in accord to above suggestions from yawning. Please review the last three commits here:

• !https://github.com/rl1987/tor/commits/bug12971_take2

Trac:
Status: needs_revision to needs_review

Squashed, function renamed yet one more time, and merged. Thanks!

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

Should we change other places in the code to use this new socks_request_set_socks5_error() function instead of doing it manually? I guess right now we distinguish between "error", which allegedly happens in only this case, and "other types of replies"? Hm.

I think that it's fine to have this function get used in more places. Having a new ticket for that would be dandy.

Replying to nickm:

I think that it's fine to have this function get used in more places. Having a new ticket for that would be dandy.

#13314 (moved). I know most of the easy changes I want to make here, so I shall do so.

closed

mentioned in issue #13314 (moved)

moved to tpo/core/tor#12971 (closed)

mentioned in issue tpo/core/tor#13314 (closed)