Fix EVP_PKEY key types handling

The internal EVP_PKEY::pkey member is an union thus we need to check for the real key type before we can return the corresponding RSA, DSA or EC public key.

Reported-by Selva Nair <[email protected]...>

Signed-off-by Emmanuel Deloget <[email protected]...>

src/openvpn/openssl_compat.h +2 -2

index 70b19aea..8b29cdaf 100644
@@ -240,7 +240,7 @@ X509_OBJECT_get_type(const X509_OBJECT *obj)
 static inline RSA *
 EVP_PKEY_get0_RSA(EVP_PKEY *pkey)
 {
-    return pkey ? pkey->pkey.rsa : NULL;
+    return (pkey && pkey->type == EVP_PKEY_RSA) ? pkey->pkey.rsa : NULL;
 }
 #endif
 

@@ -254,7 +254,7 @@ EVP_PKEY_get0_RSA(EVP_PKEY *pkey)
 static inline EC_KEY *
 EVP_PKEY_get0_EC_KEY(EVP_PKEY *pkey)
 {
-    return pkey ? pkey->pkey.ec : NULL;
+    return (pkey && pkey->type == EVP_PKEY_EC) ? pkey->pkey.ec : NULL;
 }
 #endif
 

Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as the same check is also performed in the later.

Hi,

I will defer to crypto experts for a proper review, but a quick remark

Quoted 4 lines · Emmanuel Deloget

Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as the same check is also performed in the later.

...

src/openvpn/ssl_openssl.c print_details(struct key_state_ssl *ks_ssl, const char *prefix)

Quoted 4 lines

src/openvpn/ssl_openssl.c +2 -1

+            RSA *rsa = NULL;
+            DSA *dsa = NULL;
 #ifndef OPENSSL_NO_EC
-            else if ((EVP_PKEY_id(pkey) == EVP_PKEY_EC) &&

(EVP_PKEY_get0_EC_KEY(pkey) != NULL))

Quoted 1 line

+ EC *ec = NULL;

That looks wrong: do you mean EC_KEY instead of EC?

src/openvpn/ssl_openssl.c print_details(struct key_state_ssl *ks_ssl, const char *prefix) · line 1707

Quoted 3 lines

src/openvpn/ssl_openssl.c +2 -0

+
+            if ((ec = EVP_PKEY_get0_EC_KEY(pkey)) != NULL)

Selva

Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as the same check is also performed in the later.

We also make the code a bit better by not calling the various EVP_PKEY_get0_*() functions twice (this needs a bit or reordering to avoid introducing yet another #ifndef OPENSSL_NO_EC in the code).

Signed-off-by Emmanuel Deloget <[email protected]...>

src/openvpn/ssl_openssl.c +10 -16

index 711bba11..7943fb2c 100644
@@ -1699,22 +1699,13 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix)
         EVP_PKEY *pkey = X509_get_pubkey(cert);
         if (pkey != NULL)
         {
-            if ((EVP_PKEY_id(pkey) == EVP_PKEY_RSA) && (EVP_PKEY_get0_RSA(pkey) != NULL))
-            {
-                RSA *rsa = EVP_PKEY_get0_RSA(pkey);
-                openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",
-                                 RSA_bits(rsa));
-            }
-            else if ((EVP_PKEY_id(pkey) == EVP_PKEY_DSA) && (EVP_PKEY_get0_DSA(pkey) != NULL))
-            {
-                DSA *dsa = EVP_PKEY_get0_DSA(pkey);
-                openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA",
-                                 DSA_bits(dsa));
-            }
+            RSA *rsa = NULL;
+            DSA *dsa = NULL;
 #ifndef OPENSSL_NO_EC
-            else if ((EVP_PKEY_id(pkey) == EVP_PKEY_EC) && (EVP_PKEY_get0_EC_KEY(pkey) != NULL))
+            EC_KEY *ec = NULL;
+
+            if ((ec = EVP_PKEY_get0_EC_KEY(pkey)) != NULL)
             {
-                EC_KEY *ec = EVP_PKEY_get0_EC_KEY(pkey);
                 const EC_GROUP *group = EC_KEY_get0_group(ec);
                 const char* curve;
 

@@ -1726,9 +1717,19 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix)
 
                 openvpn_snprintf(s2, sizeof(s2), ", %d bit EC, curve: %s",
                                  EC_GROUP_order_bits(group), curve);
-
-            }
+            } else
 #endif
