diff mbox series

[Openvpn-devel,v3] socks.c: fix alen for DOMAIN type addresses, bump up buffer sizes

Message ID 20200909122223.9222-1-gert@greenie.muc.de
State Accepted
Headers
Series [Openvpn-devel,v3] socks.c: fix alen for DOMAIN type addresses, bump up buffer sizes |

Commit Message

Gert Doering Sept. 9, 2020, 2:22 a.m. UTC 
  When a SOCKS5 server sends back a reply, it encodes an "address",
which can be IPv4 (4 bytes), IPv6 (16 bytes) or "a domain name",
which has a lenght (1 byte) and "a string of length <length>" - so
when copying bytes, we need to hande "length +1" bytes.

Our code totally doesn't use this variant of addresses on reception,
but since this has been pointed out by "tpw_rules" in Trac, fix it,
so if/when someone works on this again, the foundation is correct.

While at it, increase buffer size used for sending to handle domain
names longer than 122 characters (length was already checked, so a
longer name would not overflow but just "not work").

v2: increase buf[] len in recv_socks_reply() from 22 to 270 so it
    is large enough to actually copy a domain name

v3: increase buf[] len in establish_socks_proxy_passthru() from 128 to
    270, to handle long domain names in queries

Reported-By: tpw_rules in Trac
Trac: #848

Signed-off-by: Gert Doering <gert@greenie.muc.de>
---
 src/openvpn/socks.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Antonio Quartulli Sept. 13, 2020, 9:44 p.m. UTC | #1 
Hi,

On 09/09/2020 14:22, Gert Doering wrote:
> diff --git a/src/openvpn/socks.c b/src/openvpn/socks.c
> index 57f0cee2..d43d84a8 100644
> --- a/src/openvpn/socks.c
> +++ b/src/openvpn/socks.c
> @@ -312,7 +312,7 @@ recv_socks_reply(socket_descriptor_t sd,
>      char atyp = '\0';
>      int alen = 0;
>      int len = 0;
> -    char buf[22];
> +    char buf[270];		/* 4 + alen(max 256) + 2 */
>      const int timeout_sec = 5;
>  
>      if (addr != NULL)
> @@ -381,7 +381,10 @@ recv_socks_reply(socket_descriptor_t sd,
>                      break;
>  
>                  case '\x03':    /* DOMAINNAME */
> -                    alen = (unsigned char) c;
> +                    /* RFC 1928, section 5: 1 byte length, <n> bytes name,
> +                     * so the total "address length" is (length+1)
> +                     */
> +                    alen = (unsigned char) c +1;

since you are touching this line...how about making it straight?
1) why casting to unsigned char? "alen" is int.
2) please add a space after the '+' operator.

Regards,
Antonio Quartulli Sept. 13, 2020, 11:04 p.m. UTC | #2 
Hi,

On 09/09/2020 14:22, Gert Doering wrote:
> When a SOCKS5 server sends back a reply, it encodes an "address",
> which can be IPv4 (4 bytes), IPv6 (16 bytes) or "a domain name",
> which has a lenght (1 byte) and "a string of length <length>" - so
> when copying bytes, we need to hande "length +1" bytes.
> 
> Our code totally doesn't use this variant of addresses on reception,
> but since this has been pointed out by "tpw_rules" in Trac, fix it,
> so if/when someone works on this again, the foundation is correct.
> 
> While at it, increase buffer size used for sending to handle domain
> names longer than 122 characters (length was already checked, so a
> longer name would not overflow but just "not work").
> 
> v2: increase buf[] len in recv_socks_reply() from 22 to 270 so it
>     is large enough to actually copy a domain name
> 
> v3: increase buf[] len in establish_socks_proxy_passthru() from 128 to
>     270, to handle long domain names in queries
> 
> Reported-By: tpw_rules in Trac
> Trac: #848
> 
> Signed-off-by: Gert Doering <gert@greenie.muc.de>

After a quick discussion on IRC I am fine with this patch, assuming the
whitespace is added after the '+' operator.

Further refactoring of this code will be carried on in later patches.

Regards,
Antonio Quartulli Sept. 13, 2020, 11:27 p.m. UTC | #3 
Hi,

On 14/09/2020 11:04, Antonio Quartulli wrote:
> Hi,
> 
> On 09/09/2020 14:22, Gert Doering wrote:
>> When a SOCKS5 server sends back a reply, it encodes an "address",
>> which can be IPv4 (4 bytes), IPv6 (16 bytes) or "a domain name",
>> which has a lenght (1 byte) and "a string of length <length>" - so
>> when copying bytes, we need to hande "length +1" bytes.
>>
>> Our code totally doesn't use this variant of addresses on reception,
>> but since this has been pointed out by "tpw_rules" in Trac, fix it,
>> so if/when someone works on this again, the foundation is correct.
>>
>> While at it, increase buffer size used for sending to handle domain
>> names longer than 122 characters (length was already checked, so a
>> longer name would not overflow but just "not work").
>>
>> v2: increase buf[] len in recv_socks_reply() from 22 to 270 so it
>>     is large enough to actually copy a domain name
>>
>> v3: increase buf[] len in establish_socks_proxy_passthru() from 128 to
>>     270, to handle long domain names in queries
>>
>> Reported-By: tpw_rules in Trac
>> Trac: #848
>>
>> Signed-off-by: Gert Doering <gert@greenie.muc.de>
> 
> After a quick discussion on IRC I am fine with this patch, assuming the
> whitespace is added after the '+' operator.
> 
> Further refactoring of this code will be carried on in later patches.

forgot to add:

Acked-by: Antonio Quartulli <a@unstable.cc>
Gert Doering Sept. 13, 2020, 11:42 p.m. UTC | #4 
Thanks for the review.

Patch has been applied to many branches (bugfix)...

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

     Signed-off-by: Gert Doering <gert@greenie.muc.de>
     Acked-by: Antonio Quartulli <a@unstable.cc>
     Message-Id: <20200909122223.9222-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20928.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/socks.c b/src/openvpn/socks.c
index 57f0cee2..d43d84a8 100644
--- a/src/openvpn/socks.c
+++ b/src/openvpn/socks.c
@@ -312,7 +312,7 @@  recv_socks_reply(socket_descriptor_t sd,
     char atyp = '\0';
     int alen = 0;
     int len = 0;
-    char buf[22];
+    char buf[270];		/* 4 + alen(max 256) + 2 */
     const int timeout_sec = 5;
 
     if (addr != NULL)
@@ -381,7 +381,10 @@  recv_socks_reply(socket_descriptor_t sd,
                     break;
 
                 case '\x03':    /* DOMAINNAME */
-                    alen = (unsigned char) c;
+                    /* RFC 1928, section 5: 1 byte length, <n> bytes name,
+                     * so the total "address length" is (length+1)
+                     */
+                    alen = (unsigned char) c +1;
                     break;
 
                 case '\x04':    /* IP V6 */
@@ -451,7 +454,7 @@  establish_socks_proxy_passthru(struct socks_proxy_info *p,
                                const char *servname,    /* openvpn server port */
                                volatile int *signal_received)
 {
-    char buf[128];
+    char buf[270];
     size_t len;
 
     if (!socks_handshake(p, sd, signal_received))