Skip to content

Fix #361 adding conditions to differentiate OpenSSL v1.0.2 and v1.1.x in CryptoImpl #362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Fix #361 adding conditions to differentiate OpenSSL v1.0.2 and v1.1.x in CryptoImpl #362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ec7b1a
CryptoImpl: prepare for OpenSSL v1.1.x support
Dec 5, 2016
dda23b3
CryptoImpl: support for OpenSSL v1.1.x
Dec 5, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion aws-cpp-sdk-core/include/aws/core/utils/crypto/openssl/CryptoImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,10 @@ namespace Aws

virtual size_t GetKeyLengthBits() const = 0;

EVP_CIPHER_CTX m_ctx;
EVP_CIPHER_CTX *m_ctx;
#if OPENSSL_VERSION_NUMBER < 0x10100003L
EVP_CIPHER_CTX _m_ctx;
#endif

void CheckInitEncryptor();
void CheckInitDecryptor();
Expand Down
104 changes: 62 additions & 42 deletions aws-cpp-sdk-core/source/utils/crypto/openssl/CryptoImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,16 +35,19 @@ namespace Aws
{
namespace OpenSSL
{
#if OPENSSL_VERSION_NUMBER < 0x10100003L
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@andred andred Dec 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

static const char* OPENSSL_INTERNALS_TAG = "OpenSSLCallbackState";
static std::mutex* locks(nullptr);

#endif
GetTheLights getTheLights;

void init_static_state()
{
ERR_load_CRYPTO_strings();
OPENSSL_add_all_algorithms_noconf();

#if OPENSSL_VERSION_NUMBER < 0x10100003L
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@jliang1010 jliang1010 Dec 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
    */

if (!CRYPTO_get_locking_callback())
{
locks = Aws::NewArray<std::mutex>(static_cast<size_t>(CRYPTO_num_locks()),
Expand All @@ -56,12 +59,14 @@ namespace Aws
{
CRYPTO_set_id_callback(&id_fn);
}
#endif

RAND_poll();
}

void cleanup_static_state()
{
#if OPENSSL_VERSION_NUMBER < 0x10100003L
if (CRYPTO_get_locking_callback() == &locking_fn)
{
CRYPTO_set_id_callback(nullptr);
Expand All @@ -74,8 +79,10 @@ namespace Aws
{
CRYPTO_set_locking_callback(nullptr);
}
#endif
}

#if OPENSSL_VERSION_NUMBER < 0x10100003L
void locking_fn(int mode, int n, const char*, int)
{
if (mode & CRYPTO_LOCK)
Expand All @@ -92,6 +99,7 @@ namespace Aws
{
return static_cast<unsigned long>(std::hash<std::thread::id>()(std::this_thread::get_id()));
}
#endif
}

void SecureRandomBytes_OpenSSLImpl::GetBytes(unsigned char* buffer, size_t bufferSize)
Expand Down Expand Up @@ -204,14 +212,24 @@ namespace Aws
ByteBuffer digest(length);
memset(digest.GetUnderlyingData(), 0, length);

HMAC_CTX ctx;
HMAC_CTX_init(&ctx);
HMAC_CTX *ctx;
#if OPENSSL_VERSION_NUMBER < 0x10100003L
HMAC_CTX _ctx;
ctx = &_ctx;
HMAC_CTX_init(ctx);
#else
ctx=HMAC_CTX_new();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we now allocating things on the heap? Is this required for the latest OpenSSL?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#endif

HMAC_Init_ex(&ctx, secret.GetUnderlyingData(), static_cast<int>(secret.GetLength()), EVP_sha256(),
HMAC_Init_ex(ctx, secret.GetUnderlyingData(), static_cast<int>(secret.GetLength()), EVP_sha256(),
NULL);
HMAC_Update(&ctx, toSign.GetUnderlyingData(), toSign.GetLength());
HMAC_Final(&ctx, digest.GetUnderlyingData(), &length);
HMAC_CTX_cleanup(&ctx);
HMAC_Update(ctx, toSign.GetUnderlyingData(), toSign.GetLength());
HMAC_Final(ctx, digest.GetUnderlyingData(), &length);
#if OPENSSL_VERSION_NUMBER < 0x10100003L
HMAC_CTX_cleanup(ctx);
#else
HMAC_CTX_free(ctx);
#endif

return HashResult(std::move(digest));
}
Expand All @@ -238,10 +256,7 @@ namespace Aws
OpenSSLCipher::OpenSSLCipher(OpenSSLCipher&& toMove) : SymmetricCipher(std::move(toMove)),
m_encDecInitialized(false)
{
m_ctx = toMove.m_ctx;
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually needs updating, as m_ctx hasn't been assigned yet. I believe a call to init() should be sufficient.


m_encDecInitialized = toMove.m_encDecInitialized;
m_encryptionMode = toMove.m_encryptionMode;
Expand Down Expand Up @@ -271,7 +286,13 @@ namespace Aws

void OpenSSLCipher::Init()
{
EVP_CIPHER_CTX_init(&m_ctx);
#if OPENSSL_VERSION_NUMBER < 0x10100003L
m_ctx = &_m_ctx;
EVP_CIPHER_CTX_init(m_ctx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why this is inside the compiletime ifdefs, isn't it the same between versions?

Copy link
Contributor

@andred andred Apr 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

#else
m_ctx = EVP_CIPHER_CTX_new();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

EVP_CIPHER_CTX_init(m_ctx);
#endif
}

void OpenSSLCipher::CheckInitEncryptor()
Expand Down Expand Up @@ -312,7 +333,7 @@ namespace Aws
int lengthWritten = static_cast<int>(unEncryptedData.GetLength() + (GetBlockSizeBytes() - 1));
CryptoBuffer encryptedText(static_cast<size_t>( lengthWritten + (GetBlockSizeBytes() - 1)));

if (!EVP_EncryptUpdate(&m_ctx, encryptedText.GetUnderlyingData(), &lengthWritten,
if (!EVP_EncryptUpdate(m_ctx, encryptedText.GetUnderlyingData(), &lengthWritten,
unEncryptedData.GetUnderlyingData(),
static_cast<int>(unEncryptedData.GetLength())))
{
Expand Down Expand Up @@ -340,7 +361,7 @@ namespace Aws

CryptoBuffer finalBlock(GetBlockSizeBytes());
int writtenSize = 0;
if (!EVP_EncryptFinal_ex(&m_ctx, finalBlock.GetUnderlyingData(), &writtenSize))
if (!EVP_EncryptFinal_ex(m_ctx, finalBlock.GetUnderlyingData(), &writtenSize))
{
m_failure = true;
LogErrors();
Expand All @@ -361,7 +382,7 @@ namespace Aws
int lengthWritten = static_cast<int>(encryptedData.GetLength() + (GetBlockSizeBytes() - 1));
CryptoBuffer decryptedText(static_cast<size_t>(lengthWritten));

if (!EVP_DecryptUpdate(&m_ctx, decryptedText.GetUnderlyingData(), &lengthWritten,
if (!EVP_DecryptUpdate(m_ctx, decryptedText.GetUnderlyingData(), &lengthWritten,
encryptedData.GetUnderlyingData(),
static_cast<int>(encryptedData.GetLength())))
{
Expand Down Expand Up @@ -389,7 +410,7 @@ namespace Aws

CryptoBuffer finalBlock(GetBlockSizeBytes());
int writtenSize = static_cast<int>(finalBlock.GetLength());
if (!EVP_DecryptFinal_ex(&m_ctx, finalBlock.GetUnderlyingData(), &writtenSize))
if (!EVP_DecryptFinal_ex(m_ctx, finalBlock.GetUnderlyingData(), &writtenSize))
{
m_failure = true;
LogErrors();
Expand All @@ -411,14 +432,13 @@ namespace Aws
m_encryptionMode = false;
m_decryptionMode = false;

if (m_ctx.cipher || m_ctx.cipher_data || m_ctx.engine)
{
EVP_CIPHER_CTX_cleanup(&m_ctx);
}
#if OPENSSL_VERSION_NUMBER < 0x10100003L
EVP_CIPHER_CTX_cleanup(m_ctx);
#else
EVP_CIPHER_CTX_free(m_ctx);
#endif

m_ctx.cipher = nullptr;
m_ctx.cipher_data = nullptr;
m_ctx.engine = nullptr;
m_ctx = nullptr;
}

size_t AES_CBC_Cipher_OpenSSL::BlockSizeBytes = 16;
Expand All @@ -439,7 +459,7 @@ namespace Aws

void AES_CBC_Cipher_OpenSSL::InitEncryptor_Internal()
{
if (!EVP_EncryptInit_ex(&m_ctx, EVP_aes_256_cbc(), nullptr, m_key.GetUnderlyingData(),
if (!EVP_EncryptInit_ex(m_ctx, EVP_aes_256_cbc(), nullptr, m_key.GetUnderlyingData(),
m_initializationVector.GetUnderlyingData()))
{
m_failure = true;
Expand All @@ -449,7 +469,7 @@ namespace Aws

void AES_CBC_Cipher_OpenSSL::InitDecryptor_Internal()
{
if (!EVP_DecryptInit_ex(&m_ctx, EVP_aes_256_cbc(), nullptr, m_key.GetUnderlyingData(),
if (!EVP_DecryptInit_ex(m_ctx, EVP_aes_256_cbc(), nullptr, m_key.GetUnderlyingData(),
m_initializationVector.GetUnderlyingData()))
{
m_failure = true;
Expand Down Expand Up @@ -486,9 +506,9 @@ namespace Aws

void AES_CTR_Cipher_OpenSSL::InitEncryptor_Internal()
{
if (!(EVP_EncryptInit_ex(&m_ctx, EVP_aes_256_ctr(), nullptr, m_key.GetUnderlyingData(),
if (!(EVP_EncryptInit_ex(m_ctx, EVP_aes_256_ctr(), nullptr, m_key.GetUnderlyingData(),
m_initializationVector.GetUnderlyingData())
&& EVP_CIPHER_CTX_set_padding(&m_ctx, 0)))
&& EVP_CIPHER_CTX_set_padding(m_ctx, 0)))
{
m_failure = true;
LogErrors(CTR_LOG_TAG);
Expand All @@ -497,9 +517,9 @@ namespace Aws

void AES_CTR_Cipher_OpenSSL::InitDecryptor_Internal()
{
if (!(EVP_DecryptInit_ex(&m_ctx, EVP_aes_256_ctr(), nullptr, m_key.GetUnderlyingData(),
if (!(EVP_DecryptInit_ex(m_ctx, EVP_aes_256_ctr(), nullptr, m_key.GetUnderlyingData(),
m_initializationVector.GetUnderlyingData())
&& EVP_CIPHER_CTX_set_padding(&m_ctx, 0)))
&& EVP_CIPHER_CTX_set_padding(m_ctx, 0)))
{
m_failure = true;
LogErrors(CTR_LOG_TAG);
Expand Down Expand Up @@ -541,7 +561,7 @@ namespace Aws
{
CryptoBuffer&& finalBuffer = OpenSSLCipher::FinalizeEncryption();
m_tag = CryptoBuffer(TagLengthBytes);
if (!EVP_CIPHER_CTX_ctrl(&m_ctx, EVP_CTRL_CCM_GET_TAG, static_cast<int>(m_tag.GetLength()),
if (!EVP_CIPHER_CTX_ctrl(m_ctx, EVP_CTRL_CCM_GET_TAG, static_cast<int>(m_tag.GetLength()),
m_tag.GetUnderlyingData()))
{
m_failure = true;
Expand All @@ -554,10 +574,10 @@ namespace Aws

void AES_GCM_Cipher_OpenSSL::InitEncryptor_Internal()
{
if (!(EVP_EncryptInit_ex(&m_ctx, EVP_aes_256_gcm(), nullptr, nullptr, nullptr) &&
EVP_EncryptInit_ex(&m_ctx, nullptr, nullptr, m_key.GetUnderlyingData(),
if (!(EVP_EncryptInit_ex(m_ctx, EVP_aes_256_gcm(), nullptr, nullptr, nullptr) &&
EVP_EncryptInit_ex(m_ctx, nullptr, nullptr, m_key.GetUnderlyingData(),
m_initializationVector.GetUnderlyingData()) &&
EVP_CIPHER_CTX_set_padding(&m_ctx, 0)))
EVP_CIPHER_CTX_set_padding(m_ctx, 0)))
{
m_failure = true;
LogErrors(GCM_LOG_TAG);
Expand All @@ -566,10 +586,10 @@ namespace Aws

void AES_GCM_Cipher_OpenSSL::InitDecryptor_Internal()
{
if (!(EVP_DecryptInit_ex(&m_ctx, EVP_aes_256_gcm(), nullptr, nullptr, nullptr) &&
EVP_DecryptInit_ex(&m_ctx, nullptr, nullptr, m_key.GetUnderlyingData(),
if (!(EVP_DecryptInit_ex(m_ctx, EVP_aes_256_gcm(), nullptr, nullptr, nullptr) &&
EVP_DecryptInit_ex(m_ctx, nullptr, nullptr, m_key.GetUnderlyingData(),
m_initializationVector.GetUnderlyingData()) &&
EVP_CIPHER_CTX_set_padding(&m_ctx, 0)))
EVP_CIPHER_CTX_set_padding(m_ctx, 0)))
{
m_failure = true;
LogErrors(GCM_LOG_TAG);
Expand All @@ -588,7 +608,7 @@ namespace Aws
return;
}

if (!EVP_CIPHER_CTX_ctrl(&m_ctx, EVP_CTRL_GCM_SET_TAG, static_cast<int>(m_tag.GetLength()),
if (!EVP_CIPHER_CTX_ctrl(m_ctx, EVP_CTRL_GCM_SET_TAG, static_cast<int>(m_tag.GetLength()),
m_tag.GetUnderlyingData()))
{
m_failure = true;
Expand Down Expand Up @@ -672,7 +692,7 @@ namespace Aws
memcpy(tempInput.GetUnderlyingData() + BlockSizeBytes, r, BlockSizeBytes);

//encrypt the concatenated A and R[I] and store it in B
if (!EVP_EncryptUpdate(&m_ctx, b.GetUnderlyingData(), &outLen,
if (!EVP_EncryptUpdate(m_ctx, b.GetUnderlyingData(), &outLen,
tempInput.GetUnderlyingData(), static_cast<int>(tempInput.GetLength())))
{
LogErrors(KEY_WRAP_TAG);
Expand Down Expand Up @@ -749,7 +769,7 @@ namespace Aws
memcpy(tempInput.GetUnderlyingData() + BlockSizeBytes, r, BlockSizeBytes);

//Decrypt the concatenated buffer
if(!EVP_DecryptUpdate(&m_ctx, b.GetUnderlyingData(), &outLen,
if(!EVP_DecryptUpdate(m_ctx, b.GetUnderlyingData(), &outLen,
tempInput.GetUnderlyingData(), static_cast<int>(tempInput.GetLength())))
{
m_failure = true;
Expand Down Expand Up @@ -784,8 +804,8 @@ namespace Aws

void AES_KeyWrap_Cipher_OpenSSL::InitEncryptor_Internal()
{
if (!EVP_EncryptInit_ex(&m_ctx, EVP_aes_256_ecb(), nullptr, m_key.GetUnderlyingData(), nullptr) &&
EVP_CIPHER_CTX_set_padding(&m_ctx, 0))
if (!EVP_EncryptInit_ex(m_ctx, EVP_aes_256_ecb(), nullptr, m_key.GetUnderlyingData(), nullptr) &&
EVP_CIPHER_CTX_set_padding(m_ctx, 0))
{
m_failure = true;
LogErrors(KEY_WRAP_TAG);
Expand All @@ -794,8 +814,8 @@ namespace Aws

void AES_KeyWrap_Cipher_OpenSSL::InitDecryptor_Internal()
{
if (!(EVP_DecryptInit_ex(&m_ctx, EVP_aes_256_ecb(), nullptr, m_key.GetUnderlyingData(), nullptr) &&
EVP_CIPHER_CTX_set_padding(&m_ctx, 0)))
if (!(EVP_DecryptInit_ex(m_ctx, EVP_aes_256_ecb(), nullptr, m_key.GetUnderlyingData(), nullptr) &&
EVP_CIPHER_CTX_set_padding(m_ctx, 0)))
{
m_failure = true;
LogErrors(KEY_WRAP_TAG);
Expand Down