+            if ((rsa = EVP_PKEY_get0_RSA(pkey)) != NULL)
+            {
+                openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",
+                                 RSA_bits(rsa));

Hi,

Quoted 6 lines · Emmanuel Deloget

Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as the same check is also performed in the later.

We also make the code a bit better by not calling the various EVP_PKEY_get0_*() functions twice (this needs a bit or reordering to avoid introducing yet another #ifndef OPENSSL_NO_EC in the code).

Checking double is a bit silly, but we can fix that by simply removing the "EVP_PKEY_id() == ... && " occurrences. (That still allows us to remove it from the compat wrapper.)

I'm not sure that moving the variables outside the if() scope actually improves the code. At least I think the original flow is easier to read. Mostly due to the #ifdef noise, but still. In a path that's not performance-critical, I prefer readability over performance.

src/openvpn/ssl_openssl.c print_details(struct key_state_ssl *ks_ssl,

Quoted 8 lines

Signed-off-by Emmanuel Deloget <[email protected]...>

src/openvpn/ssl_openssl.c +0 -0

index 711bba11..7943fb2c 100644
@@ -1699,22 +1699,13 @@ print_details(struct key_state_ssl *ks_ssl,

const char *prefix)

src/openvpn/ssl_openssl.c print_details(struct key_state_ssl *ks_ssl, const char *prefix)

Quoted 4 lines

src/openvpn/ssl_openssl.c +0 -1

         EVP_PKEY *pkey = X509_get_pubkey(cert);
         if (pkey != NULL)
         {
-            if ((EVP_PKEY_id(pkey) == EVP_PKEY_RSA) &&

(EVP_PKEY_get0_RSA(pkey) != NULL))

src/openvpn/ssl_openssl.c print_details(struct key_state_ssl *ks_ssl, const char *prefix)

Quoted 6 lines

src/openvpn/ssl_openssl.c +0 -6

-            {
-                RSA *rsa = EVP_PKEY_get0_RSA(pkey);
-                openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",
-                                 RSA_bits(rsa));
-            }
-            else if ((EVP_PKEY_id(pkey) == EVP_PKEY_DSA) &&

(EVP_PKEY_get0_DSA(pkey) != NULL))

src/openvpn/ssl_openssl.c print_details(struct key_state_ssl *ks_ssl, const char *prefix)

Quoted 9 lines

src/openvpn/ssl_openssl.c +2 -6

-            {
-                DSA *dsa = EVP_PKEY_get0_DSA(pkey);
-                openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA",
-                                 DSA_bits(dsa));
-            }
+            RSA *rsa = NULL;
+            DSA *dsa = NULL;
 #ifndef OPENSSL_NO_EC
-            else if ((EVP_PKEY_id(pkey) == EVP_PKEY_EC) &&

(EVP_PKEY_get0_EC_KEY(pkey) != NULL))

src/openvpn/ssl_openssl.c print_details(struct key_state_ssl *ks_ssl, const

Quoted 9 lines

src/openvpn/ssl_openssl.c +3 -1

+            EC_KEY *ec = NULL;
+
+            if ((ec = EVP_PKEY_get0_EC_KEY(pkey)) != NULL)
             {
-                EC_KEY *ec = EVP_PKEY_get0_EC_KEY(pkey);
                 const EC_GROUP *group = EC_KEY_get0_group(ec);
                 const char* curve;
 

@@ -1726,9 +1717,19 @@ print_details(struct key_state_ssl *ks_ssl, const

char *prefix)

src/openvpn/ssl_openssl.c print_details(struct key_state_ssl *ks_ssl, const · line 1717

Quoted 2 lines

src/openvpn/ssl_openssl.c +0 -0

                 openvpn_snprintf(s2, sizeof(s2), ", %d bit EC, curve:

%s",

Quoted 14 lines

src/openvpn/ssl_openssl.c +10 -2

                                  EC_GROUP_order_bits(group), curve);
-
-            }
+            } else
 #endif
+            if ((rsa = EVP_PKEY_get0_RSA(pkey)) != NULL)
+            {
+                openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",
+                                 RSA_bits(rsa));
+            }
+            else if ((dsa = EVP_PKEY_get0_DSA(pkey)) != NULL)
+            {
+                openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA",
+                                 DSA_bits(dsa));

src/openvpn/ssl_openssl.c print_details(struct key_state_ssl *ks_ssl, const char *prefix)

Quoted 6 lines

src/openvpn/ssl_openssl.c +2 -0

+            }
+
             EVP_PKEY_free(pkey);
         }
         X509_free(cert);
 

