| Message ID | 20211019183127.614175-15-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | OpenSSL 3.0 improvements for OpenVPN | expand |
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.
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 <<a href="mailto:maximilian.fillinger@foxcrypto.com">maximilian.fillinger@foxcrypto.com</a>> 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> > With OpenSSL 3.0 the use of nid values is deprecated and new algorithms<br> > do not even have NID values anymore.<br> > <br> > This also works nicely with providers now:<br> > <br> > openvpn --provider legacy:default --show-ciphers<br> > <br> > shows more ciphers (e.g. BF-CBC) than just<br> > <br> > openvpn --show-ciphers<br> > <br> > when compiled with OpenSSL 3.0<br> > <br> > Signed-off-by: Arne Schwabe <<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>><br> <br> Looks good, and the tests work with OpenSSL 3 and OpenSSL 1.1.1 when I <br> also apply the "Do not allow CTS ciphers" patch.<br> <br> One nitpick:<br> <br> > +struct collect_ciphers {<br> > + /* If we ever exceed this, we must be more selective */<br> > + const EVP_CIPHER *list[1000];<br> > + size_t num;<br> > +};<br> > +<br> > +static void collect_ciphers(EVP_CIPHER *cipher, void *list)<br> > +{<br> > + struct collect_ciphers* cipher_list = list;<br> > + if (cipher_list->num == (sizeof(cipher_list->list)/sizeof(*cipher_list->list)))<br> > + {<br> > + msg(M_WARN, "WARNING: Too many ciphers, not showing all");<br> > + return;<br> > + }<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 --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"); }
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(-)