-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
} | ||
|
||
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,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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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(); | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tniessen I know your change is for
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
} | ||
|
@@ -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_; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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_; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Micro-nit: superfluous parens.