crypto: fix Node_SignFinal

[davidben]

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]

ping @nodejs/crypto

[shigeki]

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.

[davidben]

Wrote something to that effect.

[shigeki]

It looks good to me.

[shigeki]

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?

[davidben]

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.

[shigeki]

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]

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]

(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]

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

Done.

[shigeki]

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]

This looks like being semver-major to me?

[davidben]

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]

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]

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]

Top! Thanks for the clarification for me @davidben

[shigeki]

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]

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]

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

[BridgeAR]

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

[BridgeAR]

@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]

Landed in 6ebdb69