| Message ID | 20201130123813.21388-1-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v2] Fix port-share option with TLS-Crypt v2 | expand |
Hi, On 30-11-2020 13:38, Arne Schwabe wrote: > The port-share option assumed that all openvpn initial reset packets > are between 14 and 255 bytes long. This is not true for tls-crypt-v2. > > Patch V2: use correct length for TLS-Crypt v2, use length variable > non-tlscryptv2 test > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> > --- > src/openvpn/ps.c | 34 +++++++++++++++++++++++++++++----- > 1 file changed, 29 insertions(+), 5 deletions(-) > > diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c > index 2089e6b9..5d760782 100644 > --- a/src/openvpn/ps.c > +++ b/src/openvpn/ps.c > @@ -983,14 +983,38 @@ is_openvpn_protocol(const struct buffer *buf) > const int len = BLEN(buf); > if (len >= 3) > { > - return p[0] == 0 > - && p[1] >= 14 > - && (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V2 << P_OPCODE_SHIFT) > - || p[2] == (P_CONTROL_HARD_RESET_CLIENT_V3 << P_OPCODE_SHIFT)); > + int plen = (p[0] << 8) | p[1]; > + > + if (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V3 << P_OPCODE_SHIFT)) > + { > + /* WKc is at least 290 byte (not including metadata): > + * > + * 16 bit len + 256 bit HMAC + 2048 bit Kc = 2320 bit > + * > + * This is increased by the normal length of client handshake + > + * tls-crypt overhead (32) > + * > + * For metadata tls-crypt-v2.txt does not explicitly specify > + * an upper limit but we also have TLS_CRYPT_V2_MAX_WKC_LEN > + * as 1024 bytes. We err on the safe side with 255 extra overhead > + * > + * We don't do the 2 byte check for tls-crypt-v2 because it is very > + * unrealistic to have only 2 bytes available. > + */ > + return (plen >= 336 && plen < (1024 + 255)); > + } > + else > + { > + /* For non tls-crypt2 we assume the packet length to valid between > + * 14 and 255 */ > + return plen >= 14 && plen <= 255 > + && (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V2 << P_OPCODE_SHIFT)); > + } > } > else if (len >= 2) > { > - return p[0] == 0 && p[1] >= 14; > + int plen = (p[0] << 8) | p[1]; > + return plen >= 14 && plen <= 255; > } > else > { > Thanks. This looks good to me now. Ran some tests to check that this correctly resolves the issue when using --tls-crypt-v2 in combination with --port-share. Acked-by: Steffan Karger <steffan@karger.me> -Steffan
Magic heuristics... but if Arne and Steffan agree that this is correct,
who am I to dispute it :-) - it passes client tests and is not
"obviously wrong".
Your patch has been applied to the master and release/2.5 branch.
commit 1387f52682dcd3789c56c9979ccedca281ff88f4 (master)
commit c27f97dc0ac99344659cca57bd048543a9899266 (release/2.5)
Author: Arne Schwabe
Date: Mon Nov 30 13:38:13 2020 +0100
Fix port-share option with TLS-Crypt v2
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Steffan Karger <steffan.karger@foxcrypto.com>
Message-Id: <20201130123813.21388-1-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21290.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c index 2089e6b9..5d760782 100644 --- a/src/openvpn/ps.c +++ b/src/openvpn/ps.c @@ -983,14 +983,38 @@ is_openvpn_protocol(const struct buffer *buf) const int len = BLEN(buf); if (len >= 3) { - return p[0] == 0 - && p[1] >= 14 - && (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V2 << P_OPCODE_SHIFT) - || p[2] == (P_CONTROL_HARD_RESET_CLIENT_V3 << P_OPCODE_SHIFT)); + int plen = (p[0] << 8) | p[1]; + + if (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V3 << P_OPCODE_SHIFT)) + { + /* WKc is at least 290 byte (not including metadata): + * + * 16 bit len + 256 bit HMAC + 2048 bit Kc = 2320 bit + * + * This is increased by the normal length of client handshake + + * tls-crypt overhead (32) + * + * For metadata tls-crypt-v2.txt does not explicitly specify + * an upper limit but we also have TLS_CRYPT_V2_MAX_WKC_LEN + * as 1024 bytes. We err on the safe side with 255 extra overhead + * + * We don't do the 2 byte check for tls-crypt-v2 because it is very + * unrealistic to have only 2 bytes available. + */ + return (plen >= 336 && plen < (1024 + 255)); + } + else + { + /* For non tls-crypt2 we assume the packet length to valid between + * 14 and 255 */ + return plen >= 14 && plen <= 255 + && (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V2 << P_OPCODE_SHIFT)); + } } else if (len >= 2) { - return p[0] == 0 && p[1] >= 14; + int plen = (p[0] << 8) | p[1]; + return plen >= 14 && plen <= 255; } else {
The port-share option assumed that all openvpn initial reset packets are between 14 and 255 bytes long. This is not true for tls-crypt-v2. Patch V2: use correct length for TLS-Crypt v2, use length variable non-tlscryptv2 test Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/ps.c | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-)