FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: forward auth tag to OpenSSL immediately · nodejs/node@cec9d9d · GitHub

/ node Public

Commit cec9d9d

Browse files
tniessen
authored and
targos
committed
crypto: forward auth tag to OpenSSL immediately
This change simplifies the AEAD implementation. Instead of storing the authentication tag when the user calls `setAuthTag()` and passing it to OpenSSL later in `MaybePassAuthTagToOpenSSL()`, the modified code forwards it to OpenSSL from within `setAuthTag()` already, removing the need to store it. For clarity, I have also renamed the possible `AuthTagState` values to better reflect the actual state of the authentication tag. I assume that we did not originally do this due to issues with some old versions of OpenSSL when reordering certain function calls, but even with the recent additions I made to the relevant test (namely, 1ef9923 and 53944c4), it seems to pass in both OpenSSL 3 and OpenSSL 1.1.1 with this simplification. PR-URL: #58547 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 34e86f9 commit cec9d9d

2 files changed

Lines changed: 16 additions & 43 deletions

File tree

‎src/crypto/crypto_cipher.cc‎

Lines changed: 12 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -514,9 +514,9 @@ void CipherBase::GetAuthTag(const FunctionCallbackInfo<Value>& args) {
514514
ASSIGN_OR_RETURN_UNWRAP(&cipher, args.This());
515515

516516
// Only callable after Final and if encrypting.
517-
if (cipher->ctx_ ||
518-
cipher->kind_ != kCipher ||
519-
cipher->auth_tag_len_ == kNoAuthTagLength) {
517+
if (cipher->ctx_ || cipher->kind_ != kCipher ||
518+
cipher->auth_tag_len_ == kNoAuthTagLength ||
519+
cipher->auth_tag_state_ != kAuthTagComputed) {
520520
return;
521521
}
522522

@@ -577,29 +577,16 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
577577
}
578578

579579
cipher->auth_tag_len_ = tag_len;
580-
cipher->auth_tag_state_ = kAuthTagKnown;
581-
CHECK_LE(cipher->auth_tag_len_, sizeof(cipher->auth_tag_));
580+
CHECK_LE(cipher->auth_tag_len_, ncrypto::Cipher::MAX_AUTH_TAG_LENGTH);
582581

583-
memset(cipher->auth_tag_, 0, sizeof(cipher->auth_tag_));
584-
auth_tag.CopyTo(cipher->auth_tag_, cipher->auth_tag_len_);
582+
if (!cipher->ctx_.setAeadTag({auth_tag.data(), cipher->auth_tag_len_})) {
583+
return args.GetReturnValue().Set(false);
584+
}
585+
cipher->auth_tag_state_ = kAuthTagSetByUser;
585586

586587
args.GetReturnValue().Set(true);
587588
}
588589

589-
bool CipherBase::MaybePassAuthTagToOpenSSL() {
590-
if (auth_tag_state_ == kAuthTagKnown) {
591-
ncrypto::Buffer<const char> buffer{
592-
.data = auth_tag_,
593-
.len = auth_tag_len_,
594-
};
595-
if (!ctx_.setAeadTag(buffer)) {
596-
return false;
597-
}
598-
auth_tag_state_ = kAuthTagPassedToOpenSSL;
599-
}
600-
return true;
601-
}
602-
603590
bool CipherBase::SetAAD(
604591
const ArrayBufferOrViewContents<unsigned char>& data,
605592
int plaintext_len) {
@@ -622,10 +609,6 @@ bool CipherBase::SetAAD(
622609
return false;
623610
}
624611

625-
if (kind_ == kDecipher && !MaybePassAuthTagToOpenSSL()) {
626-
return false;
627-
}
628-
629612
ncrypto::Buffer<const unsigned char> buffer{
630613
.data = nullptr,
631614
.len = static_cast<size_t>(plaintext_len),
@@ -670,12 +653,6 @@ CipherBase::UpdateResult CipherBase::Update(
670653
return kErrorMessageSize;
671654
}
672655

673-
// Pass the authentication tag to OpenSSL if possible. This will only happen
674-
// once, usually on the first update.
675-
if (kind_ == kDecipher && IsAuthenticatedMode()) {
676-
CHECK(MaybePassAuthTagToOpenSSL());
677-
}
678-
679656
const int block_size = ctx_.getBlockSize();
680657
CHECK_GT(block_size, 0);
681658
if (len + block_size > INT_MAX) return kErrorState;
@@ -777,16 +754,11 @@ bool CipherBase::Final(std::unique_ptr<BackingStore>* out) {
777754
static_cast<size_t>(ctx_.getBlockSize()),
778755
BackingStoreInitializationMode::kUninitialized);
779756

780-
if (kind_ == kDecipher &&
781-
Cipher::FromCtx(ctx_).isSupportedAuthenticatedMode()) {
782-
MaybePassAuthTagToOpenSSL();
783-
}
784-
785757
#if (OPENSSL_VERSION_NUMBER < 0x30000000L)
786758
// OpenSSL v1.x doesn't verify the presence of the auth tag so do
787759
// it ourselves, see https://github.com/nodejs/node/issues/45874.
788760
if (kind_ == kDecipher && ctx_.isChaCha20Poly1305() &&
789-
auth_tag_state_ != kAuthTagPassedToOpenSSL) {
761+
auth_tag_state_ != kAuthTagSetByUser) {
790762
return false;
791763
}
792764
#endif
@@ -824,6 +796,9 @@ bool CipherBase::Final(std::unique_ptr<BackingStore>* out) {
824796
}
825797
ok = ctx_.getAeadTag(auth_tag_len_,
826798
reinterpret_cast<unsigned char*>(auth_tag_));
799+
if (ok) {
800+
auth_tag_state_ = kAuthTagComputed;
801+
}
827802
}
828803
}
829804

‎src/crypto/crypto_cipher.h‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,8 @@ class CipherBase : public BaseObject {
3838
};
3939
enum AuthTagState {
4040
kAuthTagUnknown,
41-
kAuthTagKnown,
42-
kAuthTagPassedToOpenSSL
41+
kAuthTagSetByUser,
42+
kAuthTagComputed,
4343
};
4444
static const unsigned kNoAuthTagLength = static_cast<unsigned>(-1);
4545

@@ -64,10 +64,8 @@ class CipherBase : public BaseObject {
6464
bool SetAutoPadding(bool auto_padding);
6565

6666
bool IsAuthenticatedMode() const;
67-
bool SetAAD(
68-
const ArrayBufferOrViewContents<unsigned char>& data,
69-
int plaintext_len);
70-
bool MaybePassAuthTagToOpenSSL();
67+
bool SetAAD(const ArrayBufferOrViewContents<unsigned char>& data,
68+
int plaintext_len);
7169

7270
static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
7371
static void Update(const v8::FunctionCallbackInfo<v8::Value>& args);

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL