| Message ID | 20200909122223.9222-1-gert@greenie.muc.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v3] socks.c: fix alen for DOMAIN type addresses, bump up buffer sizes | expand |
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,
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,
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>
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 --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))
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(-)