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

OpenSSL EVP API Fix (ECB mode failing) #1997

Closed
wants to merge 8 commits into from
Closed

OpenSSL EVP API Fix (ECB mode failing) #1997

wants to merge 8 commits into from

Conversation

KiNgMaR
Copy link

@KiNgMaR 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

…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.
@bnoordhuis
Copy link
Member

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

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?

@bnoordhuis
Copy link
Member

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()?

…ia linking against old OpenSSL and using the new test case. Also the rest of the test suite does not report any side effects.
@KiNgMaR
Copy link
Author

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

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.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bnoordhuis
Copy link
Member

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

@KiNgMaR
Copy link
Author

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.

@bnoordhuis
Copy link
Member

Thanks Ingmar, merged in 2603832.

@bnoordhuis bnoordhuis closed this Nov 30, 2011
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants