Skip to content

Commit 92aae40

Browse files
tniessenRafaelGSS
authored andcommitted
crypto: make auth tag size assumption explicit
The `CipherBase` class assumes that any authentication tag will fit into `EVP_GCM_TLS_TAG_LEN` bytes, which is true because Node.js only supports GCM with AES as the blocker cipher, and the block size of AES happens to be 16 bytes, which coincidentally is also the output size of the Poly1305 construction used by ChaCha20-Poly1305 as well as the maximum size of authentication tags produced by AES in CCM or OCB mode. This commit adds a new constant `ncrypto::Cipher::MAX_AUTH_TAG_LENGTH` which is the maximum length of authentication tags produced by algorithms that Node.js supports and replaces some constants in `CipherBase` with semantically more meaningful named constants. The OpenSSL team is debating whether a constant like `MAX_AUTH_TAG_LENGTH` (`EVP_MAX_AEAD_TAG_LENGTH`) should exist at all since its value necessarily depends on the set of AEAD algorithms supported, but I do believe that, for Node.js, this is a step in the right direction. It certainly makes more sense than to use the AES-GCM tag size as defined by TLS. PR-URL: #57803 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 5e14fd1 commit 92aae40

File tree

3 files changed

+12
-4
lines changed

3 files changed

+12
-4
lines changed

deps/ncrypto/ncrypto.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,14 @@ class Cipher final {
285285
public:
286286
static constexpr size_t MAX_KEY_LENGTH = EVP_MAX_KEY_LENGTH;
287287
static constexpr size_t MAX_IV_LENGTH = EVP_MAX_IV_LENGTH;
288+
#ifdef EVP_MAX_AEAD_TAG_LENGTH
289+
static constexpr size_t MAX_AUTH_TAG_LENGTH = EVP_MAX_AEAD_TAG_LENGTH;
290+
#else
291+
static constexpr size_t MAX_AUTH_TAG_LENGTH = 16;
292+
#endif
293+
static_assert(EVP_GCM_TLS_TAG_LEN <= MAX_AUTH_TAG_LENGTH &&
294+
EVP_CCM_TLS_TAG_LEN <= MAX_AUTH_TAG_LENGTH &&
295+
EVP_CHACHAPOLY_TLS_TAG_LEN <= MAX_AUTH_TAG_LENGTH);
288296

289297
Cipher() = default;
290298
Cipher(const EVP_CIPHER* cipher) : cipher_(cipher) {}

src/crypto/crypto_cipher.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -459,7 +459,7 @@ bool CipherBase::InitAuthenticated(std::string_view cipher_type,
459459
// authentication tag length also defaults to 16 bytes when decrypting,
460460
// whereas GCM would accept any valid authentication tag length.
461461
if (ctx_.isChaCha20Poly1305()) {
462-
auth_tag_len = 16;
462+
auth_tag_len = EVP_CHACHAPOLY_TLS_TAG_LEN;
463463
} else {
464464
THROW_ERR_CRYPTO_INVALID_AUTH_TAG(
465465
env(), "authTagLength required for %s", cipher_type);
@@ -572,7 +572,7 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
572572
}
573573

574574
if (cipher->ctx_.isGcmMode() && cipher->auth_tag_len_ == kNoAuthTagLength &&
575-
tag_len != 16 && env->EmitProcessEnvWarning()) {
575+
tag_len != EVP_GCM_TLS_TAG_LEN && env->EmitProcessEnvWarning()) {
576576
if (ProcessEmitDeprecationWarning(
577577
env,
578578
"Using AES-GCM authentication tags of less than 128 bits without "
@@ -821,7 +821,7 @@ bool CipherBase::Final(std::unique_ptr<BackingStore>* out) {
821821
// always be given by the user.
822822
if (auth_tag_len_ == kNoAuthTagLength) {
823823
CHECK(ctx_.isGcmMode());
824-
auth_tag_len_ = sizeof(auth_tag_);
824+
auth_tag_len_ = EVP_GCM_TLS_TAG_LEN;
825825
}
826826
ok = ctx_.getAeadTag(auth_tag_len_,
827827
reinterpret_cast<unsigned char*>(auth_tag_));

src/crypto/crypto_cipher.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ class CipherBase : public BaseObject {
8686
const CipherKind kind_;
8787
AuthTagState auth_tag_state_;
8888
unsigned int auth_tag_len_;
89-
char auth_tag_[EVP_GCM_TLS_TAG_LEN];
89+
char auth_tag_[ncrypto::Cipher::MAX_AUTH_TAG_LENGTH];
9090
bool pending_auth_failed_;
9191
int max_message_size_;
9292
};

0 commit comments

Comments
 (0)