Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: allow to restrict valid GCM tag length #20039

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion doc/api/crypto.md
Original file line number Diff line number Diff line change
@@ -1457,6 +1457,10 @@ to create the `Decipher` object.
<!-- YAML
added: v0.1.94
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/???
description: The `authTagLength` option can now be used to restrict accepted
GCM authentication tag lengths.
- version: v9.9.0
pr-url: https://github.com/nodejs/node/pull/18644
description: The `iv` parameter may now be `null` for ciphers which do not
@@ -1474,7 +1478,9 @@ and initialization vector (`iv`).
The `options` argument controls stream behavior and is optional except when a
cipher in CCM mode is used (e.g. `'aes-128-ccm'`). In that case, the
`authTagLength` option is required and specifies the length of the
authentication tag in bytes, see [CCM mode][].
authentication tag in bytes, see [CCM mode][]. In GCM mode, the `authTagLength`
option is not required but can be used to restrict accepted authentication tags
to those with the specified length.

The `algorithm` is dependent on OpenSSL, examples are `'aes192'`, etc. On
recent OpenSSL releases, `openssl list-cipher-algorithms` will display the
33 changes: 28 additions & 5 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
@@ -2806,6 +2806,10 @@ void CipherBase::InitIv(const FunctionCallbackInfo<Value>& args) {
}


static bool IsValidGCMTagLength(unsigned int tag_len) {
return tag_len == 4 || tag_len == 8 || tag_len >= 12 && tag_len <= 16;
Copy link
Member

@yhwang yhwang Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tniessen I just saw compiler warning today:

../src/node_crypto.cc:2801:56: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]
   return tag_len == 4 || tag_len == 8 || tag_len >= 12 && tag_len <= 16;
                                                        ^

Typically, how do we fix this? Create another PR to fix this?

}

