crypto: fix Node_SignFinal #15024

davidben · 2017-08-24T23:05:53Z

This is a bit of a mess. Originally, OpenSSL mixed up hashes with
signature algorithms. An EVP_MD was nominally a hash, but they sometimes
had key types associated with them for signining purposes. So
EVP_sha256() was secretly associated with RSA (and named both "SHA256"
and "RSA-SHA256"), but DSA was not usable with EVP_sha256() and instead
one used EVP_dss1() which is identical to EVP_sha1() but associated with
DSA instead.

Way back in OpenSSL 1.0.0, OpenSSL added the newer EVP_DigestSign* and
EVP_DigestVerify* functions which should be used instead of the legacy
EVP_Sign* and EVP_Verify* functions which Node used up until recently.
The newer functions no longer confuse hash functions and signature
algorithms and are the supported way to use more complex algorithms like
RSA-PSS. This is also why Node has this caveat about
"ecdsa-with-SHA256". It's not that OpenSSL is missing this. Node was
using the wrong API.

As part of the 1.0.0 changes, hash functions like EVP_sha256() in
OpenSSL are no longer are tied to signature algorithms and may be used
with any key. This means that "RSA-SHA256" in Node just means "SHA256".
Even before this patch, replacing "DSS1" with "RSA-SHA1" in
test-crypto-binary-default.js worked fine.

Unfortunately, the Node crypto.createSign API is incompatible with
EVP_DigestVerify* due to parameters and key being specified late, which
means it can only be used with signature algorithms which sign a
pre-hashed digest rather than something more complex. Fortunately,
that's all of them right now (though Node will need new APIs for Ed25519
in the future).

PR #11705 switched Node away from using EVP_Sign*, but not to the new
APIs. In doing so, it copied bits of EVP_SignFinal but missed a critical
check that prevents pkey->pkey.ptr from being downcast to the wrong
pointer type. That codepath is problematic for OpenSSL 1.1.0 and
redundant (note how the verify half does not have this).

Moreover, using "RSA-SHA256" with RSA-PSS does not make sense.
"RSA-SHA256" is the name of the sha256WithRSAEncryption OID, used with
PKCS#1 v1.5, not RSA-PSS. That "RSA-SHA256" works in place of "SHA256"
is the same legacy quirk of EVP_Sign* that allows it to be used with
(EC)DSA.

Rather than all this, always use the EVP_PKEY_sign codepath. Everything
that previously worked still works, but more safely and consistently.
Update the documentation to call the parameter the hash function, not
the signature algorithm, finishing a change that PR #11705 effectively
did, but not fully.

Checklist

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

Affected core subsystem(s)

crypto

jasnell · 2017-08-24T23:16:35Z

ping @nodejs/crypto

shigeki · 2017-08-26T00:31:16Z

doc/api/crypto.md

@@ -937,7 +938,7 @@ Example: Using the [`sign.update()`][] and [`sign.sign()`][] methods:



It is better to add a description about a signature algorithm in the comment of examples like

-// Prints: the calculated signature +// Prints: the calculated signature with an algorithm of the type of +// private key with SHA256. When an RSA private key is used, the +// signature algorithm is RSA-SHA256. It is ECDSA with SHA256 in case +// of an EC private key.

Wrote something to that effect.

It looks good to me.

shigeki · 2017-08-26T00:34:14Z

src/node_crypto.cc

-    return 0;
-  }
-
-  return mdctx->digest->sign(mdctx->digest->type, m, m_len, md, sig_len,


This is the same fix as in openssl-1.1.0 and EVP_MD_FLAG_PKEY_METHOD_SIGNATURE was removed in openssl/openssl@7f572e9. Is it sure we do not need this code path even in 1.0.2?

Node is already assuming this codepath isn't needed in the verify half. This codepath is because, in 0.9.8, an EVP_PKEY didn't have EVP_PKEY_sign and EVP_PKEY_verify. Those were implemented in the signature EVP_MDs. In 1.0.0, an EVP_PKEY became less tied to EVP_MD and could sign and verify digests on their own.

Thanks for your explanation about its history. I thought it might be safe for we do not have NoneWithRSA but I did not have the confidence to remove that code path. .

shigeki · 2017-08-26T00:42:25Z

I think it is better to rewrite all 'RSA-SHA' into 'SHA' in createSign/Verify in tests and benchmarks.

@davidben Can you change them?

Way back in OpenSSL 1.0.0, OpenSSL added the newer EVP_DigestSign* and
EVP_DigestVerify* functions which should be used instead of the legacy
EVP_Sign* and EVP_Verify* functions which Node used up until recently

I agree. We need to change or add a new API to use EVP_DigestSign*/Verify*.

Fortunately, that's all of them right now (though Node will need new APIs for Ed25519
in the future).

