Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

OpenSSL EVP API Fix (ECB mode failing) #1997

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
2 participants

KiNgMaR commented Nov 3, 2011

Hey there.
I was trying to decrypt some Blowfish-ECB-mode encrypted strings with node.js, but all I got was weird characters. After making an isolated test case, I discovered that node.js is not using OpenSSL's EVP_CIPHER_CTX_set_key_length correctly.

It's called after EVP_CipherInit, so it will just silently do nothing (won't return any error code either). However, unless you're using a cipher with a variable key length (such as Blowfish ECB) it will go unnoticed. The fix is to use EVP_CipherInit_ex instead and to call it two times, as (dimly) described by EVP_CipherInit_ex's man page on openssl.org[1].

So here's a pull request and a test case.

I have also changed the code to use EVP_EncryptFinal_ex to go along with the _ex init call which should not have any side effects since at least in current OpenSSLs (haven't checked 0.x ones), Final is simply an alias to Final_ex.

Also here's a standalone cpp file I used for testing: https://gist.github.com/1335823 BF_ecb_encrypt is used to verify the output.

Looking forward to reading your comments.

[1] http://www.openssl.org/docs/crypto/EVP_EncryptInit.html

KiNgMaR added some commits Nov 3, 2011

@KiNgMaR KiNgMaR node_crypto: Correctly use EVP_Cipher*_ex methods instead of 'obsolet…
…e' old versions. This also fixes an issue that made blowfish's ECB mode unusable. Need to investigate if this breaks anything related to padding.
abefd9e
@KiNgMaR KiNgMaR Add a test for aforementioned BF-ECB bug. The hex strings have been g…
…enerated from an independent C++ program.
e58e16b
Owner

bnoordhuis commented Nov 3, 2011

Some would say that ECB not working is a feature, not a bug. I'm only half-kidding.

The test case fails with the below error (tested against openssl 0.9.8k as shipped with ubuntu 10.04 on x86_64):

node-crypto : Invalid IV length 0

node.js:201
        throw e; // process.nextTick error, or 'error' event on first tick
              ^
Error: CipherInitIv error
    at Object.createCipheriv (crypto.js:140:23)
    at /home/bnoordhuis/src/nodejs/node/test/simple/test-crypto-ecb.js:39:23
    at Object.<anonymous> (/home/bnoordhuis/src/nodejs/node/test/simple/test-crypto-ecb.js:43:1)
    at Module._compile (module.js:432:26)
    at Object..js (module.js:450:10)
    at Module.load (module.js:351:31)
    at Function._load (module.js:310:12)
    at Array.0 (module.js:470:10)
    at EventEmitter._tickCallback (node.js:192:40)

It's caused by the EVP_CIPHER_iv_length() check in node_crypto[1]. The blowfish support in openssl hasn't changed since 0.9.8k (hasn't actually changed since 2003 or 2004) so I don't understand why the test passes for you.

It's trivial to fix but I would like to understand the why of it. The C++ test case does pass, by the way, but not when I add a assert(EVP_CIPHER_iv_length(EVP_bf_ecb()) == 0) in there.

On a side note, I appreciate the thorough write-up.

[1] https://github.com/joyent/node/blob/11d68eb/src/node_crypto.cc#L1854

KiNgMaR commented Nov 3, 2011

Seems like a bug in OpenSSL that went unfixed up to these commits:
http://cvs.openssl.org/chngview?cn=19276
http://cvs.openssl.org/chngview?cn=19277
http://cvs.openssl.org/chngview?cn=19278
... which makes all versions older than and including openssl-0.9.8l (lower case L) have a broken EVP_CIPHER_iv_length(EVP_bf_ecb()) ...

I have been testing on Gentoo x86 with openssl-1.0.0e so that's why it was working for me.

I'd suggest something like this:

if(EVP_CIPHER_iv_length(cipher) != iv_len &&
    !(EVP_CIPHER_mode(cipher) == EVP_CIPH_ECB_MODE && iv_len == 0))
{
     /* fail */
}

Not exactly a beauty, but yeah... Maybe wrap it in a macro, local_EVP_CIPHER_iv_length or something. What do you think?

Owner

bnoordhuis commented Nov 3, 2011

Good catch. I'm afraid patching node_crypto.cc won't fix it, the call to EVP_CipherInit_ex() would still fail.

Is there a particular reason you are testing this with crypto.createCipheriv() instead of crypto.createCipher()?

@KiNgMaR KiNgMaR This enables the previous fix to work on OpenSSL<=0.9.8l - verified v…
…ia linking against old OpenSSL and using the new test case. Also the rest of the test suite does not report any side effects.
087b5a4

KiNgMaR commented Nov 3, 2011

Tbh, I don't see why EVP_CipherInit_ex() would fail. So I went ahead and tested it, and it looks good to me. Let me know if I'm missing something :)

Re: using createCipheriv:
createCipher does some key-from-password generation stuff with hashes. The input I was going to work on does not do that, so I have to use createCipheriv with an empty IV.

KiNgMaR commented Nov 26, 2011

Any more thoughts on this? I have deployed a custom/patched node version for now, but would like to go back to the official series in the future. If the patch needs any reworking, just let me know.

@bnoordhuis bnoordhuis commented on an outdated diff Nov 27, 2011

src/node_crypto.cc
fprintf(stderr, "node-crypto : Invalid key length %d\n", key_len);
EVP_CIPHER_CTX_cleanup(&ctx);
return false;
}
+ EVP_CipherInit_ex(&ctx, NULL, NULL, (unsigned char *)key, (unsigned char *)iv, true);
@bnoordhuis

bnoordhuis Nov 27, 2011

Owner

Long line. Lines should be <= 80 characters. Happens in a few other places too.

@bnoordhuis bnoordhuis commented on an outdated diff Nov 27, 2011

test/simple/test-crypto-ecb.js
+
+var common = require('../common');
+var assert = require('assert');
+
+try {
+ var crypto = require('crypto');
+} catch (e) {
+ console.log('Not compiled with OPENSSL support.');
+ process.exit();
+}
+
+// Testing whether EVP_CipherInit_ex is functioning correctly.
+
+(function()
+{
+ var encrypt = crypto.createCipheriv('BF-ECB', 'SomeRandomBlahz0c5GZVnR', '');
@bnoordhuis

bnoordhuis Nov 27, 2011

Owner

Indentation errors, use two spaces instead of tabs.

Owner

bnoordhuis commented Nov 27, 2011

Sorry for the delay, Ingmar. If you fix up the nits, I'll merge it.

KiNgMaR commented Nov 29, 2011

Fixed up the long lines & whitespace in and around my changes, sorry for not running cpplint earlier. :)

I have signed the CLA.

Owner

bnoordhuis commented Nov 30, 2011

Thanks Ingmar, merged in 2603832.

@bnoordhuis bnoordhuis closed this Nov 30, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment