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.

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 Decipher: setAuthTag() must be called before update() #22421

Closed
achronos0 opened this issue Aug 20, 2018 · 7 comments
Closed

crypto Decipher: setAuthTag() must be called before update() #22421

achronos0 opened this issue Aug 20, 2018 · 7 comments
Assignees
Labels
crypto Issues and PRs related to the crypto subsystem.

Comments

@achronos0
Copy link

  • Version: v8.11.3
  • Platform: Darwin foo.local 17.7.0 Darwin Kernel Version 17.7.0: Thu Jun 21 22:53:14 PDT 2018; root:xnu-4570.71.2~1/RELEASE_X86_64 x86_64
  • Subsystem: crypto

crypto Decipher docs say that setAuthTag() must be called before final().
In fact setAuthTag must be called before any call to update(), in GCM mode at least.

If you call update() before setAuthTag(), final() will throw regardless of the correctness of the ciphertext and tag.

Test case:

const authTagBeforeUpdate = false; // true works, false throws

const crypto = require('crypto');
const cleartext = Buffer.from("abcdefghijklmnopqrstuvwxyz0123");
const key = Buffer.from("abcdefghijklmnopqrstuvwxyz012345");
const iv = Buffer.from("0123456789ab");
console.log('cleartext', cleartext);
console.log('key      ', key);
console.log('iv       ', iv);

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

const decipher = crypto.createDecipheriv('aes-256-gcm', key, iv);
if (authTagBeforeUpdate) {
    decipher.setAuthTag(authTag);
}
let result_update = decipher.update(ciphertext);
if (!authTagBeforeUpdate) {
    decipher.setAuthTag(authTag);
}
let result_final = decipher.final();
let result_cleartext = Buffer.concat([result_update, result_final]);
console.log('result   ', result_cleartext);

I suggest changing the API docs to make it clear that the auth tag must be supplied before any data is read in. setAuthTag() must be called before final() and should be called before any call to update().

Second: is this actually the desired behaviour?

As far as I can tell, there is no requirement for GCM to know the auth tag prior to deciphering data. OpenSSL's own example provides it after EVP_DecryptUpdate, before EVP_DecryptFinal_ex (https://wiki.openssl.org/index.php/EVP_Authenticated_Encryption_and_Decryption#Authenticated_Decryption_using_GCM_mode).

In cases where the auth tag is appended to the ciphertext (e.g. Java), as things stand the entire ciphertext must be buffered. For large data this is a serious problem. It would make more sense to decrypt chunks as they are received, treating them carefully and discarding everything if the auth tag eventually proves to be incorrect.

@achronos0
Copy link
Author

OK reading the source I can see how and why this behaviour occurs, and I have found a workaround, maybe docs could mention it.

From master/src/node_crypto.cc

  • function CipherBase::SetAuthTag does not actually set anything in the engine (it does not call EVP_CIPHER_CTX_ctrl)
  • function CipherBase::Update is where the tag is actually set, the first time it is called after the key is set (it calls EVP_CIPHER_CTX_ctrl(... EVP_CTRL_AEAD_SET_TAG ...))

So the result is that:

  • a call to (js) setAuthTag() after all calls to update() and before calling final() doesn't work, the tag is never actually set in OpenSSL
  • multiple calls to setAuthTag() e.g. setAuthTag() then update() then setAuthTag() then final() doesn't work, only the first tag is actually set in OpenSSL, the second tag is ignored

The workaround I found is to set the authtag then make one final call to update() with empty data. That works.

My example above, modified with workaround so it works in both cases:

const authTagBeforeUpdate = false; // true works, false now also works

const crypto = require('crypto');
const cleartext = Buffer.from("abcdefghijklmnopqrstuvwxyz0123");
const key = Buffer.from("abcdefghijklmnopqrstuvwxyz012345");
const iv = Buffer.from("0123456789ab");
console.log('cleartext', cleartext);
console.log('key      ', key);
console.log('iv       ', iv);

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

const decipher = crypto.createDecipheriv('aes-256-gcm', key, iv, { authTagLength: 16 });
if (authTagBeforeUpdate) {
    decipher.setAuthTag(authTag);
}
let result_update = decipher.update(ciphertext);
if (!authTagBeforeUpdate) {
    decipher.setAuthTag(authTag);

    // Workaround:
    decipher.update(Buffer.from([]));
}
let result_final = decipher.final();
let result_cleartext = Buffer.concat([result_update, result_final]);
console.log('result   ', result_cleartext);

@addaleax addaleax added the crypto Issues and PRs related to the crypto subsystem. label Aug 24, 2018
@addaleax
Copy link
Member

/cc @nodejs/crypto

@tniessen tniessen self-assigned this Aug 24, 2018
tniessen added a commit to tniessen/node that referenced this issue Aug 26, 2018
This is an attempt to make the behavior of setAuthTag match the
documentation: In GCM mode, it can be called at any time before
invoking final, even after the last call to update.

Fixes: nodejs#22421
targos pushed a commit that referenced this issue Sep 2, 2018
This is an attempt to make the behavior of setAuthTag match the
documentation: In GCM mode, it can be called at any time before
invoking final, even after the last call to update.

Fixes: #22421

PR-URL: #22538
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Sep 3, 2018
This is an attempt to make the behavior of setAuthTag match the
documentation: In GCM mode, it can be called at any time before
invoking final, even after the last call to update.

Fixes: #22421

PR-URL: #22538
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Sep 6, 2018
This is an attempt to make the behavior of setAuthTag match the
documentation: In GCM mode, it can be called at any time before
invoking final, even after the last call to update.

Fixes: #22421

PR-URL: #22538
Reviewed-By: Anna Henningsen <[email protected]>
@tniessen tniessen reopened this Sep 12, 2018
@tniessen
Copy link
Member

I just noticed that there is still an edge case where this doesn't work: If a user restricts the permitted length of GCM authentication tags to a single value while calling createDecipheriv via the authTagLength option and sets the authentication tag after calling update, verification will still fail. Working on it.

@tniessen
Copy link
Member

Fixed in #22828.

@bencmbrook
Copy link

bencmbrook commented Oct 3, 2019

Is there any way to use a decipher stream, and set the authTag later? In other words, perform the integrity check after decipher is done.

It seems like one still needs to set the authTag before calling .pipe(decipher), since the stream will automatically call decipher.final() when the stream's 'end' event fires.

@achronos0
Copy link
Author

@bencmbrook that is my original use case in this ticket as well, and yes you can.

You will need to use your own custom decipher stream, so you can set authTag before the stream finishes.
But -- you probably already need to have a custom stream, yes? In order to parse out authTag from the encrypted data?

In my case I use a Transform stream as a wrapper around the crypto stream, so my transform pre-parses the input before passing it on to the crypto decipher stream, and when it knows its found the authTag and has read all of it, it then calls decipher.setAuthTag() and then decipher.final().

@bencmbrook
Copy link

Thanks @achronos0 - put some basic source code together for the next person :)
https://github.com/transcend-io/lazydecipheriv/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
4 participants