Skip to content

Commit

Permalink
crypto: do not allow multiple calls to setAuthTag
Browse files Browse the repository at this point in the history
Calling setAuthTag multiple times can result in hard to detect bugs
since to the user, it is unclear which invocation actually affected
OpenSSL.

PR-URL: #22931
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
  • Loading branch information
tniessen committed Sep 21, 2018
1 parent 56493bf commit 058c5b8
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 6 deletions.
2 changes: 1 addition & 1 deletion doc/api/crypto.md
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ is invalid according to [NIST SP 800-38D][] or does not match the value of the
`authTagLength` option, `decipher.setAuthTag()` will throw an error.

The `decipher.setAuthTag()` method must be called before
[`decipher.final()`][].
[`decipher.final()`][] and can only be called once.

### decipher.setAutoPadding([autoPadding])
<!-- YAML
Expand Down
7 changes: 2 additions & 5 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2894,14 +2894,11 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {

if (!cipher->ctx_ ||
!cipher->IsAuthenticatedMode() ||
cipher->kind_ != kDecipher) {
cipher->kind_ != kDecipher ||
cipher->auth_tag_state_ != kAuthTagUnknown) {
return args.GetReturnValue().Set(false);
}

// TODO(tniessen): Throw if the authentication tag has already been set.
if (cipher->auth_tag_state_ == kAuthTagPassedToOpenSSL)
return args.GetReturnValue().Set(true);

unsigned int tag_len = Buffer::Length(args[0]);
const int mode = EVP_CIPHER_CTX_mode(cipher->ctx_.get());
bool is_valid;
Expand Down
26 changes: 26 additions & 0 deletions test/parallel/test-crypto-authenticated.js
Original file line number Diff line number Diff line change
Expand Up @@ -589,3 +589,29 @@ for (const test of TEST_CASES) {
}
}
}

// Test that setAuthTag can only be called once.
{
const plain = Buffer.from('Hello world', 'utf8');
const key = Buffer.from('0123456789abcdef', 'utf8');
const iv = Buffer.from('0123456789ab', 'utf8');
const opts = { authTagLength: 8 };

for (const mode of ['gcm', 'ccm', 'ocb']) {
const cipher = crypto.createCipheriv(`aes-128-${mode}`, key, iv, opts);
const ciphertext = Buffer.concat([cipher.update(plain), cipher.final()]);
const tag = cipher.getAuthTag();

const decipher = crypto.createDecipheriv(`aes-128-${mode}`, key, iv, opts);
decipher.setAuthTag(tag);
assert.throws(() => {
decipher.setAuthTag(tag);
}, errMessages.state);
// Decryption should still work.
const plaintext = Buffer.concat([
decipher.update(ciphertext),
decipher.final()
]);
assert(plain.equals(plaintext));
}
}

0 comments on commit 058c5b8

Please sign in to comment.