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 1 commit
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
Next Next commit
CryptoImpl: prepare for OpenSSL v1.1.x support 
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.
  • Loading branch information
Jian Liang committed Dec 5, 2016
commit 7ec7b1a7566bbaa37a3a9e1416feabb3cd97c67a
3 changes: 2 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,8 @@ namespace Aws

virtual size_t GetKeyLengthBits() const = 0;

EVP_CIPHER_CTX m_ctx;
EVP_CIPHER_CTX *m_ctx;
EVP_CIPHER_CTX _m_ctx;

void CheckInitEncryptor();
void CheckInitDecryptor();
Expand Down
79 changes: 37 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 @@ -204,14 +204,16 @@ namespace Aws
ByteBuffer digest(length);
memset(digest.GetUnderlyingData(), 0, length);

HMAC_CTX ctx;
HMAC_CTX_init(&ctx);
HMAC_CTX *ctx;
HMAC_CTX _ctx;
ctx = &_ctx;
HMAC_CTX_init(ctx);

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);
HMAC_CTX_cleanup(ctx);

return HashResult(std::move(digest));
}
Expand All @@ -238,10 +240,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

@JonathanHenson JonathanHenson Mar 31, 2017

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

@andred andred Apr 20, 2017

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 +270,8 @@ namespace Aws

void OpenSSLCipher::Init()
{
EVP_CIPHER_CTX_init(&m_ctx);
m_ctx = &_m_ctx;
EVP_CIPHER_CTX_init(m_ctx);
Copy link
Contributor

@JonathanHenson JonathanHenson Mar 31, 2017

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?

}

void OpenSSLCipher::CheckInitEncryptor()
Expand Down Expand Up @@ -312,7 +312,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 +340,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 +361,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 +389,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 +411,9 @@ 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);
}
EVP_CIPHER_CTX_cleanup(m_ctx);

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 +434,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 +444,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 +481,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 +492,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 +536,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 +549,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 +561,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 +583,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 +667,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 +744,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 +779,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 +789,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