From c5d03725c185f58c456d23d671375e55a5849d62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Sat, 9 Dec 2017 13:12:47 +0100 Subject: [PATCH 1/6] test: use valid authentication tag length Using authentication tags of invalid length does not conform to NIST standards. --- test/parallel/test-crypto-cipher-decipher.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-crypto-cipher-decipher.js b/test/parallel/test-crypto-cipher-decipher.js index 2a4d188dc4bb23..1e84d2ab009430 100644 --- a/test/parallel/test-crypto-cipher-decipher.js +++ b/test/parallel/test-crypto-cipher-decipher.js @@ -216,7 +216,7 @@ testCipher2(Buffer.from('0123456789abcdef')); // setAutoPadding/setAuthTag/setAAD should return `this` { const key = '0123456789'; - const tagbuf = Buffer.from('tagbuf'); + const tagbuf = Buffer.from('auth_tag'); const aadbuf = Buffer.from('aadbuf'); const decipher = crypto.createDecipher('aes-256-gcm', key); assert.strictEqual(decipher.setAutoPadding(), decipher); From fc6c652b251e3c30dc82e02de23c6e4bc8f14ac0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Sat, 9 Dec 2017 13:23:18 +0100 Subject: [PATCH 2/6] crypto: warn on invalid authentication tag length Refs: https://github.com/nodejs/node/issues/17523 --- src/node_crypto.cc | 13 +++++++++++-- test/parallel/test-crypto-authenticated.js | 19 +++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 498443f9441e4f..9cd960ab4acf05 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3767,9 +3767,18 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo& args) { return args.GetReturnValue().Set(false); } - // FIXME(bnoordhuis) Throw when buffer length is not a valid tag size. + unsigned int tag_len = Buffer::Length(args[0]); + if (EVP_CIPHER_CTX_mode(cipher->ctx_) == EVP_CIPH_GCM_MODE) { + if (tag_len > 16 || (tag_len < 12 && tag_len != 8 && tag_len != 4)) { + ProcessEmitWarning(cipher->env(), + "Permitting authentication tag lengths of %u bytes is deprecated. " + "Valid GCM tag lengths are 4, 8, 12, 13, 14, 15, 16.", + tag_len); + } + } + // Note: we don't use std::max() here to work around a header conflict. - cipher->auth_tag_len_ = Buffer::Length(args[0]); + cipher->auth_tag_len_ = tag_len; if (cipher->auth_tag_len_ > sizeof(cipher->auth_tag_)) cipher->auth_tag_len_ = sizeof(cipher->auth_tag_); diff --git a/test/parallel/test-crypto-authenticated.js b/test/parallel/test-crypto-authenticated.js index c03aa0efce54e0..68fab33b5903bd 100644 --- a/test/parallel/test-crypto-authenticated.js +++ b/test/parallel/test-crypto-authenticated.js @@ -335,6 +335,14 @@ const errMessages = { const ciphers = crypto.getCiphers(); +common.expectWarning('Warning', [ + 'Use Cipheriv for counter mode of aes-192-gcm' +].concat( + [0, 1, 2, 6, 9, 10, 11, 17] + .map((i) => `Permitting authentication tag lengths of ${i} bytes is ` + + 'deprecated. Valid GCM tag lengths are 4, 8, 12, 13, 14, 15, 16.') +)); + for (const i in TEST_CASES) { const test = TEST_CASES[i]; @@ -476,3 +484,14 @@ for (const i in TEST_CASES) { assert.throws(() => encrypt.setAAD(Buffer.from('123', 'ascii')), errMessages.state); } + +// GCM only supports specific authentication tag lengths, invalid lengths should +// produce warnings. +{ + for (const length of [0, 1, 2, 4, 6, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17]) { + const decrypt = crypto.createDecipheriv('aes-256-gcm', + 'FxLKsqdmv0E9xrQhp0b1ZgI0K7JFZJM8', + 'qkuZpJWCewa6Szih'); + decrypt.setAuthTag(Buffer.from('1'.repeat(length))); + } +} From 54d49c1c473f3900330c282dc8ab7dfa83f677b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Sun, 10 Dec 2017 12:36:01 +0100 Subject: [PATCH 3/6] [squash] remove GCM check --- src/node_crypto.cc | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 9cd960ab4acf05..f22f554274057e 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3767,14 +3767,13 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo& args) { return args.GetReturnValue().Set(false); } + // Restrict GCM tag lengths according to NIST 800-38d, page 9 unsigned int tag_len = Buffer::Length(args[0]); - if (EVP_CIPHER_CTX_mode(cipher->ctx_) == EVP_CIPH_GCM_MODE) { - if (tag_len > 16 || (tag_len < 12 && tag_len != 8 && tag_len != 4)) { - ProcessEmitWarning(cipher->env(), - "Permitting authentication tag lengths of %u bytes is deprecated. " - "Valid GCM tag lengths are 4, 8, 12, 13, 14, 15, 16.", - tag_len); - } + if (tag_len > 16 || (tag_len < 12 && tag_len != 8 && tag_len != 4)) { + ProcessEmitWarning(cipher->env(), + "Permitting authentication tag lengths of %u bytes is deprecated. " + "Valid GCM tag lengths are 4, 8, 12, 13, 14, 15, 16.", + tag_len); } // Note: we don't use std::max() here to work around a header conflict. From bbf356e78f83bd13d397b5aa4c8a86c4afd91bc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Sun, 10 Dec 2017 14:33:19 +0100 Subject: [PATCH 4/6] [squash] fix FIPS failure --- test/parallel/test-crypto-authenticated.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-crypto-authenticated.js b/test/parallel/test-crypto-authenticated.js index 68fab33b5903bd..7456946067b0e8 100644 --- a/test/parallel/test-crypto-authenticated.js +++ b/test/parallel/test-crypto-authenticated.js @@ -335,7 +335,7 @@ const errMessages = { const ciphers = crypto.getCiphers(); -common.expectWarning('Warning', [ +common.expectWarning('Warning', common.hasFipsCrypto ? [] : [ 'Use Cipheriv for counter mode of aes-192-gcm' ].concat( [0, 1, 2, 6, 9, 10, 11, 17] From 0b5c90f119c8d105ac51f15dbdc9b4998443e8da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Sun, 10 Dec 2017 15:10:15 +0100 Subject: [PATCH 5/6] [squash] fix FIPS (again) --- test/parallel/test-crypto-authenticated.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-crypto-authenticated.js b/test/parallel/test-crypto-authenticated.js index 7456946067b0e8..df6b4316390594 100644 --- a/test/parallel/test-crypto-authenticated.js +++ b/test/parallel/test-crypto-authenticated.js @@ -335,9 +335,9 @@ const errMessages = { const ciphers = crypto.getCiphers(); -common.expectWarning('Warning', common.hasFipsCrypto ? [] : [ +common.expectWarning('Warning', (common.hasFipsCrypto ? [] : [ 'Use Cipheriv for counter mode of aes-192-gcm' -].concat( +]).concat( [0, 1, 2, 6, 9, 10, 11, 17] .map((i) => `Permitting authentication tag lengths of ${i} bytes is ` + 'deprecated. Valid GCM tag lengths are 4, 8, 12, 13, 14, 15, 16.') From e69439320968faba58839f4bab5de2e51bf11638 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Mon, 11 Dec 2017 13:26:14 +0100 Subject: [PATCH 6/6] [squash] add dot --- src/node_crypto.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index f22f554274057e..923ba5c8479b6b 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3767,7 +3767,7 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo& args) { return args.GetReturnValue().Set(false); } - // Restrict GCM tag lengths according to NIST 800-38d, page 9 + // Restrict GCM tag lengths according to NIST 800-38d, page 9. unsigned int tag_len = Buffer::Length(args[0]); if (tag_len > 16 || (tag_len < 12 && tag_len != 8 && tag_len != 4)) { ProcessEmitWarning(cipher->env(),