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

More flexible crypto config #196

Open
4 tasks
past-due opened this issue Oct 17, 2021 · 3 comments
Open
4 tasks

More flexible crypto config #196

past-due opened this issue Oct 17, 2021 · 3 comments
Labels
enhancement New feature or request

Comments

@past-due
Copy link
Contributor

past-due commented Oct 17, 2021

With the protocol supporting negotiable cipher suites, some additional changes to support a more flexible crypto config seem like they could be useful.

Motivations:

There may be situations where:

  • A dependent project wishes to link with a specific crypto library (ex. libsodium) that does not offer AES-GCM on all systems (possibly depending on processor architecture / extensions).
  • Additional ciphers (like the commented-out k_ESteamNetworkingSocketsCipher_CHACHA20_POLY1305) with different performance characteristics or system / crypto library compatibility are desired.
  • A dependent project is fine with advertising "best-effort" encryption, which falls back to advertising "null cipher"-only if a particular system + crypto library combination cannot support any.

Additionally (although not considered in-scope at this time, but the work in this should help):

  • A dependent project may not wish to link GameNetworkingSockets with any of its supported crypto libraries (and may be fine with GameNetworkingSockets itself not implementing encryption at all, perhaps because it already handles this).

Tasks (WIP):

@zpostfacto
Copy link
Contributor

Hey @past-due, I was looking at refactoring the crypto code to introduce those abstract interfaces (since that's the sort of refactor I can imagine you not wanting to do.) One quick question: My understanding is that ChaCha20 works similarly AES-GCM, in that the cipher doesn't really operate directly on the data, but instead produces a keystream, which is then combined with the plaintext in a simple way. Because of this, there isn't really a "block size", it can encrypt N plaintext bytes to N ciphertext bytes. And then there is a tag. Can you confirm if my understanding is right about this? Or perhaps more directly, is there any difference in the "interface" to these two ciphers, with respect to block size, tag size, the ability to accommodate Additional Authenticated Data, etc?

I was hoping to be able to keep the interface extremely simple for now:

// 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;
};

This assumes that all concrete types will have the same characteristics regarding any block size (there is none), tag size, and support for AAD. (Actually, the tag size could differ in the existing code without too much hassle -- unencrypted mode doesn't have a tag, and that is supported).

@past-due
Copy link
Contributor Author

past-due commented Nov 5, 2021

Hi @fletcherdvalve,

Apologies - I had not yet had a chance to dig into this. From initial looks at libsodium, it appears to have exactly the same interface for both AES-GCM:

int crypto_aead_aes256gcm_encrypt(unsigned char *c,
                                  unsigned long long *clen_p,
                                  const unsigned char *m,
                                  unsigned long long mlen,
                                  const unsigned char *ad,
                                  unsigned long long adlen,
                                  const unsigned char *nsec,
                                  const unsigned char *npub,
                                  const unsigned char *k);

As well as IETF ChaCha20-Poly1305:

int crypto_aead_chacha20poly1305_ietf_encrypt(unsigned char *c,
                                              unsigned long long *clen_p,
                                              const unsigned char *m,
                                              unsigned long long mlen,
                                              const unsigned char *ad,
                                              unsigned long long adlen,
                                              const unsigned char *nsec,
                                              const unsigned char *npub,
                                              const unsigned char *k);

(It looks like the combined + precomputed interface is used for AES-GCM by this library, but that's also basically the same.)

The public nonce size for both (crypto_aead_aes256gcm_NPUBBYTES and crypto_aead_chacha20poly1305_IETF_NPUBBYTES) is defined as 12 by libsodium.

The same recommendation applies for generating the public nonce for both:

The public nonce npub should never ever be reused with the same key. The recommended way to generate it is to use randombytes_buf() for the first message, and increment it for each subsequent message using the same key.

One general point of note:

The IETF variant of the ChaCha20-Poly1305 construction can safely encrypt a practically unlimited number of messages, but individual messages cannot exceed 64*(2^32)-64 bytes (approximatively 256 GiB).

Although I can't imagine that's likely to be a problem. 😄

So overall this does appear to be compatible with such an abstract interface.

zpostfacto added a commit that referenced this issue Nov 5, 2021
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
@zpostfacto
Copy link
Contributor

Alrighty, take a look at that change. I think it should be pretty straightforward.

@zpostfacto zpostfacto added the enhancement New feature or request label Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants