Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#12971 closed defect (fixed)

Invalid SOCKS5 response to UDP associate request

Reported by: yurivict271 Owned by: rl1987
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client, easy
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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).

Child Tickets

Attachments (2)

testcase1.py (418 bytes) - added by yurivict271 5 years ago.
Test case
testcase1_v2.py (560 bytes) - added by rl1987 5 years ago.

Download all attachments as: .zip

Change History (22)

Changed 5 years ago by yurivict271

Attachment: testcase1.py added

Test case

comment:1 Changed 5 years ago by yurivict271

tor-0.2.4.23

comment:2 Changed 5 years ago by yurivict271

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.

comment:3 Changed 5 years ago by nickm

Component: - Select a componentTor
Keywords: tor-client added
Milestone: Tor: 0.2.???

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

comment:4 Changed 5 years ago by 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.

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.

comment:5 in reply to:  4 ; Changed 5 years ago by 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.

comment:6 in reply to:  5 Changed 5 years ago by yawning

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.

comment:7 Changed 5 years ago by arma

Keywords: easy added

comment:8 Changed 5 years ago by rl1987

Owner: set to rl1987
Status: newaccepted

Changed 5 years ago by rl1987

Attachment: testcase1_v2.py added

comment:9 Changed 5 years ago by rl1987

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
Last edited 5 years ago by rl1987 (previous) (diff)

comment:10 Changed 5 years ago by rl1987

Status: acceptedneeds_review

comment:11 Changed 5 years ago by yurivict271

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

comment:12 Changed 5 years ago by yawning

Status: needs_reviewneeds_revision

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.

comment:13 Changed 5 years ago by rl1987

Status: needs_revisionneeds_review

comment:14 Changed 5 years ago by nickm

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

comment:15 Changed 5 years ago by yawning

Status: needs_reviewneeds_revision

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.

comment:16 Changed 5 years ago by rl1987

Status: needs_revisionneeds_review

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

comment:17 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

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

comment:18 Changed 5 years ago by arma

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.

comment:19 Changed 5 years ago by 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.

comment:20 in reply to:  19 Changed 5 years ago by yawning

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. I know most of the easy changes I want to make here, so I shall do so.

Note: See TracTickets for help on using tickets.