Data encryption / decryption on Node.js

Question

I've implemented my first data encryption / decryption methods on Node.js. Although it might look similar to hundreds of sample implementations available online, I'm curious to get a feedback.

I'm particularly interested in:

• some flaw points, potential security, and performance issues
• In encrypt() I return an ENCRYPTED object with encrypted data after being freezed by Object.freeze(), are there any drawbacks/pitfalls of returning a freezed object besides the read-only status of the object?

const CRYPTO = require("crypto");
const ALG_CIPHER = "aes-256-gcm";
const ENC_IN = "utf8";
const ENC_OUT = "base64";
const BUFFER_SIZE = 16;
const KEY = Buffer.from("...", ENC_IN);
/**
 * Encrypt data using an initialisation vector
 *
 * @param {string} data - data to be encrypted
 * @returns {Promise<{authTag: Buffer, data: string, iv: Buffer}>} encrypted - encrypted data with decryption parameters
 */
const encrypt = async function encrypt(data) {
 const IV = Buffer.from(CRYPTO.randomBytes(BUFFER_SIZE));
 const CIPHER = CRYPTO.createCipheriv(ALG_CIPHER, KEY, IV);
 let enc = CIPHER.update(data, ENC_IN, ENC_OUT);
 enc += CIPHER.final(ENC_OUT);
 const ENCRYPTED = Object.freeze({
 data: enc,
 iv: IV,
 authTag: CIPHER.getAuthTag()
 });
 return ENCRYPTED;
};
/**
 * Decrypt data using an initialisation vector
 *
 * @param {string} data - data to be decrypted
 * @param {string} iv - initialisation vector
 * @param {Buffer} authTag - authentication tag
 * @returns {Promise<string>} decrypted - decrypted data
 */
const decrypt = async function decrypt(data, iv, authTag) {
 const DECIPHER = CRYPTO.createDecipheriv(ALG_CIPHER, KEY, iv);
 DECIPHER.setAuthTag(authTag);
 let decrypted = DECIPHER.update(data, ENC_OUT, ENC_IN);
 decrypted += DECIPHER.final(ENC_IN);
 return decrypted;
};

• Mast: Please don't change the code after receiving answers, doing so would invalidate them or otherwise create a mess.
• Mast: Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers . Please consider posting a new question instead, linking back to the old question. Question threads get terribly messy if we allow updates like this, I'm sorry.

Answer 1

Comments are below the code fragments.

const ENC_IN = "utf8";
const ENC_OUT = "base64";

With the choice of encryption and encoding, I'd include the "encoding" part fully (e.g. ENCODING_IN and ENCODING_OUT).

const KEY = Buffer.from("...", ENC_IN);

No, a key should not be a constant, and a key should certainly not be a string, even if it has a specific encoding. A key should be 16, 24 or 32 fully unpredictable bytes (for 128, 192 or 256 bit keys respectively). With UTF-8 you only have some 108 possible printable characters for each byte. Besides that, many libraries will not handle partial or overly long keys well.

If you want to use a password, then use a password based key derivation function (PBKDF) and then createCipheriv / createDecipheriv. Support for that is build in (note that createCipher / createDecipher is deprecated).

Encryption

const encrypt = async function encrypt(data) {

At this point you may want to document the character encoding and the base 64 encoding that is being applied, the mode of operation and that an IV and authentication tag is added. Document or point to the protocol in other words.

const IV = Buffer.from(CRYPTO.randomBytes(BUFFER_SIZE));

Good, random IV. For GCM though I'd use 12 bytes or the mode has to perform additional operations for no security benefit.

const ENCRYPTED = Object.freeze({

Even though you froze the variable, it's still a local variable and therefore you should not be using all uppercase.

data: enc,
iv: IV,
authTag: CIPHER.getAuthTag()

Now this is a bit weird. Unless I'm mistaken enc is now base 64 encoded, but iv and authTag are just plain buffers of binary data.

Decryption

 * @param {string} iv - initialisation vector

Strange, your iv of type Buffer seems to have magically become a string...

Otherwise the decrypt function is nicely symmetrical to the encrypt function, which is good.

---

Beware of overly stringifying your code. Beware of wrapper classes; don't start using above class for all your code: use user specific encryption routines insetead, and clearly specify your protocol.

When using GCM, you may want to allow additional authenticated data (AAD) to be included in the calculation of the tag.

• Mike: Thanks for the detailed review. Regarding a key should not be a constant, do I understand it correctly, I need to randomly generate a key every time I want to encrypt the data? If I store the keys in the DB, next to the encrypted data, then if the DB is compromised, the security of the encrypted data is also affected. Therefore, where it's better to store these random keys? Regarding const ENCRYPTED, it's a capital since it's a constant and not because it is global/local variable.
• Mike: Regarding @param {string} iv - initialisation vector, nice catch! I just forgot to update a JSDoc annotations, now it's fixed, the code snippet is updated.
• Maarten Bodewes: Comment #1. No, you don't need to generate a new key each time, you can for instance keep it in a key store. However, if you keep it in a constant it will also get in your code repo, and it will not allow you to have test keys etc. Key management is a tricky subject, but just using constant keys is not the answer. ENCRYPTED is not constant at all, it changes with the input data.
• Maarten Bodewes: About the sentence about using ENC for encoding, that was a Dutch-ism :) Sorry for the confusion (uitschrijven != write out).
• Maarten Bodewes: The problem that I have with it is that ENCRYPTED will still be different for each message that you encrypt. If developers see an all caps constant, they expect it to be constant throughout the application. Being constant, in other words, is different from being a constant. In Java we have immutable types, which I use a lot. Instances are as "constant" as the JS description of const, but they are still not written using all upper caps (unless, of course, they are also static final class fields).