tls.createSecureContext SetKey doesn't respect OpenSSL engine set by crypto.setEngine

[GauriSpears]

Version

19.7.0

Platform

Linux 5.10.0-8-amd64 #1 SMP Debian 5.10.46-4 (2021-08-03) x86_64 GNU/Linux

Subsystem

crypto

What steps will reproduce the bug?

I'm trying to create TLS connection using custom OpenSSL engine (gost-engine). OpenSSL version is 3.0.8 and engine are installed correctly (openssl s_client -connect api.dom.gosuslugi.ru:443 -engine gost -verifyCAfile server.pem -partial_chain -cert public.cer -key private.pem -ignore_unexpected_eof works perfectly). Node.JS is recompiled with shared OpenSSL support (./configure --shared-openssl --shared-openssl-libpath=/usr/local/ssl/lib --shared-openssl-includes=/usr/local/ssl/include). Private key is in a .pem file so it should be loaded with "key" option, not "privateKeyIdentifier".
So I try to tls.createSecureContext with key: fs.readFileSync('/path/to/private/key.pem').

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

tls.createSecureContext should finish with no errors.

What do you see instead?

I get an error:

error Error: error:1E08010C:DECODER routines::unsupported
    at setKey (node:internal/tls/secure-context:92:11)
    at configSecureContext (node:internal/tls/secure-context:174:7)
    at Object.createSecureContext (node:_tls_common:118:3)
    at Object.connect (node:_tls_wrap:1631:48)
    at Agent.createConnection (node:https:150:22)
    at Agent.createSocket (node:_http_agent:342:26)
    at Agent.addRequest (node:_http_agent:289:10)
    at new ClientRequest (node:_http_client:338:16)
    at Object.request (node:https:360:10)
    at RedirectableRequest._performRequest (/home/GKU-data/JS/node_modules/follow-redirects/index.js:284:24) {
  library: 'DECODER routines',
  reason: 'unsupported',
  code: 'ERR_OSSL_UNSUPPORTED'

Additional information

Setting engine by crypto.setEngine doesn't help.
As far as I see, Node.JS uses PEM_read_bio_PrivateKey both in src/crypto/crypto_keys.cc/ParsePrivateKey for Sign.sign() routine and in src/crypto/crypto_context.cc/SecureContext::SetKey for tls.createSecureContext calls. While in first place it understands which engine is set, in second place the engine is ignored.

[bnoordhuis]

I'm skeptical that this is actually an engine issue because that "DECODER routines::unsupported" error means openssl doesn't understand the contents of the PEM file.

If you have a key that openssl doesn't understand natively but your custom engine does, you need to load it (by name) through ENGINE_load_private_key(); that's what the privateKeyEngine and privateKeyIdentifier options are for.

[GauriSpears]

Are you sure that you can pass a path to pem file as privateKeyIdentifier? This trick doesn't work with gost-engine. As far as I understand, purpose of privateKeyIdentifier is a support of hardware tokens (key is hidden inside engine but not in a pem file).
As I mentioned above, node.js works with gost-engine correctly in Sign.sign().
SecureContext::SetKey tries to load the same key from the same pem via the same PEM_read_bio_PrivateKey function, but fails.

[GauriSpears]

Just to make it clear why I believe it's Node.js bug:
1). I use key.pem to sign a message

const sign = crypto.createSign('md_gost12_256');
sign.update('message to sign');
const SignatureValue = sign.sign(fs.readFileSync('/path/to/key.pem'),'base64');

Call stack is:
lib/internal/crypto/sig.js Sign.prototype.sign = function sign(options, encoding)
src/crypto/crypto_sig.cc void Sign::SignFinal
src/crypto/crypto_keys.cc ManagedEVPPKey ManagedEVPPKey::GetPrivateKeyFromJs
src/crypto/crypto_keys.cc ParseKeyResult ParsePrivateKey
pkey->reset(PEM_read_bio_PrivateKey(bio.get(), nullptr, PasswordCallback, &passphrase));
And it works!
2). I use key.pem to create TLS connection:

