Skip to content
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

AES-GCM runtime availability check, Advertise / initialize AES-GCM cipher only if supported #197

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

AES-GCM runtime availability check, Advertise / initialize AES-GCM cipher only if supported #197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
410959c
AES-GCM runtime availability check
past-due Oct 17, 2021
9210335
crypto_bcrypt.cpp: Initialize hAlgAES
past-due Oct 17, 2021
b775c2b
Advertise / initialize AES_GCM cipher only if supported
past-due Oct 17, 2021
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
3 changes: 3 additions & 0 deletions src/common/crypto.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ class AES_GCM_CipherContext : public SymmetricCryptContextBase

// Initialize context with the specified private key, IV size, and tag size
bool InitCipher( const void *pKey, size_t cbKey, size_t cbIV, size_t cbTag, bool bEncrypt );

// Determine whether AES_GCM is supported on this system + crypto backend
static bool IsAvailable();
};

class AES_GCM_EncryptContext : public AES_GCM_CipherContext
Expand Down
10 changes: 10 additions & 0 deletions src/common/crypto_bcrypt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ typedef struct _BCryptContext {
ULONG cbKeyObject;

_BCryptContext() {
hAlgAES = INVALID_HANDLE_VALUE;
hKey = INVALID_HANDLE_VALUE;
pbKeyObject = NULL;
cbKeyObject = 0;
Expand Down Expand Up @@ -121,6 +122,15 @@ bool AES_GCM_CipherContext::InitCipher( const void *pKey, size_t cbKey, size_t c
return true;
}

bool AES_GCM_CipherContext::IsAvailable()
{
BCryptContext ctx;
if ( BCryptOpenAlgorithmProvider( &ctx.hAlgAES, BCRYPT_AES_ALGORITHM, nullptr, 0 ) != 0 )
return false;
AssertFatal( ctx.hAlgAES != INVALID_HANDLE_VALUE );
return true;
}

bool AES_GCM_EncryptContext::Encrypt(
const void *pPlaintextData, size_t cbPlaintextData,
const void *pIV,
Expand Down
10 changes: 10 additions & 0 deletions src/common/crypto_libsodium.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@ bool AES_GCM_CipherContext::InitCipher( const void *pKey, size_t cbKey, size_t c
return true;
}

bool AES_GCM_CipherContext::IsAvailable()
{
// Libsodium requires AES and CLMUL instructions for AES-GCM, available in
// Intel "Westmere" and up. 90.41% of Steam users have this as of the
// November 2019 survey.
// Libsodium recommends ChaCha20-Poly1305 in software if you've not got AES support
// in hardware.
return crypto_aead_aes256gcm_is_available() == 1;
}

bool AES_GCM_EncryptContext::Encrypt(
const void *pPlaintextData, size_t cbPlaintextData,
const void *pIV,
Expand Down
5 changes: 5 additions & 0 deletions src/common/crypto_openssl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,11 @@ bool AES_GCM_CipherContext::InitCipher( const void *pKey, size_t cbKey, size_t c
return true;
}

bool AES_GCM_CipherContext::IsAvailable()
{
return true;
}

bool AES_GCM_EncryptContext::Encrypt(
const void *pPlaintextData, size_t cbPlaintextData,
const void *pIV,
Expand Down
42 changes: 29 additions & 13 deletions src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1037,14 +1037,17 @@ void CSteamNetworkConnectionBase::ClearCrypto()
void CSteamNetworkConnectionBase::ClearLocalCrypto()
{
AssertLocksHeldByCurrentThread();
m_eNegotiatedCipher = k_ESteamNetworkingSocketsCipher_INVALID;
m_cbEncryptionOverhead = k_cbAESGCMTagSize;
m_keyExchangePrivateKeyLocal.Wipe();
m_msgCryptLocal.Clear();
m_msgSignedCryptLocal.Clear();
m_bCryptKeysValid = false;
m_cryptContextSend.Wipe();
m_cryptContextRecv.Wipe();
if (m_eNegotiatedCipher != k_ESteamNetworkingSocketsCipher_NULL)
{
m_cryptContextSend.Wipe();
m_cryptContextRecv.Wipe();
}
m_eNegotiatedCipher = k_ESteamNetworkingSocketsCipher_INVALID;
m_cbEncryptionOverhead = k_cbAESGCMTagSize;
Copy link
Contributor

@zpostfacto zpostfacto Oct 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It should be legal to always wipe these, even if they aren't initialized?

Stepping back:

If we're going to support more than one cipher we need to refactor this. because m_cryptContextSend is too generic of a name if it is always going to be an instance of AES-GCM. We probably should add a new abstract interface ISymmetricEncryptContext, with two virtual methods, Encrypt() and the destructor. We would never "wipe" a context, we would just delete it, and the destructor would always securely wipe it. AES_GCM_EncryptContext would implement this interface. (The old mix-in interface design pattern --- yes, multiple-base classes.) So then we would have std::unique_ptr<ISymmetricEncryptContext> m_pEncryptContext. (And the same for decrypt.) So you would deal in concrete types when you were initializing, but once initialization was complete, you would just have the abstract type.

There's a lot of stuff going on in the Steam branch that needs to be merged and cleaned up in crypto land (recently upgraded to a new version of OpenSSL), but if you want to take a stab at it, I can pull that and then I can clean this up when I get back in sync with Steam.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd debated that, but I wasn't sure how extensive a refactor would be eagerly welcomed. 😄

Will take a look when I have a moment. 👍

m_cryptIVSend.Wipe();
m_cryptIVRecv.Wipe();
}
Expand Down Expand Up @@ -1219,19 +1222,29 @@ void CSteamNetworkConnectionBase::SetCryptoCipherList()
// V
case 0:
// Not allowed
m_msgCryptLocal.add_ciphers( k_ESteamNetworkingSocketsCipher_AES_256_GCM );
if (AES_GCM_CipherContext::IsAvailable())
{
m_msgCryptLocal.add_ciphers( k_ESteamNetworkingSocketsCipher_AES_256_GCM );
}
Assert( m_msgCryptLocal.ciphers_size() > 0 );
break;
Copy link
Contributor

@zpostfacto zpostfacto Oct 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think this situation should be fatal to the connection. (If the connection is configured to require encryption, it should be fatal if encryption is not available.)


case 1:
// Allowed, but prefer encrypted
m_msgCryptLocal.add_ciphers( k_ESteamNetworkingSocketsCipher_AES_256_GCM );
if (AES_GCM_CipherContext::IsAvailable())
{
m_msgCryptLocal.add_ciphers( k_ESteamNetworkingSocketsCipher_AES_256_GCM );
}
m_msgCryptLocal.add_ciphers( k_ESteamNetworkingSocketsCipher_NULL );
break;

case 2:
// Allowed, preferred
m_msgCryptLocal.add_ciphers( k_ESteamNetworkingSocketsCipher_NULL );
m_msgCryptLocal.add_ciphers( k_ESteamNetworkingSocketsCipher_AES_256_GCM );
if (AES_GCM_CipherContext::IsAvailable())
{
m_msgCryptLocal.add_ciphers( k_ESteamNetworkingSocketsCipher_AES_256_GCM );
}
break;

case 3:
Expand Down Expand Up @@ -1693,13 +1706,16 @@ bool CSteamNetworkConnectionBase::BFinishCryptoHandshake( bool bServer )
V_memcpy( pStart, &expandTemp, sizeof(SHA256Digest_t) );
}

// Set encryption keys into the contexts, and set parameters
if (
!m_cryptContextSend.Init( cryptKeySend.m_buf, cryptKeySend.k_nSize, m_cryptIVSend.k_nSize, k_cbAESGCMTagSize )
|| !m_cryptContextRecv.Init( cryptKeyRecv.m_buf, cryptKeyRecv.k_nSize, m_cryptIVRecv.k_nSize, k_cbAESGCMTagSize ) )
if (m_eNegotiatedCipher != k_ESteamNetworkingSocketsCipher_NULL)
{
ConnectionState_ProblemDetectedLocally( k_ESteamNetConnectionEnd_Remote_BadCrypt, "Error initializing crypto" );
return false;
// Set encryption keys into the contexts, and set parameters
if (
!m_cryptContextSend.Init( cryptKeySend.m_buf, cryptKeySend.k_nSize, m_cryptIVSend.k_nSize, k_cbAESGCMTagSize )
|| !m_cryptContextRecv.Init( cryptKeyRecv.m_buf, cryptKeyRecv.k_nSize, m_cryptIVRecv.k_nSize, k_cbAESGCMTagSize ) )
{
ConnectionState_ProblemDetectedLocally( k_ESteamNetConnectionEnd_Remote_BadCrypt, "Error initializing crypto" );
return false;
}
}

//
Expand Down