crypto: don't reach into OpenSSL internals for ThrowCryptoError #16701

davidben · 2017-11-03T00:21:26Z

There is a perfectly serviceable ERR_get_error function which avoids
having to sniff through the OpenSSL ring buffer like that. It does
return the errors in the opposite order, but that's easily fixed with
std::reverse.

Note this behavior is slightly different in that an ERR_get_error loop
will ultimately clear the error queue, but this is desireable. Leaving
the error queue uncleared means errors in subsequent operations may get
mixed up and cause issues.

PS: In future, I'm happy to look over changes to crypto stuff for you all, especially when they reach into internals like that. That's rarely actually necessary. :-)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

crypto

Trott · 2017-11-03T02:42:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11162/

MylesBorins · 2017-11-03T04:20:58Z

Consistent error on windows

not ok 71 parallel/test-cli-node-options
  ---
  duration_ms: 2.245
  severity: fail
  stack: |-
    assert.js:42
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: For "--stack-trace-limit=100", failed to find /(\s*at f \(\[eval\]:1:\d*\)\n){100}/ in: <
    [eval]:1
    (function f() { f(); })();
               ^
    
    RangeError: Maximum call stack size exceeded
        at f ([eval]:1:12)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
        at f ([eval]:1:17)
    
    >
        at common.mustCall (c:\workspace\node-test-binary-windows\test\parallel\test-cli-node-options.js:58:12)
        at c:\workspace\node-test-binary-windows\test\common\index.js:522:15
        at ChildProcess.exithandler (child_process.js:279:5)
        at emitTwo (events.js:135:13)
        at ChildProcess.emit (events.js:224:7)
        at maybeClose (internal/child_process.js:943:16)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:220:5)
  ...

other failures are known flakes (fixed in master)

apapirovski · 2017-11-03T04:25:31Z

I don't think that's this PR, it's failing on others too

bnoordhuis · 2017-11-03T05:50:54Z

src/node_crypto.cc

