Ticket #887: 0001-Fix-NCP-behaviour-on-TLS-reconnect.patch

File 0001-Fix-NCP-behaviour-on-TLS-reconnect.patch, 3.0 KB (added by Gert Döring, 5 years ago)

more minimal patch, as discussed

  • src/openvpn/push.c

    From a7515f6d894e3bc9d836dcb31d7e6b3ab1219201 Mon Sep 17 00:00:00 2001
    From: Gert Doering <gert@greenie.muc.de>
    Date: Thu, 18 May 2017 12:05:48 +0200
    Subject: [PATCH] Fix NCP behaviour on TLS reconnect.
    If a client reconnects on a soft-restart from the same port (due to --bind
    in use on the client), both sides will handle this as a "reconnect" and
    not a "full new connect" internally, re-using existing crypto context.
    The client will still ask the server for pushed options, and the server
    code to handle this refuses to do NCP if a key has already been negotiated
    (because there is no way to *change* the cipher after that) - which ends
    up in "the client uses the non-negotiated cipher from the config file,
    while the server uses the previously-negotiated NCP cipher", and nothing
    The easy workaround: if we find us in the situation that we think NCP
    has already been done, just re-push "cipher o->ciphername" with the
    current cipher for this client context.
    All credits for this go to Stefan Behrens <sbehrens@giantdisaster.de>
    who found and diagnosed the issue in trac #887, came up with a first
    patch to solve the issue quite similar to this (simplified) one, and
    helped testing.
    Trac: #887
     src/openvpn/push.c | 10 ++++++----
     1 file changed, 6 insertions(+), 4 deletions(-)
    diff --git a/src/openvpn/push.c b/src/openvpn/push.c
    index 8c3104e..bcef0ef 100644
    a b prepare_push_reply(struct context *c, struct gc_arena *gc, 
    372372    /* Push cipher if client supports Negotiable Crypto Parameters */
    373373    if (tls_peer_info_ncp_ver(peer_info) >= 2 && o->ncp_enabled)
    374374    {
    375         /* if we have already created our key, we cannot change our own
    376          * cipher, so disable NCP and warn = explain why
     375        /* if we have already created our key, we cannot *change* our own
     376         * cipher -> so log the fact and push the "what we have now" cipher
     377         * (so the client is always told what we expect it to use)
    377378         */
    378379        const struct tls_session *session = &tls_multi->session[TM_ACTIVE];
    379380        if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
    380381        {
    381382            msg( M_INFO, "PUSH: client wants to negotiate cipher (NCP), but "
    382383                 "server has already generated data channel keys, "
    383                  "ignoring client request" );
     384                 "re-sending previously negotiated cipher '%s'",
     385                 o->ciphername );
    384386        }
    385387        else
    386388        {
    prepare_push_reply(struct context *c, struct gc_arena *gc, 
    388390             * TODO: actual negotiation, instead of server dictatorship. */
    389391            char *push_cipher = string_alloc(o->ncp_ciphers, &o->gc);
    390392            o->ciphername = strtok(push_cipher, ":");
    391             push_option_fmt(gc, push_list, M_USAGE, "cipher %s", o->ciphername);
    392393        }
     394        push_option_fmt(gc, push_list, M_USAGE, "cipher %s", o->ciphername);
    393395    }
    394396    else if (o->ncp_enabled)
    395397    {