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
Show file tree
Hide file tree
Changes from 1 commit
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
Expand Up @@ -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
Expand All @@ -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
Expand Down
26 changes: 23 additions & 3 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2806,6 +2806,10 @@ void CipherBase::InitIv(const FunctionCallbackInfo<Value>& args) {
}


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

Choose a reason for hiding this comment

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

Micro-nit: superfluous parens.

}

bool CipherBase::InitAuthenticated(const char *cipher_type, int iv_len,
int auth_tag_len) {
CHECK(IsAuthenticatedMode());
Expand All @@ -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);
Expand Down Expand Up @@ -2851,6 +2856,19 @@ 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 && !IsValidGCMTagLength(auth_tag_len)) {
char msg[50];
snprintf(msg, sizeof(msg),
"Invalid GCM authentication tag length: %u", auth_tag_len);
env()->ThrowError(msg);
Copy link
Member

Choose a reason for hiding this comment

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

Would be ideal if the error can be thrown in JS but that's not necessary for landing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I hope to have some time between the release of node 10 and node 11 to refactor the whole cipher API to use the new error system, this PR aims for consistency with the rest :)

return false;
}

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

return true;
Expand Down Expand Up @@ -2886,10 +2904,11 @@ 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_ <= 0) {
return args.GetReturnValue().SetUndefined();
}

CHECK_GE(cipher->auth_tag_len_, 0);
Local<Object> buf =
Buffer::Copy(env, cipher->auth_tag_, cipher->auth_tag_len_)
.ToLocalChecked();
Expand All @@ -2911,7 +2930,8 @@ 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_ >= 0 && cipher->auth_tag_len_ != tag_len) ||
Copy link
Member

Choose a reason for hiding this comment

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

Here too.

!IsValidGCMTagLength(tag_len)) {
char msg[50];
snprintf(msg, sizeof(msg),
"Invalid GCM authentication tag length: %u", tag_len);
Expand Down
4 changes: 2 additions & 2 deletions src/node_crypto.h
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ class CipherBase : public BaseObject {
ctx_(nullptr),
kind_(kind),
auth_tag_set_(false),
auth_tag_len_(0),
auth_tag_len_(-1),
pending_auth_failed_(false) {
MakeWeak<CipherBase>(this);
}
Expand All @@ -407,7 +407,7 @@ class CipherBase : public BaseObject {
EVP_CIPHER_CTX* ctx_;
const CipherKind kind_;
bool auth_tag_set_;
unsigned int auth_tag_len_;
int auth_tag_len_;
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer using some unsigned sentinel value, e.g.:

static const unsigned kNoAuthTag = static_cast<unsigned>(-1);

char auth_tag_[EVP_GCM_TLS_TAG_LEN];
bool pending_auth_failed_;
int max_message_size_;
Expand Down
37 changes: 37 additions & 0 deletions test/parallel/test-crypto-authenticated.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
{
Expand Down