openssl: incorrect use of HAVE_OPAQUE_EVP_PKEY #2079

Closed
dirkfeytons opened this Issue Nov 14, 2017 · 1 comment

Comments

Projects
None yet
2 participants
Contributor

dirkfeytons commented Nov 14, 2017

In #1956 a fix for #1955 was committed. The latter was caused by the functionality I added in #1904.
However, the fix seems to be incorrect. It is based on the HAVE_OPAQUE_EVP_PKEY define which is only set when OpenSSL 1.1.0+ is detected.
The problematic function EVP_PKEY_id() as reported in #1955 is however much older than that: I have traced it back to commit 7f57b076a60235a3b8c6bec703efde40c6418d07 which is from 2006-05-11 and it seems to have appeared for the first time in release 1.0.0.

What's the preferred way of fixing this? Introduce a new define specifically for EVP_PKEY_id() doesn't strike me as a good solution. Another possibility could be to not put the check for HAVE_OPAQUE_EVP_PKEY around the complete block of code but only around the use of EVP_PKEY_id()?
Something like (warning: untested):

#if !defined(OPENSSL_NO_RSA)
    {
      /* If RSA is used, don't check the private key if its flags indicate
       * it doesn't support it. */
      EVP_PKEY *priv_key = SSL_get_privatekey(ssl);
      int pktype;
#ifdef HAVE_OPAQUE_EVP_PKEY
      pktype = EVP_PKEY_id(priv_key);
#else
      pktype = priv_key->type;
#endif
      if(pktype == EVP_PKEY_RSA) {
        RSA *rsa = EVP_PKEY_get1_RSA(priv_key);
        if(RSA_flags(rsa) & RSA_METHOD_FLAG_NO_CHECK)
          check_privkey = FALSE;
        RSA_free(rsa); /* Decrement reference count */
      }
    }
#endif

I had a quick look at the OpenSSL version reported in #1955 (0.9.8e) and all the other functions that are used in the above code block seem to be available there.

(Side question: is there an official policy on the supported OpenSSL versions?)

Owner

bagder commented Nov 14, 2017

I think your suggested patch looks fine, since before HAVE_OPAQUE_EVP_PKEY the EVP_PKEY type was a struct with usable members. Please make a proper PR out of it so that we can verify that the CI builds also think its fine!

is there an official policy on the supported OpenSSL versions

Yes. It is very conservative and we actually still support 0.9.7. See the Portability section in the INTERNALS document. (I don't think a lot of people would object if we moved that to 0.9.8 but then we don't do that unless there's a need for it...)

dirkfeytons added a commit to dirkfeytons/curl that referenced this issue Nov 14, 2017

@bagder bagder closed this in d3ab7c5 Nov 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment