Re: [Openvpn-devel] [PATCH] Fix broken fragment/mssfix with NCP
Hi,
On 12-11-18 15:16, Lev Stipakov wrote:
> From: Lev Stipakov
>
> NCP negotiation replaces worst cast crypto overhead
> with actual one in data channel frame. That frame
> params are used by mssfix.
>
> Fragment frame still contains worst case overhead.
> Because of that TCP packets are fragmented, since
> MSS value exceeds max fragment size.
>
> Fix by replacing worst case crypto overhead with
> actual one for fragment frame, as it is done for data
> channel frame.
>
> Trac #1140
Thanks for digging into this!
> Signed-off-by: Lev Stipakov
> ---
> src/openvpn/forward.c | 1 +
> src/openvpn/init.c| 12 +++-
> src/openvpn/openvpn.h | 1 +
> src/openvpn/push.c| 9 -
> src/openvpn/ssl.c | 19 ++-
> src/openvpn/ssl.h | 13 -
> 6 files changed, 47 insertions(+), 8 deletions(-)
>
> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index f8faa81..773f5d7 100644
> --- a/src/openvpn/forward.c
> +++ b/src/openvpn/forward.c
> @@ -1090,6 +1090,7 @@ process_incoming_link_part1(struct context *c, struct
> link_socket_info *lsi, boo
> if (is_hard_reset(opcode, c->options.key_method))
> {
> c->c2.frame = c->c2.frame_initial;
> +c->c2.frame_fragment = c->c2.frame_fragment_initial;
This breaks --disable-fragment builds:
src/openvpn/forward.c:1096:27: error: no member named 'frame_fragment'
in 'struct context_2'
c->c2.frame_fragment = c->c2.frame_fragment_initial;
~ ^
src/openvpn/forward.c:1096:50: error: no member named
'frame_fragment_initial' in 'struct context_2'
c->c2.frame_fragment = c->c2.frame_fragment_initial;
~ ^
2 errors generated.
Makefile:676: recipe for target 'forward.o' failed
> }
>
> interval_action(>c2.tmp_int);
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 39e8ca5..db96353 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -2300,9 +2300,18 @@ do_deferred_options(struct context *c, const unsigned
> int found)
> {
> tls_poor_mans_ncp(>options,
> c->c2.tls_multi->remote_ciphername);
> }
> +struct frame *frame_fragment = NULL;
> +#ifdef ENABLE_FRAGMENT
> +if (c->options.ce.fragment)
> +{
> +frame_fragment = >c2.frame_fragment;
> +}
> +#endif
> +
> /* Do not regenerate keys if server sends an extra push reply */
> if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized
> -&& !tls_session_update_crypto_params(session, >options,
> >c2.frame))
> +&& !tls_session_update_crypto_params(session, >options,
> >c2.frame,
> + frame_fragment))
> {
> msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto
> options");
> return false;
> @@ -3082,6 +3091,7 @@ do_init_frame(struct context *c)
> */
> c->c2.frame_fragment = c->c2.frame;
> frame_subtract_extra(>c2.frame_fragment, >c2.frame_fragment_omit);
> +c->c2.frame_fragment_initial = c->c2.frame_fragment;
> #endif
>
> #if defined(ENABLE_FRAGMENT) && defined(ENABLE_OCC)
> diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
> index d11f61d..a5d250f 100644
> --- a/src/openvpn/openvpn.h
> +++ b/src/openvpn/openvpn.h
> @@ -265,6 +265,7 @@ struct context_2
> /* Object to handle advanced MTU negotiation and datagram fragmentation
> */
> struct fragment_master *fragment;
> struct frame frame_fragment;
> +struct frame frame_fragment_initial;
> struct frame frame_fragment_omit;
> #endif
>
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 72f0996..a44646a 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -274,11 +274,18 @@ incoming_push_message(struct context *c, const struct
> buffer *buffer)
> {
> if (c->options.mode == MODE_SERVER)
> {
> +struct frame *frame_fragment = NULL;
> +#ifdef ENABLE_FRAGMENT
> +if (c->options.ce.fragment)
> +{
> +frame_fragment = >c2.frame_fragment;
> +}
> +#endif
> struct tls_session *session =
> >c2.tls_multi->session[TM_ACTIVE];
> /* Do not regenerate keys if client send a second push request */
> if
> (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized
> && !tls_session_update_crypto_params(session, >options,
> - >c2.frame))
> + >c2.frame,
> frame_fragment))
> {
> msg(D_TLS_ERRORS, "TLS Error: initializing data channel
> failed");
> goto error;
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index
[Openvpn-devel] [PATCH] Fix broken fragment/mssfix with NCP
From: Lev Stipakov
NCP negotiation replaces worst cast crypto overhead
with actual one in data channel frame. That frame
params are used by mssfix.
Fragment frame still contains worst case overhead.
Because of that TCP packets are fragmented, since
MSS value exceeds max fragment size.
Fix by replacing worst case crypto overhead with
actual one for fragment frame, as it is done for data
channel frame.
Trac #1140
Signed-off-by: Lev Stipakov
---
src/openvpn/forward.c | 1 +
src/openvpn/init.c| 12 +++-
src/openvpn/openvpn.h | 1 +
src/openvpn/push.c| 9 -
src/openvpn/ssl.c | 19 ++-
src/openvpn/ssl.h | 13 -
6 files changed, 47 insertions(+), 8 deletions(-)
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index f8faa81..773f5d7 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1090,6 +1090,7 @@ process_incoming_link_part1(struct context *c, struct
link_socket_info *lsi, boo
if (is_hard_reset(opcode, c->options.key_method))
{
c->c2.frame = c->c2.frame_initial;
+c->c2.frame_fragment = c->c2.frame_fragment_initial;
}
interval_action(>c2.tmp_int);
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 39e8ca5..db96353 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2300,9 +2300,18 @@ do_deferred_options(struct context *c, const unsigned
int found)
{
tls_poor_mans_ncp(>options, c->c2.tls_multi->remote_ciphername);
}
+struct frame *frame_fragment = NULL;
+#ifdef ENABLE_FRAGMENT
+if (c->options.ce.fragment)
+{
+frame_fragment = >c2.frame_fragment;
+}
+#endif
+
/* Do not regenerate keys if server sends an extra push reply */
if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized
-&& !tls_session_update_crypto_params(session, >options,
>c2.frame))
+&& !tls_session_update_crypto_params(session, >options,
>c2.frame,
+ frame_fragment))
{
msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto
options");
return false;
@@ -3082,6 +3091,7 @@ do_init_frame(struct context *c)
*/
c->c2.frame_fragment = c->c2.frame;
frame_subtract_extra(>c2.frame_fragment, >c2.frame_fragment_omit);
+c->c2.frame_fragment_initial = c->c2.frame_fragment;
#endif
#if defined(ENABLE_FRAGMENT) && defined(ENABLE_OCC)
diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
index d11f61d..a5d250f 100644
--- a/src/openvpn/openvpn.h
+++ b/src/openvpn/openvpn.h
@@ -265,6 +265,7 @@ struct context_2
/* Object to handle advanced MTU negotiation and datagram fragmentation */
struct fragment_master *fragment;
struct frame frame_fragment;
+struct frame frame_fragment_initial;
struct frame frame_fragment_omit;
#endif
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 72f0996..a44646a 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -274,11 +274,18 @@ incoming_push_message(struct context *c, const struct
buffer *buffer)
{
if (c->options.mode == MODE_SERVER)
{
+struct frame *frame_fragment = NULL;
+#ifdef ENABLE_FRAGMENT
+if (c->options.ce.fragment)
+{
+frame_fragment = >c2.frame_fragment;
+}
+#endif
struct tls_session *session = >c2.tls_multi->session[TM_ACTIVE];
/* Do not regenerate keys if client send a second push request */
if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized
&& !tls_session_update_crypto_params(session, >options,
- >c2.frame))
+ >c2.frame,
frame_fragment))
{
msg(D_TLS_ERRORS, "TLS Error: initializing data channel
failed");
goto error;
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 74b88ce..8942f71 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1986,7 +1986,8 @@ cleanup:
bool
tls_session_update_crypto_params(struct tls_session *session,
- struct options *options, struct frame *frame)
+ struct options *options, struct frame *frame,
+ struct frame *frame_fragment)
{
if (!session->opt->server
&& 0 != strcmp(options->ciphername, session->opt->config_ciphername)
@@ -2030,6 +2031,22 @@ tls_session_update_crypto_params(struct tls_session
*session,
frame_init_mssfix(frame, options);
frame_print(frame, D_MTU_INFO, "Data Channel MTU parms");
+/*
+ * mssfix uses data channel framing, which at this point contains
+ * actual overhead. Fragmentation logic uses frame_fragment, which
