Browse code

Rework NCP compability logic and drop BF-CBC support by default

This reworks the NCP logic to be more strict about what is
considered an acceptable result of an NCP negotiation. It also
allows us to finally drop support for BF-CBC as default cipher.

All new behaviour is currently limited to server/client
mode with pull enabled. P2p mode without pull does not change.

New Server behaviour:
- when a client announces its supported ciphers through either
OCC or IV_CIPHER/IV_NCP we reject the client with a
AUTH_FAILED message if we have no common cipher.

- When a client does not announce any cipher in either
OCC or NCP we reject it unless data-ciphers-fallback is
specified in either ccd/ or config.

New client behaviour:
- When no cipher is pushed (or a cipher we refused to support)
and we also cannot support the server's cipher announced in
OCC we fail the connection and log why

- If there is no cipher in OCC but data-ciphers-fallback is
specified we will use the fallback cipher instead of failing the
connection

Both client and server behaviour:
- We only announce --cipher xyz in occ if we are willing
to support that cipher (always announce the cipher if
NCP is disabled or not in --client mode)

It means that we only announce the fallback-cipher if
it is also contained in --data-ciphers

Compatibility behaviour:

In 2.5 both client and server will use a --cipher xyz present
in the config to automatically set --data-ciphers-fallback xyz
and also append this cipher to the end of data-ciphers.

We log a warning about this and point to --data-ciphers and
--data-ciphers-fallback This also happens if the configuration
contains an explicit --cipher BF-CBC.

If --cipher is not set, we only warn that previous versions
allowed BF-CBC and point out how to re-enable BF-CBC. This will
break configs where someone connects a 2.3 client (or older)
to a 2.5 server AND has no explicit --cipher setting in the
server config. We still do it, because at some point we need
to drop the BF-CBC default - and affected users already had the
scary SWEET32 warning in their logs for a long time.

In short: If --cipher is explicitly set then 2.5 will work the
same as 2.4 did. When --cipher is not set, BF-CBC support is
dropped and we warn about it.

Examples how breaking the default BF-CBC will be logged:

Client side:
- Client connecting to server that does not push cipher but
has --cipher in OCC

OPTIONS ERROR: failed to negotiate cipher with server. Add the
server's cipher ('BF-CBC') to --data-ciphers (currently
'AES-256-GCM:AES-128-CBC') if you want to connect to this server.

- Client connecting to a server that does not support OCC:

OPTIONS ERROR: failed to negotiate cipher with server. Configure
--data-ciphers-fallback if you want connect to this server.

Server Side:
- Server has a client only supporting BF-CBC connecting:

styx/IP PUSH: No common cipher between server and client. Server
data-ciphers: 'CHACHA20-POLY1305:AES-128-GCM:AES-256-GCM:AES-256-CBC:AES-128-CBC', client supports cipher 'BF-CBC'.

- Client without OCC:

styx/IP PUSH:No NCP or OCC cipher data received from peer.
styx/IP Use --data-ciphers-fallback with the cipher the client is using
if you want to allow the client to connect

In all cases the client is rejected with this message:

AUTH: Received control message: AUTH_FAILED,Data channel cipher
negotiation failed (no shared cipher)

Signed-off-by: Arne Schwabe <arne@rfc2549.org>

Patch V2: rename fallback-cipher to data-ciphers-fallback
add all corrections from Steffan
Ignore occ cipher for clients sending IV_CIPHERS
move client side ncp in its own function
do not print INSECURE cipher warning if BF-CBC is not allowed

Patch V3: fix minor style, add null check when client sends no peerinfo at
all

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20200809141922.7853-1-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20656.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

Arne Schwabe authored on 2020/08/09 23:19:21
Showing 10 changed files
... ...
@@ -57,6 +57,9 @@ configured in a compatible way between both the local and remote side.
57 57
   http://www.cs.ucsd.edu/users/mihir/papers/hmac.html
58 58
 
59 59
 --cipher alg
