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

Use authenticated mode and GCM, and drop support for encrypting with RSA directly #4

Open
wants to merge 2 commits into
base: update-padding-scheme
Choose a base branch
from

Conversation

dsagal
Copy link
Member

@dsagal dsagal commented Feb 1, 2025

  • Switch from aes-256-cbc to aes-256-gcm and include authTag, for authenticated mode.
  • Drop support for encrypting small secrets with the public key directly. It was there for historical reasons, adds no benefit, and raises questions.

@soatok
Copy link

soatok commented Feb 1, 2025

One thing that you may also want to consider is binding the ciphertext to its context, via setAAD() (with a parameter that identifies which secret is being encrypted/decrypted).

Without such a mechanism, someone could come along and swap two ciphertexts, and get you to successfully decrypt the wrong plaintext. If both plaintext secrets are random strings (i.e., app passwords), it might not be obvious that the two have been swapped. The straightforward way to abuse this is to convince the operator that the secrets code is "flaky" and to store the secrets in plaintext instead. A decryption failure is more obvious than decrypting to the wrong password, in such a scenario.

(The term for such tactics is a "confused deputy attack", if that helps your inquiry into other designs.)

@dsagal dsagal mentioned this pull request Feb 1, 2025
@dsagal
Copy link
Member Author

dsagal commented Feb 1, 2025

Thanks for explaining the confused deputy attack. I think that's solving a different problem now. I'll stick to the very narrow use-case for this script, because my goal really isn't to market a cryptographic app.

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