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

Added additional information regarding what the ciphertext output contains. #388

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Xtrendence
Copy link

Currently, the documentation makes it seem like turning the output of CryptoJS.AES.encrypt() into a string is safe, but it doesn't clarify that the built-in .toString() method is special in that it concatenates a salt and the ciphertext, and doesn't include the decryption key with the output. If developers were to simply turn the output of the encrypt() method into a string without using said function, it would be trivial to decrypt the data. Not having this clarification in the documentation is potentially misleading and could lead to user data being stored insecurely.

Currently, the documentation makes it seem like turning the output of CryptoJS.AES.encrypt() into a string is safe, but it doesn't clarify that the built-in .toString() method is special in that it concatenates a salt and the ciphertext, and doesn't include the decryption key with the output. If developers were to simply turn the output of the encrypt() method into a string without using said function, it would be trivial to decrypt the data. Not having this clarification in the documentation is potentially misleading and could lead to user data being stored insecurely.
@Xtrendence Xtrendence changed the base branch from master to develop October 22, 2021 20:11
@OptimusPi
Copy link

OptimusPi commented Sep 7, 2022

+1 for documentation updates. (Also updating the documentation on gitbook would be great!) The file seems to be here: https://github.com/brix/crypto-js/edit/develop/docs/QuickStartGuide.wiki

The usage example is pretty clear and shows how easy it is to use. But I agree that it could be made clearer that encrypt() doesn't return a string, but instead, CipherParams.
Potentially confusing: CipherParams.ciphertext is actually a WordArray not a string.
Potentially more confusing: This comment with usage example in aes.js implies that ciphertext is returned from encrypt() instead of a CipherParams object!

However, the usage example still works, because decrypt() can take "ciphertext" of the type CipherParams | string. That's not really a real use case to simply encrypt then decrypt, but it does demonstrate that it works correctly. :)

Of course, everything in CypherParams is used to create the end result ciphertext bytes in a correct format that the caller can specify as they wish...but I do agree that the usage examples and documentation could improve.

A similar complaint I've noticed is the confusion over "key" parameter being either a string (which is a passphrase) or a WordArray (which is an actual key). It's not necessarily obvious that passing in your secret key as string (for example, base64 or hex encoded,) actually uses it as a passphrase to generate a new key.
That's probably a common pitfall for users asking "Why can't I decrypt the resulting ciphertext using a Java/C#/C++ library?"

Overall, I still loved my experience using this library! In my experience, it works great, and a cipher that I create in node.js can also be decrypted by Crpyto++ in C++ so everything checks out and it's easy to do. My use case is AES-CBC with encrypt-then-HMAC.

@evanvosberg Do you need help with updating the usage examples in README and in the QuickStartGuide.Wiki..?

p.s. GitHub built-in Wiki could also be useful here? :)

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