[Openvpn-devel,BUG] test_ncp.c failing

Message ID 1590709611.3515.22.camel@HansenPartnership.com
State Superseded
Headers show
Series
  • [Openvpn-devel,BUG] test_ncp.c failing
Related show

Commit Message

James Bottomley May 28, 2020, 11:46 p.m.
I'm getting this failure of test_ncp.c

[ RUN      ] test_check_ncp_ciphers_list
[  ERROR   ] --- 0x7d67e8 != 0
[   LINE   ] --- test_ncp.c:65: error: Failure!
[  FAILED  ] test_check_ncp_ciphers_list

I'm building under openssl-1.1.0i

The problem seems to be openssl uses a mixed case name for the cipher
and EVP_CIPHER_name() is case sensitive.  Applying the patch below
fixes this for openssl and gets make check to pass all tests, but I
rather wonder why this isn't part of cipher_kt_name() to prevent this
type of problem?

James

---

Comments

Steffan Karger May 29, 2020, 12:35 p.m. | #1
On 29-05-2020 01:46, James Bottomley wrote:
> The problem seems to be openssl uses a mixed case name for the cipher
> and EVP_CIPHER_name() is case sensitive.  Applying the patch below
> fixes this for openssl and gets make check to pass all tests, but I
> rather wonder why this isn't part of cipher_kt_name() to prevent this
> type of problem?

This was my first thought when looking at the patch too.

Probably because I wrote the cipher name translation code as one of my
very first OpenVPN contributions, and didn't get the abstractions right
at the time. Would be good to see if this can be refactored.

Slightly annoying to test if that won't break corner cases though,
because I think this part is not sufficiently covered by automated tests.

-Steffan
Arne Schwabe June 4, 2020, 10:37 p.m. | #2
Am 29.05.20 um 01:46 schrieb James Bottomley:
> I'm getting this failure of test_ncp.c
> 
> [ RUN      ] test_check_ncp_ciphers_list
> [  ERROR   ] --- 0x7d67e8 != 0
> [   LINE   ] --- test_ncp.c:65: error: Failure!
> [  FAILED  ] test_check_ncp_ciphers_list
> 
> I'm building under openssl-1.1.0i
> 
> The problem seems to be openssl uses a mixed case name for the cipher
> and EVP_CIPHER_name() is case sensitive.  Applying the patch below
> fixes this for openssl and gets make check to pass all tests, but I
> rather wonder why this isn't part of cipher_kt_name() to prevent this
> type of problem?

Without double checking if I remember correctly OpenSSL 1.1.0 is a
corner case in this regard. OpenSSL 1.1.1 accepts mixed and non mixed case.

I want to fix this oddity in a proper way and write at least a small
unit test to ensure that always end up with the same cipher on the wire
protocol (in IV_CIPHERS and in the options string).

Arne
James Bottomley June 4, 2020, 11:34 p.m. | #3
On Fri, 2020-06-05 at 00:37 +0200, Arne Schwabe wrote:
> Am 29.05.20 um 01:46 schrieb James Bottomley:
> > I'm getting this failure of test_ncp.c
> > 
> > [ RUN      ] test_check_ncp_ciphers_list
> > [  ERROR   ] --- 0x7d67e8 != 0
> > [   LINE   ] --- test_ncp.c:65: error: Failure!
> > [  FAILED  ] test_check_ncp_ciphers_list
> > 
> > I'm building under openssl-1.1.0i
> > 
> > The problem seems to be openssl uses a mixed case name for the
> > cipher and EVP_CIPHER_name() is case sensitive.  Applying the patch
> > below fixes this for openssl and gets make check to pass all tests,
> > but I rather wonder why this isn't part of cipher_kt_name() to
> > prevent this type of problem?
> 
> Without double checking if I remember correctly OpenSSL 1.1.0 is a
> corner case in this regard. OpenSSL 1.1.1 accepts mixed and non mixed
> case.

It was fixed by:

commit fc196a5eb97dc3a5465c37a6761428ddd81b023d
Author: Pauli <paul.dale@oracle.com>
Date:   Tue Sep 4 07:35:45 2018 +1000

    Make OBJ_NAME case insensitive.
    
    Reviewed-by: Richard Levitte <levitte@openssl.org>
    (Merged from https://github.com/openssl/openssl/pull/7089)

So yes, all of 1.1.1 has this fix.

James

> I want to fix this oddity in a proper way and write at least a small
> unit test to ensure that always end up with the same cipher on the
> wire protocol (in IV_CIPHERS and in the options string).
> 
> Arne
>
Gert Doering June 12, 2020, 6:23 a.m. | #4
Hi,

On Thu, May 28, 2020 at 04:46:51PM -0700, James Bottomley wrote:
> diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c
> index 19432410..f58fa2ea 100644
> --- a/tests/unit_tests/openvpn/test_ncp.c
> +++ b/tests/unit_tests/openvpn/test_ncp.c
> @@ -48,7 +48,7 @@ static void
>  test_check_ncp_ciphers_list(void **state)
>  {
>      struct gc_arena gc = gc_new();
> -    bool have_chacha = cipher_kt_get("CHACHA20-POLY1305");
> +    bool have_chacha = cipher_kt_get(translate_cipher_name_from_openvpn("CHACHA20-POLY1305"));

Am I right in assuming that this patch is no longer needed with the
two cipher_kt_ translation patches I merged yesterday?

gert
James Bottomley June 12, 2020, 2:49 p.m. | #5
On Fri, 2020-06-12 at 08:23 +0200, Gert Doering wrote:
> Hi,
> 
> On Thu, May 28, 2020 at 04:46:51PM -0700, James Bottomley wrote:
> > diff --git a/tests/unit_tests/openvpn/test_ncp.c
> > b/tests/unit_tests/openvpn/test_ncp.c
> > index 19432410..f58fa2ea 100644
> > --- a/tests/unit_tests/openvpn/test_ncp.c
> > +++ b/tests/unit_tests/openvpn/test_ncp.c
> > @@ -48,7 +48,7 @@ static void
> >  test_check_ncp_ciphers_list(void **state)
> >  {
> >      struct gc_arena gc = gc_new();
> > -    bool have_chacha = cipher_kt_get("CHACHA20-POLY1305");
> > +    bool have_chacha =
> > cipher_kt_get(translate_cipher_name_from_openvpn("CHACHA20-
> > POLY1305"));
> 
> Am I right in assuming that this patch is no longer needed with the
> two cipher_kt_ translation patches I merged yesterday?

Yes, make check now passes on openSUSE_Leap_15.1

James
Gert Doering June 12, 2020, 6:36 p.m. | #6
Hi,

On Fri, Jun 12, 2020 at 07:49:07AM -0700, James Bottomley wrote:
> > Am I right in assuming that this patch is no longer needed with the
> > two cipher_kt_ translation patches I merged yesterday?
> Yes, make check now passes on openSUSE_Leap_15.1

Cool.  Marking as "superseded" in patchwork.

One down, 73 to go... :)

gert

Patch

diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c
index 19432410..f58fa2ea 100644
--- a/tests/unit_tests/openvpn/test_ncp.c
+++ b/tests/unit_tests/openvpn/test_ncp.c
@@ -48,7 +48,7 @@  static void
 test_check_ncp_ciphers_list(void **state)
 {
     struct gc_arena gc = gc_new();
-    bool have_chacha = cipher_kt_get("CHACHA20-POLY1305");
+    bool have_chacha = cipher_kt_get(translate_cipher_name_from_openvpn("CHACHA20-POLY1305"));