AES256 decipher yields corrupted data

[havard]

The AES256 decipherer seems broken for certain inputs. This code reproduces the problem (tested against v0.4.1):

var crypto = require('crypto');
var assert = require('assert');

var key = '0123456789abcdef';
var plain = '32|RmVZZkFUVmpRRkp0TmJaUm56ZU9qcnJkaXNNWVNpTTU*|iXmckfRWZBGWWELweCBsThSsfUHLeRe0KCsK8ooHgxie0zOINpXxfZi/oNG7uq9JWFVCk70gfzQH8ZUJjAfaFg**';

var cipher = crypto.createCipher('aes256', key);
var ciphered = cipher.update(plain, 'utf8', 'base64') + cipher.final('base64');
var decipher = crypto.createDecipher('aes256', key);
var deciphered = decipher.update(ciphered, 'base64', 'utf8') + decipher.final('utf8');


assert.equal(deciphered, plain); // Fails, deciphered text is corrupt

[hueniverse]

The issue is with the base64 encoding. If you change to hex, the problem goes away.

[hueniverse]

It happens if the input is longer than 15 characters.

[TrevorBurnham]

Replicated under 0.4.2 with the same key and plain under 'aes256', 'aes192', and aes128. In all cases, deciphered and plain differ when base64 encoding is used; hex is fine.

My hunch is that the issue is in cipher.final. With the given sample code, deciphered ends with

Hgxie0zOINpXxfZi/oNG7uq9JWP�����A�Bx�cӂk

but if you forego the + cipher.final part, it ends with

Hgxie0zOINpXxfZi/oNG7uq9J

That is, with cipher.final, the deciphered string is corrupted; without it, the deciphered string is merely incomplete (as expected). If you remove both cipher.final and decipher.final, you get the same truncated result, but if you remove decipher.final and keep cipher.final, you get corruption at the end of the string.

[TrevorBurnham]

I just noticed lines 99-100 of test-crypto:

// Only use binary or hex, not base64.
ciph += cipher.final('hex');

Does test-crypto know something that the official docs don't?

[donpark]

likely a different symptom of the same base64 bug:

when each of following three plaintext lines are encrypted then decrypted:

Keep this a secret? No! Tell everyone about node.js!
Keep this a secret? No! Tell everyone about node.js!!
Keep this a secret? No! Tell everyone about node.js!!!

result are:

Keep this a secret? No! Tell everyone about node.js
Keep this a secret? No! Tell everyone about node.js
Keep this a secret? No! Tell everyone about node.js!!!

lengths are 52, 53, 54 and only the last one gets full original text back.

Problem appears to be in base64 because encrypted text are same length for first two.

[donpark]

Looking at file node_crypto.cc, it appears that Cipher.final is calling the function base64 which in turn calls OpenSSL's BIO_new with BIO_f_base64 filter then frees the BIO before returning. base64() is also called from Cipher.update.

Unless I'm mistaken, this is not how base64 BIO was designed to be used as you concatenating multiple independently encoded base64 fragments won't make it whole with paddings getting in the way.

Come to think of it, I think Cipher/Decipher interface is weird too in that output encoding has to be passed for each update calls as well as final call when it should have been set in createCipher.

As to why 'hex' encoding works, it's handled entirely by an local function HexEncode and also because hex strings can be concatenated w/o problems unlike base64.

[hueniverse]

It probably makes sense to add native base64 support to the crypto lib and just use that. Seems like too many apps are reinventing base64 for no good reason.

[TrevorBurnham]

I made a simple little non-native wrapper around openssl:

https://github.com/TrevorBurnham/cipherpipe

All of the above examples work in it, and it generates base64-encoded output. However, its functionality is much more limited than crypto's. I hope crypto will be fixed...

[arenstar]

I would hope to see a non openssl solution in the future..
Perhaps creating some C bindings to cryptopp ...

[cesare]

Hi guys,
I've fixed this issue with cesare@922d790
Could you try this one? I hope this patch solves your problems.

[aklt]

Hi,

I just tried pasting the changes you made to node_crypto.cc, but this still fails unfortunately.

[koichik]

@aklt - @cesare's patch fixes this issue. Can you open new issue for your problem?

[aklt]

Will do, thanks

[aklt]

I have added #1395