Calling crypto.createCipheriv with a key of invalid length can cause errors in other consumers of the OpenSSL error queue

[z0w0]

• Version: v9.3.0 (initially discovered on v8.x but seems relevant to later versions too)
• Platform: Windows 64-bit + ArchLinux 64-bit
• Subsystem: Crypto

N.B. What follows is a naive speculation about what's going wrong since I'm not all too familiar with Node.js' internals.

When crypto.createCipheriv is called, it sets the key length using EVP_CIPHER_CTX_set_key_length. For some ciphers this will fail if it doesn't match a certain length. The crypto module throws a JS error if this is the case. If you look at the OpenSSL code for EVP_CIPHER_CTX_set_key_length, this error also goes into the OpenSSL error queue but is never removed by the Crypto module.

Because this error queue seems to be thread-global, you can get into situations where other stuff using OpenSSL (such as HTTPS/TLS) can think that the error was caused by its actions, not by a stale error being on the queue. Like so:

// This example shows how having a invalid key length call to createCipheriv
// caught and ignored while a TLS request is going on can cause the TLS request to fail,
// even though the error is completely separate from the TLS connection.
const crypto = require('crypto');
const https = require('https');

setInterval(() => {
  // Internally set_key_length is called but the error it produces (using EVPErr) isn't thrown away quick enough.
  try {
      crypto.createCipheriv('aes-256-ctr', 'bad key', crypto.randomBytes(16));
  } catch (err) {
    console.log(err.message);

    // Do nothing so that we continue with the old error on the OpenSSL error queue.
    // It would be there anyway, but in normal usage it would be more obvious why things are breaking because
    // you would never try/catch the createCipheriv. That's how this issue was discovered.
  }
}, 50);

const options = {
  hostname: 'google.com',
  port: 443,
  path: '/',
  method: 'GET'
};

const req = https.request(options, (res) => {
  console.log('statusCode:', res.statusCode);
  console.log('headers:', res.headers);

  res.on('data', (d) => {
    process.stdout.write(d);
  });
});

req.on('error', (err) => {
  // throws the EVPErr produces by createCipheriv.
  // At some point the Node.js TLS stack (possibly during TLS read)
  // throws the error off the OpenSSL error queue.
  throw err;
});

req.end();

Obviously this is a bad way to use createCipheriv, but it seems almost certainly wrong for the error to leak outside of the cipher code. Weirdly enough it seems like something else is pulling out the errors on its own, but if you create enough of them (hence the low ms setInterval) then it produces the error.

[addaleax]

@nodejs/crypto

[ryzokuken]

@z0w0 could you please mention all the versions of Node that you found to be affected by these? v9.x isn't receiving updates anymore and will reach it's EOL this month so I'd rather not fix this, but if this also affects v8.x then sure. Also, did you check if master was affected?

[z0w0]

It's almost certainly present in the v8.x LTS, as I discovered this from a production issue at my work which has been using the v8 LTS for some months. I just reproduced on v8.11.2 (ArchLinux) using the above script. The statement I made about it affecting other releases was a generalisation because I checked the master code and it looked like it wasn't clearing out the errors generate by EVP_CIPHER_CTX_set_key_length still. I just tried it on master and confirm it is indeed broken there too.

Expected output if the bug is present is something like:

Invalid key length
Invalid key length
Invalid key length
Invalid key length
Invalid key length
Invalid key length
Invalid key length
Invalid key length
Invalid key length
Invalid key length
statusCode: 301
headers: { location: 'https://www.google.com/',
  'content-type': 'text/html; charset=UTF-8',
  date: 'Tue, 12 Jun 2018 10:40:00 GMT',
  expires: 'Thu, 12 Jul 2018 10:40:00 GMT',
  'cache-control': 'public, max-age=2592000',
  server: 'gws',
  'content-length': '220',
  'x-xss-protection': '1; mode=block',
  'x-frame-options': 'SAMEORIGIN',
  'alt-svc': 'quic=":443"; ma=2592000; v="43,42,41,39,35"',
  connection: 'close' }
<HTML><HEAD><meta http-equiv="content-type" content="text/html;charset=utf-8">
<TITLE>301 Moved</TITLE></HEAD><BODY>
<H1>301 Moved</H1>
The document has moved
<A HREF="https://www.google.com/">here</A>.
</BODY></HTML>
/home/zack/bug.js:40
  throw err;
  ^

Error: 140658076088128:error:0607A082:digital envelope routines:EVP_CIPHER_CTX_set_key_length:invalid key length:../deps/openssl/openssl/crypto/evp/evp_enc.c:595:
140658076088128:error:0607A082:digital envelope routines:EVP_CIPHER_CTX_set_key_length:invalid key length:../deps/openssl/openssl/crypto/evp/evp_enc.c:595:
140658076088128:error:0607A082:digital envelope routines:EVP_CIPHER_CTX_set_key_length:invalid key length:../deps/openssl/openssl/crypto/evp/evp_enc.c:595:

[tniessen]

I think our cipher implementation has failed to properly clear the error queue for some time now. I have a patch for the "Invalid key length" error, but I'd like to explore which other situations are affected.

[z0w0]

Yep, it's easy enough just to fix this particular bug by adding a ClearErrorOnReturn struct initialization to the top of the code that throws the JS error. I was going to open a PR but I'll leave it to you :)

[ryzokuken]

Yeah, I had been planning to fix it too, but @tniessen is better qualified. Let me try looking deeper into the error queue issue.

[tniessen]

@z0w0 @ryzokuken Sorry, didn't want to take this away from either of you, please, go ahead! Just let me know which part you'd like to work on 😃

[ryzokuken]

@tniessen nah, you're better at handling this. I'll try looking into the underlying issue with the error queue.

[jrasanen]

@tniessen @ryzokuken I'd be interested in learning more node core things. In my work, I've done quite a bit with node crypto. So if this is something I can contribute to, I'd be interested. (and if you haven't already fixed it) 🤔

[tniessen]

@jrasanen Sorry, I already went ahead after @ryzokuken's #21281 (comment) and opened #21287 and #21288. The former fixes a bug that would prevent the latter from working.

[jrasanen]

No worries! :)

[ryzokuken]

@jrasanen that said, please feel free to follow crypto issues in this repo and keep diving deeper into the implementations. Crypto is a relatively easy subsystem to crack if you know your OpenSSL calls.