require('crypto') & DES inconsistent

[igalco]

• Version: v6.7.0
• Platform: Darwin 16.0.0 Darwin Kernel Version 16.0.0: Mon Aug 29 17:56:20 PDT 2016; root:xnu-3789.1.32~3/RELEASE_X86_64 x86_64
• Subsystem: OpenSSL 0.9.8zh 14 Jan 2016

Hello,

I have an issue with the standard crypto package in node js.
The output do not always return the correct answer when using the DES algorithm.

Exemple :

var crypto = require('crypto');

var key = new Buffer("0131517010204061", "hex");
var buffer = new Buffer("1daae21c126127e4", "hex");

for (var i = 0; i < 10; i++) {

    var cipher = crypto.createCipheriv('des', key, Buffer.alloc(0));
    cipher.setAutoPadding(false);
    var crypted = cipher.update(buffer);
    // cipher.final(); is missing because we are only encrypting 1 block of data                                                                                                                       

    console.log("[iteration:%d] key=%s + data=%s => %s", i, key.toString("hex"), buffer.toString("hex"), crypted.toString("hex"));
}

result :
[iteration:0] key=0131517010204061 + data=1daae21c126127e4 => 959f39b6951d75e6
[iteration:1] key=0131517010204061 + data=1daae21c126127e4 => 959f39b6951d75e6
[iteration:2] key=0131517010204061 + data=1daae21c126127e4 => 959f39b6951d75e6
[iteration:3] key=0131517010204061 + data=1daae21c126127e4 => 959f39b6951d75e6
[iteration:4] key=0131517010204061 + data=1daae21c126127e4 => a3201c51a48d3df8
[iteration:5] key=0131517010204061 + data=1daae21c126127e4 => 959f39b6951d75e6
[iteration:6] key=0131517010204061 + data=1daae21c126127e4 => a3201c51a48d3df8
[iteration:7] key=0131517010204061 + data=1daae21c126127e4 => 7971aa42de5e626b
[iteration:8] key=0131517010204061 + data=1daae21c126127e4 => b37129ad8d2b91be
[iteration:9] key=0131517010204061 + data=1daae21c126127e4 => 959f39b6951d75e6

[igalco]

on an other machine (debian)

user@srv:/tmp$ nodejs --version
v0.12.16
user@srv:/tmp$ uname -a
Linux srv 3.16.0-4-amd64 $1 SMP Debian 3.16.7-ckt25-2+deb8u3 (2016-07-02) x86_64 GNU/Linux
user@srv:/tmp$ openssl version
OpenSSL 1.0.1t  3 May 2016
user@srv:/tmp$ cat test.js 
var crypto = require('crypto');

var key = new Buffer("0131517010204061", "hex");
var buffer = new Buffer("1daae21c126127e4", "hex");
var ref = "959f39b6951d75e6";

for (var i = 0; i < 10; i++) {

    var cipher = crypto.createCipheriv('des', key, new Buffer(8));
    cipher.setAutoPadding(false);
    var crypted = cipher.update(buffer).toString("hex");
    // cipher.final(); is missing because we are only encrypting 1 block of data

    console.log("[iteration:%d] key=%s + data=%s => %s", i, key.toString("hex"), buffer.toString("hex"), crypted, crypted != ref ? "error !": "");
}
user@srv:/tmp$ nodejs test.js 
[iteration:0] key=0131517010204061 + data=1daae21c126127e4 => 959f39b6951d75e6 
[iteration:1] key=0131517010204061 + data=1daae21c126127e4 => 959f39b6951d75e6 
[iteration:2] key=0131517010204061 + data=1daae21c126127e4 => 959f39b6951d75e6 
[iteration:3] key=0131517010204061 + data=1daae21c126127e4 => 7b2662610fc143df error !
[iteration:4] key=0131517010204061 + data=1daae21c126127e4 => 959f39b6951d75e6 
[iteration:5] key=0131517010204061 + data=1daae21c126127e4 => 6d85a9d9104b0f14 error !
[iteration:6] key=0131517010204061 + data=1daae21c126127e4 => dc8891c064913d33 error !
[iteration:7] key=0131517010204061 + data=1daae21c126127e4 => 749895dd3831915e error !
[iteration:8] key=0131517010204061 + data=1daae21c126127e4 => c275a25594cdafda error !
[iteration:9] key=0131517010204061 + data=1daae21c126127e4 => 6414dfd69afe6af7 error !
user@srv:/tmp$ nodejs test.js 
[iteration:0] key=0131517010204061 + data=1daae21c126127e4 => 959f39b6951d75e6 
[iteration:1] key=0131517010204061 + data=1daae21c126127e4 => 959f39b6951d75e6 
[iteration:2] key=0131517010204061 + data=1daae21c126127e4 => 959f39b6951d75e6 
[iteration:3] key=0131517010204061 + data=1daae21c126127e4 => 7b2662610fc143df error !
[iteration:4] key=0131517010204061 + data=1daae21c126127e4 => 959f39b6951d75e6 
[iteration:5] key=0131517010204061 + data=1daae21c126127e4 => 6d85a9d9104b0f14 error !
[iteration:6] key=0131517010204061 + data=1daae21c126127e4 => dc8891c064913d33 error !
[iteration:7] key=0131517010204061 + data=1daae21c126127e4 => 3b6302cb8fe84b52 error !
[iteration:8] key=0131517010204061 + data=1daae21c126127e4 => 5698547daa672d28 error !
[iteration:9] key=0131517010204061 + data=1daae21c126127e4 => 85726b0440ba171f error !
user@srv:/tmp$ 

