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.

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

Conversation

past-due
Copy link
Contributor

Implements the first two tasks of #196

Opening as a draft for feedback.

Some backends may not support AES-GCM on all systems (example: libsodium). Add a new static AES_GCM_CipherContext::IsAvailable() function.
Some backends may not support AES-GCM on all systems. Be more precise in what algorithms are advertised / initialized.
@past-due past-due mentioned this pull request Oct 17, 2021
4 tasks
@past-due past-due changed the title AES-GCM runtime availability check, Advertise / initialize AES_GCM cipher only if supported AES-GCM runtime availability check, Advertise / initialize AES-GCM cipher only if supported Oct 17, 2021
{
m_msgCryptLocal.add_ciphers( k_ESteamNetworkingSocketsCipher_AES_256_GCM );
}
Assert( m_msgCryptLocal.ciphers_size() > 0 );
break;
Copy link
Contributor

@zpostfacto zpostfacto Oct 18, 2021

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.)

m_cryptContextRecv.Wipe();
}
m_eNegotiatedCipher = k_ESteamNetworkingSocketsCipher_INVALID;
m_cbEncryptionOverhead = k_cbAESGCMTagSize;
Copy link
Contributor

@zpostfacto zpostfacto Oct 18, 2021

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. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants