Fix #361 adding conditions to differentiate OpenSSL v1.0.2 and v1.1.x in CryptoImpl #362
Conversation
OpenSSL v1.1 doesn't expose structures directly, instead pointers are used. Change the existing code to prepare for that to make it clearer what the actual OpenSSL v1.1 changes entail.
- Insert conditions to prevent incompatibility with OpenSSL v1.0.x - Align function calls and access to variables
|
I've pulled this into an internal repo where we can have our security team review it. Thanks for doing this work. |
| { | ||
| namespace OpenSSL | ||
| { | ||
| #if OPENSSL_VERSION_NUMBER < 0x10100003L |
There was a problem hiding this comment.
I believe the version numbers should have a 1 (or a zero, doesn't really matter) as the last digit, so that people on 1.1b and 1.1a work as well.
There was a problem hiding this comment.
From https://wiki.openssl.org/index.php/Manual:OPENSSL_VERSION_NUMBER(3)
MNNFFPPS: Major miNor miNor Fix Fix Patch Patch Status
The last nibble is for the release candidates. The api changes were introduced in 1.1 rc 3, which is not the same as 1.1c
| ERR_load_CRYPTO_strings(); | ||
| OPENSSL_add_all_algorithms_noconf(); | ||
|
|
||
| #if OPENSSL_VERSION_NUMBER < 0x10100003L |
There was a problem hiding this comment.
Why is the locking setup removed here? Is there some documentation somewhere on why it's no longer necessary. I can't find anything about it being deprecated.
There was a problem hiding this comment.
Check the source code of openssl-1.1.0, include/openssl/crypto.h, from line 208
/*
- The old locking functions have been removed completely without compatibility
- macros. This is because the old functions either could not properly report
- errors, or the returned error values were not clearly documented.
- Replacing the locking functions with with no-ops would cause race condition
- issues in the affected applications. It is far better for them to fail at
- compile time.
- On the other hand, the locking callbacks are no longer used. Consequently,
- the callback management functions can be safely replaced with no-op macros.
*/
|
Any update on this? Is anything wrong with the patches? Also, without OpenSSL 1.1 support, TLSv1.3 won't be possible either... |
|
We've got a lot on our plate at the moment so no one's had a chance to go through and test it thoroughly yet. |
|
Thanks for the update Bret. |
|
What's up with this PR? |
| ctx = &_ctx; | ||
| HMAC_CTX_init(ctx); | ||
| #else | ||
| ctx=HMAC_CTX_new(); |
There was a problem hiding this comment.
Why are we now allocating things on the heap? Is this required for the latest OpenSSL?
There was a problem hiding this comment.
I believe so. Several structures have been made opaque in OpenSSL 1.1, so the OpenSSL allocators have to be used, see https://www.openssl.org/docs/man1.1.0/crypto/HMAC_CTX_free.html unless there is something I missed.
| toMove.m_ctx.cipher = nullptr; | ||
| toMove.m_ctx.cipher_data = nullptr; | ||
| toMove.m_ctx.engine = nullptr; | ||
| EVP_CIPHER_CTX_copy(m_ctx, toMove.m_ctx); |
There was a problem hiding this comment.
I like this change!
There was a problem hiding this comment.
This actually needs updating, as m_ctx hasn't been assigned yet. I believe a call to init() should be sufficient.
| EVP_CIPHER_CTX_init(&m_ctx); | ||
| #if OPENSSL_VERSION_NUMBER < 0x10100003L | ||
| m_ctx = &_m_ctx; | ||
| EVP_CIPHER_CTX_init(m_ctx); |
There was a problem hiding this comment.
not sure why this is inside the compiletime ifdefs, isn't it the same between versions?
There was a problem hiding this comment.
Thanks, well spotted. EVP_CIPHER_CTX_init() is obsolete in OpenSSL 1.1, and they
# define EVP_CIPHER_CTX_init(c) EVP_CIPHER_CTX_reset(c) in evp.h
So the call should really be to EVP_CIPHER_CTX_reset() instead. Can you apply that change before merging, or do you want me to update the commit?
| m_ctx = &_m_ctx; | ||
| EVP_CIPHER_CTX_init(m_ctx); | ||
| #else | ||
| m_ctx = EVP_CIPHER_CTX_new(); |
There was a problem hiding this comment.
Uggh, I just tried to make this not need all the #ifdef checks until I realized the *new and *free functions where not part of the 1.0 interface. I'll be merging this.
In the meantime the new and free needs to be moved to constructor and destructors, then I'll merge it over.
|
I am closing this PR because it's similar with #507 |
6 participants
OpenSSL v1.1 issue has been mentioned in both #361