Skip to content

Commit

Permalink
Refactor to add some crypto agility.
Browse files Browse the repository at this point in the history 
Introduce ISymmetricEncryptContext and ISymmetricDecryptContext abstract
interfaces.

Connection class will hold a pointer to these, instead of having members to
AES-GCM concrete types directly.

In a few places we no longer need a switch on the cipher type, since we
will rely on virtual function dispatch instead.

This should make it much easier to address #196

P4:6878145
  • Loading branch information
@zpostfacto
zpostfacto committed Nov 5, 2021
1 parent f6c05bb commit 299cbad
Show file tree
Hide file tree
Showing 4 changed files with 159 additions and 125 deletions.
49 changes: 40 additions & 9 deletions src/common/crypto.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,36 @@ class SymmetricCryptContextBase
uint32 m_cbIV, m_cbTag;
};

// Base class for AES-GCM encryption and ddecryption
/// Abstract interface for symmetric encryption.
struct ISymmetricEncryptContext
{
virtual ~ISymmetricEncryptContext() {}

// Encrypt data and append auth tag
virtual bool Encrypt(
const void *pPlaintextData, size_t cbPlaintextData,
const void *pIV,
void *pEncryptedDataAndTag, uint32 *pcbEncryptedDataAndTag,
const void *pAdditionalAuthenticationData, size_t cbAuthenticationData // Optional additional authentication data. Not encrypted, but will be included in the tag, so it can be authenticated.
) = 0;
};

/// Abstract interface for symmetric decryption
struct ISymmetricDecryptContext
{
virtual ~ISymmetricDecryptContext() {}

// Decrypt data and check auth tag, which is assumed to be at the end
virtual bool Decrypt(
const void *pEncryptedDataAndTag, size_t cbEncryptedDataAndTag,
const void *pIV,
void *pPlaintextData, uint32 *pcbPlaintextData,
const void *pAdditionalAuthenticationData, size_t cbAuthenticationData // Optional additional authentication data. Not encrypted, but will be included in the tag, so it can be authenticated.
) = 0;
};


/// Base class for AES-GCM encryption and decryption
class AES_GCM_CipherContext : public SymmetricCryptContextBase
{
public:
Expand All @@ -39,7 +68,8 @@ class AES_GCM_CipherContext : public SymmetricCryptContextBase
bool InitCipher( const void *pKey, size_t cbKey, size_t cbIV, size_t cbTag, bool bEncrypt );
};

class AES_GCM_EncryptContext : public AES_GCM_CipherContext
/// Encryption context for AES-GCM
class AES_GCM_EncryptContext final : public AES_GCM_CipherContext, public ISymmetricEncryptContext
{
public:

Expand All @@ -49,16 +79,17 @@ class AES_GCM_EncryptContext : public AES_GCM_CipherContext
return InitCipher( pKey, cbKey, cbIV, cbTag, true );
}

// Encrypt data and append auth tag
bool Encrypt(
// Implements ISymmetricEncryptContext
virtual bool Encrypt(
const void *pPlaintextData, size_t cbPlaintextData,
const void *pIV,
void *pEncryptedDataAndTag, uint32 *pcbEncryptedDataAndTag,
const void *pAdditionalAuthenticationData, size_t cbAuthenticationData // Optional additional authentication data. Not encrypted, but will be included in the tag, so it can be authenticated.
);
) override;
};

class AES_GCM_DecryptContext : public AES_GCM_CipherContext
/// Decryption context for AES-GCM
class AES_GCM_DecryptContext final : public AES_GCM_CipherContext, public ISymmetricDecryptContext
{
public:

Expand All @@ -68,13 +99,13 @@ class AES_GCM_DecryptContext : public AES_GCM_CipherContext
return InitCipher( pKey, cbKey, cbIV, cbTag, false );
}

// Decrypt data and check auth tag, which is assumed to be at the end
bool Decrypt(
// Implements ISymmetricDecryptContext
virtual bool Decrypt(
const void *pEncryptedDataAndTag, size_t cbEncryptedDataAndTag,
const void *pIV,
void *pPlaintextData, uint32 *pcbPlaintextData,
const void *pAdditionalAuthenticationData, size_t cbAuthenticationData // Optional additional authentication data. Not encrypted, but will be included in the tag, so it can be authenticated.
);
) override;
};

namespace CCrypto
Expand Down
137 changes: 75 additions & 62 deletions src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1043,8 +1043,8 @@ void CSteamNetworkConnectionBase::ClearLocalCrypto()
m_msgCryptLocal.Clear();
m_msgSignedCryptLocal.Clear();
m_bCryptKeysValid = false;
m_cryptContextSend.Wipe();
m_cryptContextRecv.Wipe();
m_pCryptContextSend.reset();
m_pCryptContextRecv.reset();
m_cryptIVSend.Wipe();
m_cryptIVRecv.Wipe();
}
Expand Down Expand Up @@ -1712,12 +1712,32 @@ ESteamNetConnectionEnd CSteamNetworkConnectionBase::FinishCryptoHandshake( bool
}