If we add Ed25519 in the future, will it be like crypto.createDigestSign(null, private_key) ?

davidben · 2017-08-30T16:01:50Z

(Will upload a new revision shortly to address your comments.)

If we add Ed25519 in the future, will it be like crypto.createDigestSign(null, private_key)?

Depends on what you all wish to do, I guess. If you want to mimic the OpenSSL API, they do EVP_DigestSignInit with NULL digest and you call EVP_DigestSign instead of EVP_DigestSignUpdate + EVP_DigestSignFinal. Ed25519 is fun in several ways:

It isn't structured like a pre-hash + signing, which means the Node API where the key is passed in late won't work, unless you wish to buffer the entire payload.
Ed25519 is single-shot, not streaming. So it doesn't fit the Init/Update/Update/Update/Final pattern. OpenSSL is adding EVP_DigestSign which is the same as a single EVP_DigestSignUpdate + EVP_DigestSignFinal for existing algorithms and does the right thing for single-shot algorithms like Ed25519.

(The word "digest" here is kinda meaningless and is vestigial. :-) We did EVP_PKEY_sign_message but upstream preferred to keep the EVP_MD_CTX in there, so we changed ours to match.)

davidben · 2017-08-30T18:48:29Z

I think it is better to rewrite all 'RSA-SHA' into 'SHA' in createSign/Verify in tests and benchmarks.

Done.

shigeki · 2017-09-01T16:58:29Z

Yes, I agree that Ed25519 is a single shot API so we need to consider its difference from existing APIs.

(The word "digest" here is kinda meaningless and is vestigial. :-) We did EVP_PKEY_sign_message but upstream preferred to keep the EVP_MD_CTX in there, so we changed ours to match.)

I've just found https://boringssl.googlesource.com/boringssl/+/19670949ca665ed577ddb12c0f567cb943dd4796.

CI is running on https://ci.nodejs.org/job/node-test-pull-request/9916/

BridgeAR · 2017-09-02T06:39:12Z

This looks like being semver-major to me?

davidben · 2017-09-02T17:00:18Z

This isn't API-incompatible, no more than #11705 was. This is fixing a bunch of problems with #11705. I can undo the documentations if you like, but they're consequences of how you all added RSA-PSS.

BridgeAR · 2017-09-02T17:21:46Z

Oh, yes, I overlooked that the legacy methods still work as they used to. As I did not look deeply into this - is this more a kind of a bugfix or a new feature?

davidben · 2017-09-02T18:25:50Z

It fixes an issue where you would get a memory error if you signed, say, "DSS1" with an RSA key. The copied code in Node_SignFinal missed a type check. Otherwise it's cleanup in preparation for 1.1.0 and pointing folks in documentation to a slightly simpler API. (Using the sigalg names is awkward since it doesn't work for ECDSA+SHA256.) The hash names always worked, but I think it's a little more uniform to recommend them. You don't have to remember ECDSA is weird.

BridgeAR · 2017-09-03T23:11:42Z

Top! Thanks for the clarification for me @davidben

shigeki · 2017-09-05T16:24:46Z

I agree that this is not semver-major but it looks so much changed in the doc for readers.
I thinks this does not need to be backported to LTS of v6 and v4 for fear that it would surprise users.
@davidben Do you agree it?

The previous CI has some failures in Jenkins and tests not related this fix. I submitted a new CI job again in https://ci.nodejs.org/job/node-test-pull-request/9949/ but another Jenkins error and test failures not related this PR are still shown. I can say that this PR can be passed in CI.

I'm going to wait for another reviewers in a few days before landing.

davidben · 2017-09-05T21:21:44Z

I thinks this does not need to be backported to LTS of v6 and v4 for fear that it would surprise users.

Agreed. Node_SignFinal function wasn't backported to v6 and v4 anyway. :-)

