Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit 624e516

Browse filesBrowse files
tniessentargos
authored andcommitted
crypto: fix edge case in authenticated encryption
Restricting the authentication tag length and calling update or setAAD before setAuthTag caused an incorrect authentication tag to be passed to OpenSSL: The auth_tag_len_ field was already set, so the implementation assumed that the tag itself was known as well. PR-URL: #22828 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
1 parent 329ac60 commit 624e516
Copy full SHA for 624e516

File tree

Expand file treeCollapse file tree

3 files changed

+38
-20
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

3 files changed

+38
-20
lines changed
Open diff view settings
Collapse file

‎src/node_crypto.cc‎

Copy file name to clipboardExpand all lines: src/node_crypto.cc
+7-2Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2891,6 +2891,10 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
28912891
return args.GetReturnValue().Set(false);
28922892
}
28932893

2894+
// TODO(tniessen): Throw if the authentication tag has already been set.
2895+
if (cipher->auth_tag_state_ == kAuthTagPassedToOpenSSL)
2896+
return args.GetReturnValue().Set(true);
2897+
28942898
unsigned int tag_len = Buffer::Length(args[0]);
28952899
const int mode = EVP_CIPHER_CTX_mode(cipher->ctx_.get());
28962900
if (mode == EVP_CIPH_GCM_MODE) {
@@ -2923,6 +2927,7 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
29232927

29242928
// Note: we don't use std::min() here to work around a header conflict.
29252929
cipher->auth_tag_len_ = tag_len;
2930+
cipher->auth_tag_state_ = kAuthTagKnown;
29262931
if (cipher->auth_tag_len_ > sizeof(cipher->auth_tag_))
29272932
cipher->auth_tag_len_ = sizeof(cipher->auth_tag_);
29282933

@@ -2934,14 +2939,14 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
29342939

29352940

29362941
bool CipherBase::MaybePassAuthTagToOpenSSL() {
2937-
if (!auth_tag_set_ && auth_tag_len_ != kNoAuthTagLength) {
2942+
if (auth_tag_state_ == kAuthTagKnown) {
29382943
if (!EVP_CIPHER_CTX_ctrl(ctx_.get(),
29392944
EVP_CTRL_AEAD_SET_TAG,
29402945
auth_tag_len_,
29412946
reinterpret_cast<unsigned char*>(auth_tag_))) {
29422947
return false;
29432948
}
2944-
auth_tag_set_ = true;
2949+
auth_tag_state_ = kAuthTagPassedToOpenSSL;
29452950
}
29462951
return true;
29472952
}
Collapse file

‎src/node_crypto.h‎

Copy file name to clipboardExpand all lines: src/node_crypto.h
+7-2Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,11 @@ class CipherBase : public BaseObject {
363363
kErrorMessageSize,
364364
kErrorState
365365
};
366+
enum AuthTagState {
367+
kAuthTagUnknown,
368+
kAuthTagKnown,
369+
kAuthTagPassedToOpenSSL
370+
};
366371
static const unsigned kNoAuthTagLength = static_cast<unsigned>(-1);
367372

368373
void Init(const char* cipher_type,
@@ -404,7 +409,7 @@ class CipherBase : public BaseObject {
404409
: BaseObject(env, wrap),
405410
ctx_(nullptr),
406411
kind_(kind),
407-
auth_tag_set_(false),
412+
auth_tag_state_(kAuthTagUnknown),
408413
auth_tag_len_(kNoAuthTagLength),
409414
pending_auth_failed_(false) {
410415
MakeWeak();
@@ -413,7 +418,7 @@ class CipherBase : public BaseObject {
413418
private:
414419
DeleteFnPtr<EVP_CIPHER_CTX, EVP_CIPHER_CTX_free> ctx_;
415420
const CipherKind kind_;
416-
bool auth_tag_set_;
421+
AuthTagState auth_tag_state_;
417422
unsigned int auth_tag_len_;
418423
char auth_tag_[EVP_GCM_TLS_TAG_LEN];
419424
bool pending_auth_failed_;
Collapse file

‎test/parallel/test-crypto-authenticated.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-crypto-authenticated.js
+24-16Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -579,27 +579,35 @@ for (const test of TEST_CASES) {
579579
}
580580

581581
// Test that the authentication tag can be set at any point before calling
582-
// final() in GCM mode.
582+
// final() in GCM or OCB mode.
583583
{
584584
const plain = Buffer.from('Hello world', 'utf8');
585585
const key = Buffer.from('0123456789abcdef', 'utf8');
586586
const iv = Buffer.from('0123456789ab', 'utf8');
587587

588-
const cipher = crypto.createCipheriv('aes-128-gcm', key, iv);
589-
const ciphertext = Buffer.concat([cipher.update(plain), cipher.final()]);
590-
const authTag = cipher.getAuthTag();
591-
592-
for (const authTagBeforeUpdate of [true, false]) {
593-
const decipher = crypto.createDecipheriv('aes-128-gcm', key, iv);
594-
if (authTagBeforeUpdate) {
595-
decipher.setAuthTag(authTag);
596-
}
597-
const resultUpdate = decipher.update(ciphertext);
598-
if (!authTagBeforeUpdate) {
599-
decipher.setAuthTag(authTag);
588+
for (const mode of ['gcm', 'ocb']) {
589+
for (const authTagLength of mode === 'gcm' ? [undefined, 8] : [8]) {
590+
const cipher = crypto.createCipheriv(`aes-128-${mode}`, key, iv, {
591+
authTagLength
592+
});
593+
const ciphertext = Buffer.concat([cipher.update(plain), cipher.final()]);
594+
const authTag = cipher.getAuthTag();
595+
596+
for (const authTagBeforeUpdate of [true, false]) {
597+
const decipher = crypto.createDecipheriv(`aes-128-${mode}`, key, iv, {
598+
authTagLength
599+
});
600+
if (authTagBeforeUpdate) {
601+
decipher.setAuthTag(authTag);
602+
}
603+
const resultUpdate = decipher.update(ciphertext);
604+
if (!authTagBeforeUpdate) {
605+
decipher.setAuthTag(authTag);
606+
}
607+
const resultFinal = decipher.final();
608+
const result = Buffer.concat([resultUpdate, resultFinal]);
609+
assert(result.equals(plain));
610+
}
600611
}
601-
const resultFinal = decipher.final();
602-
const result = Buffer.concat([resultUpdate, resultFinal]);
603-
assert(result.equals(plain));
604612
}
605613
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.