bool CipherBase::InitAuthenticated(const char *cipher_type, int iv_len,
int auth_tag_len) {
CHECK(IsAuthenticatedMode());
@@ -2818,7 +2822,8 @@ bool CipherBase::InitAuthenticated(const char *cipher_type, int iv_len,
return false;
}

if (EVP_CIPHER_CTX_mode(ctx_) == EVP_CIPH_CCM_MODE) {
const int mode = EVP_CIPHER_CTX_mode(ctx_);
if (mode == EVP_CIPH_CCM_MODE) {
if (auth_tag_len < 0) {
char msg[128];
snprintf(msg, sizeof(msg), "authTagLength required for %s", cipher_type);
@@ -2851,6 +2856,21 @@ bool CipherBase::InitAuthenticated(const char *cipher_type, int iv_len,
} else {
max_message_size_ = INT_MAX;
}
} else {
CHECK_EQ(mode, EVP_CIPH_GCM_MODE);

if (auth_tag_len >= 0) {
if (!IsValidGCMTagLength(auth_tag_len)) {
char msg[50];
snprintf(msg, sizeof(msg),
"Invalid GCM authentication tag length: %u", auth_tag_len);
env()->ThrowError(msg);
return false;
}

// Remember the given authentication tag length for later.
auth_tag_len_ = auth_tag_len;
}
}

return true;
@@ -2886,7 +2906,7 @@ void CipherBase::GetAuthTag(const FunctionCallbackInfo<Value>& args) {
// Only callable after Final and if encrypting.
if (cipher->ctx_ != nullptr ||
cipher->kind_ != kCipher ||
cipher->auth_tag_len_ == 0) {
cipher->auth_tag_len_ == kNoAuthTagLength) {
return args.GetReturnValue().SetUndefined();
}

@@ -2911,7 +2931,9 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
unsigned int tag_len = Buffer::Length(args[0]);
const int mode = EVP_CIPHER_CTX_mode(cipher->ctx_);
if (mode == EVP_CIPH_GCM_MODE) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tniessen I know your change is for GCM. I just think what's the minimal check we should do for CCM? Something like:

(cipher->auth_tag_len_ >= 0 && cipher->auth_tag_len_ != tag_len) || tag_len > EVP_GCM_TLS_TAG_LEN

Copy link
Member Author

@tniessen tniessen Apr 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yhwang This is basically the same problem as we discussed in #20040. I will fix it in that PR so this can remain semver-minor. This patch should work either way, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes!

if (tag_len > 16 || (tag_len < 12 && tag_len != 8 && tag_len != 4)) {
if (cipher->auth_tag_len_ != kNoAuthTagLength &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compiler warning:

../src/node_crypto.cc:2925:51: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]
     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);
@@ -2947,7 +2969,8 @@ bool CipherBase::SetAAD(const char* data, unsigned int len, int plaintext_len) {
if (!CheckCCMMessageLength(plaintext_len))
return false;

if (kind_ == kDecipher && !auth_tag_set_ && auth_tag_len_ > 0) {
if (kind_ == kDecipher && !auth_tag_set_ && auth_tag_len_ > 0 &&
auth_tag_len_ != kNoAuthTagLength) {
if (!EVP_CIPHER_CTX_ctrl(ctx_,
EVP_CTRL_CCM_SET_TAG,
auth_tag_len_,
@@ -3000,7 +3023,7 @@ CipherBase::UpdateResult CipherBase::Update(const char* data,

// on first update:
if (kind_ == kDecipher && IsAuthenticatedMode() && auth_tag_len_ > 0 &&
!auth_tag_set_) {
auth_tag_len_ != kNoAuthTagLength && !auth_tag_set_) {
EVP_CIPHER_CTX_ctrl(ctx_,
EVP_CTRL_GCM_SET_TAG,
auth_tag_len_,
3 changes: 2 additions & 1 deletion src/node_crypto.h
Original file line number Diff line number Diff line change
@@ -359,6 +359,7 @@ class CipherBase : public BaseObject {
kErrorMessageSize,
kErrorState
};
static const unsigned kNoAuthTagLength = static_cast<unsigned>(-1);

void Init(const char* cipher_type,
const char* key_buf,
@@ -398,7 +399,7 @@ class CipherBase : public BaseObject {
ctx_(nullptr),
kind_(kind),
auth_tag_set_(false),
auth_tag_len_(0),
auth_tag_len_(kNoAuthTagLength),
pending_auth_failed_(false) {
MakeWeak<CipherBase>(this);
}
37 changes: 37 additions & 0 deletions test/parallel/test-crypto-authenticated.js
Original file line number Diff line number Diff line change
@@ -726,9 +726,46 @@ for (const test of TEST_CASES) {
type: Error,
message: `Invalid GCM authentication tag length: ${length}`
});

common.expectsError(() => {
crypto.createDecipheriv('aes-256-gcm',
'FxLKsqdmv0E9xrQhp0b1ZgI0K7JFZJM8',
'qkuZpJWCewa6Szih',
{
authTagLength: length
});
}, {
type: Error,
message: `Invalid GCM authentication tag length: ${length}`
});
}
}

// Test that users can manually restrict the GCM tag length to a single value.
{
const decipher = crypto.createDecipheriv('aes-256-gcm',
'FxLKsqdmv0E9xrQhp0b1ZgI0K7JFZJM8',
'qkuZpJWCewa6Szih', {
authTagLength: 8
});

common.expectsError(() => {
// This tag would normally be allowed.
decipher.setAuthTag(Buffer.from('1'.repeat(12)));
}, {
type: Error,
message: 'Invalid GCM authentication tag length: 12'
});

// The Decipher object should be left intact.
decipher.setAuthTag(Buffer.from('445352d3ff85cf94', 'hex'));
const text = Buffer.concat([
decipher.update('3a2a3647', 'hex'),
decipher.final()
]);
assert.strictEqual(text.toString('utf8'), 'node');
}

// Test that create(De|C)ipher(iv)? throws if the mode is CCM and an invalid
// authentication tag length has been specified.
{