diff mbox series

[Openvpn-devel,v3,14/21,OSSL,3.0] Use TYPE_do_all_provided function for listing cipher/digest

Message ID 20211019183127.614175-15-arne@rfc2549.org
State Superseded
Headers
Series OpenSSL 3.0 improvements for OpenVPN |

Commit Message

Arne Schwabe Oct. 19, 2021, 7:31 a.m. UTC 
  With OpenSSL 3.0 the use of nid values is deprecated and new algorithms
do not even have NID values anymore.

This also works nicely with providers now:

   openvpn --provider legacy:default --show-ciphers

shows more ciphers (e.g. BF-CBC) than just

   openvpn --show-ciphers

when compiled with OpenSSL 3.0

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/crypto_openssl.c | 95 +++++++++++++++++++++++-------------
 1 file changed, 61 insertions(+), 34 deletions(-)

Comments

Maximilian Fillinger Oct. 26, 2021, 3:37 a.m. UTC | #1 
On 19/10/2021 20:31, Arne Schwabe wrote:
> With OpenSSL 3.0 the use of nid values is deprecated and new algorithms
> do not even have NID values anymore.
> 
> This also works nicely with providers now:
> 
>     openvpn --provider legacy:default --show-ciphers
> 
> shows more ciphers (e.g. BF-CBC) than just
> 
>     openvpn --show-ciphers
> 
> when compiled with OpenSSL 3.0
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

Looks good, and the tests work with OpenSSL 3 and OpenSSL 1.1.1 when I 
also apply the "Do not allow CTS ciphers" patch.

One nitpick:

