diff mbox series

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

Message ID 3ea860bd412bbc9638cb54ade1a85ee08caf1b42.1515775195.git.logout@free.fr
State Rejected
Headers show
Series Fix EVP_PKEY key types handling | expand

Commit Message

Emmanuel Deloget Jan. 12, 2018, 5:48 a.m. UTC 
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>

Comments

Selva Nair Jan. 12, 2018, 6:09 a.m. UTC | #1 
Hi,

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

On Fri, Jan 12, 2018 at 11:48 AM, Emmanuel Deloget <logout@free.fr> wrote:
> Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as
> the same check is also performed in the later.
>
>
...

> +            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 *ec = NULL;

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

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

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. 12, 2018, 10:28 a.m. UTC | #2 
Hello Selva,

On Fri, Jan 12, 2018 at 6:09 PM, Selva Nair <selva.nair@gmail.com> wrote:

> Hi,
>
> I will defer to crypto experts for a proper review, but a quick remark
>
> On Fri, Jan 12, 2018 at 11:48 AM, Emmanuel Deloget <logout@free.fr> wrote:
> > Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as
> > the same check is also performed in the later.
> >
> >
> ...
>
> > +            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 *ec = NULL;
>
> That looks wrong: do you mean EC_KEY instead of EC?
>


​I definitely mean EC_KEY instead of EC. ​

My bad. I'm sending a new version of this patch.


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

​BR,

-- Emmanuel Deloget​
<div dir="ltr"><div class="gmail_default" style="font-family:monospace,monospace">Hello Selva, </div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 12, 2018 at 6:09 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
I will defer to crypto experts for a proper review, but a quick remark<br>
<span class=""><br>
On Fri, Jan 12, 2018 at 11:48 AM, Emmanuel Deloget &lt;<a href="mailto:logout@free.fr">logout@free.fr</a>&gt; wrote:<br>
&gt; Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as<br>
&gt; the same check is also performed in the later.<br>
&gt;<br>
&gt;<br>
</span>...<br>
<span class=""><br>
&gt; +            RSA *rsa = NULL;<br>
&gt; +            DSA *dsa = NULL;<br>
&gt;  #ifndef OPENSSL_NO_EC<br>
&gt; -            else if ((EVP_PKEY_id(pkey) == EVP_PKEY_EC) &amp;&amp; (EVP_PKEY_get0_EC_KEY(pkey) != NULL))<br>
&gt; +            EC *ec = NULL;<br>
<br>
</span>That looks wrong: do you mean EC_KEY instead of EC?<br></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 definitely mean EC_KEY instead of EC. ​</div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">My bad. I&#39;m sending a new version of this patch. </div></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
&gt; +<br>
&gt; +            if ((ec = EVP_PKEY_get0_EC_KEY(pkey)) != NULL)<br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">Selva<br></font></span></blockquote><div><br></div><div class="gmail_default" style="font-family:monospace,monospace">​BR, </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><br></div></div>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff mbox series

Patch

diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 711bba11..9f74acaa 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 = 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 *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));
+            }
+            else if ((dsa = EVP_PKEY_get0_DSA(pkey)) != NULL)
+            {
+                openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA",
+                                 DSA_bits(dsa));
+            }
+
             EVP_PKEY_free(pkey);
         }
         X509_free(cert);