-      es->top = (((es->top - 1) % ERR_NUM_ERRORS) + ERR_NUM_ERRORS) %
-          ERR_NUM_ERRORS;
+  std::vector<Local<String>> errors;
+  while (true) {


Most minor of nits but could you use for (;;) { here? That's what we most commonly use for indefinite loops.

There is a perfectly serviceable ERR_get_error function which avoids having to sniff through the OpenSSL ring buffer like that. It does return the errors in the opposite order, but that's easily fixed with std::reverse. Note this behavior is slightly different in that an ERR_get_error loop will ultimately clear the error queue, but this is desireable. Leaving the error queue uncleared means errors in subsequent operations may get mixed up and cause issues.

cjihrig · 2017-11-04T18:35:22Z

A new Coverity defect report came out, listing this:

** CID 178579:  Null pointer dereferences  (NULL_RETURNS)
/src/node_crypto.cc: 267 in node::crypto::ThrowCryptoError(node::Environment *, unsigned long, const char *)()

>>>     CID 178579:  Null pointer dereferences  (NULL_RETURNS)
>>>     Dereferencing a null pointer "es".
267       if (es->bottom != es->top) {

I was going to PR a fix, but it looks like this PR will take care of it by removing that code.

apapirovski · 2017-12-08T20:40:00Z

ping @nodejs/crypto & @bnoordhuis — can/should this land?

apapirovski · 2017-12-08T20:40:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11988/

bnoordhuis · 2017-12-09T13:07:35Z

@apapirovski Yes, it can land, unless more people want/should sign off on it. The freebsd failure seems to be an unrelated flake.

apapirovski · 2017-12-09T20:10:48Z

Landed in 7db5370

There is a perfectly serviceable ERR_get_error function which avoids having to sniff through the OpenSSL ring buffer like that. It does return the errors in the opposite order, but that's easily fixed with std::reverse. Note this behavior is slightly different in that an ERR_get_error loop will ultimately clear the error queue, but this is desirable. Leaving the error queue uncleared means errors in subsequent operations may get mixed up and cause issues. PR-URL: #16701 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

gibfahn · 2017-12-20T22:34:34Z

I backported this to v8.x-staging, and I'm getting this error on Ubuntu 16.04:

../src/node_crypto.cc: In function ‘void node::crypto::PBKDF2(const v8::FunctionCallbackInfo<v8::Value>&)’:
../src/node_crypto.cc:5416:43: error: ‘isnan’ was not declared in this scope
   if (raw_keylen < 0.0 || isnan(raw_keylen) || isinf(raw_keylen) ||
                                           ^
../src/node_crypto.cc:5416:43: note: suggested alternative:
In file included from /usr/include/c++/5/random:38:0,
                 from /usr/include/c++/5/bits/stl_algo.h:66,
                 from /usr/include/c++/5/algorithm:62,
                 from ../src/node_crypto.cc:46:
/usr/include/c++/5/cmath:641:5: note:   ‘std::isnan’
     isnan(_Tp __x)
     ^
../src/node_crypto.cc:5416:64: error: ‘isinf’ was not declared in this scope
   if (raw_keylen < 0.0 || isnan(raw_keylen) || isinf(raw_keylen) ||
                                                                ^
../src/node_crypto.cc:5416:64: note: suggested alternative:
In file included from /usr/include/c++/5/random:38:0,
                 from /usr/include/c++/5/bits/stl_algo.h:66,
                 from /usr/include/c++/5/algorithm:62,
                 from ../src/node_crypto.cc:46:
/usr/include/c++/5/cmath:621:5: note:   ‘std::isinf’
     isinf(_Tp __x)
     ^
  g++ '-DNODE_ARCH="x64"' '-DNODE_PLATFORM="linux"' '-DNODE_WANT_INTERNALS=1' '-DV8_DEPRECATION_WARNINGS=1' '-DHAVE_INSPECTOR=1' '-D__POSIX__' '-DHAVE_OPENSSL=1' '-DNODE_USE_V8_PLATFORM=1' '-DNODE_HAVE_I18N_SUPPORT=1' '-DNODE_HAVE_SMALL_ICU=1' '-DUCONFIG_NO_SERVICE=1' '-DUCONFIG_NO_REGULAR_EXPRESSIONS=1' '-DU_ENABLE_DYLOAD=0' '-DU_STATIC_IMPLEMENTATION=1' '-DU_HAVE_STD_STRING=0' '-DUCONFIG_NO_BREAK_ITERATION=0' '-DHTTP_PARSER_STRICT=0' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-D_POSIX_C_SOURCE=200112' '-DNGHTTP2_STATICLIB' -I../src -I../tools/msvs/genfiles -I/home/gib/node/out/Release/obj/gen -I/home/gib/node/out/Release/obj/gen/include -I../deps/openssl/openssl/include -I../deps/v8/include -I../deps/icu-small/source/i18n -I../deps/icu-small/source/common -I../deps/zlib -I../deps/http_parser -I../deps/cares/include -I../deps/uv/include -I../deps/nghttp2/lib/includes  -pthread -Wall -Wextra -Wno-unused-parameter -m64 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++0x -MMD -MF /home/gib/node/out/Release/.deps//home/gib/node/out/Release/obj.target/node/src/node_crypto_clienthello.o.d.raw   -c -o /home/gib/node/out/Release/obj.target/node/src/node_crypto_clienthello.o ../src/node_crypto_clienthello.cc
  g++ '-DNODE_ARCH="x64"' '-DNODE_PLATFORM="linux"' '-DNODE_WANT_INTERNALS=1' '-DV8_DEPRECATION_WARNINGS=1' '-DHAVE_INSPECTOR=1' '-D__POSIX__' '-DHAVE_OPENSSL=1' '-DNODE_USE_V8_PLATFORM=1' '-DNODE_HAVE_I18N_SUPPORT=1' '-DNODE_HAVE_SMALL_ICU=1' '-DUCONFIG_NO_SERVICE=1' '-DUCONFIG_NO_REGULAR_EXPRESSIONS=1' '-DU_ENABLE_DYLOAD=0' '-DU_STATIC_IMPLEMENTATION=1' '-DU_HAVE_STD_STRING=0' '-DUCONFIG_NO_BREAK_ITERATION=0' '-DHTTP_PARSER_STRICT=0' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-D_POSIX_C_SOURCE=200112' '-DNGHTTP2_STATICLIB' -I../src -I../tools/msvs/genfiles -I/home/gib/node/out/Release/obj/gen -I/home/gib/node/out/Release/obj/gen/include -I../deps/openssl/openssl/include -I../deps/v8/include -I../deps/icu-small/source/i18n -I../deps/icu-small/source/common -I../deps/zlib -I../deps/http_parser -I../deps/cares/include -I../deps/uv/include -I../deps/nghttp2/lib/includes  -pthread -Wall -Wextra -Wno-unused-parameter -m64 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++0x -MMD -MF /home/gib/node/out/Release/.deps//home/gib/node/out/Release/obj.target/node/src/tls_wrap.o.d.raw   -c -o /home/gib/node/out/Release/obj.target/node/src/tls_wrap.o ../src/tls_wrap.cc
node.target.mk:199: recipe for target '/home/gib/node/out/Release/obj.target/node/src/node_crypto.o' failed
make[1]: *** [/home/gib/node/out/Release/obj.target/node/src/node_crypto.o] Error 1

See the CI run for more info https://ci.nodejs.org/job/node-test-commit/14987/

@yhwang took a look, and said:

it should be stdc++ version issue
I added -D_GLIBCXX_USE_C99_FP_MACROS_DYNAMIC=1 and the compile passed

I've removed it from staging for now. Assuming this should be backported, could someone raise a PR to backport to v8.x-staging? The guide is here.

There is a perfectly serviceable ERR_get_error function which avoids having to sniff through the OpenSSL ring buffer like that. It does return the errors in the opposite order, but that's easily fixed with std::reverse. Note this behavior is slightly different in that an ERR_get_error loop will ultimately clear the error queue, but this is desirable. Leaving the error queue uncleared means errors in subsequent operations may get mixed up and cause issues. PR-URL: nodejs#16701 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

There is a perfectly serviceable ERR_get_error function which avoids having to sniff through the OpenSSL ring buffer like that. It does return the errors in the opposite order, but that's easily fixed with std::reverse. Note this behavior is slightly different in that an ERR_get_error loop will ultimately clear the error queue, but this is desirable. Leaving the error queue uncleared means errors in subsequent operations may get mixed up and cause issues. PR-URL: #16701 Backport-PR-URL: #18327 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Nov 3, 2017

bnoordhuis approved these changes Nov 3, 2017

View reviewed changes

davidben force-pushed the err-get-sanity branch from 8aba98b to 7844116 Compare November 3, 2017 06:31

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

apapirovski closed this Dec 9, 2017

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

gibfahn added dont-land-on-v6.x labels Dec 20, 2017

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17772

Closed

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17774

Merged

yhwang mentioned this pull request Jan 23, 2018

[v8.x backport] crypto: do not reach into OpenSSL internals for ThrowCryptoError #18327

Closed

MylesBorins added backport-requested-v8.x and removed land-on-v8.x labels Jan 23, 2018

richardlau mentioned this pull request Mar 7, 2018

[v8.x] node_crypto: Use <cmath> definitions of isnan and isinf #19196

Closed

4 tasks

BethGriggs added backported-to-v8.x and removed backport-requested-v8.x labels Jul 11, 2018

This was referenced Mar 9, 2020

[Snyk] Security upgrade gatsby from 2.13.73 to 2.13.75 kissofire/node#4

Open

[Snyk] Security upgrade gatsby from 2.13.73 to 2.13.75 meghasfdc/node#5

Open

snyk-bot mentioned this pull request Jul 15, 2022

[Snyk] Security upgrade gatsby from 2.13.73 to 2.13.75 kissofire/node#72

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: don't reach into OpenSSL internals for ThrowCryptoError #16701

crypto: don't reach into OpenSSL internals for ThrowCryptoError #16701

davidben commented Nov 3, 2017 •

edited

Loading

Trott commented Nov 3, 2017

MylesBorins commented Nov 3, 2017

apapirovski commented Nov 3, 2017

bnoordhuis Nov 3, 2017

davidben Nov 3, 2017

cjihrig commented Nov 4, 2017

apapirovski commented Dec 8, 2017

apapirovski commented Dec 8, 2017

bnoordhuis commented Dec 9, 2017

apapirovski commented Dec 9, 2017

gibfahn commented Dec 20, 2017 •

edited

Loading

crypto: don't reach into OpenSSL internals for ThrowCryptoError #16701

crypto: don't reach into OpenSSL internals for ThrowCryptoError #16701

Conversation

davidben commented Nov 3, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Trott commented Nov 3, 2017

MylesBorins commented Nov 3, 2017

apapirovski commented Nov 3, 2017

bnoordhuis Nov 3, 2017

Choose a reason for hiding this comment

davidben Nov 3, 2017

Choose a reason for hiding this comment

cjihrig commented Nov 4, 2017

apapirovski commented Dec 8, 2017

apapirovski commented Dec 8, 2017

bnoordhuis commented Dec 9, 2017

apapirovski commented Dec 9, 2017

gibfahn commented Dec 20, 2017 • edited Loading

davidben commented Nov 3, 2017 •

edited

Loading

gibfahn commented Dec 20, 2017 •

edited

Loading