[Openvpn-devel,2/3] OpenSSL: remove some EVP_PKEY type checks
Commit Message
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
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
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"><<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>></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 <<a href="mailto:logout@free.fr">logout@free.fr</a>> wrote:<br>
> Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as<br>
> the same check is also performed in the later.<br>
><br>
><br>
</span>...<br>
<span class=""><br>
> + RSA *rsa = NULL;<br>
> + DSA *dsa = NULL;<br>
> #ifndef OPENSSL_NO_EC<br>
> - else if ((EVP_PKEY_id(pkey) == EVP_PKEY_EC) && (EVP_PKEY_get0_EC_KEY(pkey) != NULL))<br>
> + 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'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>
> +<br>
> + 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
@@ -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);