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

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

Closed
anst270125 opened this issue Dec 15, 2022 · 8 comments · Fixed by #46185
Closed

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

anst270125 opened this issue Dec 15, 2022 · 8 comments · Fixed by #46185
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem.

Comments

@anst270125
Copy link

anst270125 commented Dec 15, 2022

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
Copy link

mudlee commented Dec 16, 2022

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 panva added the crypto Issues and PRs related to the crypto subsystem. label Dec 16, 2022
@panva
Copy link
Member

panva commented Dec 16, 2022

cc @tniessen @nodejs/crypto

@mudlee
Copy link

mudlee commented Jan 12, 2023

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

@bnoordhuis
Copy link
Member

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 added a commit to bnoordhuis/io.js that referenced this issue Jan 12, 2023
Because OpenSSL v1.x doesn't do that by itself (OpenSSL v3.x does.)

Fixes: nodejs#45874
@bnoordhuis
Copy link
Member

Someone want to send a pull request?

/me raises hand

#46185

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Jan 12, 2023
Because OpenSSL v1.x doesn't do that by itself (OpenSSL v3.x does.)

Fixes: nodejs#45874
@mudlee
Copy link

mudlee commented Jan 12, 2023

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

@bnoordhuis
Copy link
Member

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

...Yes, eventually.

@mudlee
Copy link

mudlee commented Jan 12, 2023

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

@panva panva added the confirmed-bug Issues with confirmed bugs. label Jan 12, 2023
nodejs-github-bot pushed a commit that referenced this issue Jan 14, 2023
Because OpenSSL v1.x doesn't do that by itself (OpenSSL v3.x does.)

Fixes: #45874
PR-URL: #46185
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit to RafaelGSS/node that referenced this issue Jan 17, 2023
Because OpenSSL v1.x doesn't do that by itself (OpenSSL v3.x does.)

Fixes: nodejs#45874
PR-URL: nodejs#46185
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this issue Jan 20, 2023
Because OpenSSL v1.x doesn't do that by itself (OpenSSL v3.x does.)

Fixes: #45874
PR-URL: #46185
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this issue Jan 26, 2023
Because OpenSSL v1.x doesn't do that by itself (OpenSSL v3.x does.)

Fixes: #45874
PR-URL: #46185
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this issue Jan 31, 2023
Because OpenSSL v1.x doesn't do that by itself (OpenSSL v3.x does.)

Fixes: #45874
PR-URL: #46185
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this issue Mar 27, 2023
Because OpenSSL v1.x doesn't do that by itself (OpenSSL v3.x does.)

Fixes: #45874
PR-URL: #46185
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants