[Openvpn-devel,4/4] Normalise ncp-ciphers option and restrict it to 127 bytes

Message ID 20191117181243.28919-4-arne@rfc2549.org
State Superseded
Headers show
Series
  • [Openvpn-devel,1/4] Only announce IV_NCP=2 when we are willing to support these ciphers
Related show

Commit Message

Arne Schwabe Nov. 17, 2019, 6:12 p.m.
In scenarios of mbed TLS vs OpenSSL we already normalise the ciphers
that are send via the wire protocol via OCC to not have a mismatch
warning between server and client. This is done by
translate_cipher_name_from_openvpn. The same applies also to the
ncp-ciphers list. Specifying non normalised names in ncp-ciphers will
cause negotation not to succeed if ciphers are not in the same form.
Therefore we will normalise the ciphers in options_postmutate.

The alternative and a lot less user friendly alternative would be to
bail if on of the ciphers in ncp-ciphers is not in its normalised form.

Also restrict the ncp-ciphers list to 127. This is somewhat arbitrary
but should prevent too large IV_CIPHER messages and problems sending
those. The server will accept also large IV_CIPHER values from clients.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 doc/openvpn.8                       |  3 ++
 src/openvpn/options.c               | 14 +++++---
 src/openvpn/ssl_ncp.c               | 55 +++++++++++++++++++++++++----
 src/openvpn/ssl_ncp.h               | 19 ++++++++--
 tests/unit_tests/openvpn/test_ncp.c | 40 ++++++++++++++-------
 5 files changed, 104 insertions(+), 27 deletions(-)

Comments

Arne Schwabe Nov. 18, 2019, 7:20 a.m. | #1
> 
> Second thing, more of a comment: 127 feels really low.  'AES-256-GCM' is
> 11 characters, so 127 / 12 (11 chars and a separator) says you're
> limiting to about 10 ciphers.  If I do `openvpn --show-ciphers` there's
> a LOT of data there.  I'd think, for future-proofing, you'd want to
> allow for a lot more possibilities.  Just an opinion though.
>

The main problem is the wire protocol at the moment. The whole packet
with IVs has to fit into one control channel packet with 1280 bytes. So
I want to be conservative here.

And limiting to 10 ciphers should be more than enough. I don't think any
reasonable setup should have that many allowed ciphers. At the moment we
have AES-GCM-256 or AES-GCM-128 and ChaCha20-Poly1305. Maybe 2 others.
For your VPN setup you probably pick one of those, maybe 2. Then there
is still more than enough room for the future expansions.

And if I look at show-ciphers of my OpenSSL 1.1.1 compiled OpenVPN and
ignore duplicate/weak ciphers and limit myself to one block size, the
list is not very long:

ARIA-256-CBC, ChaCha20-Poly1305, AES-256-GCM, CAMELLIA-256-CBC,
SEED-CBC, SM4-CBC.

Arne
Lev Stipakov Feb. 13, 2020, 12:34 p.m. | #2
Hi,

su 17. marrask. 2019 klo 20.13 Arne Schwabe (arne@rfc2549.org) kirjoitti:
>
> -        if (!cipher_kt_get(translate_cipher_name_from_openvpn(token)))
> +        /* translate_cipher_name_from_openvpn also normalises the cipher
name,
> +         * e.g. replacing AeS-128-gCm with AES-128-GCM
> +         */

I think this comment is a bit misleading -
 translate_cipher_name_from_openvpn()
translates openvpn cipher name (value in --ncp-ciphers) to crypto library
cipher name,
for example from "aES-256-GCM" to "idaes_256_GCM" which contradicts the
comment.

Maybe we could factor this code out into separate normalize_cipher_name()
function, which

 - translates (possibly non-normalized) openvpn cipher name into cryptolib
cipher name
 - translates crypto cipher name back to openvpn cipher name, this time
normalized

> +        if (!ktc)
>          {
>              msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s",
token);
> -            unsupported_cipher_found = true;
> +            error_found = true;

It seems that mutate_ncp_cipher_list() returns NULL if error_found is true.
Maybe we could goto
out of the loop? The label could be added before free() calls.

