Skip to content

Commit faf449c

Browse files
committed
crypto: throw in setAuthTag on invalid length
The current implementation performs limited checks only and silently ignores superfluous bytes of the authentication tag. This change makes setAuthTag throw when - the user-specified authTagLength does not match the actual tag length, especially when the authentication tag is longer than 16 bytes, and when - the mode is GCM, no authTagLength option has been specified and the tag length is not a valid GCM tag length. This change makes the conditional assignment in SetAuthTag unnecessary, which is replaced with a CHECK. Refs: #17825 PR-URL: #20040 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent cb3d049 commit faf449c

File tree

2 files changed

+25
-20
lines changed

2 files changed

+25
-20
lines changed

src/node_crypto.cc

+18-15
Original file line numberDiff line numberDiff line change
@@ -2806,9 +2806,7 @@ bool CipherBase::InitAuthenticated(const char* cipher_type, int iv_len,
28062806
return false;
28072807
}
28082808

2809-
// When decrypting in CCM mode, this field will be set in setAuthTag().
2810-
if (kind_ == kCipher)
2811-
auth_tag_len_ = auth_tag_len;
2809+
auth_tag_len_ = auth_tag_len;
28122810

28132811
// The message length is restricted to 2 ^ (8 * (15 - iv_len)) - 1 bytes.
28142812
CHECK(iv_len >= 7 && iv_len <= 13);
@@ -2824,7 +2822,7 @@ bool CipherBase::InitAuthenticated(const char* cipher_type, int iv_len,
28242822
if (!IsValidGCMTagLength(auth_tag_len)) {
28252823
char msg[50];
28262824
snprintf(msg, sizeof(msg),
2827-
"Invalid GCM authentication tag length: %u", auth_tag_len);
2825+
"Invalid authentication tag length: %u", auth_tag_len);
28282826
env()->ThrowError(msg);
28292827
return false;
28302828
}
@@ -2891,21 +2889,26 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
28912889
// Restrict GCM tag lengths according to NIST 800-38d, page 9.
28922890
unsigned int tag_len = Buffer::Length(args[0]);
28932891
const int mode = EVP_CIPHER_CTX_mode(cipher->ctx_.get());
2892+
bool is_valid;
28942893
if (mode == EVP_CIPH_GCM_MODE) {
2895-
if ((cipher->auth_tag_len_ != kNoAuthTagLength &&
2896-
cipher->auth_tag_len_ != tag_len) ||
2897-
!IsValidGCMTagLength(tag_len)) {
2898-
char msg[50];
2899-
snprintf(msg, sizeof(msg),
2900-
"Invalid GCM authentication tag length: %u", tag_len);
2901-
return cipher->env()->ThrowError(msg);
2902-
}
2894+
is_valid = (cipher->auth_tag_len_ == kNoAuthTagLength ||
2895+
cipher->auth_tag_len_ == tag_len) &&
2896+
IsValidGCMTagLength(tag_len);
2897+
} else {
2898+
CHECK_EQ(mode, EVP_CIPH_CCM_MODE);
2899+
CHECK_NE(cipher->auth_tag_len_, kNoAuthTagLength);
2900+
is_valid = cipher->auth_tag_len_ == tag_len;
2901+
}
2902+
2903+
if (!is_valid) {
2904+
char msg[50];
2905+
snprintf(msg, sizeof(msg),
2906+
"Invalid authentication tag length: %u", tag_len);
2907+
return cipher->env()->ThrowError(msg);
29032908
}
29042909

2905-
// Note: we don't use std::min() here to work around a header conflict.
29062910
cipher->auth_tag_len_ = tag_len;
2907-
if (cipher->auth_tag_len_ > sizeof(cipher->auth_tag_))
2908-
cipher->auth_tag_len_ = sizeof(cipher->auth_tag_);
2911+
CHECK_LE(cipher->auth_tag_len_, sizeof(cipher->auth_tag_));
29092912

29102913
memset(cipher->auth_tag_, 0, sizeof(cipher->auth_tag_));
29112914
memcpy(cipher->auth_tag_, Buffer::Data(args[0]), cipher->auth_tag_len_);

test/parallel/test-crypto-authenticated.js

+7-5
Original file line numberDiff line numberDiff line change
@@ -724,7 +724,7 @@ for (const test of TEST_CASES) {
724724
decrypt.setAuthTag(Buffer.from('1'.repeat(length)));
725725
}, {
726726
type: Error,
727-
message: `Invalid GCM authentication tag length: ${length}`
727+
message: `Invalid authentication tag length: ${length}`
728728
});
729729

730730
common.expectsError(() => {
@@ -736,7 +736,7 @@ for (const test of TEST_CASES) {
736736
});
737737
}, {
738738
type: Error,
739-
message: `Invalid GCM authentication tag length: ${length}`
739+
message: `Invalid authentication tag length: ${length}`
740740
});
741741

742742
common.expectsError(() => {
@@ -748,7 +748,7 @@ for (const test of TEST_CASES) {
748748
});
749749
}, {
750750
type: Error,
751-
message: `Invalid GCM authentication tag length: ${length}`
751+
message: `Invalid authentication tag length: ${length}`
752752
});
753753
}
754754
}
@@ -783,7 +783,7 @@ for (const test of TEST_CASES) {
783783
decipher.setAuthTag(Buffer.from('1'.repeat(12)));
784784
}, {
785785
type: Error,
786-
message: 'Invalid GCM authentication tag length: 12'
786+
message: 'Invalid authentication tag length: 12'
787787
});
788788

789789
// The Decipher object should be left intact.
@@ -985,7 +985,7 @@ for (const test of TEST_CASES) {
985985
}
986986
}
987987

988-
// Test that setAAD throws in CCM mode when no authentication tag is provided.
988+
// Test that final() throws in CCM mode when no authentication tag is provided.
989989
{
990990
if (!common.hasFipsCrypto) {
991991
const key = Buffer.from('1ed2233fa2223ef5d7df08546049406c', 'hex');
@@ -1000,6 +1000,8 @@ for (const test of TEST_CASES) {
10001000
decrypt.setAAD(Buffer.from('63616c76696e', 'hex'), {
10011001
plaintextLength: ct.length
10021002
});
1003+
decrypt.update(ct);
1004+
decrypt.final();
10031005
}, errMessages.state);
10041006
}
10051007
}

0 commit comments

Comments
 (0)