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.

Sign up for GitHub

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

Jump to bottom

crypto: fix edge case in authenticated encryption #22828

Closed
wants to merge 3 commits into from
Closed

crypto: fix edge case in authenticated encryption #22828

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
57c0dbe
crypto: fix edge case in authenticated encryption
tniessen Sep 12, 2018
520717b
Match old behavior when called multiple times
tniessen Sep 14, 2018
994d6b7
Remove redundant state
tniessen Sep 14, 2018
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
7 changes: 5 additions & 2 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2818,6 +2818,7 @@ bool CipherBase::InitAuthenticated(const char* cipher_type, int iv_len,

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

if (mode == EVP_CIPH_CCM_MODE) {
// Restrict the message length to min(INT_MAX, 2^(8*(15-iv_len))-1) bytes.
Expand All @@ -2840,6 +2841,7 @@ bool CipherBase::InitAuthenticated(const char* cipher_type, int iv_len,

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

Expand Down Expand Up @@ -2921,6 +2923,7 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
}

cipher->auth_tag_len_ = tag_len;
cipher->auth_tag_state_ = kAuthTagKnown;
tniessen marked this conversation as resolved.
Show resolved Hide resolved
CHECK_LE(cipher->auth_tag_len_, sizeof(cipher->auth_tag_));

memset(cipher->auth_tag_, 0, sizeof(cipher->auth_tag_));
Expand All @@ -2931,14 +2934,14 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {


bool CipherBase::MaybePassAuthTagToOpenSSL() {
if (!auth_tag_set_ && auth_tag_len_ != kNoAuthTagLength) {
if (auth_tag_state_ == kAuthTagKnown) {
if (!EVP_CIPHER_CTX_ctrl(ctx_.get(),
EVP_CTRL_AEAD_SET_TAG,
auth_tag_len_,
reinterpret_cast<unsigned char*>(auth_tag_))) {
return false;
}
auth_tag_set_ = true;
auth_tag_state_ = kAuthTagPassedToOpenSSL;
}
return true;
}
Expand Down
10 changes: 8 additions & 2 deletions src/node_crypto.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,12 @@ class CipherBase : public BaseObject {
kErrorMessageSize,
kErrorState
};
enum AuthTagState {
kAuthTagUnknown,
kAuthTagLengthKnown,
Copy link
Member

Choose a reason for hiding this comment

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

Is there currently a difference between these two states? Sorry for asking questions, but I’m not sure I’m fully qualified to review this…

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you can use GCM (e.g. aes-128-gcm) without specifying the authentication tag length, then the state will be kAuthTagUnknown, but other modes such as CCM (aes-128-ccm) and OCB (aes-128-ocb) require the length of the authentication tag in advance (via the authTagLength option), so the cipher will start in the state kAuthTagLengthKnown. Once the user calls setAuthTag, the state becomes kAuthTagKnown, which implies that the length is known as well.

(Never apologize for asking questions. 😉)

Copy link
Member

Choose a reason for hiding this comment

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

Thank you! Is there any difference in how we treat those states after they have been entered?

Copy link
Member Author

@tniessen tniessen Sep 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Technically, we do. But:
If auth_tag_state_ is either kAuthTagUnknown or kAuthTagLengthKnown, then auth_tag_state_ == kAuthTagUnknown iff auth_tag_len_ == kNoAuthTagLength. So you are right, we could totally drop the second state! (Or remove kNoAuthTagLength.)

kAuthTagKnown,
kAuthTagPassedToOpenSSL
};
static const unsigned kNoAuthTagLength = static_cast<unsigned>(-1);

void Init(const char* cipher_type,
Expand Down Expand Up @@ -404,7 +410,7 @@ class CipherBase : public BaseObject {
: BaseObject(env, wrap),
ctx_(nullptr),
kind_(kind),
auth_tag_set_(false),
auth_tag_state_(kAuthTagUnknown),
auth_tag_len_(kNoAuthTagLength),
pending_auth_failed_(false) {
MakeWeak();
Expand All @@ -413,7 +419,7 @@ class CipherBase : public BaseObject {
private:
DeleteFnPtr<EVP_CIPHER_CTX, EVP_CIPHER_CTX_free> ctx_;
const CipherKind kind_;
bool auth_tag_set_;
AuthTagState auth_tag_state_;
unsigned int auth_tag_len_;
char auth_tag_[EVP_GCM_TLS_TAG_LEN];
bool pending_auth_failed_;
Expand Down
40 changes: 24 additions & 16 deletions test/parallel/test-crypto-authenticated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,27 +557,35 @@ for (const test of TEST_CASES) {
}

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

const cipher = crypto.createCipheriv('aes-128-gcm', key, iv);
const ciphertext = Buffer.concat([cipher.update(plain), cipher.final()]);
const authTag = cipher.getAuthTag();

for (const authTagBeforeUpdate of [true, false]) {
const decipher = crypto.createDecipheriv('aes-128-gcm', key, iv);
if (authTagBeforeUpdate) {
decipher.setAuthTag(authTag);
}
const resultUpdate = decipher.update(ciphertext);
if (!authTagBeforeUpdate) {
decipher.setAuthTag(authTag);
for (const mode of ['gcm', 'ocb']) {
for (const authTagLength of mode === 'gcm' ? [undefined, 8] : [8]) {
const cipher = crypto.createCipheriv(`aes-128-${mode}`, key, iv, {
authTagLength
});
const ciphertext = Buffer.concat([cipher.update(plain), cipher.final()]);
const authTag = cipher.getAuthTag();

for (const authTagBeforeUpdate of [true, false]) {
const decipher = crypto.createDecipheriv(`aes-128-${mode}`, key, iv, {
authTagLength
});
if (authTagBeforeUpdate) {
decipher.setAuthTag(authTag);
}
const resultUpdate = decipher.update(ciphertext);
if (!authTagBeforeUpdate) {
decipher.setAuthTag(authTag);
}
const resultFinal = decipher.final();
const result = Buffer.concat([resultUpdate, resultFinal]);
assert(result.equals(plain));
}
}
const resultFinal = decipher.final();
const result = Buffer.concat([resultUpdate, resultFinal]);
assert(result.equals(plain));
}
}