nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.31k stars 29.45k forks source link

chacha20-poly1305 decryption works without setting authTag on decypher in v16 #45874

Closed anst270125 closed 1 year ago

anst270125 commented 1 year ago

Version

v16.19.0 / v18.12.1

Platform

Linux andi-vm 5.15.0-56-generic #62-Ubuntu SMP Tue Nov 22 19:54:14 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

    import { createCipheriv, createDecipheriv, randomBytes } from 'crypto';

    const nonce = 12;
    const algorithm = 'chacha20-poly1305';
    const encryptionKey = 'qwertyuiopasdfghjklzxcvbnm123456';
    const file = Buffer.from('Some file', 'utf-8');

    const iv1 = randomBytes(nonce);
    const cipher = createCipheriv(algorithm, encryptionKey, iv1, { authTagLength: 16 });
    const encrypted = Buffer.concat([iv1, cipher.update(file), cipher.final()]);
    const authTag = cipher.getAuthTag();

    const iv2 = encrypted.slice(0, nonce);
    const toDecrypt = encrypted.slice(nonce);
    const decipher = createDecipheriv(algorithm, encryptionKey, iv2, { authTagLength: 16 });
    //decipher.setAuthTag(authTag);
    const result = Buffer.concat([decipher.update(toDecrypt), decipher.final()]);
    console.log(result.toString());

How often does it reproduce? Is there a required condition?

If running the code using the node v16.19.0, the code runs without throwing an error. If I change to node v18.12.1, an error is thrown, as per the docs.

What is the expected behavior?

According to the docs:

[...] the decipher.setAuthTag() method is used to pass in the received authentication tag. If no tag is provided, or if the cipher text has been tampered with, decipher.final() will throw, indicating that the cipher text should be discarded due to failed authentication.[...]

I would expect the code snipped above to fail, since the authTag is not set on the decypher.

What do you see instead?

The code snippet above runs without throwing any error. It successfully prints "Some file".

Additional information

As soon as I switch from node v16 to v18, the code snipped throws an error, as expected. Uncommenting the setAuthTag line makes the code work. If this is expected behavior in v16 (vs v18), I'd be happy to be guided to a doc/changelog that explains this change.

mudlee commented 1 year ago

We experience the same. We use similar file encryption/decryption, and what we observed is that with node 16 it just works as it is, but after upgrading to node 18, it still does work, but we do get an error at decryption, which does not affect the decrypted result. See below.

The error I get:

    Error: Unsupported state or unable to authenticate data
        at Decipheriv._flush (node:internal/crypto/cipher:160:29)
        at Decipheriv.final [as _final] (node:internal/streams/transform:132:10)
        at callFinal (node:internal/streams/writable:698:12)
        at prefinish (node:internal/streams/writable:710:7)
        at finishMaybe (node:internal/streams/writable:720:5)
        at Decipheriv.Writable.end (node:internal/streams/writable:634:5)
        at Readable.onend (node:internal/streams/readable:705:10)
        at Object.onceWrapper (node:events:627:28)
        at Readable.emit (node:events:525:35)
        at endReadableNT (node:internal/streams/readable:1359:12)
        at processTicksAndRejections (node:internal/process/task_queues:82:21)

Code

Class

const iv = '123456789012'; // for testing

export class FileEncrypter {
  constructor(
    private readonly nonceLength: number,
    private readonly algorithm: CipherCCMTypes,
    private readonly encryptionKey: string,
  ) {}

  encrypt(file: Buffer): Promise<Buffer> {
    return new Promise<Buffer>((res, rej) => {
      const cipher = createCipheriv(this.algorithm, this.encryptionKey, iv, { authTagLength: 16 });
      const input = Readable.from(file);
      const output = new PassThrough();
      const outputData: Buffer[] = [];

      output.write(iv);
      output.on('readable', () => {
        const data = output.read();
        if (data) {
          outputData.push(data);
        }
      });
      output.on('end', () => {
        const encrypted = Buffer.concat(outputData);
        res(encrypted);
      });

      pipeline(input, cipher, output, (err) => {
        if (err) {
          rej(err);
        }
      });
    });
  }

  decrypt(file: StreamableFile): Promise<Buffer> {
    return new Promise<Buffer>((res, rej) => {
      const input = file.getStream();
      const output = new PassThrough();

      input.once('readable', () => {
        const iv = input.read(12);
        const cipher = createDecipheriv(this.algorithm, this.encryptionKey, iv, { authTagLength: 16 });

        output.once('readable', () => {
          const encrypted = output.read();
          res(encrypted);
        });

        pipeline(input, cipher, output, (err) => {
          if (err) {
            rej(err);
          }
        });
      });
    });
  }
}

Test

const testImage = readFileSync(`${E2E_TESTS_ASSETS_DIR}/simonstalenhag.jpg`);
const encrypter = new FileEncrypter(12, 'chacha20-poly1305', ENCRYPTION_KEY);

const encryptedFile = await encrypter.encrypt(testImage);
const decryptedFile = await encrypter.decrypt(new StreamableFile(encryptedFile));

console.log('decrypted');
console.log(decryptedFile);
console.log('original');
console.log(testImage);
expect(Buffer.compare(decryptedFile, testImage)).toStrictEqual(0);

AND here decryptedFile and testImage are the same, despite I get the error above.

panva commented 1 year ago

cc @tniessen @nodejs/crypto

mudlee commented 1 year ago

any update on this guys? @tniessen @panva? thanks!

bnoordhuis commented 1 year ago

The "unable to authenticate data" error is expected; its absence in v16 isn't. I speculate openssl 1.x didn't enforce it but openssl 3.x does.

(There is some special handling for chacha20-poly1305 inside node itself but v16.19.0 and v18.12.1 are identical in that respect so that can't explain it.)

The best suggestion I have is to special-case it in CipherBase::Final() in src/crypto/crypto_cipher.cc. Someone want to send a pull request?

bnoordhuis commented 1 year ago

Someone want to send a pull request?

/me raises hand

46185

mudlee commented 1 year ago

Will it be available in 18LTS and in 16LTS?

bnoordhuis commented 1 year ago

Whatever happened to "thank you, you're awesome, I want to have your babies"?

...Yes, eventually.

mudlee commented 1 year ago

Yes, you're right, I'm sorry. Nothing happened to kindness, I just simply forgot it. :) thanks for your good work!