> +struct collect_ciphers {
> +    /* If we ever exceed this, we must be more selective */
> +    const EVP_CIPHER *list[1000];
> +    size_t num;
> +};
> +
> +static void collect_ciphers(EVP_CIPHER *cipher, void *list)
> +{
> +    struct collect_ciphers* cipher_list = list;
> +    if (cipher_list->num == (sizeof(cipher_list->list)/sizeof(*cipher_list->list)))
> +    {
> +        msg(M_WARN, "WARNING: Too many ciphers, not showing all");
> +        return;
> +    }

I think it would be more readable to use a const (or a #define) for the 
length of the cipher list array, instead of using sizeof.
Selva Nair Oct. 26, 2021, 7:25 a.m. UTC | #2 
On Tue, Oct 26, 2021 at 1:50 PM Max Fillinger <
maximilian.fillinger@foxcrypto.com> wrote:

> On 19/10/2021 20:31, Arne Schwabe wrote:
> > With OpenSSL 3.0 the use of nid values is deprecated and new algorithms
> > do not even have NID values anymore.
> >
> > This also works nicely with providers now:
> >
> >     openvpn --provider legacy:default --show-ciphers
> >
> > shows more ciphers (e.g. BF-CBC) than just
> >
> >     openvpn --show-ciphers
> >
> > when compiled with OpenSSL 3.0
> >
> > Signed-off-by: Arne Schwabe <arne@rfc2549.org>
>
> Looks good, and the tests work with OpenSSL 3 and OpenSSL 1.1.1 when I
> also apply the "Do not allow CTS ciphers" patch.
>
> One nitpick:
>
> > +struct collect_ciphers {
> > +    /* If we ever exceed this, we must be more selective */
> > +    const EVP_CIPHER *list[1000];
> > +    size_t num;
> > +};
> > +
> > +static void collect_ciphers(EVP_CIPHER *cipher, void *list)
> > +{
> > +    struct collect_ciphers* cipher_list = list;
> > +    if (cipher_list->num ==
> (sizeof(cipher_list->list)/sizeof(*cipher_list->list)))
> > +    {
> > +        msg(M_WARN, "WARNING: Too many ciphers, not showing all");
> > +        return;
> > +    }
>
> I think it would be more readable to use a const (or a #define) for the
> length of the cipher list array, instead of using sizeof.
>

IIRC, we have SIZE(x) = sizeof(x)/sizeof(*x) defined in some header for
this.

Selva
<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 26, 2021 at 1:50 PM Max Fillinger &lt;<a href="mailto:maximilian.fillinger@foxcrypto.com">maximilian.fillinger@foxcrypto.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 19/10/2021 20:31, Arne Schwabe wrote:<br>
&gt; With OpenSSL 3.0 the use of nid values is deprecated and new algorithms<br>
&gt; do not even have NID values anymore.<br>
&gt; <br>
&gt; This also works nicely with providers now:<br>
&gt; <br>
&gt;     openvpn --provider legacy:default --show-ciphers<br>
&gt; <br>
&gt; shows more ciphers (e.g. BF-CBC) than just<br>
&gt; <br>
&gt;     openvpn --show-ciphers<br>
&gt; <br>
&gt; when compiled with OpenSSL 3.0<br>
&gt; <br>
&gt; Signed-off-by: Arne Schwabe &lt;<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>&gt;<br>
<br>
Looks good, and the tests work with OpenSSL 3 and OpenSSL 1.1.1 when I <br>
also apply the &quot;Do not allow CTS ciphers&quot; patch.<br>
<br>
One nitpick:<br>
<br>
&gt; +struct collect_ciphers {<br>
&gt; +    /* If we ever exceed this, we must be more selective */<br>
&gt; +    const EVP_CIPHER *list[1000];<br>
&gt; +    size_t num;<br>
&gt; +};<br>
&gt; +<br>
&gt; +static void collect_ciphers(EVP_CIPHER *cipher, void *list)<br>
&gt; +{<br>
&gt; +    struct collect_ciphers* cipher_list = list;<br>
&gt; +    if (cipher_list-&gt;num == (sizeof(cipher_list-&gt;list)/sizeof(*cipher_list-&gt;list)))<br>
&gt; +    {<br>
&gt; +        msg(M_WARN, &quot;WARNING: Too many ciphers, not showing all&quot;);<br>
&gt; +        return;<br>
&gt; +    }<br>
<br>
I think it would be more readable to use a const (or a #define) for the <br>
length of the cipher list array, instead of using sizeof.<br></blockquote><div><br></div><div>IIRC, we have SIZE(x) = sizeof(x)/sizeof(*x) defined in some header for this.</div><div><br></div><div>Selva</div></div></div>
diff mbox series

Patch

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 1900ccc1b..ab552efab 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -311,86 +311,113 @@  cipher_name_cmp(const void *a, const void *b)
     return strcmp(cipher_kt_name(*cipher_a), cipher_kt_name(*cipher_b));
 }
 
+struct collect_ciphers {
+    /* If we ever exceed this, we must be more selective */
+    const EVP_CIPHER *list[1000];
+    size_t num;
+};
+
+static void collect_ciphers(EVP_CIPHER *cipher, void *list)
+{
+    struct collect_ciphers* cipher_list = list;
+    if (cipher_list->num == (sizeof(cipher_list->list)/sizeof(*cipher_list->list)))
+    {
+        msg(M_WARN, "WARNING: Too many ciphers, not showing all");
+        return;
+    }
+
+    if (cipher && (cipher_kt_mode_cbc(cipher)
+#ifdef ENABLE_OFB_CFB_MODE
+        || cipher_kt_mode_ofb_cfb(cipher)
+#endif
+        || cipher_kt_mode_aead(cipher)
+    ))
+    {
+        cipher_list->list[cipher_list->num++] = cipher;
+    }
+}
+
 void
 show_available_ciphers(void)
 {
-    int nid;
-    size_t i;
+    struct collect_ciphers cipher_list = { 0 };
 
-    /* If we ever exceed this, we must be more selective */
-    const EVP_CIPHER *cipher_list[1000];
-    size_t num_ciphers = 0;
 #ifndef ENABLE_SMALL
     printf("The following ciphers and cipher modes are available for use\n"
            "with " PACKAGE_NAME ".  Each cipher shown below may be used as a\n"
            "parameter to the --data-ciphers (or --cipher) option. In static \n"
-           "key mode only CBC mode is allowed.\n\n");
+           "key mode only CBC mode is allowed.\n");
+    printf("See also openssl list -cipher-algorithms\n\n");
 #endif
 
-    for (nid = 0; nid < 10000; ++nid)
+#if OPENSSL_VERSION_NUMBER >= 0x30000000L
+    EVP_CIPHER_do_all_provided(NULL, collect_ciphers, &cipher_list);
+#else
+    for (int nid = 0; nid < 10000; ++nid)
     {
         const EVP_CIPHER *cipher = EVP_get_cipherbynid(nid);
-        if (cipher && (cipher_kt_mode_cbc(cipher)
-#ifdef ENABLE_OFB_CFB_MODE
-                       || cipher_kt_mode_ofb_cfb(cipher)
-#endif
-                       || cipher_kt_mode_aead(cipher)
-                       ))
-        {
-            cipher_list[num_ciphers++] = cipher;
-        }
-        if (num_ciphers == (sizeof(cipher_list)/sizeof(*cipher_list)))
-        {
-            msg(M_WARN, "WARNING: Too many ciphers, not showing all");
-            break;
-        }
+        /* We cast the const away so we can keep the function prototype
+         * compatible with EVP_CIPHER_do_all_provided */
+        collect_ciphers((EVP_CIPHER *)cipher, &cipher_list);
     }
+#endif
 
     /* cast to non-const to prevent warning */
-    qsort((EVP_CIPHER *)cipher_list, num_ciphers, sizeof(*cipher_list), cipher_name_cmp);
+    qsort((EVP_CIPHER *)cipher_list.list, cipher_list.num, sizeof(*cipher_list.list), cipher_name_cmp);
 
-    for (i = 0; i < num_ciphers; i++)
+    for (size_t i = 0; i < cipher_list.num; i++)
     {
-        if (!cipher_kt_insecure(cipher_list[i]))
+        if (!cipher_kt_insecure(cipher_list.list[i]))
         {
-            print_cipher(cipher_list[i]);
+            print_cipher(cipher_list.list[i]);
         }
     }
 
     printf("\nThe following ciphers have a block size of less than 128 bits, \n"
            "and are therefore deprecated.  Do not use unless you have to.\n\n");
-    for (i = 0; i < num_ciphers; i++)
+    for (int i = 0; i < cipher_list.num; i++)
     {
-        if (cipher_kt_insecure(cipher_list[i]))
+        if (cipher_kt_insecure(cipher_list.list[i]))
         {
-            print_cipher(cipher_list[i]);
+            print_cipher(cipher_list.list[i]);
         }
     }
     printf("\n");
 }
 
 void
-show_available_digests(void)
+print_digest(EVP_MD* digest, void* unused)
 {
-    int nid;
+    printf("%s %d bit digest size\n", EVP_MD_get0_name(digest),
+           EVP_MD_size(digest) * 8);
+}
 
+void
+show_available_digests(void)
+{
 #ifndef ENABLE_SMALL
     printf("The following message digests are available for use with\n"
            PACKAGE_NAME ".  A message digest is used in conjunction with\n"
            "the HMAC function, to authenticate received packets.\n"
            "You can specify a message digest as parameter to\n"
-           "the --auth option.\n\n");
+           "the --auth option.\n");
+    printf("See also openssl list -digest-algorithms\n\n");
 #endif
 
-    for (nid = 0; nid < 10000; ++nid)
+#if OPENSSL_VERSION_NUMBER >= 0x30000000L
+    EVP_MD_do_all_provided(NULL, print_digest, NULL);
+#else
+    for (int nid = 0; nid < 10000; ++nid)
     {
         const EVP_MD *digest = EVP_get_digestbynid(nid);
         if (digest)
         {
-            printf("%s %d bit digest size\n",
-                   OBJ_nid2sn(nid), EVP_MD_size(digest) * 8);
+            /* We cast the const away so we can keep the function prototype
+             * compatible with EVP_MD_do_all_provided */
+            print_digest((EVP_MD *)digest, NULL);
         }
     }
+#endif
     printf("\n");
 }