[Openvpn-devel,v3,1/2] OpenSSL: remove some EVP_PKEY type checks

Message ID 20180117131046.32402-1-logout@free.fr
State Rejected
Headers show
Series
  • [Openvpn-devel,v3,1/2] OpenSSL: remove some EVP_PKEY type checks
Related show

Commit Message

Emmanuel Deloget Jan. 17, 2018, 1:10 p.m.
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 <logout@free.fr>
---
 src/openvpn/ssl_openssl.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

Comments

Steffan Karger Jan. 20, 2018, 11:30 a.m. | #1
Hi,

On 17-01-18 14:10, Emmanuel Deloget wrote:
> 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 <logout@free.fr>
> ---
>  src/openvpn/ssl_openssl.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 86318d4c..7061f536 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -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));
> +            }
> +            else if (dsa != NULL)
> +            {
> +                openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA",
> +                                 DSA_bits(dsa));
> +            }
> +
>              EVP_PKEY_free(pkey);
>          }
>          X509_free(cert);
> 

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...

-Steffan

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Selva Nair Jan. 20, 2018, 2:22 p.m. | #2
Hi,

On Sat, Jan 20, 2018 at 6:30 AM, Steffan Karger <steffan@karger.me> wrote:
> Hi,
>
> On 17-01-18 14:10, Emmanuel Deloget wrote:
>> 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 <logout@free.fr>
>> ---

..

> 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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Emmanuel Deloget Jan. 24, 2018, 11:33 a.m. | #3
Hello,

and sorry for the delay (things like 'real life', you know).

On Sat, Jan 20, 2018 at 3:22 PM, Selva Nair <selva.nair@gmail.com> wrote:

> Hi,
>
> On Sat, Jan 20, 2018 at 6:30 AM, Steffan Karger <steffan@karger.me> wrote:
> > Hi,
> >
> > On 17-01-18 14:10, Emmanuel Deloget wrote:
> >> 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 <logout@free.fr>
> >> ---
>
> ..
>
> > 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 should have spotted that before I sent the change, but I only tested it
with 1.0.2 and compile-tested it against 1.1. I'm going to refactor my test
environment in order to avoid any further error like this one.



>
> 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.
>
>
​I like the switch - I find it elegant - but it does not avoid an internal
'if', so the code would look like:

switch (EVP_PKEY_id(foo)) {
case ..RSA..: {
    RSA *rsa = EVP_PKEY_get0_RSA(foo);
    if (rsa) {
       openvpn_printf(...);
    }
  }
  break;
case ...
}

​If this is acceptable, I can ​respin patch 2. Otherwise, feel free to
discard both patch 2 and patch 3 of the series as any change in patch 2
would not be a largen enhancement of the already existing code.



> Selva
>
>
​Best regards,