> +            if (buf_len(&new_list)> 0)
> +            {
> +                /* The next if condition ensure there is always space for
> +                 * a :
> +                 */
> +                buf_puts(&new_list, ":");
> +            }
> +
> +            /* Ensure buffer has capacity for cipher name + : + \0 */
> +            if (!(buf_forward_capacity(&new_list) >
> +                  strlen(ovpn_cipher_name) + 2))

This doesn't handle the case when buffer capacity is just enough
to fit the last cipher - for that it is enough to fit cipher name and \0.
Could we move

  >          token = strtok(NULL, ":");

here and do something like

  /* for the last cipher, token is NULL, enough to fit cipher and \0 */
  strlen(ovpn_cipher_name) + (token ? 2 : 1);

> +            {
> +                msg(M_WARN, "Length of --ncp-ciphers is over the"
> +                    "limit of 127 chars");
> +                error_found = true;

Same as above, cannot we "goto out"?

We could put out: right before next line.

>      free(tmp_ciphers);
> +    free_buf(&new_list);
>
> -    return 0 < strlen(list) && !unsupported_cipher_found;
> +    return ret;
>  }

-Lev
<div dir="ltr">Hi,<br><br>su 17. marrask. 2019 klo 20.13 Arne Schwabe (<a href="mailto:arne@rfc2549.org">arne@rfc2549.org</a>) kirjoitti:<br>&gt;<br>&gt; -        if (!cipher_kt_get(translate_cipher_name_from_openvpn(token)))<br>&gt; +        /* translate_cipher_name_from_openvpn also normalises the cipher name,<br>&gt; +         * e.g. replacing AeS-128-gCm with AES-128-GCM<br>&gt; +         */<br><br>I think this comment is a bit misleading -  translate_cipher_name_from_openvpn()<br>translates openvpn cipher name (value in --ncp-ciphers) to crypto library cipher name,<br>for example from &quot;aES-256-GCM&quot; to &quot;idaes_256_GCM&quot; which contradicts the comment.<br><br>Maybe we could factor this code out into separate normalize_cipher_name() function, which<br><br> - translates (possibly non-normalized) openvpn cipher name into cryptolib cipher name<br> - translates crypto cipher name back to openvpn cipher name, this time normalized<br><br>&gt; +        if (!ktc)<br>&gt;          {<br>&gt;              msg(M_WARN, &quot;Unsupported cipher in --ncp-ciphers: %s&quot;, token);<br>&gt; -            unsupported_cipher_found = true;<br>&gt; +            error_found = true;<br><br>It seems that mutate_ncp_cipher_list() returns NULL if error_found is true. Maybe we could goto<br>out of the loop? The label could be added before free() calls.<br><br>&gt; +            if (buf_len(&amp;new_list)&gt; 0)<br>&gt; +            {<br>&gt; +                /* The next if condition ensure there is always space for<br>&gt; +                 * a :<br>&gt; +                 */<br>&gt; +                buf_puts(&amp;new_list, &quot;:&quot;);<br>&gt; +            }<br>&gt; +<br>&gt; +            /* Ensure buffer has capacity for cipher name + : + \0 */<br>&gt; +            if (!(buf_forward_capacity(&amp;new_list) &gt;<br>&gt; +                  strlen(ovpn_cipher_name) + 2))<br><br>This doesn&#39;t handle the case when buffer capacity is just enough<br>to fit the last cipher - for that it is enough to fit cipher name and \0.<div>Could we move</div><div><br>  &gt;          token = strtok(NULL, &quot;:&quot;); <br><br>here and do something like<br><br>  /* for the last cipher, token is NULL, enough to fit cipher and \0 */<br>  strlen(ovpn_cipher_name) + (token ? 2 : 1);<br><br>&gt; +            {<br>&gt; +                msg(M_WARN, &quot;Length of --ncp-ciphers is over the&quot;<br>&gt; +                    &quot;limit of 127 chars&quot;);<br>&gt; +                error_found = true;<br><br>Same as above, cannot we &quot;goto out&quot;?<br><br>We could put out: right before next line.<br><br>&gt;      free(tmp_ciphers);<br>&gt; +    free_buf(&amp;new_list);<br>&gt;<br>&gt; -    return 0 &lt; strlen(list) &amp;&amp; !unsupported_cipher_found;<br>&gt; +    return ret;<br>&gt;  }</div><div><br></div><div>-Lev</div></div>
Arne Schwabe Feb. 13, 2020, 2:14 p.m. | #3
Am 13.02.20 um 13:34 schrieb Lev Stipakov:
> Hi,
> 
> su 17. marrask. 2019 klo 20.13 Arne Schwabe (arne@rfc2549.org
> <mailto:arne@rfc2549.org>) kirjoitti:
>>
>> -        if (!cipher_kt_get(translate_cipher_name_from_openvpn(token)))
>> +        /* translate_cipher_name_from_openvpn also normalises the
> cipher name,
>> +         * e.g. replacing AeS-128-gCm with AES-128-GCM
>> +         */
> 
> I think this comment is a bit misleading -
>  translate_cipher_name_from_openvpn()
> translates openvpn cipher name (value in --ncp-ciphers) to crypto
> library cipher name,
> for example from "aES-256-GCM" to "idaes_256_GCM" which contradicts the
> comment.
> 

Will update the comment.

> Maybe we could factor this code out into separate
> normalize_cipher_name() function, which
> 
>  - translates (possibly non-normalized) openvpn cipher name into
> cryptolib cipher name
>  - translates crypto cipher name back to openvpn cipher name, this time
> normalized


Hm, it would be only used here and make error handling more complicated,
so I think we should just stick with the two functions.

>> +        if (!ktc)
>>          {
>>              msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s",
> token);
>> -            unsupported_cipher_found = true;
>> +            error_found = true;
> 
> It seems that mutate_ncp_cipher_list() returns NULL if error_found is
> true. Maybe we could goto
> out of the loop? The label could be added before free() calls.

Yes. But I found that less user friendly. The approach here will give
you an error for every unsupported cipher. Otherwise you get only an
error for the first unsupported cipher.


>> +            if (buf_len(&new_list)> 0)
>> +            {
>> +                /* The next if condition ensure there is always space for
>> +                 * a :
>> +                 */
>> +                buf_puts(&new_list, ":");
>> +            }
>> +
>> +            /* Ensure buffer has capacity for cipher name + : + \0 */
>> +            if (!(buf_forward_capacity(&new_list) >
>> +                  strlen(ovpn_cipher_name) + 2))
> 
> This doesn't handle the case when buffer capacity is just enough
> to fit the last cipher - for that it is enough to fit cipher name and \0.
> Could we move
> 
>   >          token = strtok(NULL, ":");
> 
> here and do something like
> 
>   /* for the last cipher, token is NULL, enough to fit cipher and \0 */
>   strlen(ovpn_cipher_name) + (token ? 2 : 1);
> 
>> +            {
>> +                msg(M_WARN, "Length of --ncp-ciphers is over the"
>> +                    "limit of 127 chars");
>> +                error_found = true;


I don't think this worth fixing. If you are close or at the 127 char
limit then your cipher list is too long anyway. That we might bail out
at 126 char is acceptable. I rather prefer the simple code here.


Arne

Patch

diff --git a/doc/openvpn.8 b/doc/openvpn.8
index 4f0df786..a71b1c14 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -4400,6 +4400,9 @@  AES\-256\-GCM:AES\-256\-CBC" set can either specify "\-\-cipher BF\-CBC" or
 
 Note, for using NCP with a OpenVPN 2.4 peer this list must include
 the AES\-256\-GCM and AES\-128\-GCM ciphers.
+
+This list is restricted to be 127 chars long after conversion to OpenVPN
+ciphers.
 .\"*********************************************************
 .TP
 .B \-\-ncp\-disable
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 9be5a826..fd069c75 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -2500,11 +2500,6 @@  options_postprocess_verify_ce(const struct options *options, const struct connec
     }
 #endif /* P2MP_SERVER */
 
-    if (options->ncp_enabled && !tls_check_ncp_cipher_list(options->ncp_ciphers))
-    {
-        msg(M_USAGE, "NCP cipher list contains unsupported ciphers.");
-    }
-
     if (options->keysize)
     {
         msg(M_WARN, "WARNING: --keysize is DEPRECATED and will be removed in OpenVPN 2.6");
@@ -3079,6 +3074,15 @@  options_postprocess_mutate(struct options *o)
 
     options_postprocess_mutate_invariant(o);
 
+    if (o->ncp_enabled)
+    {
+        o->ncp_ciphers = mutate_ncp_cipher_list(o->ncp_ciphers, &o->gc);
+        if (o->ncp_ciphers == NULL)
+        {
+            msg(M_USAGE, "NCP cipher list contains unsupported ciphers or is too long.");
+        }
+    }
+
     if (o->remote_list && !o->connection_list)
     {
         /*
diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index 029ac1d5..ee3c2cb7 100644
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -91,27 +91,68 @@  tls_peer_supports_ncp(const char *peer_info)
     }
 }
 
-bool
-tls_check_ncp_cipher_list(const char *list)
+char *
+mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
 {
-    bool unsupported_cipher_found = false;
+    bool error_found = false;
 
-    ASSERT(list);
+    struct buffer new_list  = alloc_buf(MAX_NCP_CIPHERS_LENGTH);
 
     char *const tmp_ciphers = string_alloc(list, NULL);
     const char *token = strtok(tmp_ciphers, ":");
     while (token)
     {
-        if (!cipher_kt_get(translate_cipher_name_from_openvpn(token)))
+        /* translate_cipher_name_from_openvpn also normalises the cipher name,
+         * e.g. replacing AeS-128-gCm with AES-128-GCM
+         */
+        const char *cipher_name = translate_cipher_name_from_openvpn(token);
+        const cipher_kt_t *ktc = cipher_kt_get(cipher_name);
+        if (!ktc)
         {
             msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
-            unsupported_cipher_found = true;
+            error_found = true;
+        }
+        else
+        {
+            const char *ovpn_cipher_name =
+                translate_cipher_name_to_openvpn(cipher_kt_name(ktc));
+
+            if (buf_len(&new_list)> 0)
+            {
+                /* The next if condition ensure there is always space for
+                 * a :
+                 */
+                buf_puts(&new_list, ":");
+            }
+
+            /* Ensure buffer has capacity for cipher name + : + \0 */
+            if (!(buf_forward_capacity(&new_list) >
+                  strlen(ovpn_cipher_name) + 2))
+            {
+                msg(M_WARN, "Length of --ncp-ciphers is over the"
+                    "limit of 127 chars");
+                error_found = true;
+            }
+            else
+            {
+                buf_puts(&new_list, ovpn_cipher_name);
+            }
         }
         token = strtok(NULL, ":");
     }
+
+
+
+    char *ret = NULL;
+    if (!error_found && buf_len(&new_list) > 0)
+    {
+        buf_null_terminate(&new_list);
+        ret = string_alloc(buf_str(&new_list), gc);
+    }
     free(tmp_ciphers);
+    free_buf(&new_list);
 
-    return 0 < strlen(list) && !unsupported_cipher_found;
+    return ret;
 }
 
 bool
diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
index 82d008d8..62822e89 100644
--- a/src/openvpn/ssl_ncp.h
+++ b/src/openvpn/ssl_ncp.h
@@ -87,10 +87,17 @@  tls_peer_ncp_list(const char *peer_info);
  * Check whether the ciphers in the supplied list are supported.
  *
  * @param list          Colon-separated list of ciphers
+ * @parms gc            gc_arena to allocate the returned string
  *
- * @returns true iff all ciphers in list are supported.
+ * @returns             colon separated string of normalised (via
+ *                      translate_cipher_name_from_openvpn) and
+ *                      zero terminated string iff all ciphers
+ *                      in list are supported and the total length
+ *                      is short than MAX_NCP_CIPHERS_LENGTH. NULL
+ *                      otherwise.
  */
-bool tls_check_ncp_cipher_list(const char *list);
+char *
+mutate_ncp_cipher_list(const char *list, struct gc_arena *gc);
 
 /**
  * Return true iff item is present in the colon-separated zero-terminated
@@ -98,4 +105,12 @@  bool tls_check_ncp_cipher_list(const char *list);
  */
 bool tls_item_in_cipher_list(const char *item, const char *list);
 
+/**
+ * The maximum length of a ncp-cipher string that is accepted.
+ *
+ * Since this list needs to be pushed as IV_CIPHERS, we are conservative
+ * about its length.
+ */
+#define MAX_NCP_CIPHERS_LENGTH 127
+
 #endif /* ifndef OPENVPN_SSL_NCP_H */
diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c
index e71f4825..7684eed4 100644
--- a/tests/unit_tests/openvpn/test_ncp.c
+++ b/tests/unit_tests/openvpn/test_ncp.c
@@ -47,10 +47,24 @@  const char *aes_ciphers = "AES-256-GCM:AES-128-GCM";
 static void
 test_check_ncp_ciphers_list(void **state)
 {
-    assert_true(tls_check_ncp_cipher_list(aes_ciphers));
-    assert_true(tls_check_ncp_cipher_list(bf_chacha));
-    assert_false(tls_check_ncp_cipher_list("vollbit"));
-    assert_false(tls_check_ncp_cipher_list("AES-256-GCM:vollbit"));
+    struct gc_arena gc = gc_new();
+
+    assert_string_equal(mutate_ncp_cipher_list(aes_ciphers, &gc), aes_ciphers);
+
+    assert_string_equal(mutate_ncp_cipher_list(bf_chacha, &gc), bf_chacha);
+
+    assert_string_equal(mutate_ncp_cipher_list("BF-cbc:ChaCha20-Poly1305", &gc),
+                        bf_chacha);
+
+
+    assert_ptr_equal(mutate_ncp_cipher_list("vollbit", &gc), NULL);
+    assert_ptr_equal(mutate_ncp_cipher_list("AES-256-GCM:vollbit", &gc), NULL);
+    assert_ptr_equal(mutate_ncp_cipher_list("", &gc), NULL);
+
+    assert_ptr_equal(mutate_ncp_cipher_list(
+                         "ChaCha20-Poly1305:ChaCha20-Poly1305:ChaCha20-Poly1305:"
+                         "ChaCha20-Poly1305:ChaCha20-Poly1305:ChaCha20-Poly1305:"
+                         "ChaCha20-Poly1305", &gc), NULL);
 }
 
 static void
@@ -64,7 +78,7 @@  test_extract_client_ciphers(void **state)
     assert_string_equal(aes_ciphers,peer_list);
     assert_true(tls_peer_supports_ncp(client_peer_info));
     free(peer_list);
-                
+
     client_peer_info = "foo=bar\nIV_foo=y\nIV_NCP=2\nIV_CIPHERS=BF-CBC";
     peer_list = tls_peer_ncp_list(client_peer_info);
     assert_string_equal("BF-CBC", peer_list);
@@ -101,8 +115,8 @@  test_poor_man(void **state)
     struct gc_arena gc = gc_new();
     char *best_cipher;
 
-    const char *serverlist="CHACHA20_POLY1305:AES-128-GCM";
-    
+    const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM";
+
     best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
                                       "IV_YOLO=NO\nIV_BAR=7",
                                       "BF-CBC", &gc);
@@ -131,8 +145,8 @@  test_ncp_best(void **state)
     struct gc_arena gc = gc_new();
     char *best_cipher;
 
-    const char *serverlist="CHACHA20_POLY1305:AES-128-GCM:AES-256-GCM";
-    
+    const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM:AES-256-GCM";
+
     best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
                                       "IV_YOLO=NO\nIV_NCP=2\nIV_BAR=7",
                                       "BF-CBC", &gc);
@@ -150,7 +164,7 @@  test_ncp_best(void **state)
     best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
                                       "IV_CIPHERS=AES-128-GCM",
                                       "AES-256-CBC", &gc);
-    
+
 
     assert_string_equal(best_cipher, "AES-128-GCM");
 
@@ -167,16 +181,16 @@  test_ncp_best(void **state)
 
 
 
-
 const struct CMUnitTest ncp_tests[] = {
     cmocka_unit_test(test_check_ncp_ciphers_list),
-    cmocka_unit_test(test_extract_client_ciphers),    
+    cmocka_unit_test(test_extract_client_ciphers),
     cmocka_unit_test(test_poor_man),
     cmocka_unit_test(test_ncp_best)
 };
 
 
-int main(void)
+int
+main(void)
 {
     return cmocka_run_group_tests(ncp_tests, NULL, NULL);
 }