-Steffan

Hello Steffan,

Quoted 10 lines · Steffan Karger

Hi,

Quoted 6 lines · Emmanuel Deloget

Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as the same check is also performed in the later.

We also make the code a bit better by not calling the various EVP_PKEY_get0_*() functions twice (this needs a bit or reordering to avoid introducing yet another #ifndef OPENSSL_NO_EC in the code).

Quoted 9 lines

Checking double is a bit silly, but we can fix that by simply removing the "EVP_PKEY_id() == ... && " occurrences. (That still allows us to remove it from the compat wrapper.)

I'm not sure that moving the variables outside the if() scope actually improves the code. At least I think the original flow is easier to read. Mostly due to the #ifdef noise, but still. In a path that's not performance-critical, I prefer readability over performance.

​Well, to be honest, I was definitely not trying to make any kind of performance gain :)​

The whole idea was to make code easier to read and to remove some now weird duplication of functions (the kind of duplication that will come and bites you later).

For the variables outside the ifs, the next C standard should allow us to write something like:

if ((RSA *rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) {
...

That should be easier to read (but then, I'm not sure when this will be considered as widespread ; I guess we're slated for 2025 or so :))

Anyway, moving the variables within the if means we're going to call EVP_PKEY_get0_RSA() twice which, I feel, is not a good thing to do. But even if I'm not very comfortable with this, I can still propose a patch that des it (it's lighter than the currently proposed change). You tell me :)

Quoted 39 lines · Emmanuel Deloget

Hello Steffan,

Quoted 10 lines · Steffan Karger

Hi,

Quoted 6 lines · Emmanuel Deloget

Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as the same check is also performed in the later.

We also make the code a bit better by not calling the various EVP_PKEY_get0_*() functions twice (this needs a bit or reordering to avoid introducing yet another #ifndef OPENSSL_NO_EC in the code).

Quoted 9 lines

Checking double is a bit silly, but we can fix that by simply removing the "EVP_PKEY_id() == ... && " occurrences. (That still allows us to remove it from the compat wrapper.)

I'm not sure that moving the variables outside the if() scope actually improves the code. At least I think the original flow is easier to read. Mostly due to the #ifdef noise, but still. In a path that's not performance-critical, I prefer readability over performance.

​Well, to be honest, I was definitely not trying to make any kind of performance gain :)​

The whole idea was to make code easier to read and to remove some now weird duplication of functions (the kind of duplication that will come and bites you later).

For the variables outside the ifs, the next C standard should allow us to write something like:

if ((RSA *rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) {
...

Quoted 9 lines

That should be easier to read (but then, I'm not sure when this will be considered as widespread ; I guess we're slated for 2025 or so :))

Anyway, moving the variables within the if means we're going to call EVP_PKEY_get0_RSA() twice which, I feel, is not a good thing to do. But even if I'm not very comfortable with this, I can still propose a patch that des it (it's lighter than the currently proposed change). You tell me :)

Can I try one last time to convince you guys to keep EVP_PKEY_id()? If I was targeting only 1.1.0+ that's how most of those checks would have been done, isn't? Makes life easier, code simpler as I have said before..

Just throwing in my 0.02c, feel free to ignore.

Best,

Selva

Hello Selva,

Quoted 6 lines · Selva Nair

Can I try one last time to convince you guys to keep EVP_PKEY_id()? If I was targeting only 1.1.0+ that's how most of those checks would have been done, isn't? Makes life easier, code simpler as I have said before..

Quoted 6 lines

Just throwing in my 0.02c, feel free to ignore.

Best,

Selva

OpenSSL 1.1.0+ and previous versions allows you to create a typed EVP_PKEY without any key inside. For example, you can do

  EVP_PKEY *pkey = EVP_PKEY_new();
  EVP_PKEY_set_type(pkey, EVP_PKEY_RSA);

This is plainly valid: the key is prepared for later use.

So testing for EVP_PKEY_id() is not sufficient to make sure that current and future code won't mess with us.

To be extra sure, one can do

if (EVP_PKEY_id() == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey)) { ... }

Which is exactly what the current code does. Yet the idea (at least mine) is to remove that to simplify code a bit.

Now, I agree, this is quite easy to read and to understand.

We have 3 solutions:

1) keep the code as is

if (EVP_PKEY_id() == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey) != NULL) { ... }

2) remove EVP_PKEY_id() but keep a test on EVP_PKEY_getO_RSA()

