Opened 4 years ago

Closed 6 months ago

#848 closed Bug / Defect (fixed)

SOCKS5 Replies Parsed Incorrectly

Reported by: tpw_rules Owned by: Gert Döring
Priority: major Milestone:
Component: Generic / unclassified Version: OpenVPN git master branch (Community Ed)
Severity: Not set (select this one, unless your'e a OpenVPN developer) Keywords:
Cc:

Description

OpenVPN parses SOCKS5 replies incorrectly when establishing a connection. The RFC specifies that type 03 (Domain) server names are composed of a length byte which tells how many bytes follow. In the source code the length byte is not accounted for when calculating how many bytes to receive for the reply. There should be a + 1 to account for the length byte. As a result, the entire SOCKS5 reply is not read and the header of the next received packet has the low byte of the port prepended to it, usually resulting in a "bad encapsulated packet length" error and connection failure.

Change History (8)

comment:1 Changed 4 years ago by tpw_rules

To clarify: RFC1928 page 4 explains the server name encoding. Line 385 (alen = (unsigned char) c;) in function recv_socks_reply in src/openvpn/socks.c is what I believe to be the problem.

comment:2 Changed 4 years ago by Gert Döring

Owner: set to Gert Döring
Status: newaccepted

thanks. Will look into it.

comment:3 Changed 4 years ago by tpw_rules

Is there any update on this issue? It's a simple two character fix. I can open a PR or such if that would help resolve it faster.

comment:4 Changed 6 months ago by Gert Döring

Sorry for never following up. My work queue was just too long.

Yes, a patch sent to openvpn-devel@…, based on git master branch, is the way to submit patches. Ideally, together with a description how to trigger this. (My current test set excercises SOCKS for UDP- and TCP-based OpenVPN sessions, and hasn't hit a problem here)

comment:5 Changed 6 months ago by Gert Döring

Resolution: invalid
Status: acceptedclosed

Actually reading the RFC now. It says

5.  Addressing
...
          o  X'03'

   the address field contains a fully-qualified domain name.  The first
   octet of the address field contains the number of octets of name that
   follow, there is no terminating NUL octet.

which says "the number of octets of name", not "the number of octets of name, including the length field". Unfortunately, there is no example to just see which interpretation is right.

Looking into OpenSSH, I see this code in channels.c:

        switch (s5_req.atyp){
        case SSH_SOCKS5_IPV4:
                addrlen = 4;
                af = AF_INET;
                break;
        case SSH_SOCKS5_DOMAIN:
                addrlen = p[sizeof(s5_req)];
                af = -1;
                break;
...

which matches our code: take the byte "as it is" as length field.

Now, OpenSSH might have made the same mistake, so I went and looked at the DANTE socks proxy. It's "clientprotocol.c" has this:

      case PROXY_SOCKS_V5:
...
         /* ATYP */
         if ((rc = socks_recvfromn(s,
                                   &host->atype, sizeof(host->atype),
..
            case SOCKS_ADDR_DOMAIN: {
               unsigned char alen;

               /* read length of domain name. */
               if ((rc = socks_recvfromn(s,
                                         &alen,
                                         sizeof(alen),
...
               OCTETIFY(alen);
...
               /* BND.ADDR, alen octets */
               if ((rc = socks_recvfromn(s,
                                         host->addr.domain,
                                         (size_t)alen,

so, this is also treating the first byte as "lenght of the hostname, not including the length byte itself".

Taking all this, I claim our code is correct, and will close the ticket now.

If you can provide some example setups how to make our code fail, plus logs, I am happy to reopen and revisit.

comment:6 Changed 6 months ago by Gert Döring

Resolution: invalid
Status: closedreopened

Mmmh. Okay, it's a matter of interpretation what alen does, here - is this the "lenght of the name" or is "the length of the address field" (which could certainly include the length field, so, +1).

The way this could fail is "set alen to a value which is too small by 1, and then fail to read the buffer up to the end" in here

    while (len < 4 + alen + 2)
    {

it is inconsequential, as we never actually care for "do we have the full buffer or not" but still incorrect. Thanks for making me investigate.

comment:7 Changed 6 months ago by Gert Döring

Last edited 6 months ago by Gert Döring (previous) (diff)

comment:8 Changed 6 months ago by Gert Döring

Resolution: fixed
Status: reopenedclosed

commit eebeaa02367d247fc2549df3edf8e598c58c3572 (master)
commit c7f0d7b95bff05b0a5ddab15318cd53fcc91d60a (release/2.5)
commit 64a76533b676ad441ca20bab0c8b2e387bd56ebe (release/2.4)
Author: Gert Doering
Date: Wed Sep 9 14:22:23 2020 +0200

socks.c: fix alen for DOMAIN type addresses, bump up buffer sizes

Note: See TracTickets for help on using tickets.