Skip to content
This repository

AES encryption and decryption gives incorrect results #1318

Closed
drdaeman opened this Issue July 12, 2011 · 7 comments

5 participants

Aleksey Zhukov starlinholmes Don Park Koichi Kobayashi liting
Aleksey Zhukov

I believe crypto module is severely broken for AES (and, probably, other ciphers) encryption and decryption. To be sure, I've tried with a test vector from ecb_tbl.txt:

I=1
KEY=00010203050607080A0B0C0D0F101112
PT=506812A45F08C889B97F5980038B8359
CT=D8F532538289EF7D06B506A4FD5BE9C9

The results are certainly wrong:

$ ./node -v
v0.5.1-pre
$ git show -s --format=%H
92057554d59e9875407fa944c73eb93354418a02
$ ./node
> var crypto = require("crypto");
> var c = crypto.createCipher("aes-128-ecb", new Buffer("00010203050607080a0b0c0d0f101112", "hex").toString("binary"));
> var s = c.update(new Buffer("506812a45f08c889b97f5980038b8359", "hex").toString("binary"));
> console.log(new Buffer(s + c.final(), "binary").toString("hex"));
357b9018178ee4123697013fe65d24be2d395194e71b5ce17251443a58cbb3a1

However, it success to decrypt this incorrect result back to original value:

> var d = crypto.createDecipher("aes-128-ecb", new Buffer("00010203050607080a0b0c0d0f101112", "hex").toString("binary"));
> var s = c.update(new Buffer("357b9018178ee4123697013fe65d24be2d395194e71b5ce17251443a58cbb3a1", "hex"));
> console.log(new Buffer(s + d.final(), "binary").toString("hex"));
506812a45f08c889b97f5980038b8359

I've made sure that OpenSSL provides correct results:

echo "506812a45f08c889b97f5980038b8359" \
    | perl -pe 's/(..)/chr hex $1/eg;' \
    | openssl enc -aes-128-ecb -K "00010203050607080a0b0c0d0f101112" -nopad \
    | hexdump -e '/1 "%02x"'

And, as expected, it returned "d8f532538289ef7d06b506a4fd5be9c9".

Unfortunately, I don't know much about Node internals and OpenSSL programming, so my attempts to debug were unsuccessful and I didn't understood what's happening under the hood.

On a side note, while it seems (through shotgun-debugging with printf) that Decipher::DecipherInit receives correct key data, it should accept buffers in addition to strings.

starlinholmes

This is almost certainly a padding issue, of which there are multiple types; PKCS7 being the one probably used by Node. I'm not going to examine the implementation details since grokking the OpenSSL source tree is a headache I'll spare myself.

Edit: actually that's not necessarily the case since the first block should be the same. Could be a buffer encoding issue, too.

starlinholmes

OK, I found the solution.

Node MD5's the key that you input using the function EVP_BytesToKey. OpenSSL doesn't call this function when you pass a hex key on the command line, but it does if you supply a passphrase. However, in that case it also generates a random salt value. To verify this, run openssl without "-nopad" and with "-nosalt" and enter a passphrase instead of supplying a hex key. Then Node and OpenSSL's output should match.

When I modified Node to not use padding and to use the raw input key rather than the derived one, it matched OpenSSL's output:

Ciphername: AES-128-ECB
Key Buf(16 bytes): 00010203050607080a0b0c0d0f101112
Derived Key(EVP_BytesToKey with MD5, 16 bytes): 3310ac2b768e62138a737f36ccd0ad41
EVP_CipherUpdate Input (16 bytes) : 506812a45f08c889b97f5980038b8359
EVP_CipherUpdate Output (16 bytes): d8f532538289ef7d06b506a4fd5be9c9
Ciphertext: d8f532538289ef7d06b506a4fd5be9c9

I suppose it's debatable whether node should be using your raw key. It probably should since EVP_BytesToKey is a terrible key derivation function, especially for a passphrase. Then either the user could be responsible for key derivation, or could optionally tell Node to derive a key with something like PBKDF2 (which would have to be implemented in the node_crypto.cc since it would be ridiculously slow in javascript).

Aleksey Zhukov

Found it. This is, in fact, a documentation, not implementation problem. My bad for blaming Node.

The "key" passed to a Cipher::CipherInit is not a key itself, it's a password, passed to a EVP_BytesToKey. Docs should be clear on this. And to pass raw key material one should use Cipher::CipherInitIv (exposed as crypto.createCipheriv) in this way:

var crypto = require("crypto");
var c = crypto.createCipheriv("aes-128-ecb", new Buffer("00010203050607080a0b0c0d0f101112", "hex").toString("binary"), "");
var s = c.update(new Buffer("506812a45f08c889b97f5980038b8359", "hex"));
console.log(new Buffer(s + c.final(), "binary").toString("hex"));

This yields "d8f532538289ef7d06b506a4fd5be9c96de5f607ab7eb8202f3957703b04e8b5", which, leaving the padding issues aside, is a correct value.

The only remaining interoperability problem is that Decipher fails with unpadded values. I've tried to use finaltol() instead of final(), but still got an empty string:

> var crypto = require("crypto");
> var d = crypto.createDecipheriv("aes-128-ecb", new Buffer("00010203050607080a0b0c0d0f101112", "hex").toString("binary"), "");
> var s = d.update(new Buffer("d8f532538289ef7d06b506a4fd5be9c9", "hex"));
> s
''
> d.finaltol()
''

Upd: While I was writing this comment I didn't notice @starlinholmes wrote his one. I believe this issue should be split in two (failure to decode unpadded data is still a compatibility issue), this one retagged as "doc" and closed when documentation is updated.

Don Park

Here are some notes I wrote down on createCipher and createCipheriv in March when I ran into similar compatibility issue. Hope it helps.

https://gist.github.com/870585

Koichi Kobayashi koichik referenced this issue from a commit in koichik/node July 24, 2011
Koichi Kobayashi Doc improvements and change argument name.
Fixes #1318.
f311a0c
Koichi Kobayashi
Collaborator

@drdaeman - How about f311a0c?

Aleksey Zhukov drdaeman closed this July 25, 2011
Aleksey Zhukov

@koichik - Yes, but now docs won't confuse users. Thank you.

Peter Bright DrPizza referenced this issue from a commit July 26, 2011
Commit has since been removed from the repository and is no longer available.
Koichi Kobayashi koichik referenced this issue from a commit in koichik/node July 24, 2011
Koichi Kobayashi Doc improvements and change argument name.
Fixes #1318.
d32971a
liting

谢谢你的解答,终于明白了,createDecipheriv() ecb no iv,so i used createDecipher().but error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.