BridgeAR

As far as I can judge on this it is looking good to me. But the commit message is a bit verbose and it would be nice if that could be limited to the most important parts.

davidben · 2017-09-09T22:42:31Z

Yeah, I do tend to be a little long-winded. :-) Is this new one better?

PR nodejs#11705 switched Node away from using using OpenSSL's legacy EVP_Sign* and EVP_Verify* APIs. Instead, it computes a hash normally via EVP_Digest* and then uses EVP_PKEY_sign and EVP_PKEY_verify to verify the hash directly. This change corrects two problems: 1. The documentation still recommends the signature algorithm EVP_MD names of OpenSSL's legacy APIs. OpenSSL has since moved away from thosee, which is why ECDSA was strangely inconsistent. (This is why "ecdsa-with-SHA256" was missing.) 2. Node_SignFinal copied some code from EVP_SignFinal's internals. This is problematic for OpenSSL 1.1.0 and is missing a critical check that prevents pkey->pkey.ptr from being cast to the wrong type. To resolve this, remove the non-EVP_PKEY_sign codepath. This codepath is no longer necessary. PR nodejs#11705's verify half was already assuming all EVP_PKEYs supported EVP_PKEY_sign and EVP_PKEY_verify. Also, in the documentation, point users towards using hash function names which are more consisent. This avoids an ECDSA special-case and some strangeness around RSA-PSS ("RSA-SHA256" is the OpenSSL name of the sha256WithRSAEncryption OID which is not used for RSA-PSS).

BridgeAR · 2017-09-11T03:17:48Z

My review is only about the js and doc part, not the c++ part!

BridgeAR · 2017-09-11T03:19:44Z

@davidben just as a notice because you had a max line length of 80 in the commit message - our max line length is actually 72 :-)

BridgeAR · 2017-09-11T03:27:09Z

Landed in 6ebdb69

PR nodejs#11705 switched Node away from using using OpenSSL's legacy EVP_Sign* and EVP_Verify* APIs. Instead, it computes a hash normally via EVP_Digest* and then uses EVP_PKEY_sign and EVP_PKEY_verify to verify the hash directly. This change corrects two problems: 1. The documentation still recommends the signature algorithm EVP_MD names of OpenSSL's legacy APIs. OpenSSL has since moved away from thosee, which is why ECDSA was strangely inconsistent. (This is why "ecdsa-with-SHA256" was missing.) 2. Node_SignFinal copied some code from EVP_SignFinal's internals. This is problematic for OpenSSL 1.1.0 and is missing a critical check that prevents pkey->pkey.ptr from being cast to the wrong type. To resolve this, remove the non-EVP_PKEY_sign codepath. This codepath is no longer necessary. PR nodejs#11705's verify half was already assuming all EVP_PKEYs supported EVP_PKEY_sign and EVP_PKEY_verify. Also, in the documentation, point users towards using hash function names which are more consisent. This avoids an ECDSA special-case and some strangeness around RSA-PSS ("RSA-SHA256" is the OpenSSL name of the sha256WithRSAEncryption OID which is not used for RSA-PSS). PR-URL: nodejs#15024 Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR #11705 switched Node away from using using OpenSSL's legacy EVP_Sign* and EVP_Verify* APIs. Instead, it computes a hash normally via EVP_Digest* and then uses EVP_PKEY_sign and EVP_PKEY_verify to verify the hash directly. This change corrects two problems: 1. The documentation still recommends the signature algorithm EVP_MD names of OpenSSL's legacy APIs. OpenSSL has since moved away from thosee, which is why ECDSA was strangely inconsistent. (This is why "ecdsa-with-SHA256" was missing.) 2. Node_SignFinal copied some code from EVP_SignFinal's internals. This is problematic for OpenSSL 1.1.0 and is missing a critical check that prevents pkey->pkey.ptr from being cast to the wrong type. To resolve this, remove the non-EVP_PKEY_sign codepath. This codepath is no longer necessary. PR #11705's verify half was already assuming all EVP_PKEYs supported EVP_PKEY_sign and EVP_PKEY_verify. Also, in the documentation, point users towards using hash function names which are more consisent. This avoids an ECDSA special-case and some strangeness around RSA-PSS ("RSA-SHA256" is the OpenSSL name of the sha256WithRSAEncryption OID which is not used for RSA-PSS). PR-URL: #15024 Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Ref: auth0#8 Ref: auth0#23 Ref: nodejs/node#15024

