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

Allow using OpenSSL instead of GnuTLS #1515

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

hrxi
Copy link
Contributor

@hrxi hrxi commented Nov 24, 2023

In preparation of Windows support.

@mar-v-in
Copy link
Member

mar-v-in commented Dec 3, 2023

SymmetricCipher now has different behavior depending on the backend. A more unified behavior would be better to ensure the code behaves the same on all platforms:

  • With gcrypt backend, a SymmetricCipher created from SymmetricCipher.encryption can also be used for decryption, with openssl backend this is not possible.
  • With gcrypt backend, any size of IV and key are allowed on AES-GCM algorithm, openssl backend requires 128-bit key and 96 bit IV.

The latter is especially problematic as Dino might call the decryption function with 128 bit IV (if files are received that are encrypted using older clients that used 128 bit IV).

As such I would suggest to:

  1. Include the key length in the constructor (same as algo name and information if it's for encryption or decryption)
  2. Make sure the openssl backend supports 128 bit IV. While generally 96 bit IV are suggested for AES-128-GCM, the security reasoning for this suggestion does not apply in our case (it only applies when a) the key is reused for multiple encryptions and b) the IV is generated deterministically, e.g. using a counter). Yes, 128 bit IV is still worse performance wise, but it was used for a while and files uploaded by clients using 128 bit IV still exist in the wild.
  3. Enforce key length and encryption/decryption capability for the set_key and encrypt/decrypt functions in the gcrypt backend to make it behave the same as the openssl backend

Futher, some quality of life improvements could be:

  • Make SymmetricCipher an interface with two implementations (one for each backend), provided in different files (with the file to be included decided at compile time). This would allow to just have the creation function use a preprocessor directive and thus could make the code easier to read.
  • Use actually two interfaces, one for encryption and one for decryption. Could in the end be implemented in the same class (as that's probably easier) but would enforce correct usage of encrypt/decrypt function already at compile time
  • If key length is included in constructor and enforced from now on anyways, we can add it into the algo name. "AES-128-GCM" is a common way to denote AES GCM with 128 bit key, so doing it the same way would certainly not hurt readability.

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.

None yet

2 participants