Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: add sign/verify support for RSASSA-PSS #11705

Closed
wants to merge 17 commits into from

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Mar 6, 2017

Adds support for the PSS padding scheme. Until now, the sign/verify functions used the old EVP_Sign*/EVP_Verify* OpenSSL API, making it impossible to change the padding scheme. Fixed by first computing the message digest and then signing/verifying with a custom EVP_PKEY_CTX, allowing us to specify options such as the padding scheme and the PSS salt length.

Fixes: #1127

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto, src, doc, test

@nodejs-github-bot 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 Mar 6, 2017
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up!

/cc @nodejs/crypto

The optional `options` argument is an object which specifies additional
cryptographic parameters. Currently, the following options are supported:

* `padding` : {String} - RSA padding, either `'pkcs1'` for RSASSA-PKCS1-v1_5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use lowercase string here and drop the space before the :?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but note that the rest of the document seems to use the uppercase variant and has a space before the colon.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tniessen Thanks for pointing that out – we’re moving to consistenly using lowercase in #11697, so it would be good if this PR was in line with that. I’ll comment that I’d prefer to have the space before the colon dropped there, too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that an options object is introduced, it seems the current positional parameter, output_format, should be possible to express as an option. encoding would be the more common name for it, I think.

(default) or `'pss'` for RSASSA-PSS
* `saltLength` : {number} - salt length for RSASSA-PSS. If this is set to `-1`,
the salt length will be set to the digest size. If this is set to `-2`
(default), the salt length will be set to the maximum permissible value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice to have these two values exposed as named constants on crypto.constants… what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, will do so! What is the best way to define a constant which is not #defined? I have only seen people using NODE_DEFINE_CONSTANT with existing defs. Should I just #define RSA_PKCS1_PSS_MAX_SALT_LENGTH -2 and then use NODE_DEFINE_CONSTANT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I just #define RSA_PKCS1_PSS_MAX_SALT_LENGTH -2 and then use NODE_DEFINE_CONSTANT?

I don’t see any problem with that. :) It’s quite possible that somebody else here comes up with a better suggestion, but for the time being, I’d recommend just doing it the way you just described.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest OpenSSL (1.1.1-dev) changes its definitions as digest, max and auto(only in verification). https://github.com/openssl/openssl/blob/master/doc/man1/pkeyutl.pod#rsa-algorithm
I guess that auto can be implemented even in openssl-1.0.2. I think it is better to follow it.

Copy link
Contributor

@sam-github sam-github Mar 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

named constants would be better, or perhaps strings ('digest', 'max', and 'auto'), or perhaps both (named constants with the values being strings, not magic negative numbers)

