crypto: allow custom generator for DiffieHellman #7086

mscdex · 2014-02-09T11:33:43Z

Nodejs-Jenkins · 2014-02-09T11:33:46Z

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

Brian White

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

indutny · 2014-02-13T16:39:05Z

lib/crypto.js

-    this._binding = new binding.DiffieHellman(sizeOrKey);
+    if (keyEncoding) {
+      if (typeof keyEncoding !== 'string'
+          || (!Buffer.isEncoding(keyEncoding) && keyEncoding !== 'buffer')) {


Please place || at the end of the previous line.

This wasn't fixed.

mscdex · 2014-02-13T18:48:29Z

Incorporated suggested changes.

indutny · 2014-02-13T19:10:12Z

src/node_crypto.cc

    return ThrowError("off + len > buffer.length");
+  }


Please remove braces, they are unnecessary here and only adding excessive changes.

Ah ok, I added them out of habit since that used to be the style for the C++-side of node stuff.

mscdex · 2014-02-13T19:37:15Z

Incorporated additional suggested changes.

indutny · 2014-02-13T19:53:20Z

lib/crypto.js


  if (!sizeOrKey)
-    this._binding = new binding.DiffieHellman();
+    throw new Error('Initialization failed');
  else {


I guess else is not required anymore.

mscdex · 2014-02-13T22:58:36Z

I just found something interesting while adding another test....

Currently the Init() that takes in two buffers does not call VerifyContext() after setting up dh->p and dh->g and also DiffieHellmanGroup() does not check the return value of that Init(). So what has been happening all along is that whenever you use one of the built-in 'modp' groups, no errors occur because these checks are not being done, but in reality an error (DH_NOT_SUITABLE_GENERATOR) is being set (at least for modp1) and is ignored.

The issue of ignoring/enforcing this particular error was discussed in #2338. So we need to decide whether we should always check for errors in all Init() methods, ignore this particular error everywhere, or make the error simply a warning somehow.

indutny · 2014-02-14T09:31:24Z

@mscdex I think it should throw, unless @bnoordhuis could weigh in with some objections.

indutny · 2014-02-14T09:34:25Z

Generally your PR LGTM

mscdex · 2014-02-14T13:34:00Z

Ordinarily I would agree that it should throw on the error (DH_NOT_SUITABLE_GENERATOR), but in my particular case with ssh 2, modp1 and modp14 are still used out in the wild since not all servers have the DH group exchange support yet. So that means I would have to ignore the error with a try-catch, possibly resulting in some v8 deoptimizations which I would prefer to avoid.

indutny · 2014-02-14T13:39:19Z

Sorry, could you please write some explanation about it. Is this error not fatal? I'm afraid I'm not really aware of all aspects of this topic.

mscdex · 2014-02-14T14:29:36Z

DH_NOT_SUITABLE_GENERATOR (probably along with DH_CHECK_P_NOT_SAFE_PRIME) is more of a warning than a fatal error. It's basically alerting you to the fact that OpenSSL thinks the generator supplied is insecure because of some mathematical properties of using certain primes and certain generators together. There was some discussion about this on the OpenSSL-dev list back in 2002 and there was a SO question about it with a fairly good answer/description of the problem.

Additionally from RFC 2412 (which defines some of the groups in node's 'modp*' groups):

   Note that 2 is technically not a generator in the number theory sense,
   because it omits half of the possible residues mod P.  From a cryptographic
   viewpoint, this is a virtue.

I think that either all DH_check() errors except DH_NOT_SUITABLE_GENERATOR (and possibly DH_CHECK_P_NOT_SAFE_PRIME) should throw and we set some properties on the DH object to indicate non-suitable generator and/or unsafe prime, or we allow a boolean to be passed in to allow skipping of DH_check() altogether.

indutny · 2014-02-14T18:55:27Z

Anyway, let's add VerifyCheck to that another init.

indutny · 2014-02-14T18:57:24Z

Or set a error property like verifyError, both works just fine to me. Just make it consistent, please ;)

mscdex · 2014-02-15T06:08:40Z

Alright, how does that look?

indutny · 2014-02-17T16:55:54Z

doc/api/crypto.markdown

-`'base64'`.  If no encoding is specified, then a buffer is expected.
+Creates a Diffie-Hellman key exchange object using the supplied prime and an
+optional specific generator.
+Generator can be a number, string, or Buffer.


Please use:

`generator`, `prime_encoding` and `generator_encoding`

indutny · 2014-02-17T16:58:56Z

Very minor style nits, otherwise LGTM

indutny · 2014-02-17T16:59:33Z

src/node_crypto.cc

@@ -3139,6 +3141,9 @@ void Verify::VerifyFinal(const FunctionCallbackInfo<Value>& args) {
 void DiffieHellman::Initialize(Environment* env, Handle<Object> target) {
  Local<FunctionTemplate> t = FunctionTemplate::New(New);

+  enum PropertyAttribute attributes =


Could it be a static variable?

Possibly, I was just basing that on another instance of PropertyAttribute usage elsewhere in node core.

Ok then :P We will address it later.

mscdex · 2014-02-18T01:52:23Z

Fixes made.

indutny · 2014-02-18T11:52:21Z

Thank you, landed in a226be4

indutny reviewed Feb 13, 2014
View reviewed changes

indutny reviewed Feb 17, 2014
View reviewed changes

crypto: allow custom generator for DiffieHellman

abb0702

trevnorris added the tls label Feb 18, 2014

indutny closed this Feb 18, 2014

mscdex deleted the dhgex branch July 17, 2014 04:50

sam-github mentioned this pull request Sep 13, 2019

Update openssl1.1.1d nodejs/node#29550

Closed

4 tasks

tniessen mentioned this pull request Dec 28, 2019

doc: fix createDiffieHellman generator type nodejs/node#31121

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: allow custom generator for DiffieHellman #7086

crypto: allow custom generator for DiffieHellman #7086

mscdex commented Feb 9, 2014

Nodejs-Jenkins commented Feb 9, 2014

indutny Feb 13, 2014

indutny Feb 13, 2014

mscdex commented Feb 13, 2014

indutny Feb 13, 2014

mscdex Feb 13, 2014

mscdex commented Feb 13, 2014

indutny Feb 13, 2014

mscdex commented Feb 13, 2014

indutny commented Feb 14, 2014

indutny commented Feb 14, 2014

mscdex commented Feb 14, 2014

indutny commented Feb 14, 2014

mscdex commented Feb 14, 2014

indutny commented Feb 14, 2014

indutny commented Feb 14, 2014

mscdex commented Feb 15, 2014

indutny Feb 17, 2014

indutny commented Feb 17, 2014

indutny Feb 17, 2014

mscdex Feb 17, 2014

indutny Feb 17, 2014

mscdex commented Feb 18, 2014

indutny commented Feb 18, 2014

crypto: allow custom generator for DiffieHellman #7086

crypto: allow custom generator for DiffieHellman #7086

Conversation

mscdex commented Feb 9, 2014

Nodejs-Jenkins commented Feb 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Feb 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Feb 13, 2014

Choose a reason for hiding this comment

mscdex commented Feb 13, 2014

indutny commented Feb 14, 2014

indutny commented Feb 14, 2014

mscdex commented Feb 14, 2014

indutny commented Feb 14, 2014

mscdex commented Feb 14, 2014

indutny commented Feb 14, 2014

indutny commented Feb 14, 2014

mscdex commented Feb 15, 2014

Choose a reason for hiding this comment

indutny commented Feb 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Feb 18, 2014

indutny commented Feb 18, 2014