crypto - can't use Second Oakley Group from RFC 2412 for DH Exchange

[thinred]

Hi,
I am trying to use the group described in http://tools.ietf.org/html/rfc2412 to perform Diffie-Hellman Key Exchange. Unfortunately I get this:

crypto = require('crypto');
p = 'FFFFFFFFFFFFFFFFC90FDAA22168C234C4C6628B80DC1CD129024E088A67CC74020BBEA63B139B22514A08798E3404DDEF9519B3CD3A431B302B0A6DF25F14374FE1356D6D51C245E485B576625E7EC6F44C42E9A637ED6B0BFF5CB6F406B7EDEE386BFB5A899FA5AE9F24117C4B1FE649286651ECE65381FFFFFFFFFFFFFFFF'
dh = crypto.createDiffieHellman(p, 'hex')
dh.generateKeys();
Error: Not initialized
at repl:1:4
at REPLServer.eval (repl.js:80:21)
at repl.js:190:20
at REPLServer.eval (repl.js:87:5)
at Interface.<anonymous> (repl.js:182:12)
at Interface.emit (events.js:67:17)
at Interface._onLine (readline.js:162:10)
at Interface._line (readline.js:426:8)
at Interface._ttyWrite (readline.js:603:14)
at ReadStream.<anonymous> (readline.js:82:12)

I've traced the problem to DiffieHellman::VerifyContext where DH_check is used. It returns DH_NOT_SUITABLE_GENERATOR from OpenSSL and NodeJS object is not initialized. If you take a look at the OpenSSL library, you will notice that if generator is 2 (and this is the case here) the prime is checked to be congruent to 11 modulo 24. The prime above is in fact congruent to 23 and there is the problem.
In fact, the RFC mentions that: [...] 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. [...]. This is true but NodeJS will complain. I've also found this discussion from 2002 which is about the same problem: http://www.mail-archive.com/[email protected]/msg11453.html .

An easy fix would be to add flag (defaults to false) to ignore DH_check result (or at least DH_NOT_SUITABLE_GENERATOR flag). I would be happy to prepare the patch, but I would like to know what you guys think.

Cheers!
Tomek

[thinred]

One more thing: I think that the initialization error should be given at createDiffieHellman function because as it is now was a bit confusing to me (I thought I'm using it the wrong way or something).

[bnoordhuis]

Confirmed.

I think that the initialization error should be given at createDiffieHellman function

Agreed. There's a check in place but the result is being ignored right now. I'll fix it.

An easy fix would be to add flag (defaults to false) to ignore DH_check result (or at least DH_NOT_SUITABLE_GENERATOR flag).

I don't like that, it opens up too many potential vulnerabilities. Imagine some random coder trying to add DH key exchange to his application. It fails with some cryptic error message. He doesn't understand why, opens up the docs and hey, here is this magic flag that makes the problem go away. It's a security disaster waiting to happen.

I'm kind of surprised the openssl people haven't fixed it, the (p % 24 == 11) check still exists in openssl's bleeding edge. I assume they don't want to break backwards compatibility?

[thinred]

Unfortunately, I agree that this would open potential vulnerabilities...
I think the reason for OpenSSL developers not fixing this is that if you really want to work with well-known groups (like the 2nd Oakley Group) then you can manually ignore DH_NOT_SUITABLE_GENERATOR and proceed.
With Node (at least right now) you cannot bypass this step by any means.

Probably the best approach will be to ask OpenSSL devs what they think. I'm going to do this right away.

[thinred]

No response so far, but I see that in the master the initialization error is thrown where we discussed it should be. Thanks.
For now, I've commented out the test for unsuitable generator so that I can work until this is officially resolved.
BTW, shouldn't the functions in crypto module return Buffers? I was hit by this when converting binary string to a buffer. I see also that issue #1393 is considering buffers as input arguments, but I am asking about returned results. Shall I open a new issue?

[bnoordhuis]

BTW, shouldn't the functions in crypto module return Buffers?

Yes. The problem is that it would break a lot of existing code.

[thinred]

I see a way to workaround my original issue (using -2 instead of 2 as a generator and doing the necessary conversions on-the-fly), but I would need a way to specify a generator for the DH exchange. I propose a backwards compatible definition of:
* crypto.createDiffieHellman(prime_length, [ generator ], [ generator_encoding ])
* crypto.createDiffieHellman(prime, [encoding], [ generator ], [ generator_encoding ])
Alternatively, the generator may be just a Buffer (and encoding need not be specified).
I can write patch for that, if you find it acceptable.

[bnoordhuis]

Alternatively, the generator may be just a Buffer (and encoding need not be specified).

I think I can live with that. What openssl function will you be passing the buffer's data to?

[thinred]

Just as now we have:

dh->g = BN_new();
if (!BN_set_word(dh->g, 2)) return false;

this will change roughly, to:

dh->g = BN_bin2bn(g, g_len, 0);

Of course a bit more code will be needed, but that should not be a big problem.
And a small update:

crypto.createDiffieHellman(prime_length, [ generator ], [ generator_encoding ])

effectively selects a random prime, so there is no way to be sure that the supplied number is a generator.
Therefore I suggest only to provide:

crypto.createDiffieHellman(prime, [encoding], [ generator ])

where generator is a Buffer and defaults to [ 2 ].

And now for sth completely different: is a change to support only Buffers for input/output planned at some point? 1.0 maybe?

[thinred]

I started to implement this and found that DH_check returns DH_UNABLE_TO_CHECK_GENERATOR if the generator is not 2 or 5. It means that nodejs will still complain in VerifyContext.

I'm giving up here. I will implement DH exchange with Oakley Group with https://github.com/iriscouch/bigdecimal.js . Thanks!

[thinred]

Ok, I've implemented this (but using this: http://www-cs-students.stanford.edu/~tjw/jsbn/). However, I have another proposal :D.

We can have a function like createOakley2 that will create an object like createDiffieHellman does, but the value of prime and generator would be internally set to those from RFC. Apart from that, the behavior would be the same. What do you think?

[bnoordhuis]

We can have a function like createOakley2 that will create an object like createDiffieHellman does, but the value of prime and generator would be internally set to those from RFC. Apart from that, the behavior would be the same. What do you think?

If it's not a big patch and doesn't lead to massive code duplication, then I'm okay with it.

And now for sth completely different: is a change to support only Buffers for input/output planned at some point? 1.0 maybe?

Well... that's undecided so far. I would like to but it's a big backwards compatibility breaking change.

[thinred]

Ok, my first attempt with a patch. It is in my branch: https://github.com/thinred/node/tree/well-known-groups
Beware though! This is my first contact with v8 internals.

I've added function getDiffieHellman - it accepts a string (the name of a group, like "modp1", "modp16"). In a header file there are all modular groups I've found in RFCs. The groups I'm personally interested in are "modp2" and "modp14".

I've basically copy-pasted the code in New method and adjusted it a bit. I'm a bit confused with "new DiffieHellman()" allocation - doesn't it lead to memory leak when we return in exceptional cases? Or v8 magically takes care of that?

Anyway, I think this is useful - the user can create a well known group on both client and server without need to transfer the generator and prime. Moreover, these groups are believed to be secure.

You can see it working here https://gist.github.com/1663533 (scroll down to see the real testcases).

[bnoordhuis]

Looks good. Can you submit it as a PR and include the test cases (maybe as a new file test/simple/test-crypto-dh.js)? I have one or two questions / suggestions but that's easier to do from inside a PR.

[bnoordhuis]

This was fixed in c6a04ce.