if (EVP_PKEY_get0_RSA(pkey) != NULL) {
RSA *rsa = EVP_PKEY_get0_RSA(pkey);
...
}

3) rework it as proposed in the patch

RSA *rsa = NULL;
if ((rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { ... }

3) rework it as proposed in the patch

RSA *rsa = NULL;
if ((rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { ... }

For the variables outside the ifs, the next C standard should allow us to write something like:

if ((RSA *rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) {

Quoted 4 lines · Emmanuel Deloget

3) rework it as proposed in the patch

RSA *rsa = NULL;
if ((rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { ... }

I do not particularily like assignments in if() clauses, especially when the variable is declared right above it. So if we go for (3), my favourite would be

RSA *rsa = EVP_PKEY_get0_RSA(pkey);
if (rsa != NULL) { ... }

but this is just a side remark. The question on "1, 2 or 3" I'll defer to you folks that are maintaing this code (Steffan and Selva, mostly :) ).

Hi,

Quoted 4 lines · Emmanuel Deloget

For the variables outside the ifs, the next C standard should allow us to write something like:

if ((RSA *rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) {

Yeah, such a shame that this didn't make it into C11. Scoping a variable to an if block is often useful. If only for error checking like if(int err = foo()) { handle error }. Anyway, we can't have it :(

Quoted 10 lines · Gert Doering

Quoted 4 lines · Emmanuel Deloget

3) rework it as proposed in the patch

RSA *rsa = NULL;
if ((rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { ... }

I do not particularily like assignments in if() clauses, especially when the variable is declared right above it. So if we go for (3), my favourite would be

Quoted 6 lines

RSA *rsa = EVP_PKEY_get0_RSA(pkey);
if (rsa != NULL) { ... }

but this is just a side remark. The question on "1, 2 or 3" I'll defer to you folks that are maintaing this code (Steffan and Selva, mostly :)

).

This indeed looks a bit better that initializing to NULL if we're going to take the variables outside the scope.

Seems I've been overruled by majority regarding the way to go, and with the suggestion from cron2 taken into account I think 3 is the way to go.

-Steffan

Of course, this one is not strictly needed. If you want to keep the function around, feel free to ignore the patch.

Given the relative simplicity of the change, one can also consider that if the function is needed again we could simply revert this patch.

Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as the same check is also performed in the later.

We also make the code a bit better by not calling the various EVP_PKEY_get0_*() functions twice (this needs a bit or reordering to avoid introducing yet another #ifndef OPENSSL_NO_EC in the code).

Signed-off-by Emmanuel Deloget <[email protected]...>

---
 src/openvpn/ssl_openssl.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

src/openvpn/ssl_openssl.c +10 -16

index 86318d4c..7061f536 100644
@@ -1699,22 +1699,13 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix)
         EVP_PKEY *pkey = X509_get_pubkey(cert);
         if (pkey != NULL)
         {
-            if ((EVP_PKEY_id(pkey) == EVP_PKEY_RSA) && (EVP_PKEY_get0_RSA(pkey) != NULL))
-            {
-                RSA *rsa = EVP_PKEY_get0_RSA(pkey);
-                openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",
-                                 RSA_bits(rsa));
-            }
-            else if ((EVP_PKEY_id(pkey) == EVP_PKEY_DSA) && (EVP_PKEY_get0_DSA(pkey) != NULL))
-            {
-                DSA *dsa = EVP_PKEY_get0_DSA(pkey);
-                openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA",
-                                 DSA_bits(dsa));
-            }
+            RSA *rsa = EVP_PKEY_get0_RSA(pkey);
+            DSA *dsa = EVP_PKEY_get0_DSA(pkey);
 #ifndef OPENSSL_NO_EC
-            else if ((EVP_PKEY_id(pkey) == EVP_PKEY_EC) && (EVP_PKEY_get0_EC_KEY(pkey) != NULL))
+            EC_KEY *ec = EVP_PKEY_get0_EC_KEY(pkey);
+
+            if (ec != NULL)
             {
-                EC_KEY *ec = EVP_PKEY_get0_EC_KEY(pkey);
                 const EC_GROUP *group = EC_KEY_get0_group(ec);
                 const char* curve;
 

@@ -1726,9 +1717,19 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix)
 
                 openvpn_snprintf(s2, sizeof(s2), ", %d bit EC, curve: %s",
                                  EC_GROUP_order_bits(group), curve);
-
-            }
+            } else
 #endif
+            if (rsa != NULL)
+            {
+                openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",
+                                 RSA_bits(rsa));

Hi,

Quoted 9 lines · Emmanuel Deloget

Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as the same check is also performed in the later.

We also make the code a bit better by not calling the various EVP_PKEY_get0_*() functions twice (this needs a bit or reordering to avoid introducing yet another #ifndef OPENSSL_NO_EC in the code).

Signed-off-by Emmanuel Deloget <[email protected]...>

---

Unfortunately, OpenSSL 1.1.0 treats calling EVP_PKEY_get0_foo on a non-foo key as an error. Running 'make check' with this patch and openssl 1.1.0 fails:

Sat Jan 20 12:23:41 2018 Control Channel: TLSv1.2, cipher TLSv1.2 ECDHE-RSA-AES256-GCM-SHA384, 2048 bit RSA
Sat Jan 20 12:23:41 2018 OpenSSL: error:06078081:digital envelope routines:EVP_PKEY_get0_DSA:expecting a dsa key
Sat Jan 20 12:23:41 2018 OpenSSL: error:0608308E:digital envelope routines:EVP_PKEY_get0_EC_KEY:expecting a ec key
Sat Jan 20 12:23:41 2018 TLS_ERROR: BIO read tls_read_plaintext error

So, NAK. (And Selva gets to keep EPV_PKEY_id() ;-) )

Sorry for not spotting this earlier, could have saved us quite a bit of work...

Hi,

Quoted 6 lines · Steffan Karger

Hi,

Quoted 3 lines · Emmanuel Deloget

Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as the same check is also performed in the later.

Quoted 6 lines

Quoted 6 lines

We also make the code a bit better by not calling the various EVP_PKEY_get0_*() functions twice (this needs a bit or reordering to avoid introducing yet another #ifndef OPENSSL_NO_EC in the code).

Signed-off-by Emmanuel Deloget <[email protected]...>

---

..

Quoted 16 lines

Unfortunately, OpenSSL 1.1.0 treats calling EVP_PKEY_get0_foo on a non-foo key as an error. Running 'make check' with this patch and openssl 1.1.0 fails:

Sat Jan 20 12:23:41 2018 Control Channel: TLSv1.2, cipher TLSv1.2 ECDHE-RSA-AES256-GCM-SHA384, 2048 bit RSA
Sat Jan 20 12:23:41 2018 OpenSSL: error:06078081:digital envelope routines:EVP_PKEY_get0_DSA:expecting a dsa key
Sat Jan 20 12:23:41 2018 OpenSSL: error:0608308E:digital envelope routines:EVP_PKEY_get0_EC_KEY:expecting a ec key
Sat Jan 20 12:23:41 2018 TLS_ERROR: BIO read tls_read_plaintext error

So, NAK. (And Selva gets to keep EPV_PKEY_id() ;-) )

Sorry for not spotting this earlier, could have saved us quite a bit of work...

I'm surprised that my argument about

if (EVP_PKEY_id(foo) == ...EC..) { do EC stuff } else if (EVP_PKEY_id(foo) == ..RSA..) { do RSA stuff }

or switch(EVP_PKEY_id(foo))

being stylistically better[tm] did not work!

Still pleased by the end result. Now I get to spit out some of the "if (EVP_PKEY_get0_RSA(..))" that was pushed down my throat :). For patches already on the ML will do so after review.

Selva

The function is no longer used so we don't need to keep it in the OpenSSL 1.1 compatibility layer.

Signed-off-by Emmanuel Deloget <[email protected]...>

configure.ac +0 -1

index b4fd1b3f..716b45dc 100644
@@ -925,7 +925,6 @@ if test "${with_crypto_library}" = "openssl"; then
 			X509_STORE_get0_objects \
 			X509_OBJECT_free \
 			X509_OBJECT_get_type \
-			EVP_PKEY_id \
 			EVP_PKEY_get0_RSA \
 			EVP_PKEY_get0_DSA \
 			EVP_PKEY_get0_EC_KEY \

src/openvpn/openssl_compat.h +0 -7

index 8b29cdaf..2c4a08c1 100644
@@ -258,20 +258,6 @@ EVP_PKEY_get0_EC_KEY(EVP_PKEY *pkey)
 }
 #endif
 
-#if !defined(HAVE_EVP_PKEY_ID)
-/**
- * Get the PKEY type
- *
- * @param pkey                Public key object
- * @return                    The key type
- */