Hi Steffan,
On Wed, Feb 22, 2017 at 11:13 PM, Steffan Karger <stef...@karger.me> wrote:
> Hi,
>
> On 17-02-17 23:00, log...@free.fr wrote:
>> From: Emmanuel Deloget <log...@free.fr>
>>
>> OpenSSL 1.1 does not allow us to directly access the internal of
>> any data type, including RSA_METHOD. We have to use the defined
>> functions to do so.
>>
>> Compatibility with OpenSSL 1.0 is kept by defining the corresponding
>> functions when they are not found in the library.
>>
>> Signed-off-by: Emmanuel Deloget <log...@free.fr>
>> ---
>> configure.ac | 9 +++
>> src/openvpn/openssl_compat.h | 186
>> +++++++++++++++++++++++++++++++++++++++++++
>> src/openvpn/ssl_openssl.c | 22 ++---
>> 3 files changed, 206 insertions(+), 11 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index
>> 789ad08fbaa3b3fc4c95d2b7a22332c0a93aeab4..6f31609d0aeedd2c7841d271ecadd1aa6f3b11da
>> 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -905,6 +905,15 @@ if test "${enable_crypto}" = "yes" -a
>> "${with_crypto_library}" = "openssl"; then
>> X509_STORE_get0_objects \
>> X509_OBJECT_free \
>> X509_OBJECT_get_type \
>> + RSA_meth_new \
>> + RSA_meth_free \
>> + RSA_meth_set_pub_enc \
>> + RSA_meth_set_pub_dec \
>> + RSA_meth_set_priv_enc \
>> + RSA_meth_set_priv_dec \
>> + RSA_meth_set_init \
>> + RSA_meth_set_finish \
>> + RSA_meth_set0_app_data \
>> ],
>> ,
>> []
>> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
>> index
>> 458a6adbe2b3fcd5ea63dcea6596cc24315d463c..b1748754f821f472cf9ed7083ade918336c9b075
>> 100644
>> --- a/src/openvpn/openssl_compat.h
>> +++ b/src/openvpn/openssl_compat.h
>> @@ -41,6 +41,8 @@
>> #include "config-msvc.h"
>> #endif
>>
>> +#include "buffer.h"
>> +
>> #include <openssl/ssl.h>
>> #include <openssl/x509.h>
>>
>> @@ -117,4 +119,188 @@ X509_OBJECT_get_type(const X509_OBJECT *obj)
>> }
>> #endif
>>
>> +#if !defined(HAVE_RSA_METH_NEW)
>> +/**
>> + * Allocate a new RSA method object
>> + *
>> + * @param name The object name
>> + * @param flags Configuration flags
>> + * @return A new RSA method object
>> + */
>> +static inline RSA_METHOD *
>> +RSA_meth_new(const char *name, int flags)
>> +{
>> + RSA_METHOD *rsa_meth = NULL;
>> + ALLOC_OBJ_CLEAR(rsa_meth, RSA_METHOD);
>> + rsa_meth->name = name;
>> + rsa_meth->flags = flags;
>> + return rsa_meth;
>> +}
>> +#endif
>> +
>> +#if !defined(HAVE_RSA_METH_FREE)
>> +/**
>> + * Free an existing RSA_METHOD object
>> + *
>> + * @param meth The RSA_METHOD object
>> + */
>> +static inline void
>> +RSA_meth_free(RSA_METHOD *meth)
>> +{
>> + free(meth);
>> +}
>> +#endif
>
> I think it would be nicer to more closely mimic the 1.1 behaviour in
> RSA_meth_{new,free}(), and copy the name string in new() and free it
> again in free(). That could prevent a future use-after-free that would
> occur for pre-1.1.0, but not 1.1.0+:
I failed to see that when I implemented my solution. I'll give a look
as soon as possible.
> char *mystring = calloc(50, 1);
> RSA_METHOD *meth = RSA_meth_new(mystring, 0);
> free(mystring);
>
> meth.smoke();
> ^^ might cause problems
>
> (Hint: use string_alloc(x, NULL).)
>
>> +
>> +#if !defined(HAVE_RSA_METH_SET_PUB_ENC)
>> +/**
>> + * Set the public encoding function of an RSA_METHOD object
>> + *
>> + * @param meth The RSA_METHOD object
>> + * @param pub_enc the public encoding function
>> + * @return 1 on success, 0 on error
>> + */
>> +static inline int
>> +RSA_meth_set_pub_enc(RSA_METHOD *meth,
>> + int (*pub_enc) (int flen, const unsigned char *from,
>> + unsigned char *to, RSA *rsa,
>> + int padding))
>> +{
>> + if (meth)
>> + {
>> + meth->rsa_pub_enc = pub_enc;
>> + return 1;
>> + }
>> + return 0;
>> +}
>> +#endif
>> +
>> +#if !defined(HAVE_RSA_METH_SET_PUB_DEC)
>> +/**
>> + * Set the public decoding function of an RSA_METHOD object
>> + *
>> + * @param meth The RSA_METHOD object
>> + * @param pub_dec the public decoding function
>> + * @return 1 on success, 0 on error
>> + */
>> +static inline int
>> +RSA_meth_set_pub_dec(RSA_METHOD *meth,
>> + int (*pub_dec) (int flen, const unsigned char *from,
>> + unsigned char *to, RSA *rsa,
>> + int padding))
>> +{
>> + if (meth)
>> + {
>> + meth->rsa_pub_dec = pub_dec;
>> + return 1;
>> + }
>> + return 0;
>> +}
>> +#endif
>> +
>> +#if !defined(HAVE_RSA_METH_SET_PRIV_ENC)
>> +/**
>> + * Set the private encoding function of an RSA_METHOD object
>> + *
>> + * @param meth The RSA_METHOD object
>> + * @param priv_enc the private encoding function
>> + * @return 1 on success, 0 on error
>> + */
>> +static inline int
>> +RSA_meth_set_priv_enc(RSA_METHOD *meth,
>> + int (*priv_enc) (int flen, const unsigned char *from,
>> + unsigned char *to, RSA *rsa,
>> + int padding))
>> +{
>> + if (meth)
>> + {
>> + meth->rsa_priv_enc = priv_enc;
>> + return 1;
>> + }
>> + return 0;
>> +}
>> +#endif
>> +
>> +#if !defined(HAVE_RSA_METH_SET_PRIV_DEC)
>> +/**
>> + * Set the private decoding function of an RSA_METHOD object
>> + *
>> + * @param meth The RSA_METHOD object
>> + * @param priv_dec the private decoding function
>> + * @return 1 on success, 0 on error
>> + */
>> +static inline int
>> +RSA_meth_set_priv_dec(RSA_METHOD *meth,
>> + int (*priv_dec) (int flen, const unsigned char *from,
>> + unsigned char *to, RSA *rsa,
>> + int padding))
>> +{
>> + if (meth)
>> + {
>> + meth->rsa_priv_dec = priv_dec;
>> + return 1;
>> + }
>> + return 0;
>> +}
>> +#endif
>> +
>> +#if !defined(HAVE_RSA_METH_SET_INIT)
>> +/**
>> + * Set the init function of an RSA_METHOD object
>> + *
>> + * @param meth The RSA_METHOD object
>> + * @param init the init function
>> + * @return 1 on success, 0 on error
>> + */
>> +static inline int
>> +RSA_meth_set_init(RSA_METHOD *meth, int (*init) (RSA *rsa))
>> +{
>> + if (meth)
>> + {
>> + meth->init = init;
>> + return 1;
>> + }
>> + return 0;
>> +}
>> +#endif
>> +
>> +#if !defined(HAVE_RSA_METH_SET_FINISH)
>> +/**
>> + * Set the finish function of an RSA_METHOD object
>> + *
>> + * @param meth The RSA_METHOD object
>> + * @param finish the finish function
>> + * @return 1 on success, 0 on error
>> + */
>> +static inline int
>> +RSA_meth_set_finish(RSA_METHOD *meth, int (*finish) (RSA *rsa))
>> +{
>> + if (meth)
>> + {
>> + meth->finish = finish;
>> + return 1;
>> + }
>> + return 0;
>> +}
>> +#endif
>> +
>> +#if !defined(HAVE_RSA_METH_SET0_APP_DATA)
>> +/**
>> + * Set the application data of an RSA_METHOD object
>> + *
>> + * @param meth The RSA_METHOD object
>> + * @param app_data Application data
>> + * @return 1 on success, 0 on error
>> + */
>> +static inline int
>> +RSA_meth_set0_app_data(RSA_METHOD *meth, void *app_data)
>> +{
>> + if (meth)
>> + {
>> + meth->app_data = app_data;
>> + return 1;
>> + }
>> + return 0;
>> +}
>> +#endif
>> +
>> #endif /* OPENSSL_COMPAT_H_ */
>> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
>> index
>> bf0f643f25439f71cbfe71bf5a7e8eb834b0f012..f011e06702529ff34e91f6d0169d1adf8cc9d767
>> 100644
>> --- a/src/openvpn/ssl_openssl.c
>> +++ b/src/openvpn/ssl_openssl.c
>> @@ -978,7 +978,7 @@ rsa_priv_dec(int flen, const unsigned char *from,
>> unsigned char *to, RSA *rsa, i
>> static int
>> rsa_finish(RSA *rsa)
>> {
>> - free((void *)rsa->meth);
>> + RSA_meth_free(rsa->meth);
>> rsa->meth = NULL;
>> return 1;
>> }
>
> This change still works, but he follow up change in this method in 07/15
> causes problems:
>
> static int
> rsa_finish(RSA *rsa)
> {
> - RSA_meth_free(rsa->meth);
> - rsa->meth = NULL;
> + RSA_METHOD *meth = (RSA_METHOD *)RSA_get_method(rsa);
> + RSA_meth_free(meth);
> + RSA_set_method(rsa, NULL);
> return 1;
> }
>
> Casting away const on the object returned by RSA_get_method() is a
> smell, but it really fails because RSA_set_method(rsa, NULL) calls
> rsa->meth->finish(), which is implemented by this very function. That
> means that RSA_meth_free() will perform a double free on meth->name.
>
> I briefly looked into a fix, but didn't immediately see a nice solution
> here. At least the RSA_set_method(rsa, RSA_get_default_method())
> doesn't work either, because you'll end up with rsa_finish() and
> RSA_set_method() calling each other infinitely...
Ouch. I failed to see that as well. Sorry :)
I'll try to find a better solution to this issue as well.
> (Noting this here, because the fix for 07 might cause changes to this
> patch too.)
>
>> @@ -1053,16 +1053,16 @@ tls_ctx_use_external_private_key(struct tls_root_ctx
>> *ctx,
>> ASSERT(NULL != cert);
>>
>> /* allocate custom RSA method object */
>> - ALLOC_OBJ_CLEAR(rsa_meth, RSA_METHOD);
>> - rsa_meth->name = "OpenVPN external private key RSA Method";
>> - rsa_meth->rsa_pub_enc = rsa_pub_enc;
>> - rsa_meth->rsa_pub_dec = rsa_pub_dec;
>> - rsa_meth->rsa_priv_enc = rsa_priv_enc;
>> - rsa_meth->rsa_priv_dec = rsa_priv_dec;
>> - rsa_meth->init = NULL;
>> - rsa_meth->finish = rsa_finish;
>> - rsa_meth->flags = RSA_METHOD_FLAG_NO_CHECK;
>> - rsa_meth->app_data = NULL;
>> + rsa_meth = RSA_meth_new("OpenVPN external private key RSA Method",
>> + RSA_METHOD_FLAG_NO_CHECK);
>> + check_malloc_return(rsa_meth);
>> + RSA_meth_set_pub_enc(rsa_meth, rsa_pub_enc);
>> + RSA_meth_set_pub_dec(rsa_meth, rsa_pub_dec);
>> + RSA_meth_set_priv_enc(rsa_meth, rsa_priv_enc);
>> + RSA_meth_set_priv_dec(rsa_meth, rsa_priv_dec);
>> + RSA_meth_set_init(rsa_meth, NULL);
>> + RSA_meth_set_finish(rsa_meth, rsa_finish);
>> + RSA_meth_set0_app_data(rsa_meth, NULL);
>>
>> /* allocate RSA object */
>> rsa = RSA_new();
>>
>
> -Steffan
>
Best regards,
-- Emmanuel Deloget
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
