asmcrypto / asmcrypto.js

JavaScript Cryptographic Library with performance in mind.
MIT License
659 stars 182 forks source link

Error when using AES-GCM or AES-CCM in some cases #92

Closed pohutukawa closed 7 years ago

pohutukawa commented 8 years ago

When I'm using AES in GCM mode, I'm sometimes getting encryption errors:

var key = asmCrypto.string_to_bytes(atob('dGQhii+B7+eLLHRiOA690w=='));
var nonce = asmCrypto.string_to_bytes(atob('R8q1njARXS7urWv3'));
var plainText = atob('dGQhwoovwoHDr8OnwossdGI4DsK9w5M=');
var result = asmCrypto.AES_GCM.encrypt(plainText, key, nonce, undefined, 16);

I'm getting the following error:

Index is out of range.
    at set ([native code])
    at gcm_aes_encrypt_process (/home/guy/workspace/webclient/js/vendor/asmcrypto.js:3132:33)
    at gcm_aes_encrypt (/home/guy/workspace/webclient/js/vendor/asmcrypto.js:3171:51)
    at gcm_aes_encrypt_bytes (/home/guy/workspace/webclient/js/vendor/asmcrypto.js:3362:19)
    at /home/guy/workspace/webclient/test/xxx_test.js:55:38

However, if I'm switching the same thing to use AES-CCM, I'm not having that problem. If I'm extending the plain text by a single byte (from 23 to 24 bytes), the problem vanishes.

Any ideas on what the problem may be? After all, they're both "counter modes" in different flavours.

vibornoff commented 8 years ago

Performing steps you submitted I got L3zqVYAOsRk7zMg2KsNTVShcad8TjIQ7umfsvia21QO0XTj8vaeR and it correctly decrypts back.

Seems you have outdated version of asmcrypto. Check if the issues still present with the updated lib.

vibornoff commented 8 years ago

BTW, there was a similar issue #70.

pohutukawa commented 8 years ago

Interesting, indeed. I have just upgraded our version to tag v0.0.9 and surely AES-GCM works as expected.

However ... now AES-CCM misbehaves:

I'm not able to use it to encrypt or decrypt any content without passing in anything for the adata parameter. I believe that the GCM implementation is behaving there properly, but the CCM implementation is off. It looks like the culprit is AES_CCM_reset. The GCM implementation equivalent here has a different conditional in the outer if statement for the adata field check. The function also checks for a value of iv, which is not a parameter for CCM, but rather nonce takes that place, which is providing a start for the counter. So, as iv and adata are missing, it is throwing an exception in every case without adata.

I know, this is a different issue (as opposed to the one I've opened here), but I'm just hesitant to open a new issue for this to keep the discussion flowing on this one ticket.

Again, some test code:

var adataCases = [undefined, null, '', new Uint8Array()];
var key = asmCrypto.string_to_bytes(atob('dGQhii+B7+eLLHRiOA690w=='));
var nonce = asmCrypto.string_to_bytes(atob('R8q1njARXS7urWv3'));
var plainText = atob('dGQhwoovwoHDr8OnwossdGI4DsK9w5M=');
for (var i = 0; i < adataCases.length; i++) {
    var result = asmCrypto.AES_CCM.encrypt(plainText, key, nonce, adataCases[i], 16);
}
pohutukawa commented 8 years ago

OK, it has gotten somewhat better, but it still won't fully work.

It probably would be quite good if the test coverage for AES-CCM does also include test vectors with no adata (maybe the same ones AES-GCM uses?).

Anyway, here's a simple test I'm using to show what's still broken. It's a simple round trip (en-/decryption), which should result in what was put in initially:

var clearText = '42';
var nonce = asmCrypto.hex_to_bytes('000102030405060708090a0b');
var key = asmCrypto.hex_to_bytes('0f0e0d0c0b0a09080706050403020100');

var cipherText = asmCrypto.AES_CCM.encrypt(clearText, key, nonce, undefined, 16);
var reClear = asmCrypto.AES_CCM.decrypt(cipherText, key, nonce, undefined, 16);
assert.strictEqual(asmCrypto.bytes_to_string(reClear), clearText);
pohutukawa commented 8 years ago

Oh, another one: Once it's fixed, I think it'd be good to have a next tagged version (e. g. v0.0.10), so that we can document what version is working and automatically build it for our code base.