Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

AES256 decipher yields corrupted data #738

Closed
havard opened this issue Mar 1, 2011 · 14 comments
Closed

AES256 decipher yields corrupted data #738

havard opened this issue Mar 1, 2011 · 14 comments
Labels

Comments

@havard
Copy link

havard commented Mar 1, 2011

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
Copy link

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

@hueniverse
Copy link

It happens if the input is longer than 15 characters.

@TrevorBurnham
Copy link

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
Copy link

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
Copy link

donpark commented Mar 14, 2011

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
Copy link

donpark commented Mar 14, 2011

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
Copy link

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
Copy link

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
Copy link

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

cesare added a commit to cesare/node that referenced this issue Jun 19, 2011
@cesare
Copy link

cesare commented Jun 19, 2011

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

@aklt
Copy link

aklt commented Jun 23, 2011

Hi,

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

@koichik
Copy link

koichik commented Jul 23, 2011

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

@aklt
Copy link

aklt commented Jul 25, 2011

Will do, thanks

@aklt
Copy link

aklt commented Jul 25, 2011

I have added #1395

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

8 participants