const https = require('https');
const httpsAgent = new https.Agent({
  ca: fs.readFileSync('/path/to/server_cert.pem'),
  rejectUnauthorized: false,
  secureOptions: crypto.constants.SSL_OP_IGNORE_UNEXPECTED_EOF,
  key: fs.readFileSync('/path/to/key.pem'),
  cert: fs.readFileSync('/path/topublic.cer')
});
const axios = require ('axios');
const response = async () => {
  return await axios.get("https://api.dom.gosuslugi.ru:443", {httpsAgent})
}
response()
.then(res => {
  console.log('result', res.data);
})
.catch(err => {
  console.log('error', err);
})

Call stack is:
lib/internal/tls/secure-context.js configSecureContext
lib/internal/tls/secure-context.js setKey
src/crypto/crypto_context.cc void SecureContext::SetKey
EVPKeyPointer key(PEM_read_bio_PrivateKey(bio.get(), nullptr, PasswordCallback, &pass_ptr));
And it fails!

As you can see, the same key.pem, the same OpenSSL function PEM_read_bio_PrivateKey, but it works in one place and fails in another.

[bnoordhuis]

Yeah, node calls PEM_read_bio_PrivateKey() in both cases. It seems unlikely that it would parse the key successfully in one place but not the other.

Something else is probably going on but you have a bespoke setup. There's no way for me to reproduce any of that locally and no way to confirm whether it's actually a node issue. Your bug report is basically inactionable.

If you have time and inclination to investigate yourself, great. If not, we might as well close this.

[GauriSpears]

Do you have any clue why Node loads engine in one case and ignores it in another?

[bnoordhuis]

I doubt that's what happens. Once an engine is loaded, it's loaded. What flags do you pass to crypto.setEngine()?

[GauriSpears]

I don't pass any, because ALL is default.

[GauriSpears]

The most weird thing is that tls.getCiphers() result includes ciphers added by engine even without crypto.setEngine.

[bnoordhuis]

You may want to investigate if it isn't a problem with the openssl library you link to.

[GauriSpears]

Actually, setEngine is not needed in my configuration because engine is correctly described in openssl.cnf.
Sign.sign works correctly without setEngine. Tls.getCiphers shows needed cipher without setEngine. But configSecureContext fails.
I don't suppose this to be an OpenSSL issue because, as I mentioned in the original post, TLS connection using OpenSSL command line works fine:
openssl s_client -connect api.dom.gosuslugi.ru:443 -engine gost -verifyCAfile server.pem -partial_chain -cert public.cer -key private.pem -ignore_unexpected_eof

[GauriSpears]

Do I understand correctly that when I create new SecureContext it is different from the default Node.JS SecureContext, it's absolutely clean, doesn't load system openssl.cnf and doesn't know anything about engines etc?

[bnoordhuis]

No. SecureContext maps directly to openssl's SSL_CTX. It's still affected by things like crypto.setEngine() because that's a global operation, not a per-context setting.

[GauriSpears]

I found it! It turned out to be some error in your BIO loader. I've replaced in "void SecureContext::SetKey" function of src/crypto/crypto_context.cc the code:
BIOPointer bio(LoadBIO(env, args[0]));
with

ByteSource bkey = ByteSource::FromStringOrBuffer(env, args[0]);
BIOPointer bio(BIO_new_mem_buf(bkey.data<char>(), bkey.size()));

and everything works! The correct code was taken from ParsePrivateKey function of src/crypto/crypto_keys.cc

[bnoordhuis]

I looked at NodeBIO but I don't think it does anything materially different from BIO_new_mem_buf() so... 🤷

If you can identify where it diverges, it can probably be fixed.

[GauriSpears]