int SignBase::GetRSAOptions(Environment *env, v8::Local<v8::Object> options,
int *padding, int *saltlen) {
Local<Value> key = String::NewFromUtf8(env->isolate(), "padding");
MaybeLocal<Value> maybePadding = options->Get(key);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to add this to the PER_ISOLATE_STRING_PROPERTIES table in env.h and then simply use options->Get(env->padding_string()) instead

int *padding, int *saltlen) {
Local<Value> key = String::NewFromUtf8(env->isolate(), "padding");
MaybeLocal<Value> maybePadding = options->Get(key);
if (!maybePadding.IsEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If maybePadding is empty you can just return 0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering that I return 0 after throwing an exception (so 0 <=> error), I should probably return 1 here as it should be optional to specify the padding. Apart from that, I agree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering that I return 0 after throwing an exception (so 0 <=> error), I should probably return 1 here as it should be optional to specify the padding.

In the case where maybePadding is empty you can safely assume that an exception is pending (e.g. if the getter for "padding" threw an error – which is obviously mostly hypothetical, but since it’s an object passed in from userland, it’s possible). So, returning 0 and returning back to JS as quickly as possible should be the right thing to do.

If the property is missing, you’ll get an Undefined value, not an empty MaybeLocal.

Copy link
Member Author

@tniessen tniessen Mar 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the property is missing, you’ll get an Undefined value, not an empty MaybeLocal.

Oh, that makes it clear. I did not know that, sorry!

Local<Value> paddingValue = maybePadding.ToLocalChecked();
if (paddingValue->IsString()) {
v8::String::Utf8Value paddingUtf8(paddingValue);
const char *paddingName = *paddingUtf8;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: const char* (I know we’re not being consistent in the code base 😄)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually… crypto.constants.RSA_PKCS1_PSS_PADDING and crypto.constants.RSA_PKCS1_PADDING already exist and are accessible from userland. Maybe we could use these constants instead of a string?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, must have missed those when I skimmed through the constants. I just noticed that the asymmetric encrypt/decrypt API allows to specify padding as part of the first argument (where key, passphrase and padding are properties of the object). Should I remove the additional options object and add the properties to the first argument for sign() / verify() as well in order to keep it consistent?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sam-github @addaleax @bnoordhuis Thoughts on this? I have no problem keeping it as it is, with options as an additional parameter, but you might prefer to keep it consistent with publicEncrypt etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should be like publicEncrypt, etc, with .padding part of the first argument.

if (*padding == RSA_PKCS1_PSS_PADDING) {
key = String::NewFromUtf8(env->isolate(), "saltLength");
MaybeLocal<Value> maybeSaltlen = options->Get(key);
if (!maybeSaltlen.IsEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto, if (maybeSaltlen.IsEmpty()) return 0;

else if (strcmp(paddingName, "pss") == 0) {
*padding = RSA_PKCS1_PSS_PADDING;
}
else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: } else { on one line

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

src/node_crypto.cc:3971:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
src/node_crypto.cc:3971:  If an else has a brace on one side, it should have it on both  [readability/braces] [5]

goto err;

if (mdctx->digest->flags & EVP_MD_FLAG_PKEY_METHOD_SIGNATURE) {
size_t sltmp = (size_t)EVP_PKEY_size(pkey);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: static_cast<size_t> instead of (size_t) (assuming that works here)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes s cross initialization error during build. Move its definition before goto.

../src/node_crypto.cc: In function ‘int node::crypto::Node_SignFinal(EVP_MD_CTX*, unsigned char*, unsigned int*, EVP_PKEY*, int, int)’:
../src/node_crypto.cc:4109:4: error: jump to label ‘err’ [-fpermissive]
    err:
    ^
../src/node_crypto.cc:4086:10: note:   from here
     goto err;
          ^
../src/node_crypto.cc:4089:12: note:   crosses initialization of ‘size_t sltmp’
     size_t sltmp = (size_t)EVP_PKEY_size(pkey);
            ^

goto err;
if (padding == RSA_PKCS1_PSS_PADDING)
if (EVP_PKEY_CTX_set_rsa_pss_saltlen(pkctx, saltlen) <= 0)
goto err;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: can you add {} if the body of an if spans more than a single line?

@@ -4357,8 +4477,15 @@ void Verify::VerifyFinal(const FunctionCallbackInfo<Value>& args) {
hbuf = Buffer::Data(args[1]);
}

int padding = RSA_PKCS1_PADDING;
int saltlen = -2;
if (args.Length() >= 4 && args[3]->IsObject()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw you don’t need the args.Length() part of the check, if args[3] is out of bounds it will just be undefined. I don’t mind keeping it if you think that helps with readability, though.

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 6, 2017
int padding = RSA_PKCS1_PADDING;
int saltlen = -2;
if (args.Length() >= 4 && args[3]->IsObject()) {
verify->GetRSAOptions(env, args[3]->ToObject(), &padding, &saltlen);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to comment … can you check the return value here and return from the function if there was an error?

lib/crypto.js Outdated
var key = options.key || options;
var passphrase = options.passphrase || null;
var ret = this._handle.sign(toBuf(key), null, passphrase);
if (typeof encoding == 'object' && !options) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think === is better than ==.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it does not make a difference when using typeof, but I can surely change it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, doesn't make a difference, but node style is currently to always use ===, unless using == is necessary (mostly for comparisons against unsigned and null in a single operation)

lib/crypto.js Outdated
Verify.prototype.verify = function verify(object, signature, sigEncoding) {
Verify.prototype.verify = function verify(object, signature, sigEncoding,
options) {
if (typeof sigEncoding == 'object' && !options) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

goto err;

if (mdctx->digest->flags & EVP_MD_FLAG_PKEY_METHOD_SIGNATURE) {
size_t sltmp = (size_t)EVP_PKEY_size(pkey);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes s cross initialization error during build. Move its definition before goto.

../src/node_crypto.cc: In function ‘int node::crypto::Node_SignFinal(EVP_MD_CTX*, unsigned char*, unsigned int*, EVP_PKEY*, int, int)’:
../src/node_crypto.cc:4109:4: error: jump to label ‘err’ [-fpermissive]
    err:
    ^
../src/node_crypto.cc:4086:10: note:   from here
     goto err;
          ^
../src/node_crypto.cc:4089:12: note:   crosses initialization of ‘size_t sltmp’
     size_t sltmp = (size_t)EVP_PKEY_size(pkey);
            ^

if (strcmp(paddingName, "pkcs1") == 0) {
*padding = RSA_PKCS1_PADDING;
}
else if (strcmp(paddingName, "pss") == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint error.

src/node_crypto.cc:3968:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
src/node_crypto.cc:3968:  If an else has a brace on one side, it should have it on both  [readability/braces] [5]

else if (strcmp(paddingName, "pss") == 0) {
*padding = RSA_PKCS1_PSS_PADDING;
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

src/node_crypto.cc:3971:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
src/node_crypto.cc:3971:  If an else has a brace on one side, it should have it on both  [readability/braces] [5]

(default) or `'pss'` for RSASSA-PSS
* `saltLength` : {number} - salt length for RSASSA-PSS. If this is set to `-1`,
the salt length will be set to the digest size. If this is set to `-2`
(default), the salt length will be set to the maximum permissible value.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest OpenSSL (1.1.1-dev) changes its definitions as digest, max and auto(only in verification). https://github.com/openssl/openssl/blob/master/doc/man1/pkeyutl.pod#rsa-algorithm
I guess that auto can be implemented even in openssl-1.0.2. I think it is better to follow it.

* `saltLength` : {number} - salt length for RSASSA-PSS. If this is set to `-1`,
the salt length will be set to the digest size. If this is set to `-2`
(default), the salt length will be set to the maximum permissible value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no descriptions for mgf1 and its hash. The default behavior is that the mgf1 hash is the same as the sign/verify hash. I think it needs to not to add an additional option parameter for mgf1 but its spec should be described in the doc.

});
});
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are test vectors for RSA-PSS in the link of https://www.emc.com/emc-plus/rsa-labs/standards-initiatives/pkcs-rsa-cryptography-standard.htm . We can make verify tests by using this test vectors.
If you cannot get the test vector file, it is in https://github.com/shigeki/ohtsu_rsa_pss_js/blob/master/tests/vectors/pss-vect.txt. Please refer related test vectors of RSA-PSS(example1 for RSA1024bits, and example10 for RSA2048bits) in JavaScript are in https://github.com/shigeki/ohtsu_rsa_pss_js/tree/master/tests/ .

Unfortunately, openssl cannot accept explicit salt. So we can make sign tests for RSA-PSS by writing signature data to files and verifying its signature by using openssl dgst command with -signopt rsa_padding_mode:pss.

@@ -1039,6 +1048,16 @@ the `signature_format` which can be `'latin1'`, `'hex'` or `'base64'`.
If a `signature_format` is specified, the `signature` is expected to be a
string; otherwise `signature` is expected to be a [`Buffer`][].

The optional `options` argument is an object which specifies additional
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as for sign, signature_format should be possible to pass as encoding in the options

unrelated, but

If a signature_format is specified, the signature is expected to be a
string; otherwise signature is expected to be a [Buffer][].

is a bizarre restriction, I wonder if it is even accurate? I will investigate.

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs more tests for the various argument pemutations (with options, without options, options in the various argument positions)

lib/crypto.js Outdated
var key = options.key || options;
var passphrase = options.passphrase || null;
var ret = this._handle.sign(toBuf(key), null, passphrase);
if (typeof encoding == 'object' && !options) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if typeof encoding == object, && there are also options, that should be an error

lib/crypto.js Outdated
}

var key = privateKey.key || privateKey;
var passphrase = privateKey.passphrase || null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about when privateKey is passed in options? It would not make sense that .passPhrase is supported when the options object is first argument, but not supported when it is second or third argument

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still relevant when changing the options object to be the first argument, such that the first argument is either the privateKey itself or an options object with properties key and optionally passphrase, padding and/or saltLength?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it would not be, if there is only one options object, there can be no inconsistency between the options allowed in the various options objects

lib/crypto.js Outdated
var key = options.key || options;
var passphrase = options.passphrase || null;
var ret = this._handle.sign(toBuf(key), null, passphrase);
if (typeof encoding == 'object' && !options) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, doesn't make a difference, but node style is currently to always use ===, unless using == is necessary (mostly for comparisons against unsigned and null in a single operation)

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code mostly looks good to me, I’ll just refrain from approving the PR because I don’t understand the subject matter well enough

* `passphrase`: {string} - passphrase for the private key
* `padding`: {String} - RSA padding, either `RSA_PKCS1_PADDING` (default) or
`RSA_PKCS1_PSS_PADDING`
* `saltLength`: {number} - salt length for RSASSA-PSS. The special value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

padding and saltLength should both be integer, right? (Or at least number. But integer seems to be what’s coming out of the discussion @ #11697)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is weird, I thought I had already fixed the documentation regarding the use of integer constants. Seems like I messed that up when committing. Fixed in the latest commit :)

* `key`: {string} - PEM encoded private key (required)
* `padding`: {string} - RSA padding, either `'pkcs1'` for RSASSA-PKCS1-v1_5
(default) or `'pss'` for RSASSA-PSS
* `saltLength`: {number} - salt length for RSASSA-PSS. If this is set to `-1`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

* `padding`: {string} - RSA padding, either `'pkcs1'` for RSASSA-PKCS1-v1_5
(default) or `'pss'` for RSASSA-PSS
* `saltLength`: {number} - salt length for RSASSA-PSS. If this is set to `-1`,
the salt length will be set to the digest size. A value of `-2` (default)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use the constant name instead of -2 for specifying the default? Ideally, users won’t have to care what particular value the constant has

*padding = paddingValue->Int32Value();
break;
default:
env->ThrowError("Padding must be RSA_PKCS1_PADDING or "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: lowercase padding (or generally whatever it corresponds to in the documentation)


#ifndef RSA_PSS_SALTLEN_AUTO
#define RSA_PSS_SALTLEN_AUTO -2
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if you could #define these constants in a header and then include it in the source files that rely on the specific value. node_constants.h probably works for that, or maybe node_crypto.h.

EVP_PKEY_CTX *pkctx = nullptr;

*s = 0;
if (!EVP_DigestFinal_ex(mdctx, &(m[0]), &m_len))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d personally prefer just m instead of &(m[0]) here

@tniessen
Copy link
Member Author

tniessen commented Mar 8, 2017

@addaleax I noticed, thought I had changed both occurrences to ::Cast before the first commit. You are right, ::Cast is what I wanted.

@@ -4132,6 +4212,14 @@ void Sign::SignFinal(const FunctionCallbackInfo<Value>& args) {
size_t buf_len = Buffer::Length(args[0]);
char* buf = Buffer::Data(args[0]);

int padding = RSA_PKCS1_PADDING;
int saltlen = -2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh, sorry if I wasn’t clear, but my idea was that you could add #include "node_constants.h" at the top of this file and then use the constants here where you currently use the magic numbers directly

* `passphrase`: {string} - passphrase for the private key
* `padding`: {integer} - RSA padding, either `RSA_PKCS1_PADDING` (default) or
`RSA_PKCS1_PSS_PADDING`
* `saltLength`: {integer} - salt length for RSASSA-PSS. The special value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace RSASSA-PSS with "when padding is RSA_PKCS1_PSS_PADDING"

one an RSA public key, a DSA public key, or an X.509 certificate.
The `object` argument can be either a string containing a PEM encoded object,
which can be one an RSA public key, a DSA public key, or an X.509 certificate,
or an object with some of the following properties:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the way the first argument is described here much better than how it is described for sign. It would be nice to align them, though the inconsistency is pre-existing, and if you don't have the time that's OK.

* `key`: {string} - PEM encoded private key (required)
* `padding`: {integer} - RSA padding, either `RSA_PKCS1_PADDING` (default) or
`RSA_PKCS1_PSS_PADDING`
* `saltLength`: {integer} - salt length for RSASSA-PSS. The special value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"for when padding is RSA_PKCS1_PSS_PADDING" - because people may not know the relationship between RSA_PKCS1_PSS_PADDING and RSASSA-PSS

@@ -1966,6 +1981,18 @@ the `crypto`, `tls`, and `https` modules and are generally specific to OpenSSL.
<td></td>
</tr>
<tr>
<td><code>RSA_PSS_SALTLEN_DIGEST</code></td>
<td></td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a little bit of docs are useful here, because the constants are asymetrical. DIGEST can be used with sign and verify, MAX_SIGN only with sign, and _AUTO only with verify. It surprised me when I saw the last two constants had the same value, and I had to go back to the docs and compare the sign and verify docs to realize that some of the PSS constants were method specific, and some weren't.

Local<Value> paddingValue = maybePadding.ToLocalChecked();
if (paddingValue->IsNumber()) {
switch (paddingValue->Int32Value()) {
case RSA_PKCS1_PADDING:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node is a bit irregular on this, but every case statement in node_crypto.cc is indented 2 spaces more than the switch, best to be consistent

unsigned int *sig_len) {
unsigned int *sig_len,
int padding,
int saltlen) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sig_len, therefor salt_len

if (EVP_PKEY_CTX_set_signature_md(pkctx, mdctx_.digest) <= 0)
goto err;
r = EVP_PKEY_verify(pkctx,
reinterpret_cast<const unsigned char*>(sig),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node_crypto.cc is a bit schismatic, but style is either to have all the args aligned four spaces from start of line, or to align
them with the column after the (

bool verify_result;
Error err = verify->VerifyFinal(kbuf, klen, hbuf, hlen, &verify_result);
Error err = verify->VerifyFinal(kbuf, klen, hbuf, hlen, padding, saltlen,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verify_result, therefor salt_len

@@ -556,6 +556,8 @@ class SignBase : public BaseObject {

protected:
void CheckThrow(Error error);
int GetRSAOptions(Environment *env, v8::Local<v8::Object> options,
int *padding, int *saltlen);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

salt_len

@@ -71,6 +71,54 @@ const keyPem = fs.readFileSync(common.fixturesDir + '/test_key.pem', 'ascii');
assert.strictEqual(verified, true, 'sign and verify (stream)');
}

{
[ 'RSA-SHA1', 'RSA-SHA256' ].forEach((algo) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

['RSA...SHA256'], and below

@sam-github
Copy link
Contributor

coming along well, I like the new method calling convention

@addaleax
Copy link
Member

addaleax commented Mar 9, 2017

@tniessen Looks like this needs to be rebased against master, can you do that? I’d like to start CI for this but I don’t think that works well if there are merge conflicts

@tniessen
Copy link
Member Author

tniessen commented Mar 9, 2017

@addaleax That should do it, hope I did not mess anything up.

@addaleax
Copy link
Member

addaleax commented Mar 9, 2017

@addaleax
Copy link
Member

addaleax commented Mar 9, 2017

CI is green except for a weird OS X failure @ https://ci.nodejs.org/job/node-test-commit-osx/8287/nodes=osx1010/console @nodejs/build

Another attempt to be sure it’s unrelated: https://ci.nodejs.org/job/node-test-commit/8353/

@@ -935,10 +935,16 @@ Calculates the signature on all the data passed through using either

The `private_key` argument can be an object or a string. If `private_key` is a
string, it is treated as a raw key with no passphrase. If `private_key` is an
object, it is interpreted as a hash containing two properties:
object, it is interpreted as a hash containing some of these properties:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"one or more of the following" sounds a bit better to my ears.

Tangential aside: 'hash' is a somewhat unfortunate choice of words here, seeing how we describe hash functions a few lines up.

* `saltLength`: {integer} - salt length for when padding is
`RSA_PKCS1_PSS_PADDING`. The special value `RSA_PSS_SALTLEN_DIGEST` sets the
salt length to the digest size, `RSA_PSS_SALTLEN_MAX_SIGN` (default) sets it
to the maximum permissible value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be clearer on the fact that you obtain the constants from crypto.constants. Likewise verifier.update().

@@ -3952,6 +3954,37 @@ void SignBase::CheckThrow(SignBase::Error error) {
}
}

int SignBase::GetRSAOptions(Environment* env, v8::Local<v8::Object> options,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the return type to bool. Better yet, I'd do the validation in lib/crypto.js, not here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But doesn't that imply passing the padding etc. from JavaScript to the binding as additional arguments instead of an options object? I considered that as well, but assuming that RSASSA-PSS is not the last addition to sign()/verify(), it decided it could become pretty clumsy to pass all options as positional parameters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Positional arguments are easier and more efficient than wrangling objects in C++ land.

As well, it would get rid of the ThrowError(). Validation code that throws JS exceptions as a side effect tends to be pretty brittle. Callers need to be very careful not to re-enter the VM on return but that is easy to get wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis fwiw, I’m good with the current approach. It’s readable (imho) and microperformance probably isn’t too relevant when compared against the cost of the crypto operations itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis I can fix this by adding more positional arguments if you think that is the reasonable way. I do not know enough about node internals to fully evaluate the effects. I guess I should move default values to JS as well, such that the native code does not need to handle undefined values.

I am still a little concerned considering that it might be necessary to add more options in the future and all of these are optional, meaning that we might end up passing useless arguments most of the time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I should move default values to JS as well, such that the native code does not need to handle undefined values.

Yes, that's a good idea.

it might be necessary to add more options in the future and all of these are optional, meaning that we might end up passing useless arguments most of the time.

Efficiency-wise a few extra parameters won't break the bank (they're just stack slots) and like Anna says, the cost of crypto itself probably dwarfs the overhead.

Copy link
Member Author

@tniessen tniessen Mar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I should move default values to JS as well, such that the native code does not need to handle undefined values.

Yes, that's a good idea.

But what if args[3]->IsInt32() etc. returns false/fails? Should I just skip these tests, throw an exception, return silently...? (Or just keep "additional" default values in the native code).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can CHECK(args[3]->IsInt32()), it's a bug when it asserts.

@@ -3952,6 +3954,37 @@ void SignBase::CheckThrow(SignBase::Error error) {
}
}

int SignBase::GetRSAOptions(Environment* env, v8::Local<v8::Object> options,
int* padding, int* saltlen) {
MaybeLocal<Value> maybePadding = options->Get(env->padding_string());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: use snake_case (i.e., maybe_padding) for locals.

Substance: can you use options->Get(env->context(), env->padding_string()) here? You're currently using the overload that returns a Local<Value> which is then implicitly converted to MaybeLocal<Value>.


Local<Value> paddingValue = maybePadding.ToLocalChecked();
if (paddingValue->IsNumber()) {
*padding = paddingValue->Int32Value();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking IsNumber() then coercing to int32_t is debatable, not every number is an int32_t.

Can you use the overload that takes a v8::Context?

if (mdctx->digest->flags & EVP_MD_FLAG_PKEY_METHOD_SIGNATURE) {
size_t sltmp = static_cast<size_t>(EVP_PKEY_size(pkey));
pkctx = EVP_PKEY_CTX_new(pkey, nullptr);
if (!pkctx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use pkctx == nullptr.

return rv;
}

if (!mdctx->digest->sign) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use mdctx->digest->sign == nullptr.

}

return (mdctx->digest->sign(mdctx->digest->type, m, m_len, md, sig_len,
pkey->pkey.ptr));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can drop the extra parens and please line up the arguments.

int padding = RSA_PKCS1_PADDING;
int salt_len = RSA_PSS_SALTLEN_MAX_SIGN;
if (args[3]->IsObject()) {
Local<Object> options = Local<Object>::Cast(args[3]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use args[3].As<Object>() here, that's what we use most in core.

if (EVP_PKEY_CTX_set_rsa_pss_saltlen(pkctx, saltlen) <= 0)
goto err;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably be shared with the same logic in Node_SignFinal() with some more effort.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I could add a static function just to check whether it is RSA and set the two parameters if it is. Is that what you suggest?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving it into a separate function is what I had in mind, yes.

@addaleax
Copy link
Member

@tniessen
Copy link
Member Author

@bnoordhuis I think I addressed all requested changes with the last commits.

@addaleax I do not really understand the build failures. Let's see what failed in build #8393:

node-test-binary-arm/6624:

warning: failed to remove out/Release/.nfs00000000002419ff00000223
Build step 'Execute shell' marked build as failure

node-test-commit-linux/8401

not ok 1266 parallel/test-writeuint
  ---
  duration_ms: 0.115
  severity: fail
  stack: |-
    module.js:487
        throw err;
        ^
    
    Error: Cannot find module '/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1404-32/test/parallel/test-writeuint.js'
        at Function.Module._resolveFilename (module.js:485:15)
        at Function.Module._load (module.js:437:25)
        at Module.runMain (module.js:620:10)
        at run (bootstrap_node.js:425:7)
        at startup (bootstrap_node.js:146:9)
        at bootstrap_node.js:540:3
  ...
make[1]: *** [test-ci] Error 1
make[1]: Leaving directory `/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1404-32'
make: *** [run-ci] Error 2
Build step 'Execute shell' marked build as failure

node-compile-windows/7507 (VS2015)

c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src/counters.h(825): fatal error C1060: compiler is out of heap space (compiling source file compiler\js-intrinsic-lowering.cc) [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]
c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src/utils.h(307): fatal error C1060: compiler is out of heap space (compiling source file compiler\js-inlining.cc) [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\xutility(299): fatal error C1060: compiler is out of heap space (compiling source file compiler\js-global-object-specialization.cc) [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]

And later:

cl : Command line error D8040: error creating or communicating with child process [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_3.vcxproj]
  Internal Compiler Error in C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\amd64\CL.exe.  You will be prompted to send an error report to Microsoft later.
  INTERNAL COMPILER ERROR in 'C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\amd64\CL.exe'
      Please choose the Technical Support command on the Visual C++
      Help menu, or open the Technical Support help file for more information

node-compile-windows/7507 (VCBT2015)

 > git fetch --tags --progress git@github.com:janeasystems/node_binary_tmp.git +refs/heads/jenkins-node-test-commit-windows-fanned-7601:refs/remotes/jenkins_tmp/_jenkins_local_branch # timeout=20
ERROR: Error fetching remote repo 'jenkins_tmp'
hudson.plugins.git.GitException: Failed to fetch from git@github.com:janeasystems/node_binary_tmp.git
	at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:806)
	at hudson.plugins.git.GitSCM.retrieveChanges(GitSCM.java:1070)
	at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1101)
	at hudson.scm.SCM.checkout(SCM.java:495)
	at hudson.model.AbstractProject.checkout(AbstractProject.java:1278)
	at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:604)
	at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:529)
	at hudson.model.Run.execute(Run.java:1728)
	at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
	at hudson.model.ResourceController.execute(ResourceController.java:98)
	at hudson.model.Executor.run(Executor.java:404)
Caused by: hudson.plugins.git.GitException: Command "git fetch --tags --progress git@github.com:janeasystems/node_binary_tmp.git +refs/heads/jenkins-node-test-commit-windows-fanned-7601:refs/remotes/jenkins_tmp/_jenkins_local_branch" returned status code 128:
stdout: 
stderr: remote: Counting objects: 1173, done.        
remote: Compressing objects:   1% (1/78)           
...
remote: Compressing objects: 100% (78/78), done.        
Receiving objects:   0% (1/1173)   
Receiving objects:   1% (12/1173)   
fatal: mmap failed: No error
fatal: index-pack failed

	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1793)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandWithCredentials(CliGitAPIImpl.java:1519)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.access$300(CliGitAPIImpl.java:64)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$1.execute(CliGitAPIImpl.java:315)
	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler$1.call(RemoteGitImpl.java:153)
	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler$1.call(RemoteGitImpl.java:146)
	at hudson.remoting.UserRequest.perform(UserRequest.java:153)
	at hudson.remoting.UserRequest.perform(UserRequest.java:50)
	at hudson.remoting.Request$2.run(Request.java:336)
	at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68)
	at java.util.concurrent.FutureTask.run(Unknown Source)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
	at hudson.remoting.Engine$1$1.run(Engine.java:94)
	at java.lang.Thread.run(Unknown Source)
	at ......remote call to Channel to /162.242.156.145(Native Method)
	at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1537)
	at hudson.remoting.UserResponse.retrieve(UserRequest.java:253)
	at hudson.remoting.Channel.call(Channel.java:822)
	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler.execute(RemoteGitImpl.java:146)
	at sun.reflect.GeneratedMethodAccessor586.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler.invoke(RemoteGitImpl.java:132)
	at com.sun.proxy.$Proxy86.execute(Unknown Source)
	at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:804)
	... 11 more
ERROR: null

I do not see any relation to my changes. Maybe some problems with the build system?

@addaleax
Copy link
Member

@tniessen Yeah, all of these look like infrastructure-related problems.

CI: https://ci.nodejs.org/job/node-test-commit/8440/

@@ -3994,6 +3994,19 @@ bool SignBase::GetRSAOptions(Environment* env, v8::Local<v8::Object> options,
return true;
}

static bool ApplyRSAOptions(EVP_PKEY* pkey, EVP_PKEY_CTX* pkctx, int padding, int salt_len) {
Copy link
Member

@bnoordhuis bnoordhuis Mar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long line. Can you run make test?

EDIT: Comment crossed with a follow-up commit, disregard.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fixed in the last commit, sorry. The problem is that I cannot perform linting on Windows and cannot run the tests on Linux, so I have to push from my Windows machine after running the tests before linting it on my Linux machine.

@bnoordhuis
Copy link
Member

@tniessen Thoughts on #11705 (comment)?

@tniessen
Copy link
Member Author

@addaleax Looks better, I just don't get why GitHub thinks test/arm failed. According to Jenkins, the build was successful and all tests passed. Am I missing something?

@bnoordhuis
Copy link
Member

I just don't get why GitHub thinks test/arm failed

It's a known issue with the CI. The reporter doesn't always report the status correctly for some reason.

@gibfahn
Copy link
Member

gibfahn commented Jul 18, 2017

As this is semver-minor, it'll need to be approved in an LTS meeting before being backported. @tniessen you're welcome to raise the PR anyway, but there's always the option that @nodejs/lts will choose to punt on it for this release (and leave it to bake a while longer).

@davidben davidben mentioned this pull request Aug 24, 2017
3 tasks
davidben added a commit to davidben/node that referenced this pull request Sep 9, 2017
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 pushed a commit to BridgeAR/node that referenced this pull request Sep 11, 2017
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>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
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>
jasnell pushed a commit that referenced this pull request Sep 20, 2017
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>
tniessen added a commit to tniessen/node that referenced this pull request Sep 20, 2017
Adds support for the PSS padding scheme. Until now, the sign/verify
functions used the old EVP_Sign*/EVP_Verify* OpenSSL API, making it
impossible to change the padding scheme. Fixed by first computing the
message digest and then signing/verifying with a custom EVP_PKEY_CTX,
allowing us to specify options such as the padding scheme and the PSS
salt length.

Fixes: nodejs#1127
PR-URL: nodejs#11705
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2017
Adds support for the PSS padding scheme. Until now, the sign/verify
functions used the old EVP_Sign*/EVP_Verify* OpenSSL API, making it
impossible to change the padding scheme. Fixed by first computing the
message digest and then signing/verifying with a custom EVP_PKEY_CTX,
allowing us to specify options such as the padding scheme and the PSS
salt length.

Fixes: #1127
PR-URL: #11705
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Oct 17, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
Adds support for the PSS padding scheme. Until now, the sign/verify
functions used the old EVP_Sign*/EVP_Verify* OpenSSL API, making it
impossible to change the padding scheme. Fixed by first computing the
message digest and then signing/verifying with a custom EVP_PKEY_CTX,
allowing us to specify options such as the padding scheme and the PSS
salt length.

Fixes: #1127
PR-URL: #11705
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
MylesBorins added a commit that referenced this pull request Nov 6, 2017
Notable Changes:

* assert:
  - assert.fail() can now take one or two arguments (Rich Trott)
    #12293
* crypto:
  - add sign/verify support for RSASSA-PSS (Tobias Nießen)
    #11705
* deps:
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu)
    #16691
  - upgrade libuv to 1.15.0 (cjihrig)
    #15745
  - upgrade libuv to 1.14.1 (cjihrig)
    #14866
  - upgrade libuv to 1.13.1 (cjihrig)
    #14117
  - upgrade libuv to 1.12.0 (cjihrig)
    #13306
* fs:
  - Add support for fs.write/fs.writeSync(fd, buffer, cb) and
    fs.write/fs.writeSync(fd, buffer, offset, cb) as documented
    (Andreas Lind) #7856
* inspector:
  - enable --inspect-brk (Refael Ackermann)
    #12615
* process:
  - add --redirect-warnings command line argument (James M Snell)
    #10116
* src:
  - allow CLI args in env with NODE_OPTIONS (Sam Roberts)
    #12028)
  - --abort-on-uncaught-exception in NODE_OPTIONS (Sam Roberts)
    #13932
  - allow --tls-cipher-list in NODE_OPTIONS (Sam Roberts)
    #13172
  - use SafeGetenv() for NODE_REDIRECT_WARNINGS (Sam Roberts)
    #12677
* test:
  - remove common.fail() (Rich Trott)
    #12293

PR-URL: #16263
MylesBorins added a commit that referenced this pull request Nov 7, 2017
Notable Changes:

* assert:
  - assert.fail() can now take one or two arguments (Rich Trott)
    #12293
* crypto:
  - add sign/verify support for RSASSA-PSS (Tobias Nießen)
    #11705
* deps:
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu)
    #16691
  - upgrade libuv to 1.15.0 (cjihrig)
    #15745
  - upgrade libuv to 1.14.1 (cjihrig)
    #14866
  - upgrade libuv to 1.13.1 (cjihrig)
    #14117
  - upgrade libuv to 1.12.0 (cjihrig)
    #13306
* fs:
  - Add support for fs.write/fs.writeSync(fd, buffer, cb) and
    fs.write/fs.writeSync(fd, buffer, offset, cb) as documented
    (Andreas Lind) #7856
* inspector:
  - enable --inspect-brk (Refael Ackermann)
    #12615
* process:
  - add --redirect-warnings command line argument (James M Snell)
    #10116
* src:
  - allow CLI args in env with NODE_OPTIONS (Sam Roberts)
    #12028)
  - --abort-on-uncaught-exception in NODE_OPTIONS (Sam Roberts)
    #13932
  - allow --tls-cipher-list in NODE_OPTIONS (Sam Roberts)
    #13172
  - use SafeGetenv() for NODE_REDIRECT_WARNINGS (Sam Roberts)
    #12677
* test:
  - remove common.fail() (Rich Trott)
    #12293

PR-URL: #16263
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crypto: sign/verify support for RSASSA-PSS
10 participants