Repository: denoland/deno
Pull request: fix(ext/node): crypto `Cipheriv` and `Decipheriv` base64 encoding

Fixes #30722

The root cause comes from the `.update()` and `.final()` methods of `Cipheriv` and `Decipheriv`, which internally calls `.toString()` method of Buffer. Doing so adds padding to make the output length a multiple of 4, which we don't want for this case. This PR replaces that to use `StringDecoder` instead, making it more aligned with the Node.js implementation.

Changed files:
filename: ext/node/polyfills/internal/crypto/cipher.ts
patch: @@ -52,6 +52,9 @@ import {
   isArrayBufferView,
 } from "ext:deno_node/internal/util/types.ts";
 import { ERR_CRYPTO_INVALID_STATE } from "ext:deno_node/internal/errors.ts";
+import { StringDecoder } from "node:string_decoder";
+import assert from "node:assert";
+import { normalizeEncoding } from "ext:deno_node/internal/util.mjs";
 
 const FastBuffer = Buffer[SymbolSpecies];
 
@@ -173,14 +176,14 @@ export class Cipheriv extends Transform implements Cipher {
   /** plaintext data cache */
   #cache: BlockModeCache;
 
+  #decoder?: StringDecoder;
+
   #needsBlockCache: boolean;
 
   #authTag?: Buffer;
 
   #autoPadding = true;
 
-  #encoding: string | undefined;
-
   constructor(
     cipher: string,
     key: CipherKey,
@@ -210,14 +213,15 @@ export class Cipheriv extends Transform implements Cipher {
   }
 
   final(encoding: string = getDefaultEncoding()): Buffer | string {
-    this.#validateOutputEncoding(encoding);
+    this.#lazyInitDecoder(encoding);
 
     const buf = new FastBuffer(16);
     if (this.#cache.cache.byteLength == 0) {
       const maybeTag = op_node_cipheriv_take(this.#context);
       if (maybeTag) this.#authTag = Buffer.from(maybeTag);
       return encoding === "buffer" ? Buffer.from([]) : "";
     }
+
     if (!this.#autoPadding && this.#cache.cache.byteLength != 16) {
       throw new Error("Invalid final block size");
     }
@@ -231,7 +235,12 @@ export class Cipheriv extends Transform implements Cipher {
       this.#authTag = Buffer.from(maybeTag);
       return encoding === "buffer" ? Buffer.from([]) : "";
     }
-    return encoding === "buffer" ? buf : buf.toString(encoding);
+
+    if (encoding !== "buffer") {
+      return this.#decoder!.end(buf);
+    }
+
+    return buf;
   }
 
   getAuthTag(): Buffer {
@@ -266,16 +275,18 @@ export class Cipheriv extends Transform implements Cipher {
     if (typeof data === "string") {
       buf = Buffer.from(data, inputEncoding);
     }
+    this.#lazyInitDecoder(outputEncoding);
 
-    this.#validateOutputEncoding(outputEncoding);
-
-    let output;
+    let output: Buffer;
     if (!this.#needsBlockCache) {
       output = Buffer.allocUnsafe(buf.length);
       op_node_cipheriv_encrypt(this.#context, buf, output);
-      return outputEncoding === "buffer"
-        ? output
-        : output.toString(outputEncoding);
+
+      if (outputEncoding !== "buffer") {
+        return this.#decoder!.write(output);
+      }
+
+      return output;
     }
 
     this.#cache.add(buf);
@@ -287,25 +298,28 @@ export class Cipheriv extends Transform implements Cipher {
       output = Buffer.allocUnsafe(input.length);
       op_node_cipheriv_encrypt(this.#context, input, output);
     }
-    return outputEncoding === "buffer"
-      ? output
-      : output.toString(outputEncoding);
+
+    if (outputEncoding !== "buffer") {
+      return this.#decoder!.write(output);
+    }
+
+    return output;
   }
 
-  // This is only for validation of encoding change during the update.
-  #validateOutputEncoding(encoding: string) {
+  #lazyInitDecoder(encoding: string) {
     if (encoding === "buffer") {
       return;
     }
 
-    if (!Buffer.isEncoding(encoding)) {
-      throw new ERR_UNKNOWN_ENCODING(encoding);
-    }
+    const normalizedEncoding = normalizeEncoding(encoding);
+    this.#decoder ||= new StringDecoder(normalizedEncoding);
 
-    if (this.#encoding && this.#encoding !== encoding) {
-      throw new Error("Cannot change encoding");
+    if (this.#decoder.encoding !== normalizedEncoding) {
+      if (normalizedEncoding === undefined) {
+        throw new ERR_UNKNOWN_ENCODING(encoding);
+      }
+      assert(false, "Cannot change encoding");
     }
-    this.#encoding = encoding;
   }
 }
 
@@ -371,6 +385,8 @@ export class Decipheriv extends Transform implements Cipher {
   /** ciphertext data cache */
   #cache: BlockModeCache;
 
+  #decoder?: StringDecoder;
+
   #needsBlockCache: boolean;
 
   #authTag?: BinaryLike;
@@ -411,6 +427,8 @@ export class Decipheriv extends Transform implements Cipher {
   }
 
   final(encoding: string = getDefaultEncoding()): Buffer | string {
+    this.#lazyInitDecoder(encoding);
+
     let buf = new FastBuffer(16);
     op_node_decipheriv_final(
       this.#context,
@@ -428,7 +446,11 @@ export class Decipheriv extends Transform implements Cipher {
     }
 
     buf = buf.subarray(0, 16 - buf.at(-1)); // Padded in Pkcs7 mode
-    return encoding === "buffer" ? buf : buf.toString(encoding);
+    if (encoding !== "buffer") {
+      return this.#decoder!.end(buf);
+    }
+
+    return buf;
   }
 
   setAAD(
@@ -463,14 +485,18 @@ export class Decipheriv extends Transform implements Cipher {
     if (typeof data === "string") {
       buf = Buffer.from(data, inputEncoding);
     }
+    this.#lazyInitDecoder(outputEncoding);
 
     let output;
     if (!this.#needsBlockCache) {
       output = Buffer.allocUnsafe(buf.length);
       op_node_decipheriv_decrypt(this.#context, buf, output);
-      return outputEncoding === "buffer"
-        ? output
-        : output.toString(outputEncoding);
+
+      if (outputEncoding !== "buffer") {
+        return this.#decoder!.write(output);
+      }
+
+      return output;
     }
 
     this.#cache.add(buf);
@@ -481,9 +507,28 @@ export class Decipheriv extends Transform implements Cipher {
       output = new FastBuffer(input.length);
       op_node_decipheriv_decrypt(this.#context, input, output);
     }
-    return outputEncoding === "buffer"
-      ? output
-      : output.toString(outputEncoding);
+
+    if (outputEncoding !== "buffer") {
+      return this.#decoder!.write(output);
+    }
+
+    return output;
+  }
+
+  #lazyInitDecoder(encoding: string) {
+    if (encoding === "buffer") {
+      return;
+    }
+
+    const normalizedEncoding = normalizeEncoding(encoding);
+    this.#decoder ||= new StringDecoder(normalizedEncoding);
+
+    if (this.#decoder.encoding !== normalizedEncoding) {
+      if (normalizedEncoding === undefined) {
+        throw new ERR_UNKNOWN_ENCODING(encoding);
+      }
+      assert(false, "Cannot change encoding");
+    }
   }
 }
 

filename: tests/unit_node/crypto/crypto_cipher_gcm_test.ts
patch: @@ -17,9 +17,10 @@ const aesGcm = (bits: string, key: Uint8Array) => {
   ): [string, Buffer] => {
     const cipher = crypto.createCipheriv(ALGO, key, iv);
     cipher.setAAD(aad);
-    let enc = cipher.update(str, "base64", "base64");
-    enc += cipher.final("base64");
-    return [enc, cipher.getAuthTag()];
+    let encBuf = cipher.update(str, "base64");
+    const finalBuf = cipher.final();
+    encBuf = Buffer.concat([encBuf, finalBuf]);
+    return [encBuf.toString("base64"), cipher.getAuthTag()];
   };
 
   const decrypt = (
@@ -31,10 +32,11 @@ const aesGcm = (bits: string, key: Uint8Array) => {
     const decipher = crypto.createDecipheriv(ALGO, key, iv);
     decipher.setAuthTag(authTag);
     decipher.setAAD(aad);
-    let str = decipher.update(enc, "base64", "base64");
-    str += decipher.final("base64");
+    let buf = decipher.update(enc, "base64");
+    const finalBuf = decipher.final();
+    buf = Buffer.concat([buf, finalBuf]);
 
-    return str;
+    return buf.toString("base64");
   };
 
   return {

filename: tests/unit_node/crypto/crypto_cipher_test.ts
patch: @@ -4,6 +4,7 @@ import { Buffer } from "node:buffer";
 import { Readable } from "node:stream";
 import { buffer, text } from "node:stream/consumers";
 import { assert, assertEquals, assertThrows } from "@std/assert";
+import { AssertionError } from "node:assert";
 
 const rsaPrivateKey = Deno.readTextFileSync(
   new URL("../testdata/rsa_private.pem", import.meta.url),
@@ -488,3 +489,83 @@ Deno.test({
     );
   },
 });
+
+Deno.test({
+  name: "Cipheriv - change encoding after first update",
+  fn() {
+    const cipher = crypto.createCipheriv(
+      "aes-128-cbc",
+      new Uint8Array(16),
+      new Uint8Array(16),
+    );
+    cipher.update(new Uint8Array(16), undefined, "hex");
+    assertThrows(
+      () => {
+        cipher.final("utf-8");
+      },
+      AssertionError,
+      "Cannot change encoding",
+    );
+
+    cipher.final();
+  },
+});
+
+Deno.test({
+  name: "Decipheriv - change encoding after first update",
+  fn() {
+    const decipher = crypto.createDecipheriv(
+      "aes-256-cbc",
+      new Uint8Array(32),
+      new Uint8Array(16),
+    );
+    decipher.update(new Uint32Array(), undefined, "hex");
+    assertThrows(
+      () => {
+        decipher.final("utf-8");
+      },
+      AssertionError,
+      "Cannot change encoding",
+    );
+
+    decipher.final();
+  },
+});
+
+// https://github.com/denoland/deno/issues/30722
+Deno.test({
+  name: "base64 cipher/decipher with non multiple of 4 char length",
+  fn() {
+    const aes256Encrypt = (plaintext: string, key: string) => {
+      const hash = crypto.createHash("sha256").update(key).digest();
+      const iv = crypto.randomBytes(16);
+      const cipher = crypto.createCipheriv("aes-256-cbc", hash, iv);
+
+      let encrypted = cipher.update(plaintext, "utf8", "base64");
+      encrypted += cipher.final("base64");
+
+      return iv.toString("base64") + "." + encrypted;
+    };
+
+    const aes256Decrypt = (encryptedText: string, key: string): string => {
+      const hash = crypto.createHash("sha256").update(key).digest();
+      const [ivPart, encryptedPart] = encryptedText.split(".");
+
+      const iv = Buffer.from(ivPart, "base64");
+      const decipher = crypto.createDecipheriv("aes-256-cbc", hash, iv);
+
+      let decrypted = decipher.update(encryptedPart, "base64", "utf8");
+      decrypted += decipher.final("utf8");
+
+      return decrypted;
+    };
+
+    const key = "my secret key";
+    const text = "The quick brown fox jumps over the lazy dog";
+
+    const cipherText = aes256Encrypt(text, key);
+    const decryptedText = aes256Decrypt(cipherText, key);
+
+    assertEquals(decryptedText, text);
+  },
+});