Fixes: auth0#8 Ref: auth0#23 Ref: nodejs/node#15024 PR-URL: auth0#26

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 Aug 24, 2017

shigeki suggested changes Aug 26, 2017

View reviewed changes

shigeki approved these changes Sep 1, 2017

View reviewed changes

BridgeAR requested changes Sep 8, 2017

View reviewed changes

davidben force-pushed the fix-sign branch 2 times, most recently from 3fe7a38 to 1e97015 Compare September 9, 2017 22:42

davidben force-pushed the fix-sign branch from 1e97015 to 1f57491 Compare September 9, 2017 22:43

BridgeAR approved these changes Sep 11, 2017

View reviewed changes

BridgeAR closed this Sep 11, 2017

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

MylesBorins added the land-on-v6.x label Oct 17, 2017

MylesBorins mentioned this pull request Oct 17, 2017

v6.12.0 proposal #16263

Merged

MylesBorins mentioned this pull request Nov 3, 2017

v4.8.6 proposal #16500

Merged

omsmith mentioned this pull request May 23, 2018

Using RS512 algorithm with EC512 key-pair works auth0/node-jwa#23

Open

omsmith added a commit to omsmith/node-jwa that referenced this pull request May 23, 2018

use genericized names for openssl signed digests

88068c1

Ref: auth0#8 Ref: auth0#23 Ref: nodejs/node#15024

omsmith mentioned this pull request May 23, 2018

use genericized names for openssl signed digests auth0/node-jwa#26

Merged

omsmith added a commit to omsmith/node-jwa that referenced this pull request May 23, 2018

use genericized names for openssl signed digests

0bdf973

Fixes: auth0#8 Ref: auth0#23 Ref: nodejs/node#15024 PR-URL: auth0#26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: fix Node_SignFinal #15024

crypto: fix Node_SignFinal #15024

davidben commented Aug 24, 2017

jasnell commented Aug 24, 2017

shigeki Aug 26, 2017

davidben Aug 30, 2017 •

edited

Loading

shigeki Sep 1, 2017

shigeki Aug 26, 2017

davidben Aug 30, 2017

shigeki Sep 1, 2017

shigeki commented Aug 26, 2017

davidben commented Aug 30, 2017

davidben commented Aug 30, 2017

shigeki commented Sep 1, 2017

BridgeAR commented Sep 2, 2017

davidben commented Sep 2, 2017

BridgeAR commented Sep 2, 2017

davidben commented Sep 2, 2017

BridgeAR commented Sep 3, 2017

shigeki commented Sep 5, 2017

davidben commented Sep 5, 2017

BridgeAR left a comment

davidben commented Sep 9, 2017

BridgeAR commented Sep 11, 2017

BridgeAR commented Sep 11, 2017

BridgeAR commented Sep 11, 2017

		@@ -937,7 +938,7 @@ Example: Using the [`sign.update()`][] and [`sign.sign()`][] methods:

crypto: fix Node_SignFinal #15024

crypto: fix Node_SignFinal #15024

Conversation

davidben commented Aug 24, 2017

Checklist

Affected core subsystem(s)

jasnell commented Aug 24, 2017

shigeki Aug 26, 2017

Choose a reason for hiding this comment

davidben Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

shigeki Sep 1, 2017

Choose a reason for hiding this comment

shigeki Aug 26, 2017

Choose a reason for hiding this comment

davidben Aug 30, 2017

Choose a reason for hiding this comment

shigeki Sep 1, 2017

Choose a reason for hiding this comment

shigeki commented Aug 26, 2017

davidben commented Aug 30, 2017

davidben commented Aug 30, 2017

shigeki commented Sep 1, 2017

BridgeAR commented Sep 2, 2017

davidben commented Sep 2, 2017

BridgeAR commented Sep 2, 2017

davidben commented Sep 2, 2017

BridgeAR commented Sep 3, 2017

shigeki commented Sep 5, 2017

davidben commented Sep 5, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

davidben commented Sep 9, 2017

BridgeAR commented Sep 11, 2017

BridgeAR commented Sep 11, 2017

BridgeAR commented Sep 11, 2017

davidben Aug 30, 2017 •

edited

Loading