// 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 ) )
switch ( m_eNegotiatedCipher )
{
V_strcpy_safe( errMsg, "Error initializing crypto" );
return k_ESteamNetConnectionEnd_Remote_BadCrypt;
default:
Assert( false );
V_strcpy_safe( errMsg, "Internal error!" );
return k_ESteamNetConnectionEnd_Misc_InternalError;

case k_ESteamNetworkingSocketsCipher_NULL:
break;

case k_ESteamNetworkingSocketsCipher_AES_256_GCM:
{
auto *pSend = new AES_GCM_EncryptContext;
auto *pRecv = new AES_GCM_DecryptContext;
m_pCryptContextSend.reset( pSend );
m_pCryptContextRecv.reset( pRecv );

if (
!pSend->Init( cryptKeySend.m_buf, cryptKeySend.k_nSize, m_cryptIVSend.k_nSize, k_cbAESGCMTagSize )
|| !pRecv->Init( cryptKeyRecv.m_buf, cryptKeyRecv.k_nSize, m_cryptIVRecv.k_nSize, k_cbAESGCMTagSize ) )
{
V_strcpy_safe( errMsg, "Error initializing crypto" );
return k_ESteamNetConnectionEnd_Remote_BadCrypt;
}
} break;

}

//
Expand Down Expand Up @@ -2112,66 +2132,59 @@ bool CSteamNetworkConnectionBase::DecryptDataChunk( uint16 nWireSeqNum, int cbPa
}

