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

Add encrypt_text/decrypt_text function using aes256 EBC algorithm #17721

Conversation

xsellier
Copy link
Contributor

@xsellier xsellier commented Mar 23, 2018

Porting #17366 to master branch. Fix this issue: #1969

Warning it uses AES EBC encryption and CBC would be a better alternative.

@xsellier xsellier changed the title Add encrypt_text/decrypt_text function using aes256 EBC algorithm WIP - Add encrypt_text/decrypt_text function using aes256 EBC algorithm Mar 23, 2018
@xsellier xsellier force-pushed the feature/add-encrypt-decrypt-string-master branch from 4c1a257 to c347e5d Compare March 23, 2018 16:26
@xsellier xsellier changed the title WIP - Add encrypt_text/decrypt_text function using aes256 EBC algorithm Add encrypt_text/decrypt_text function using aes256 EBC algorithm Mar 23, 2018
@akien-mga akien-mga added this to the 3.1 milestone Mar 23, 2018
@mhilbrunner
Copy link
Member

mhilbrunner commented Apr 22, 2018

Warning it uses AES EBC encryption and CBC would be a better alternative.

I think Chacha20-Poly1305 or AES-GCM would be the best alternative.
But this really should be at least CBC or come with some warning.
Sure, if you're only encrypting save files or something its not that important, but if you're using it in network communication or somewhere more important its another thing.

Instead of encrypt_text() / decrypt_text(), I suggest encrypt_aes_ebc() and decrypt_aes_ebc().

That way we can always add additional algos and modes without breaking backwards compability by changing what encrypt_text() does under the hood.

Also, the name should really make clear the algo and mode.

If we go with only encrypt_text(), it shouldn't be a weak default. At least AES-CBC, probably better AES-GCM.

@xsellier
Copy link
Contributor Author

I agree, I will upgrade the PR soon to implement those changes. Meaning renaming
encrypt_text/decrypt_text to encrypt_text_aes_ebc/decrypt_text_aes_cbc.

Plus I will add a warning to the documentation saying that encrypt method is weak

@Malkverbena
Copy link

Instead decrypt_text_aes_cbc, could we define the algorithm used like a parameter?
Like decrypt_text(data, key, algorithm)

@Malkverbena
Copy link

Would it be useful to networking add a function to encryp/decrypt binary datatype?

@akien-mga
Copy link
Member

Moving to 3.2 milestone as we're about to enter the beta stage and release freeze for Godot 3.1. Only bug fixes against issues of the 3.1 milestone (or enhancements to core features of the 3.1 roadmap) will be considered until 3.1-stable is released.

Note: If it's merged after 3.1-stable is released, this change might be considered for cherry-picking into a later 3.1.x release.

@akien-mga akien-mga modified the milestones: 3.1, 3.2 Dec 12, 2018
@shartte
Copy link
Contributor

shartte commented Mar 7, 2019

Few random notes on security of this:

  • Since the key is MD5-hashed and that is then used as the actual key, for clarities sake I'd rather call this a "password"
  • Since it's more like a password, it might be wise to use a proven "password based key derivation function" (i.e. PBKDF2), which is intended to make brute forcing passwords harder
  • The issues with EBC vs. more modern approaches have already been raised
  • For completeness sake, it might be nice to have two functions here, one that takes an actual binary key (128 or 256 bit) and one that takes a password (and then applies a PKBDF to it)

@xsellier xsellier closed this May 30, 2019
@xsellier xsellier deleted the feature/add-encrypt-decrypt-string-master branch June 28, 2019 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants