From c3b1dfc9615d27b7445af8c30c8f62999ae835d2 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 18 Feb 2020 12:26:49 +0100 Subject: [PATCH 1/3] test: add known issues test for #31733 Authenticated decryption works for file streams up to 32768 bytes but not beyond. Other streams and direct decryption are not affected. Refs: https://github.com/nodejs/node/issues/31733 --- .../test-crypto-authenticated-stream.js | 134 ++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 test/known_issues/test-crypto-authenticated-stream.js diff --git a/test/known_issues/test-crypto-authenticated-stream.js b/test/known_issues/test-crypto-authenticated-stream.js new file mode 100644 index 00000000000000..68fac35489b0c2 --- /dev/null +++ b/test/known_issues/test-crypto-authenticated-stream.js @@ -0,0 +1,134 @@ +'use strict'; +// Refs: https://github.com/nodejs/node/issues/31733 +const common = require('../common'); + +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const crypto = require('crypto'); +const fs = require('fs'); +const path = require('path'); +const stream = require('stream'); +const tmpdir = require('../common/tmpdir'); + +class Sink extends stream.Writable { + constructor() { + super(); + this.chunks = []; + } + + _write(chunk, encoding, cb) { + this.chunks.push(chunk); + cb(); + } +} + +function direct(config) { + const { cipher, key, iv, aad, authTagLength, plaintextLength } = config; + const expected = Buffer.alloc(plaintextLength); + + const c = crypto.createCipheriv(cipher, key, iv, { authTagLength }); + c.setAAD(aad, { plaintextLength }); + const ciphertext = Buffer.concat([c.update(expected), c.final()]); + + const d = crypto.createDecipheriv(cipher, key, iv, { authTagLength }); + d.setAAD(aad, { plaintextLength }); + d.setAuthTag(c.getAuthTag()); + const actual = Buffer.concat([d.update(ciphertext), d.final()]); + + assert.deepStrictEqual(expected, actual); +} + +function mstream(config) { + const { cipher, key, iv, aad, authTagLength, plaintextLength } = config; + const expected = Buffer.alloc(plaintextLength); + + const c = crypto.createCipheriv(cipher, key, iv, { authTagLength }); + c.setAAD(aad, { plaintextLength }); + + const plain = new stream.PassThrough(); + const crypt = new Sink(); + const chunks = crypt.chunks; + plain.pipe(c).pipe(crypt); + plain.end(expected); + + crypt.on('close', common.mustCall(() => { + const d = crypto.createDecipheriv(cipher, key, iv, { authTagLength }); + d.setAAD(aad, { plaintextLength }); + d.setAuthTag(c.getAuthTag()); + + const crypt = new stream.PassThrough(); + const plain = new Sink(); + crypt.pipe(d).pipe(plain); + for (const chunk of chunks) crypt.write(chunk); + crypt.end(); + + plain.on('close', common.mustCall(() => { + const actual = Buffer.concat(plain.chunks); + assert.deepStrictEqual(expected, actual); + })); + })); +} + +function fstream(config) { + const count = fstream.count++; + const filename = (name) => path.join(tmpdir.path, `${name}${count}`); + + const { cipher, key, iv, aad, authTagLength, plaintextLength } = config; + const expected = Buffer.alloc(plaintextLength); + fs.writeFileSync(filename('a'), expected); + + const c = crypto.createCipheriv(cipher, key, iv, { authTagLength }); + c.setAAD(aad, { plaintextLength }); + + const plain = fs.createReadStream(filename('a')); + const crypt = fs.createWriteStream(filename('b')); + plain.pipe(c).pipe(crypt); + + // Observation: 'close' comes before 'end' on |c|, which definitely feels + // wrong. Switching to `c.on('end', ...)` doesn't fix the test though. + crypt.on('close', common.mustCall(() => { + const d = crypto.createDecipheriv(cipher, key, iv, { authTagLength }); + d.setAAD(aad, { plaintextLength }); + d.setAuthTag(c.getAuthTag()); + + const crypt = fs.createReadStream(filename('b')); + const plain = fs.createWriteStream(filename('c')); + crypt.pipe(d).pipe(plain); + + plain.on('close', common.mustCall(() => { + const actual = fs.readFileSync(filename('c')); + assert.deepStrictEqual(expected, actual); + })); + })); +} +fstream.count = 0; + +function test(config) { + direct(config); + mstream(config); + fstream(config); +} + +tmpdir.refresh(); + +// OK +test({ + cipher: 'aes-128-ccm', + aad: Buffer.alloc(1), + iv: Buffer.alloc(8), + key: Buffer.alloc(16), + authTagLength: 16, + plaintextLength: 32768, +}); + +// Fails the fstream test. +test({ + cipher: 'aes-128-ccm', + aad: Buffer.alloc(1), + iv: Buffer.alloc(8), + key: Buffer.alloc(16), + authTagLength: 16, + plaintextLength: 32769, +}); From 5d80d47c1bcef3597a26374e7b8a6b8c36f93d7b Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 18 Feb 2020 12:26:49 +0100 Subject: [PATCH 2/3] fixup! add extra sanity check --- test/known_issues/test-crypto-authenticated-stream.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/known_issues/test-crypto-authenticated-stream.js b/test/known_issues/test-crypto-authenticated-stream.js index 68fac35489b0c2..76d2d1be828a9a 100644 --- a/test/known_issues/test-crypto-authenticated-stream.js +++ b/test/known_issues/test-crypto-authenticated-stream.js @@ -89,6 +89,17 @@ function fstream(config) { // Observation: 'close' comes before 'end' on |c|, which definitely feels // wrong. Switching to `c.on('end', ...)` doesn't fix the test though. crypt.on('close', common.mustCall(() => { + // Just to drive home the point that decryption does actually work: + // reading the file synchronously, then decrypting it, works. + { + const ciphertext = fs.readFileSync(filename('b')); + const d = crypto.createDecipheriv(cipher, key, iv, { authTagLength }); + d.setAAD(aad, { plaintextLength }); + d.setAuthTag(c.getAuthTag()); + const actual = Buffer.concat([d.update(ciphertext), d.final()]); + assert.deepStrictEqual(expected, actual); + } + const d = crypto.createDecipheriv(cipher, key, iv, { authTagLength }); d.setAAD(aad, { plaintextLength }); d.setAuthTag(c.getAuthTag()); From 416e2143f76e74df9a758d353b1f29c56a51f834 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 18 Feb 2020 12:26:50 +0100 Subject: [PATCH 3/3] fixup! remove hasCrypto check --- test/known_issues/test-crypto-authenticated-stream.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/test/known_issues/test-crypto-authenticated-stream.js b/test/known_issues/test-crypto-authenticated-stream.js index 76d2d1be828a9a..1e2a9edd7b0a6e 100644 --- a/test/known_issues/test-crypto-authenticated-stream.js +++ b/test/known_issues/test-crypto-authenticated-stream.js @@ -1,10 +1,7 @@ +/* eslint-disable node-core/crypto-check */ 'use strict'; // Refs: https://github.com/nodejs/node/issues/31733 const common = require('../common'); - -if (!common.hasCrypto) - common.skip('missing crypto'); - const assert = require('assert'); const crypto = require('crypto'); const fs = require('fs');