// What cipher are we using?
switch ( m_eNegotiatedCipher )
if ( m_eNegotiatedCipher == k_ESteamNetworkingSocketsCipher_NULL )
{
default:
AssertMsg1( false, "Bogus cipher %d", m_eNegotiatedCipher );
// No encryption!
ctx.m_cbPlainText = cbChunk;
ctx.m_pPlainText = pChunk;
}
else if ( !m_pCryptContextRecv )
{
AssertMsg1( false, "Bogus cipher %d", m_eNegotiatedCipher );
return false;
}
else
{
// Adjust the IV by the packet number
*(uint64 *)&m_cryptIVRecv.m_buf += LittleQWord( ctx.m_nPktNum );
//SpewMsg( "Recv decrypt IV %llu + %02x%02x%02x%02x encrypted %d %02x%02x%02x%02x\n",
// *(uint64 *)&m_cryptIVRecv.m_buf,
// m_cryptIVRecv.m_buf[8], m_cryptIVRecv.m_buf[9], m_cryptIVRecv.m_buf[10], m_cryptIVRecv.m_buf[11],
// cbChunk,
// *((byte*)pChunk + 0), *((byte*)pChunk + 1), *((byte*)pChunk + 2), *((byte*)pChunk + 3)
//);

// Decrypt the chunk and check the auth tag
uint32 cbDecrypted = sizeof(ctx.m_decrypted);
bool bDecryptOK = m_pCryptContextRecv->Decrypt(
pChunk, cbChunk, // encrypted
m_cryptIVRecv.m_buf, // IV
ctx.m_decrypted, &cbDecrypted, // output
nullptr, 0 // no AAD
);

// Restore the IV to the base value
*(uint64 *)&m_cryptIVRecv.m_buf -= LittleQWord( ctx.m_nPktNum );

// Did decryption fail?
if ( !bDecryptOK ) {

// Just drop packet.
// The assumption is that we either have a bug or some weird thing,
// or that somebody is spoofing / tampering. If it's the latter
// we don't want to magnify the impact of their efforts
SpewWarningRateLimited( ctx.m_usecNow, "[%s] Packet %lld (0x%x) decrypt failed (tampering/spoofing/bug)! mpath%d",
GetDescription(), (long long)ctx.m_nPktNum, (unsigned)nWireSeqNum, ctx.m_idxMultiPath );

// Update raw packet counters numbers, but do not update any logical state such as reply timeouts, etc
m_statsEndToEnd.m_recv.ProcessPacket( cbPacketSize );
return false;

case k_ESteamNetworkingSocketsCipher_NULL:
{

// No encryption!
ctx.m_cbPlainText = cbChunk;
ctx.m_pPlainText = pChunk;
}
break;

case k_ESteamNetworkingSocketsCipher_AES_256_GCM:
{
ctx.m_cbPlainText = (int)cbDecrypted;
ctx.m_pPlainText = ctx.m_decrypted;

// Adjust the IV by the packet number
*(uint64 *)&m_cryptIVRecv.m_buf += LittleQWord( ctx.m_nPktNum );
//SpewMsg( "Recv decrypt IV %llu + %02x%02x%02x%02x encrypted %d %02x%02x%02x%02x\n",
// *(uint64 *)&m_cryptIVRecv.m_buf,
// m_cryptIVRecv.m_buf[8], m_cryptIVRecv.m_buf[9], m_cryptIVRecv.m_buf[10], m_cryptIVRecv.m_buf[11],
// cbChunk,
// *((byte*)pChunk + 0), *((byte*)pChunk + 1), *((byte*)pChunk + 2), *((byte*)pChunk + 3)
//);

// Decrypt the chunk and check the auth tag
uint32 cbDecrypted = sizeof(ctx.m_decrypted);
bool bDecryptOK = m_cryptContextRecv.Decrypt(
pChunk, cbChunk, // encrypted
m_cryptIVRecv.m_buf, // IV
ctx.m_decrypted, &cbDecrypted, // output
nullptr, 0 // no AAD
);

// Restore the IV to the base value
*(uint64 *)&m_cryptIVRecv.m_buf -= LittleQWord( ctx.m_nPktNum );

// Did decryption fail?
if ( !bDecryptOK ) {

// Just drop packet.
// The assumption is that we either have a bug or some weird thing,
// or that somebody is spoofing / tampering. If it's the latter
// we don't want to magnify the impact of their efforts
SpewWarningRateLimited( ctx.m_usecNow, "[%s] Packet %lld (0x%x) decrypt failed (tampering/spoofing/bug)! mpath%d",
GetDescription(), (long long)ctx.m_nPktNum, (unsigned)nWireSeqNum, ctx.m_idxMultiPath );

// Update raw packet counters numbers, but do not update any logical state such as reply timeouts, etc
m_statsEndToEnd.m_recv.ProcessPacket( cbPacketSize );
return false;
}

ctx.m_cbPlainText = (int)cbDecrypted;
ctx.m_pPlainText = ctx.m_decrypted;

//SpewVerbose( "Connection %u recv seqnum %lld (gap=%d) sz=%d %02x %02x %02x %02x\n", m_unConnectionID, unFullSequenceNumber, nGap, cbDecrypted, arDecryptedChunk[0], arDecryptedChunk[1], arDecryptedChunk[2], arDecryptedChunk[3] );
}
break;
//SpewVerbose( "Connection %u recv seqnum %lld (gap=%d) sz=%d %02x %02x %02x %02x\n", m_unConnectionID, unFullSequenceNumber, nGap, cbDecrypted, arDecryptedChunk[0], arDecryptedChunk[1], arDecryptedChunk[2], arDecryptedChunk[3] );
}

// OK, we have high confidence that this packet is actually from our peer and has not
Expand Down
14 changes: 7 additions & 7 deletions src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -779,15 +779,15 @@ class CSteamNetworkConnectionBase : public ILockableThinker< ConnectionLock >
bool m_bCertHasIdentity; // Does the cert contain the identity we will use for this connection?
ESteamNetworkingSocketsCipher m_eNegotiatedCipher;

// AES keys used in each direction
// Encryption/decryption contexts. This has the negotiated key
bool m_bCryptKeysValid;
AES_GCM_EncryptContext m_cryptContextSend;
AES_GCM_DecryptContext m_cryptContextRecv;
std::unique_ptr<ISymmetricEncryptContext> m_pCryptContextSend;
std::unique_ptr<ISymmetricDecryptContext> m_pCryptContextRecv;

// Initialization vector for AES-GCM. These are combined with
// the packet number so that the effective IV is unique per
// packet. We use a 96-bit IV, which is what TLS uses (RFC5288),
// what NIST recommends (https://dl.acm.org/citation.cfm?id=2206251),
// Initialization vector for per-packet encryption. These are
// combined with the packet number so that the effective IV is
// unique per packet. We use a 96-bit IV, which is what TLS uses
// (RFC5288), what NIST recommends (https://dl.acm.org/citation.cfm?id=2206251),
// and what makes GCM the most efficient.
AutoWipeFixedSizeBuffer<12> m_cryptIVSend;
AutoWipeFixedSizeBuffer<12> m_cryptIVRecv;
Expand Down
84 changes: 37 additions & 47 deletions src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1797,55 +1797,45 @@ bool CSteamNetworkConnectionBase::SNP_SendPacket( CConnectionTransport *pTranspo
// OK, we have a plaintext payload. Encrypt and send it.
// What cipher are we using?
int nBytesSent = 0;
switch ( m_eNegotiatedCipher )
if ( m_eNegotiatedCipher == k_ESteamNetworkingSocketsCipher_NULL )
{
default:
AssertMsg1( false, "Bogus cipher %d", m_eNegotiatedCipher );
break;

case k_ESteamNetworkingSocketsCipher_NULL:
{

// No encryption!
// Ask current transport to deliver it
nBytesSent = helper.InFlightPkt().m_pTransport->SendEncryptedDataChunk( helper.payload, cbPlainText, ctx );
}
break;

case k_ESteamNetworkingSocketsCipher_AES_256_GCM:
{

Assert( m_bCryptKeysValid );

// Adjust the IV by the packet number
*(uint64 *)&m_cryptIVSend.m_buf += LittleQWord( m_statsEndToEnd.m_nNextSendSequenceNumber );

// Encrypt the chunk
uint8 arEncryptedChunk[ k_cbSteamNetworkingSocketsMaxEncryptedPayloadSend + 64 ]; // Should not need pad
uint32 cbEncrypted = sizeof(arEncryptedChunk);
DbgVerify( m_cryptContextSend.Encrypt(
helper.payload, cbPlainText, // plaintext
m_cryptIVSend.m_buf, // IV
arEncryptedChunk, &cbEncrypted, // output
nullptr, 0 // no AAD
) );

//SpewMsg( "Send encrypt IV %llu + %02x%02x%02x%02x encrypted %d %02x%02x%02x%02x\n",
// *(uint64 *)&m_cryptIVSend.m_buf,
// m_cryptIVSend.m_buf[8], m_cryptIVSend.m_buf[9], m_cryptIVSend.m_buf[10], m_cryptIVSend.m_buf[11],
// cbEncrypted,
// arEncryptedChunk[0], arEncryptedChunk[1], arEncryptedChunk[2],arEncryptedChunk[3]
//);

// Restore the IV to the base value
*(uint64 *)&m_cryptIVSend.m_buf -= LittleQWord( m_statsEndToEnd.m_nNextSendSequenceNumber );

Assert( (int)cbEncrypted >= cbPlainText );
Assert( (int)cbEncrypted <= k_cbSteamNetworkingSocketsMaxEncryptedPayloadSend ); // confirm that pad above was not necessary and we never exceed k_nMaxSteamDatagramTransportPayload, even after encrypting

// Ask current transport to deliver it
nBytesSent = helper.InFlightPkt().m_pTransport->SendEncryptedDataChunk( arEncryptedChunk, cbEncrypted, ctx );
}
// No encryption!
// Ask current transport to deliver it directly
nBytesSent = helper.InFlightPkt().m_pTransport->SendEncryptedDataChunk( helper.payload, cbPlainText, ctx );
}
else
{
Assert( m_bCryptKeysValid );

// Adjust the IV by the packet number
*(uint64 *)&m_cryptIVSend.m_buf += LittleQWord( m_statsEndToEnd.m_nNextSendSequenceNumber );

// Encrypt the chunk
uint8 arEncryptedChunk[ k_cbSteamNetworkingSocketsMaxEncryptedPayloadSend + 64 ]; // Should not need pad
uint32 cbEncrypted = sizeof(arEncryptedChunk);
DbgVerify( m_pCryptContextSend->Encrypt(
helper.payload, cbPlainText, // plaintext
m_cryptIVSend.m_buf, // IV
arEncryptedChunk, &cbEncrypted, // output
nullptr, 0 // no AAD
) );

//SpewMsg( "Send encrypt IV %llu + %02x%02x%02x%02x encrypted %d %02x%02x%02x%02x\n",
// *(uint64 *)&m_cryptIVSend.m_buf,
// m_cryptIVSend.m_buf[8], m_cryptIVSend.m_buf[9], m_cryptIVSend.m_buf[10], m_cryptIVSend.m_buf[11],
// cbEncrypted,
// arEncryptedChunk[0], arEncryptedChunk[1], arEncryptedChunk[2],arEncryptedChunk[3]
//);

// Restore the IV to the base value
*(uint64 *)&m_cryptIVSend.m_buf -= LittleQWord( m_statsEndToEnd.m_nNextSendSequenceNumber );

Assert( (int)cbEncrypted >= cbPlainText );
Assert( (int)cbEncrypted <= k_cbSteamNetworkingSocketsMaxEncryptedPayloadSend ); // confirm that pad above was not necessary and we never exceed k_nMaxSteamDatagramTransportPayload, even after encrypting

// Ask current transport to deliver it
nBytesSent = helper.InFlightPkt().m_pTransport->SendEncryptedDataChunk( arEncryptedChunk, cbEncrypted, ctx );
}
if ( nBytesSent <= 0 )
{
Expand Down

0 comments on commit 299cbad

Please sign in to comment.