errors in native modules that use openssl prevent hashes from being created

[calvinmetcalf]

There is a small regression in 5.1.0, that comes up in some edge cases that I might have been the only one using (and have fixed in my code). If

1. you have a c module that is using openssl (like raw-ecdsa).
2. if that module has an error (like in raw-ecdsa where it tries a couple different ways to parse a key in order).
3. and does not clear the error stack

then due to the addition of this line running crypto.createHash with throw an error.

[bnoordhuis]

Isn't calling that a 'regression' stretching the definition of the word? Rule of thumb when using openssl is that you leave the error stack as you found it. It sounds to me the bug is in raw-ecdsa.

[calvinmetcalf]

I 100% agree that this is a bit of a stretch of the term regression, relevant xkcd

[bnoordhuis]

Can't disagree with that :-) but I would suggest updating raw-ecdsa to use ERR_set_mark/ERR_pop_mark.

That particular check in Hash::HashInit() is easy to fix (see patch) but there are one or two other places where we peek at the error stack that are not so trivial to update. It would at best be an incomplete fix.

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 63d767a..1b4cc1a 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -3556,8 +3556,7 @@ bool Hash::HashInit(const char* hash_type) {
   if (md_ == nullptr)
     return false;
   EVP_MD_CTX_init(&mdctx_);
-  EVP_DigestInit_ex(&mdctx_, md_, nullptr);
-  if (0 != ERR_peek_error()) {
+  if (EVP_DigestInit_ex(&mdctx_, md_, nullptr) <= 0) {
     return false;
   }
   initialised_ = true;

[ncopa]

This also affects 4.2.4 release on alpine linux (see http://bugs.alpinelinux.org/issues/4999). The patch is correct as it checks function result rather than only checking error stack. So yes, this is a real regression.

Can the patch be included for 4.2.5, please?

[stefanmb]

@ncopa My initial concern was to have the hash initialization function fail gracefully if there is an OpenSSL error. For that purpose both peeking the error stack or using the return code will work. Bear with me, as I'm pretty new to OpenSSL, but could you explain what exactly is causing errors to be put on the OpenSSL thread local error stack in your case? My understanding is that there would have to be some other library or native module calling OpenSSL and causing those errors, like in the initial issue reported here by @calvinmetcalf.

For what it's worth, I tried to reproduce the Alpine Linux problem report and was unable to, here are the steps I followed:

(1) Download and install Alpine 3.3.1 and the development dependencies.
(2) Check out v4.2.4 Node.js from Github and compile.
(3) Add a PaX exception as detailed here using setfattr -n user.pax.flags -v "emr" (otherwise V8 segfaults on startup).
(4) Run node -e "console.log(require('crypto').createHash('md5').update('test').digest('hex'))"

This procedure seems to work. Is there some additional step that I'm missing? Thanks.

@bnoordhuis, I saw in you previous comment you were against the partial fix due to other uses of ERR_peek_error, however it seems your example patch has already been applied by the Alpine Linux maintainers to their packaged version of Node 4.2.4. Should we apply the patch too?

[mhart]

@stefanmb It may be because the nodejs package in the Alpine repository links to openssl dynamically (using --shared-openssl when compiling)

[mhart]

@stefanmb See here for how the package is compiled: http://git.alpinelinux.org/cgit/aports/tree/main/nodejs/APKBUILD#n30

This is not the approach with the alpine-node Docker images I create on Alpine – I compile it in statically, as per default: https://github.com/mhart/alpine-node/blob/4.2.4/Dockerfile#L15 – and I've never seen this error or anything similar.

[stefanmb]

@mhart Thank you for the clarification, I had glossed over the build log but missed the shared library part. The error does reproduce if I use OpenSSL as a shared library.

[mhart]

It raises a good question around what various configure flags should be tested in the Node.js CI environment – there are a lot of them and I'm sure it gets pretty combinatorially expensive to try and test all of them.

[ncopa]

One option might be to test all static (bundled) libs and all dynamic (system) libs.

[ncopa]

If you wonder why it fails on alpine system openssl and not on any other, then the reason is this patch: http://git.alpinelinux.org/cgit/aports/tree/main/openssl/1004-crypto-engine-autoload-padlock-dynamic-engine.patch

It tries to load padlock crypto engine (used on via cpu), which will give you an awesome performance on those VIA cpu boxes. But apparently it does save an error on the error stack when no padlock hardware exists. (So the error should not happen on the VIA cpu.)

We discussed this morning if we should clear the error stack in case of error, but its risky as the api will not let you pop the last error from the stack, only the first. This means that if there is another unhandeled error in the queue, it will free the wrong error and leave the crypto engine error in the stack. or we'd need reset the entire stack, which would mean that potential errors will get lost. Basically, the API makes it hard to solve it correctly. If we bump into this problem again then we probably will reset the error queue in our patch (which til now has worked since 2010-06-04)

In any case, I think nodejs should properly check function return status instead of only relying on error stack. Then at least the nodejs code will be robust.

[stefanmb]

@ncopa Thanks for that, I was slowly investigating it but you've saved me a bunch of time. ;)

I'll defer to @bnoordhuis about the return code patch, I can easily do a pull request if that's what we agree to do.

[ncopa]

@stefanmb I had the feeling you would investigate. Thats what I'd do (well, thats what i did), so I figured I better share :)

We may fix the alpine openssl patch, but openssl is a bit fragile so I am scared to do so. We will likely switch to libressl too so I'm holding that off for now.

[bnoordhuis]

#4731 for the patch.