I've investigated further. The problem is that when you create bio like this: BIOPointer bio(LoadBIO(env, args[0])); we get bio with seek disabled and BIO_seek(bio.get(), 0); doesn't work.
More details: I looked into OpenSSL source (crypto/pem/pem_pkey.c). PEM_read_bio_PrivateKey function first tries to read bio with pem_read_bio_key function, it fails and then pem_read_bio_key_legacy function is used. But after pem_read_bio_key bio pointer doesn't point to the beginning anymore, so BIO_seek(bio, 0) is called. In works with bio created by BIO_new_mem_buf, but not with bio created by LoadBIO.

As an experiment, I did this:

ByteSource bkey = ByteSource::FromStringOrBuffer(env, args[0]); //1st (working) way to create BIO
BIOPointer bio(BIO_new_mem_buf(bkey.data<char>(), bkey.size()));
char*linebuf = (char*)malloc(256);
int len = BIO_gets(bio.get(), linebuf, 255);
printf("%s-%i", linebuf, len); //print 1st line of BIO
BIO_seek(bio.get(), 0); //return BIO position back to 0
len = BIO_gets(bio.get(), linebuf, 255);
printf("%s-%i", linebuf, len); //print 1st line of BIO again

BIOPointer bio(LoadBIO(env, args[0])); //2nd (buggy) way to create BIO
len = BIO_gets(bio.get(), linebuf, 255);
printf("%s-%i", linebuf, len); //print 1st line of BIO
BIO_seek(bio.get(), 0); //should return BIO position back to 0, but doesn't!
len = BIO_gets(bio.get(), linebuf, 255);
printf("%s-%i", linebuf, len); //should print 1st line, but prints 2nd line of BIO!

[bnoordhuis]

NodeBIO::Ctrl() doesn't implement BIO_C_FILE_SEEK (nor BIO_C_FILE_TELL) so that does indeed seem like a likely culprit. OpenSSL doesn't normally BIO_seek() when parsing data structures so that's probably why it went unnoticed for so long.

If implementing those controls helps the GOST engine, then PR welcome.

[GauriSpears]

As far as I see, NodeBIO is based on singly-linked buffers. So I can seek within current buffer and I can switch to the next buffer. But how should I jump to the first buffer?
Please tell me the best choice:
1). We assume that 1024 bytes of single buffer is enough for any PEM key and I implement seeking withing current buffer only.
2). I make buffer chain linked in both sides.
3). I add in NodeBIO a new element - link to the first buffer chunk.

[bnoordhuis]

There's also option 4) get rid of NodeBIO in crypto_context.cc and use openssl's own memory BIOs. :-)

NodeBIO makes sense for TLS traffic but less so for loading keys or certificates. Loading private keys with BIO_s_secmem() is also desirable, see #30956.

[GauriSpears]

As an example: is it OK if I write

  BIO *bio = BIO_new(BIO_s_secmem());
  if (!bio)
    return;
  ByteSource bsrc = ByteSource::FromStringOrBuffer(env, args[0]);
  int written = BIO_write(bio, bsrc.data<char>(), bsrc.size());
  if (!written)
    return;

instead of
BIOPointer bio(LoadBIO(env, args[0])); ?
(It works)

[bnoordhuis]

The first line should be:

BIOPointer bio(BIO_new(BIO_s_secmem()));

Because automatic resource management > manually calling BIO_free.

Check that written == bsrc.size() in case of partial writes. (Unlikely with memory BIOs but still.)

[GauriSpears]

So I've rewritten LoadBIO this way:

BIOPointer LoadBIO(Environment* env, Local<Value> v) {
  if (v->IsString() || v->IsArrayBufferView()) {
    BIOPointer bio(BIO_new(BIO_s_secmem()));
    if (!bio)
      return nullptr;
    ByteSource bsrc = ByteSource::FromStringOrBuffer(env, v);
    int written = BIO_write(bio.get(), bsrc.data<char>(), bsrc.size());
    if (written != bsrc.size())
      return nullptr;
    return bio;
  }
  return nullptr;
}

Is it pretty enough?

[bnoordhuis]

Looks good to me modulo the signed-vs-unsigned comparison. If you open a pull request, I'll run it through the CI.