Skip to content

Commit

Permalink
crypto: throw in setAuthTag on invalid length
Browse files Browse the repository at this point in the history
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: nodejs#17825
  • Loading branch information
tniessen committed May 23, 2018
1 parent 0271b0f commit 00cf4aa
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 20 deletions.
33 changes: 18 additions & 15 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2811,9 +2811,7 @@ bool CipherBase::InitAuthenticated(const char *cipher_type, int iv_len,
return false;
}

// When decrypting in CCM mode, this field will be set in setAuthTag().
if (kind_ == kCipher)
auth_tag_len_ = auth_tag_len;
auth_tag_len_ = auth_tag_len;

// The message length is restricted to 2 ^ (8 * (15 - iv_len)) - 1 bytes.
CHECK(iv_len >= 7 && iv_len <= 13);
Expand All @@ -2829,7 +2827,7 @@ bool CipherBase::InitAuthenticated(const char *cipher_type, int iv_len,
if (!IsValidGCMTagLength(auth_tag_len)) {
char msg[50];
snprintf(msg, sizeof(msg),
"Invalid GCM authentication tag length: %u", auth_tag_len);
"Invalid authentication tag length: %u", auth_tag_len);
env()->ThrowError(msg);
return false;
}
Expand Down Expand Up @@ -2896,21 +2894,26 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
// Restrict GCM tag lengths according to NIST 800-38d, page 9.
unsigned int tag_len = Buffer::Length(args[0]);
const int mode = EVP_CIPHER_CTX_mode(cipher->ctx_.get());
bool is_valid;
if (mode == EVP_CIPH_GCM_MODE) {
if ((cipher->auth_tag_len_ != kNoAuthTagLength &&
cipher->auth_tag_len_ != tag_len) ||
!IsValidGCMTagLength(tag_len)) {
char msg[50];
snprintf(msg, sizeof(msg),
"Invalid GCM authentication tag length: %u", tag_len);
return cipher->env()->ThrowError(msg);
}
is_valid = (cipher->auth_tag_len_ == kNoAuthTagLength ||
cipher->auth_tag_len_ == tag_len) &&
IsValidGCMTagLength(tag_len);
} else {
CHECK_EQ(mode, EVP_CIPH_CCM_MODE);
CHECK_NE(cipher->auth_tag_len_, kNoAuthTagLength);
is_valid = cipher->auth_tag_len_ == tag_len;
}

if (!is_valid) {
char msg[50];
snprintf(msg, sizeof(msg),
"Invalid authentication tag length: %u", tag_len);
return cipher->env()->ThrowError(msg);
}

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

memset(cipher->auth_tag_, 0, sizeof(cipher->auth_tag_));
memcpy(cipher->auth_tag_, Buffer::Data(args[0]), cipher->auth_tag_len_);
Expand Down
12 changes: 7 additions & 5 deletions test/parallel/test-crypto-authenticated.js
Original file line number Diff line number Diff line change
Expand Up @@ -724,7 +724,7 @@ for (const test of TEST_CASES) {
decrypt.setAuthTag(Buffer.from('1'.repeat(length)));
}, {
type: Error,
message: `Invalid GCM authentication tag length: ${length}`
message: `Invalid authentication tag length: ${length}`
});

common.expectsError(() => {
Expand All @@ -736,7 +736,7 @@ for (const test of TEST_CASES) {
});
}, {
type: Error,
message: `Invalid GCM authentication tag length: ${length}`
message: `Invalid authentication tag length: ${length}`
});

common.expectsError(() => {
Expand All @@ -748,7 +748,7 @@ for (const test of TEST_CASES) {
});
}, {
type: Error,
message: `Invalid GCM authentication tag length: ${length}`
message: `Invalid authentication tag length: ${length}`
});
}
}
Expand Down Expand Up @@ -783,7 +783,7 @@ for (const test of TEST_CASES) {
decipher.setAuthTag(Buffer.from('1'.repeat(12)));
}, {
type: Error,
message: 'Invalid GCM authentication tag length: 12'
message: 'Invalid authentication tag length: 12'
});

// The Decipher object should be left intact.
Expand Down Expand Up @@ -985,7 +985,7 @@ for (const test of TEST_CASES) {
}
}

// Test that setAAD throws in CCM mode when no authentication tag is provided.
// Test that final() throws in CCM mode when no authentication tag is provided.
{
if (!common.hasFipsCrypto) {
const key = Buffer.from('1ed2233fa2223ef5d7df08546049406c', 'hex');
Expand All @@ -1000,6 +1000,8 @@ for (const test of TEST_CASES) {
decrypt.setAAD(Buffer.from('63616c76696e', 'hex'), {
plaintextLength: ct.length
});
decrypt.update(ct);
decrypt.final();
}, errMessages.state);
}
}
Expand Down

0 comments on commit 00cf4aa

Please sign in to comment.