[igalco]

Extending the exemple to check the success rate i have :

• osx 10.12 : Success rate : 0.143% (143 / 100000)
• debian jessie : Success rate : 77.595% (77595 / 100000)

[igalco]

Tempoary fix use :
var cipher = crypto.createCipheriv('des-ede', Buffer.concat([key,key]), Buffer.alloc(0));
Instead of :
var cipher = crypto.createCipheriv('des', key, Buffer.alloc(0));

Success rate : 100% (100000 / 100000)

But this is far from beeing satisfactory .....

[addaleax]

new Buffer(8) gives you nondeterministic output that you use as an IV (and you probably even want a random IV), so the behaviour here seems pretty okay to me.

[igalco]

'des' is ECB. Only CBC requires an IV, the parameters is here for good looks so that the toBuf() function do not throw an error.

You may use new Buffer("000000000000000", "hex"); and you will have the exact same issue (i actually tried it before posting the issue on git hub)

Beside, using des-ede (triple des encrypt/decrypt/encrypt) with key1 = key2 solves the issue

[addaleax]

'des' is ECB. Only CBC requires an IV, the parameters is here for good looks so that the toBuf() function do not throw an error.

I’m not an expert in crypto, so I can’t tell whether this is a problem in Node or not, but the passed IV does seem to be used (I get different but consistent results for different 8-byte IVs) for des. So, yeah, it’s still using uninitialized data.

The valgrind outputs for the above scripts are about what you’d expect (Invalid read of size 8/Use of uninitialised value of size 8), but for reference here are the full logs:
valgrind-gh-9024-new-buffer-0.txt
valgrind-gh-9024-new-buffer-8.txt

You may use new Buffer("000000000000000", "hex"); and you will have the exact same issue (i actually tried it before posting the issue on git hub)

Tried it, and it worked (consistently) when adding one more 0 to have a proper hex string.

[igalco]

Okay my bad, using new Buffer("0000000000000000", "hex") does solve the problem.

[addaleax]

That’s great to hear!

(I’d still be interested to hear from anyone in @nodejs/crypto whether the out-of-bounds reads for zero-length IVs should be considered problematic…)

[bnoordhuis]

Yeah, that seems like a bug. I'd expect a zero-sized IV to throw an 'invalid IV length' exception.

[bnoordhuis]

Ah...

  /* OpenSSL versions up to 0.9.8l failed to return the correct
     iv_length (0) for ECB ciphers */
  if (EVP_CIPHER_iv_length(cipher_) != iv_len &&
      !(EVP_CIPHER_mode(cipher_) == EVP_CIPH_ECB_MODE && iv_len == 0) &&
      !(EVP_CIPHER_mode(cipher_) == EVP_CIPH_GCM_MODE) && iv_len > 0) {
    return env()->ThrowError("Invalid IV length");
  }

Guess we need to fix something there.

[bnoordhuis]

Turns out that yes, the logic is faulty - there is a misplaced ) in there:

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 7ad6ece..5a89780 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -3261,7 +3261,7 @@ void CipherBase::InitIv(const char* cipher_type,
      iv_length (0) for ECB ciphers */
   if (EVP_CIPHER_iv_length(cipher_) != iv_len &&
       !(EVP_CIPHER_mode(cipher_) == EVP_CIPH_ECB_MODE && iv_len == 0) &&
-      !(EVP_CIPHER_mode(cipher_) == EVP_CIPH_GCM_MODE) && iv_len > 0) {
+      !(EVP_CIPHER_mode(cipher_) == EVP_CIPH_GCM_MODE && iv_len > 0)) {
     return env()->ThrowError("Invalid IV length");
   }

For posterity, openssl interprets "des" as DES-CBC, not DES-ECB.