60
+  This option is deprecated for server-client mode. ``--data-ciphers``
61
+  or possibly `--data-ciphers-fallback`` should be used instead.
62
+
60 63
   Encrypt data channel packets with cipher algorithm ``alg``.
61 64
 
62 65
   The default is :code:`BF-CBC`, an abbreviation for Blowfish in Cipher
... ...
@@ -183,8 +186,9 @@ configured in a compatible way between both the local and remote side.
183 183
   ``--server`` ), or if ``--pull`` is specified (client-side, implied by
184 184
   setting --client).
185 185
 
186
-  If both peers support and do not disable NCP, the negotiated cipher will
187
-  override the cipher specified by ``--cipher``.
186
+  If no common cipher is found during cipher negotiation, the connection
187
+  is terminated. To support old clients/old servers that do not provide any
188
+  cipher negotiation support see ``--data-ciphers-fallback``.
188 189
 
189 190
   Additionally, to allow for more smooth transition, if NCP is enabled,
190 191
   OpenVPN will inherit the cipher of the peer if that cipher is different
... ...
@@ -201,8 +205,18 @@ configured in a compatible way between both the local and remote side.
201 201
   This list is restricted to be 127 chars long after conversion to OpenVPN
202 202
   ciphers.
203 203
 
204
-  This option was called ``ncp-ciphers`` in OpenVPN 2.4 but has been renamed
205
-  to ``data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
204
+  This option was called ``--ncp-ciphers`` in OpenVPN 2.4 but has been renamed
205
+  to ``--data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
206
+
207
+--data-ciphers-fallback alg
208
+
209
+    Configure a cipher that is used to fall back to if we could not determine
210
+    which cipher the peer is willing to use.
211
+
212
+    This option should only be needed to
213
+    connect to peers that are running OpenVPN 2.3 and older version, and
214
+    have been configured with `--enable-small`
215
+    (typically used on routers or other embedded devices).
206 216
 
207 217
 --ncp-disable
208 218
   Disable "Negotiable Crypto Parameters". This completely disables cipher
... ...
@@ -727,7 +727,9 @@ warn_insecure_key_type(const char *ciphername, const cipher_kt_t *cipher)
727 727
     {
728 728
         msg(M_WARN, "WARNING: INSECURE cipher (%s) with block size less than 128"
729 729
             " bit (%d bit).  This allows attacks like SWEET32.  Mitigate by "
730
-            "using a --cipher with a larger block size (e.g. AES-256-CBC).",
730
+            "using a --cipher with a larger block size (e.g. AES-256-CBC). "
731
+            "Support for these insecure ciphers will be removed in "
732
+            "OpenVPN 2.6.",
731 733
             ciphername, cipher_kt_block_size(cipher)*8);
732 734
     }
733 735
 }
... ...
@@ -2365,16 +2365,9 @@ do_deferred_options(struct context *c, const unsigned int found)
2365 2365
     /* process (potentially pushed) crypto options */
2366 2366
     if (c->options.pull)
2367 2367
     {
2368
-        struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
2369
-        if (found & OPT_P_NCP)
2370
-        {
2371
-            msg(D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
2372
-        }
2373
-        else if (c->options.ncp_enabled)
2368
+        if (!check_pull_client_ncp(c, found))
2374 2369
         {
2375
-            /* If the server did not push a --cipher, we will switch to the
2376
-             * remote cipher if it is in our ncp-ciphers list */
2377
-            tls_poor_mans_ncp(&c->options, c->c2.tls_multi->remote_ciphername);
2370
+            return false;
2378 2371
         }
2379 2372
         struct frame *frame_fragment = NULL;
2380 2373
 #ifdef ENABLE_FRAGMENT
... ...
@@ -2384,6 +2377,7 @@ do_deferred_options(struct context *c, const unsigned int found)
2384 2384
         }
2385 2385
 #endif
2386 2386
 
2387
+        struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
2387 2388
         if (!tls_session_update_crypto_params(session, &c->options, &c->c2.frame,
2388 2389
                                               frame_fragment))
2389 2390
         {
... ...
@@ -2757,9 +2751,13 @@ do_init_crypto_tls_c1(struct context *c)
2757 2757
 #endif /* if P2MP */
2758 2758
         }
2759 2759
 
2760
+        /* Do not warn if we only have BF-CBC in options->ciphername
2761
+         * because it is still the default cipher */
2762
+        bool warn = !streq(options->ciphername, "BF-CBC")
2763
+             || options->enable_ncp_fallback;
2760 2764
         /* Get cipher & hash algorithms */
2761 2765
         init_key_type(&c->c1.ks.key_type, options->ciphername, options->authname,
2762
-                      options->keysize, true, true);
2766
+                      options->keysize, true, warn);
2763 2767
 
2764 2768
         /* Initialize PRNG with config-specified digest */
2765 2769
         prng_init(options->prng_hash, options->prng_nonce_secret_len);
... ...
@@ -1777,7 +1777,7 @@ multi_client_connect_setenv(struct multi_context *m,
1777 1777
  * - choosen cipher
1778 1778
  * - peer id
1779 1779
  */
1780
-static void
1780
+static bool
1781 1781
 multi_client_set_protocol_options(struct context *c)
1782 1782
 {
1783 1783
 
... ...
@@ -1807,56 +1807,85 @@ multi_client_set_protocol_options(struct context *c)
1807 1807
     }
1808 1808
 
1809 1809
     /* Select cipher if client supports Negotiable Crypto Parameters */
1810
-    if (o->ncp_enabled)
1810
+    if (!o->ncp_enabled)
1811 1811
     {
1812
-        /* if we have already created our key, we cannot *change* our own
1813
-         * cipher -> so log the fact and push the "what we have now" cipher
1814
-         * (so the client is always told what we expect it to use)
1815
-         */
1816
-        const struct tls_session *session = &tls_multi->session[TM_ACTIVE];
1817
-        if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
1812
+        return true;
1813
+    }
1814
+
1815
+    /* if we have already created our key, we cannot *change* our own
1816
+     * cipher -> so log the fact and push the "what we have now" cipher
1817
+     * (so the client is always told what we expect it to use)
1818
+     */
1819
+    const struct tls_session *session = &tls_multi->session[TM_ACTIVE];
1820
+    if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
1821
+    {
1822
+        msg(M_INFO, "PUSH: client wants to negotiate cipher (NCP), but "
1823
+            "server has already generated data channel keys, "
1824
+            "re-sending previously negotiated cipher '%s'",
1825
+            o->ciphername );
1826
+        return true;
1827
+    }
1828
+
1829
+    /*
1830
+     * Push the first cipher from --data-ciphers to the client that
1831
+     * the client announces to be supporting.
1832
+     */
1833
+    char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, peer_info,
1834
+                                            tls_multi->remote_ciphername,
1835
+                                            &o->gc);
1836
+
1837
+    if (push_cipher)
1838
+    {
1839
+        o->ciphername = push_cipher;
1840
+        return true;
1841
+    }
1842
+
1843
+    /* NCP cipher negotiation failed. Try to figure out why exactly it
1844
+     * failed and give good error messages and potentially do a fallback
1845
+     * for non NCP clients */
1846
+    struct gc_arena gc = gc_new();
1847
+    bool ret = false;
1848
+
1849
+    const char *peer_ciphers = tls_peer_ncp_list(peer_info, &gc);
1850
+    /* If we are in a situation where we know the client ciphers, there is no
1851
+     * reason to fall back to a cipher that will not be accepted by the other
1852
+     * side, in this situation we fail the auth*/
1853
+    if (strlen(peer_ciphers) > 0)
1854
+    {
1855
+        msg(M_INFO, "PUSH: No common cipher between server and client. "
1856
+            "Server data-ciphers: '%s', client supported ciphers '%s'",
1857
+            o->ncp_ciphers, peer_ciphers);
1858
+    }
1859
+    else if (tls_multi->remote_ciphername)
1860
+    {
1861
+        msg(M_INFO, "PUSH: No common cipher between server and client. "
1862
+            "Server data-ciphers: '%s', client supports cipher '%s'",
1863
+            o->ncp_ciphers, tls_multi->remote_ciphername);
1864
+    }
1865
+    else
1866
+    {
1867
+        msg(M_INFO, "PUSH: No NCP or OCC cipher data received from peer.");
1868
+
1869
+        if (o->enable_ncp_fallback && !tls_multi->remote_ciphername)
1818 1870
         {
1819
-            msg( M_INFO, "PUSH: client wants to negotiate cipher (NCP), but "
1820
-                 "server has already generated data channel keys, "
1821
-                 "re-sending previously negotiated cipher '%s'",
1822
-                 o->ciphername );
1871
+            msg(M_INFO, "Using data channel cipher '%s' since "
1872
+                "--data-ciphers-fallback is set.", o->ciphername);
1873
+            ret = true;
1823 1874
         }
1824 1875
         else
1825 1876
         {
1826
-            /*
1827
-             * Push the first cipher from --data-ciphers to the client that
1828
-             * the client announces to be supporting.
1829
-             */
1830
-            char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, o->ciphername,
1831
-                                                    peer_info,
1832
-                                                    tls_multi->remote_ciphername,
1833
-                                                    &o->gc);
1834
-
1835
-            if (push_cipher)
1836
-            {
1837
-                o->ciphername = push_cipher;
1838
-            }
1839
-            else
1840
-            {
1841
-                struct gc_arena gc = gc_new();
1842
-                const char *peer_ciphers = tls_peer_ncp_list(peer_info, &gc);
1843
-                if (strlen(peer_ciphers) > 0)
1844
-                {
1845
-                    msg(M_INFO, "PUSH: No common cipher between server and "
1846
-                        "client. Expect this connection not to work. Server "
1847
-                        "data-ciphers: '%s', client supported ciphers '%s'",
1848
-                        o->ncp_ciphers, peer_ciphers);
1849
-                }
1850
-                else
1851
-                {
1852
-                    msg(M_INFO, "No NCP data received from peer, falling back "
1853
-                        "to --cipher '%s'. Peer reports in OCC --cipher '%s'",
1854
-                        o->ciphername, np(tls_multi->remote_ciphername));
1855
-                }
1856
-                gc_free(&gc);
1857
-            }
1877
+            msg(M_INFO, "Use --data-ciphers-fallback with the cipher the "
1878
+                "client is using if you want to allow the client to connect");
1858 1879
         }
1859 1880
     }
1881
+    if (!ret)
1882
+    {
1883
+        auth_set_client_reason(tls_multi, "Data channel cipher negotiation "
1884
+                                          "failed (no shared cipher)");
1885
+    }
1886
+
1887
+    gc_free(&gc);
1888
+    return ret;
1860 1889
 }
1861 1890
 
1862 1891
 /**
... ...
@@ -2399,10 +2428,13 @@ multi_client_connect_late_setup(struct multi_context *m,
2399 2399
     mi->context.c2.context_auth = CAS_SUCCEEDED;
2400 2400
 
2401 2401
     /* authentication complete, calculate dynamic client specific options */
2402
-    multi_client_set_protocol_options(&mi->context);
2403
-
2404
-    /* Generate data channel keys */
2405
-    if (!multi_client_generate_tls_keys(&mi->context))
2402
+    if (!multi_client_set_protocol_options(&mi->context))
2403
+    {
2404
+        mi->context.c2.context_auth = CAS_FAILED;
2405
+    }
2406
+    /* Generate data channel keys only if setting protocol options
2407
+     * has not failed */
2408
+    else if (!multi_client_generate_tls_keys(&mi->context))
2406 2409
     {
2407 2410
         mi->context.c2.context_auth = CAS_FAILED;
2408 2411
     }
... ...
@@ -855,7 +855,6 @@ init_options(struct options *o, const bool init_gc)
855 855
 #if P2MP
856 856
     o->scheduled_exit_interval = 5;
857 857
 #endif
858
-    o->ciphername = "BF-CBC";
859 858
     o->ncp_enabled = true;
860 859
     o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
861 860
     o->authname = "SHA1";
... ...
@@ -2053,7 +2052,7 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
2053 2053
     if (options->inetd)
2054 2054
     {
2055 2055
         msg(M_WARN, "DEPRECATED OPTION: --inetd mode is deprecated "
2056
-                    "and will be removed in OpenVPN 2.6");
2056
+            "and will be removed in OpenVPN 2.6");
2057 2057
     }
2058 2058
 
2059 2059
     if (options->lladdr && dev != DEV_TYPE_TAP)
... ...
@@ -3047,6 +3046,67 @@ options_postprocess_verify(const struct options *o)
3047 3047
 }
3048 3048
 
3049 3049
 static void
3050
+options_postprocess_cipher(struct options *o)
3051
+{
3052
+    if (!o->pull && !(o->mode == MODE_SERVER))
3053
+    {
3054
+        /* we are in the classic P2P mode */
3055
+        o->ncp_enabled = false;
3056
+        msg( M_WARN, "Cipher negotiation is disabled since neither "
3057
+             "P2MP client nor server mode is enabled");
3058
+
3059
+        /* If the cipher is not set, use the old default of BF-CBC. We will
3060
+         * warn that this is deprecated on cipher initialisation, no need
3061
+         * to warn here as well */
3062
+        if (!o->ciphername)
3063
+        {
3064
+            o->ciphername = "BF-CBC";
3065
+        }
3066
+        return;
3067
+    }
3068
+
3069
+    /* pull or P2MP mode */
3070
+    if (!o->ciphername)
3071
+    {
3072
+        if (!o->ncp_enabled)
3073
+        {
3074
+            msg(M_USAGE, "--ncp-disable needs an explicit --cipher or "
3075
+                         "--data-ciphers-fallback config option");
3076
+        }
3077
+
3078
+        msg(M_WARN, "--cipher is not set. Previous OpenVPN version defaulted to "
3079
+            "BF-CBC as fallback when cipher negotiation failed in this case. "
3080
+            "If you need this fallback please add '--data-ciphers-fallback "
3081
+            "BF-CBC' to your configuration and/or add BF-CBC to "
3082
+            "--data-ciphers.");
3083
+
3084
+        /* We still need to set the ciphername to BF-CBC since various other
3085
+         * parts of OpenVPN assert that the ciphername is set */
3086
+        o->ciphername = "BF-CBC";
3087
+    }
3088
+    else if (!o->enable_ncp_fallback
3089
+             && !tls_item_in_cipher_list(o->ciphername, o->ncp_ciphers))
3090
+    {
3091
+        msg(M_WARN, "DEPRECATED OPTION: --cipher set to '%s' but missing in"
3092
+            " --data-ciphers (%s). Future OpenVPN version will "
3093
+            "ignore --cipher for cipher negotiations. "
3094
+            "Add '%s' to --data-ciphers or change --cipher '%s' to "
3095
+            "--data-ciphers-fallback '%s' to silence this warning.",
3096
+            o->ciphername, o->ncp_ciphers, o->ciphername,
3097
+            o->ciphername, o->ciphername);
3098
+        o->enable_ncp_fallback = true;
3099
+
3100
+        /* Append the --cipher to ncp_ciphers to allow it in NCP */
3101
+        size_t newlen = strlen(o->ncp_ciphers) + 1 + strlen(o->ciphername) + 1;
3102
+        char *ncp_ciphers = gc_malloc(newlen, false, &o->gc);
3103
+
3104
+        ASSERT(openvpn_snprintf(ncp_ciphers, newlen, "%s:%s", o->ncp_ciphers,
3105
+                                o->ciphername));
3106
+        o->ncp_ciphers = ncp_ciphers;
3107
+    }
3108
+}
3109
+
3110
+static void
3050 3111
 options_postprocess_mutate(struct options *o)
3051 3112
 {
3052 3113
     int i;
... ...
@@ -3058,6 +3118,7 @@ options_postprocess_mutate(struct options *o)
3058 3058
     helper_keepalive(o);
3059 3059
     helper_tcp_nodelay(o);
3060 3060
 
3061
+    options_postprocess_cipher(o);
3061 3062
     options_postprocess_mutate_invariant(o);
3062 3063
 
3063 3064
     if (o->ncp_enabled)
... ...
@@ -3118,16 +3179,6 @@ options_postprocess_mutate(struct options *o)
3118 3118
             "include this in your server configuration");
3119 3119
         o->dh_file = NULL;
3120 3120
     }
3121
-
3122
-    /* cipher negotiation (NCP) currently assumes --pull or --mode server */
3123
-    if (o->ncp_enabled
3124
-        && !(o->pull || o->mode == MODE_SERVER) )
3125
-    {
3126
-        msg( M_WARN, "disabling NCP mode (--ncp-disable) because not "
3127
-             "in P2MP client or server mode" );
3128
-        o->ncp_enabled = false;
3129
-    }
3130
-
3131 3121
 #if ENABLE_MANAGEMENT
3132 3122
     if (o->http_proxy_override)
3133 3123
     {
... ...
@@ -3663,14 +3714,21 @@ options_string(const struct options *o,
3663 3663
      */
3664 3664
 
3665 3665
     buf_printf(&out, ",dev-type %s", dev_type_string(o->dev, o->dev_type));
3666
-    buf_printf(&out, ",link-mtu %u", (unsigned int) calc_options_string_link_mtu(o, frame));
3666
+    /* the link-mtu that we send has only a meaning if have a fixed
3667
+     * cipher (p2p) or have a fallback cipher configured for older non
3668
+     * ncp clients. But not sending it will make even 2.4 complain
3669
+     * about it being missing. So still send it. */
3670
+    buf_printf(&out, ",link-mtu %u",
3671
+               (unsigned int) calc_options_string_link_mtu(o, frame));
3672
+
3667 3673
     buf_printf(&out, ",tun-mtu %d", PAYLOAD_SIZE(frame));
3668 3674
     buf_printf(&out, ",proto %s",  proto_remote(o->ce.proto, remote));
3669 3675
 
3676
+    bool p2p_nopull = o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o);
3670 3677
     /* send tun_ipv6 only in peer2peer mode - in client/server mode, it
3671 3678
      * is usually pushed by the server, triggering a non-helpful warning
3672 3679
      */
3673
-    if (o->ifconfig_ipv6_local && o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o))
3680
+    if (o->ifconfig_ipv6_local && p2p_nopull)
3674 3681
     {
3675 3682
         buf_printf(&out, ",tun-ipv6");
3676 3683
     }
... ...
@@ -3700,7 +3758,7 @@ options_string(const struct options *o,
3700 3700
         }
3701 3701
     }
3702 3702
 
3703
-    if (tt && o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o))
3703
+    if (tt && p2p_nopull)
3704 3704
     {
3705 3705
         const char *ios = ifconfig_options_string(tt, remote, o->ifconfig_nowarn, gc);
3706 3706
         if (ios && strlen(ios))
... ...
@@ -3756,8 +3814,14 @@ options_string(const struct options *o,
3756 3756
 
3757 3757
         init_key_type(&kt, o->ciphername, o->authname, o->keysize, true,
3758 3758
                       false);
3759
-
3760
-        buf_printf(&out, ",cipher %s", cipher_kt_name(kt.cipher));
3759
+        /* Only announce the cipher to our peer if we are willing to
3760
+         * support it */
3761
+        const char *ciphername = cipher_kt_name(kt.cipher);
3762
+        if (p2p_nopull || !o->ncp_enabled
3763
+            || tls_item_in_cipher_list(ciphername, o->ncp_ciphers))
3764
+        {
3765
+            buf_printf(&out, ",cipher %s", ciphername);
3766
+        }
3761 3767
         buf_printf(&out, ",auth %s", md_kt_name(kt.digest));
3762 3768
         buf_printf(&out, ",keysize %d", kt.cipher_length * 8);
3763 3769
         if (o->shared_secret_file)
... ...
@@ -3875,7 +3939,8 @@ options_warning_safe_scan2(const int msglevel,
3875 3875
         || strprefix(p1, "keydir ")
3876 3876
         || strprefix(p1, "proto ")
3877 3877
         || strprefix(p1, "tls-auth ")
3878
-        || strprefix(p1, "tun-ipv6"))
3878
+        || strprefix(p1, "tun-ipv6")
3879
+        || strprefix(p1, "cipher "))
3879 3880
     {
3880 3881
         return;
3881 3882
     }
... ...
@@ -7865,14 +7930,20 @@ add_option(struct options *options,
7865 7865
         VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
7866 7866
         options->ciphername = p[1];
7867 7867
     }
7868
+    else if (streq(p[0], "data-ciphers-fallback") && p[1] && !p[2])
7869
+    {
7870
+        VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
7871
+        options->ciphername = p[1];
7872
+        options->enable_ncp_fallback = true;
7873
+    }
7868 7874
     else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
7869
-            && p[1] && !p[2])
7875
+             && p[1] && !p[2])
7870 7876
     {
7871 7877
         VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
7872 7878
         if (streq(p[0], "ncp-ciphers"))
7873 7879
         {
7874 7880
             msg(M_INFO, "Note: Treating option '--ncp-ciphers' as "
7875
-                        " '--data-ciphers' (renamed in OpenVPN 2.5).");
7881
+                " '--data-ciphers' (renamed in OpenVPN 2.5).");
7876 7882
         }
7877 7883
         options->ncp_ciphers = p[1];
7878 7884
     }
... ...
@@ -7880,9 +7951,9 @@ add_option(struct options *options,
7880 7880
     {
7881 7881
         VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
7882 7882
         options->ncp_enabled = false;
7883
-        msg(M_WARN, "DEPRECATED OPTION: ncp-disable. Disabling dynamic "
7884
-                    "cipher negotiation is a deprecated debug feature that "
7885
-                    "will be removed in OpenVPN 2.6");
7883
+        msg(M_WARN, "DEPRECATED OPTION: ncp-disable. Disabling "
7884
+            "cipher negotiation is a deprecated debug feature that "
7885
+            "will be removed in OpenVPN 2.6");
7886 7886
     }
7887 7887
     else if (streq(p[0], "prng") && p[1] && !p[3])
7888 7888
     {
... ...
@@ -503,6 +503,8 @@ struct options
503 503
     bool shared_secret_file_inline;
504 504
     int key_direction;
505 505
     const char *ciphername;
506
+    bool enable_ncp_fallback;      /**< If defined fall back to
507
+                                    * ciphername if NCP fails */
506 508
     bool ncp_enabled;
507 509
     const char *ncp_ciphers;
508 510
     const char *authname;
... ...
@@ -1932,13 +1932,14 @@ tls_session_update_crypto_params(struct tls_session *session,
1932 1932
         return true;
1933 1933
     }
1934 1934
 
1935
-    if (!session->opt->server
1936
-        && 0 != strcmp(options->ciphername, session->opt->config_ciphername)
1935
+    bool cipher_allowed_as_fallback = options->enable_ncp_fallback
1936
+                                      && streq(options->ciphername, session->opt->config_ciphername);
1937
+
1938
+    if (!session->opt->server && !cipher_allowed_as_fallback
1937 1939
         && !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
1938 1940
     {
1939
-        msg(D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
1940
-            options->ciphername, session->opt->config_ciphername,
1941
-            options->ncp_ciphers);
1941
+        msg(D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s",
1942
+            options->ciphername, options->ncp_ciphers);
1942 1943
         /* undo cipher push, abort connection setup */
1943 1944
         options->ciphername = session->opt->config_ciphername;
1944 1945
         return false;
... ...
@@ -1956,9 +1957,9 @@ tls_session_update_crypto_params(struct tls_session *session,
1956 1956
     }
1957 1957
     else
1958 1958
     {
1959
-      /* Very hacky workaround and quick fix for frame calculation
1960
-       * different when adjusting frame size when the original and new cipher
1961
-       * are identical to avoid a regression with client without NCP */
1959
+        /* Very hacky workaround and quick fix for frame calculation
1960
+         * different when adjusting frame size when the original and new cipher
1961
+         * are identical to avoid a regression with client without NCP */
1962 1962
         return tls_session_generate_data_channel_keys(session);
1963 1963
     }
1964 1964
 
... ...
@@ -48,6 +48,7 @@
48 48
 #include "common.h"
49 49
 
50 50
 #include "ssl_ncp.h"
51
+#include "openvpn.h"
51 52
 
52 53
 /**
53 54
  * Return the Negotiable Crypto Parameters version advertised in the peer info
... ...
@@ -211,9 +212,8 @@ tls_peer_ncp_list(const char *peer_info, struct gc_arena *gc)
211 211
 }
212 212
 
213 213
 char *
214
-ncp_get_best_cipher(const char *server_list, const char *server_cipher,
215
-                    const char *peer_info,  const char *remote_cipher,
216
-                    struct gc_arena *gc)
214
+ncp_get_best_cipher(const char *server_list, const char *peer_info,
215
+                    const char *remote_cipher, struct gc_arena *gc)
217 216
 {
218 217
     /*
219 218
      * The gc of the parameter is tied to the VPN session, create a
... ...
@@ -226,7 +226,10 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
226 226
     const char *peer_ncp_list = tls_peer_ncp_list(peer_info, &gc_tmp);
227 227
 
228 228
     /* non-NCP client without OCC?  "assume nothing" */
229
-    if (remote_cipher == NULL)
229
+    /* For client doing the newer version of NCP (that send IV_CIPHER)
230
+     * we cannot assume that they will accept remote_cipher */
231
+    if (remote_cipher == NULL ||
232
+        (peer_info && strstr(peer_info, "IV_CIPHERS=")))
230 233
     {
231 234
         remote_cipher = "";
232 235
     }
... ...
@@ -242,15 +245,6 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
242 242
             break;
243 243
         }
244 244
     }
245
-    /* We have not found a common cipher, as a last resort check if the
246
-     * server cipher can be used
247
-     */
248
-    if (token == NULL
249
-        && (tls_item_in_cipher_list(server_cipher, peer_ncp_list)
250
-            || streq(server_cipher, remote_cipher)))
251
-    {
252
-        token = server_cipher;
253
-    }
254 245
 
255 246
     char *ret = NULL;
256 247
     if (token != NULL)
... ...
@@ -262,16 +256,75 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
262 262
     return ret;
263 263
 }
264 264
 
265
-void
265
+/**
266
+ * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
267
+ * Allows non-NCP peers to upgrade their cipher individually.
268
+ *
269
+ * Returns true if we switched to the peer's cipher
270
+ *
271
+ * Make sure to call tls_session_update_crypto_params() after calling this
272
+ * function.
273
+ */
274
+static bool
266 275
 tls_poor_mans_ncp(struct options *o, const char *remote_ciphername)
267 276
 {
268
-    if (o->ncp_enabled && remote_ciphername
277
+    if (remote_ciphername
269 278
         && 0 != strcmp(o->ciphername, remote_ciphername))
270 279
     {
271 280
         if (tls_item_in_cipher_list(remote_ciphername, o->ncp_ciphers))
272 281
         {
273 282
             o->ciphername = string_alloc(remote_ciphername, &o->gc);
274 283
             msg(D_TLS_DEBUG_LOW, "Using peer cipher '%s'", o->ciphername);
284
+            return true;
275 285
         }
276 286
     }
287
+    return false;
277 288
 }
289
+
290
+bool
291
+check_pull_client_ncp(struct context *c, const int found)
292
+{
293
+    if (found & OPT_P_NCP)
294
+    {
295
+        msg(D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
296
+        return true;
297
+    }
298
+
299
+    if (!c->options.ncp_enabled)
300
+    {
301
+        return true;
302
+    }
303
+    /* If the server did not push a --cipher, we will switch to the
304
+     * remote cipher if it is in our ncp-ciphers list */
305
+    bool useremotecipher = tls_poor_mans_ncp(&c->options,
306
+                                             c->c2.tls_multi->remote_ciphername);
307
+
308
+
309
+    /* We could not figure out the peer's cipher but we have fallback
310
+     * enabled */
311
+    if (!useremotecipher && c->options.enable_ncp_fallback)
312
+    {
313
+        return true;
314
+    }
315
+
316
+    /* We failed negotiation, give appropiate error message */
317
+    if (c->c2.tls_multi->remote_ciphername)
318
+    {
319
+        msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to negotiate "
320
+            "cipher with server.  Add the server's "
321
+            "cipher ('%s') to --data-ciphers (currently '%s') if "
322
+            "you want to connect to this server.",
323
+            c->c2.tls_multi->remote_ciphername,
324
+            c->options.ncp_ciphers);
325
+        return false;
326
+
327
+    }
328
+    else
329
+    {
330
+        msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to negotiate "
331
+            "cipher with server. Configure "
332
+            "--data-ciphers-fallback if you want to connect "
333
+            "to this server.");
334
+        return false;
335
+    }
336
+}
278 337
\ No newline at end of file
... ...
@@ -40,14 +40,17 @@
40 40
 bool
41 41
 tls_peer_supports_ncp(const char *peer_info);
42 42
 
43
+/* forward declaration to break include dependency loop */
44
+struct context;
45
+
43 46
 /**
44
- * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
45
- * Allows non-NCP peers to upgrade their cipher individually.
47
+ * Checks whether the cipher negotiation is in an acceptable state
48
+ * and we continue to connect or should abort.
46 49
  *
47
- * Make sure to call tls_session_update_crypto_params() after calling this
48
- * function.
50
+ * @return  Wether the client NCP process suceeded or failed
49 51
  */
50
-void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
52
+bool
53
+check_pull_client_ncp(struct context *c, int found);
51 54
 
52 55
 /**
53 56
  * Iterates through the ciphers in server_list and return the first
... ...
@@ -67,9 +70,8 @@ void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
67 67
  * cipher
68 68
  */
69 69
 char *
70
-ncp_get_best_cipher(const char *server_list, const char *server_cipher,
71
-                    const char *peer_info, const char *remote_cipher,
72
-                    struct gc_arena *gc);
70
+ncp_get_best_cipher(const char *server_list, const char *peer_info,
71
+                    const char *remote_cipher, struct gc_arena *gc);
73 72
 
74 73
 
75 74
 /**
... ...
@@ -139,25 +139,36 @@ test_poor_man(void **state)
139 139
     char *best_cipher;
140 140
 
141 141
     const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM";
142
+    const char *serverlistbfcbc = "CHACHA20_POLY1305:AES-128-GCM:BF-CBC";
142 143
 
143
-    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
144
+    best_cipher = ncp_get_best_cipher(serverlist,
145
+                                      "IV_YOLO=NO\nIV_BAR=7",
146
+                                      "BF-CBC", &gc);
147
+
148
+    assert_ptr_equal(best_cipher, NULL);
149
+
150
+
151
+    best_cipher = ncp_get_best_cipher(serverlistbfcbc,
144 152
                                       "IV_YOLO=NO\nIV_BAR=7",
145 153
                                       "BF-CBC", &gc);
146 154
 
147 155
     assert_string_equal(best_cipher, "BF-CBC");
148 156
 
149
-    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
157
+
158
+    best_cipher = ncp_get_best_cipher(serverlist,
150 159
                                       "IV_NCP=1\nIV_BAR=7",
151 160
                                       "AES-128-GCM", &gc);
152 161
 
153 162
     assert_string_equal(best_cipher, "AES-128-GCM");
154 163
 
155
-    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
156
-                                      NULL,
164
+    best_cipher = ncp_get_best_cipher(serverlist, NULL,
157 165
                                       "AES-128-GCM", &gc);
158 166
 
159 167
     assert_string_equal(best_cipher, "AES-128-GCM");
160 168
 
169
+    best_cipher = ncp_get_best_cipher(serverlist, NULL,NULL, &gc);
170
+    assert_ptr_equal(best_cipher, NULL);
171
+
161 172
     gc_free(&gc);
162 173
 }
163 174
 
... ...
@@ -170,29 +181,27 @@ test_ncp_best(void **state)
170 170
 
171 171
     const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM:AES-256-GCM";
172 172
 
173
-    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
173
+    best_cipher = ncp_get_best_cipher(serverlist,
174 174
                                       "IV_YOLO=NO\nIV_NCP=2\nIV_BAR=7",
175 175
                                       "BF-CBC", &gc);
176 176
 
177 177
     assert_string_equal(best_cipher, "AES-128-GCM");
178 178
 
179 179
     /* Best cipher is in --cipher of client */
180
-    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
181
-                                      "IV_NCP=2\nIV_BAR=7",
180
+    best_cipher = ncp_get_best_cipher(serverlist, "IV_NCP=2\nIV_BAR=7",
182 181
                                       "CHACHA20_POLY1305", &gc);
183 182
 
184 183
     assert_string_equal(best_cipher, "CHACHA20_POLY1305");
185 184
 
186 185
     /* Best cipher is in --cipher of client */
187
-    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
188
-                                      "IV_CIPHERS=AES-128-GCM",
186
+    best_cipher = ncp_get_best_cipher(serverlist, "IV_CIPHERS=AES-128-GCM",
189 187
                                       "AES-256-CBC", &gc);
190 188
 
191 189
 
192 190
     assert_string_equal(best_cipher, "AES-128-GCM");
193 191
 
194 192
     /* IV_NCP=2 should be ignored if IV_CIPHERS is sent */
195
-    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
193
+    best_cipher = ncp_get_best_cipher(serverlist,
196 194
                                       "IV_FOO=7\nIV_CIPHERS=AES-256-GCM\nIV_NCP=2",
197 195
                                       "AES-256-CBC", &gc);
198 196