-- Emmanuel Deloget
​
<div dir="ltr"><div class="gmail_default" style="font-family:monospace,monospace">Hello, </div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">and sorry for the delay (things like &#39;real life&#39;, you know). </div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Jan 20, 2018 at 3:22 PM, Selva Nair <span dir="ltr">&lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<span class="gmail-"><br>
On Sat, Jan 20, 2018 at 6:30 AM, Steffan Karger &lt;<a href="mailto:steffan@karger.me">steffan@karger.me</a>&gt; wrote:<br>
&gt; Hi,<br>
&gt;<br>
&gt; On 17-01-18 14:10, Emmanuel Deloget wrote:<br>
&gt;&gt; Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as<br>
&gt;&gt; the same check is also performed in the later.<br>
&gt;&gt;<br>
&gt;&gt; We also make the code a bit better by not calling the various<br>
&gt;&gt; EVP_PKEY_get0_*() functions twice (this needs a bit or reordering to<br>
&gt;&gt; avoid introducing yet another #ifndef OPENSSL_NO_EC in the code).<br>
&gt;&gt;<br>
&gt;&gt; Signed-off-by: Emmanuel Deloget &lt;<a href="mailto:logout@free.fr">logout@free.fr</a>&gt;<br>
&gt;&gt; ---<br>
<br>
</span>..<br>
<span class="gmail-"><br>
&gt; Unfortunately, OpenSSL 1.1.0 treats calling EVP_PKEY_get0_foo on a<br>
&gt; non-foo key as an error.  Running &#39;make check&#39; with this patch and<br>
&gt; openssl 1.1.0 fails:<br>
&gt;<br>
&gt; Sat Jan 20 12:23:41 2018 Control Channel: TLSv1.2, cipher TLSv1.2<br>
&gt; ECDHE-RSA-AES256-GCM-SHA384, 2048 bit RSA<br>
&gt; Sat Jan 20 12:23:41 2018 OpenSSL: error:06078081:digital envelope<br>
&gt; routines:EVP_PKEY_get0_DSA:<wbr>expecting a dsa key<br>
&gt; Sat Jan 20 12:23:41 2018 OpenSSL: error:0608308E:digital envelope<br>
&gt; routines:EVP_PKEY_get0_EC_KEY:<wbr>expecting a ec key<br>
&gt; Sat Jan 20 12:23:41 2018 TLS_ERROR: BIO read tls_read_plaintext error<br>
&gt;<br>
&gt; So, NAK.  (And Selva gets to keep EPV_PKEY_id() ;-) )<br>
&gt;<br>
&gt; Sorry for not spotting this earlier, could have saved us quite a bit of<br>
&gt; work...<br></span></blockquote><div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">​I should have spotted that before I sent the change, but I only tested it with 1.0.2 and compile-tested it against 1.1. I&#39;m going to refactor my test environment in order to avoid any further error like this one. </div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">
<br>
</span>I&#39;m surprised that my argument about<br>
<br>
if (EVP_PKEY_id(foo) == ...EC..) { do EC stuff }<br>
else if (EVP_PKEY_id(foo) == ..RSA..) { do RSA stuff }<br>
<br>
or switch(EVP_PKEY_id(foo))<br>
<br>
being stylistically better[tm] did not work!<br>
<br>
Still pleased by the end result. Now I get to spit out some of the<br>
&quot;if (EVP_PKEY_get0_RSA(..))&quot;  that was pushed down my throat :).<br>
For patches already on the ML will do so after review.<br>
<span class="gmail-HOEnZb"><font color="#888888"><br></font></span></blockquote><div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace">​I like the switch - I find it elegant - but it does not avoid an internal &#39;if&#39;, so the code would look like:</div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">switch (EVP_PKEY_id(foo)) {</div><div class="gmail_default" style="font-family:monospace,monospace">case ..RSA..: {</div><div class="gmail_default" style="font-family:monospace,monospace">    RSA *rsa = EVP_PKEY_get0_RSA(foo);</div><div class="gmail_default" style="font-family:monospace,monospace">    if (rsa) {</div><div class="gmail_default" style="font-family:monospace,monospace">       openvpn_printf(...);</div><div class="gmail_default" style="font-family:monospace,monospace">    }</div><div class="gmail_default" style="font-family:monospace,monospace">  }</div><div class="gmail_default" style="font-family:monospace,monospace">  break;</div><div class="gmail_default" style="font-family:monospace,monospace">case ...</div><div class="gmail_default" style="font-family:monospace,monospace">}<br></div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace">​If this is acceptable, I can ​respin patch 2. Otherwise, feel free to discard both patch 2 and patch 3 of the series as any change in patch 2 would not be a largen enhancement of the already existing code. </div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace"><span style="font-family:arial,sans-serif"> </span><br></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-HOEnZb"><font color="#888888">
Selva<br>
</font></span><div class="gmail-HOEnZb"><div class="gmail-h5"><br></div></div></blockquote><div><br></div><div class="gmail_default" style="font-family:monospace,monospace">​Best regards, </div><div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">-- Emmanuel Deloget</div></div><div class="gmail_default" style="font-family:monospace,monospace">​</div></div></div></div>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Patch

diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 86318d4c..7061f536 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -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));
+            }
+            else if (dsa != NULL)
+            {
+                openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA",
+                                 DSA_bits(dsa));
+            }
+
             EVP_PKEY_free(pkey);
         }
         X509_free(cert);