From 4000f3b2dfd26191edabf31dde0c6dc5553ab5c5 Mon Sep 17 00:00:00 2001 From: Tomasz Buchert Date: Tue, 23 Jun 2015 21:38:24 +0200 Subject: [PATCH] Fix for the issue 25366. In particular: - DH groups of size < 1024 are disabled by default (there is only one such group: modp1) - a new cmdline switch --enable-small-dh-groups and SMALL_DH_GROUPS_ENABLE env. variable are introduced; they override the default setting and therefore enable modp1 group - the docs & tests are updated --- doc/api/crypto.markdown | 30 +++++++++++++++++++----------- src/node.cc | 11 +++++++++++ src/node_crypto.cc | 7 ++++++- src/node_crypto.h | 1 + src/node_crypto_groups.h | 34 +++++++++++++++++++++------------- test/simple/test-crypto.js | 33 +++++++++++++++++++++++++++++++++ 6 files changed, 91 insertions(+), 25 deletions(-) diff --git a/doc/api/crypto.markdown b/doc/api/crypto.markdown index 7a1abb649297..bf69f74d10fc 100644 --- a/doc/api/crypto.markdown +++ b/doc/api/crypto.markdown @@ -425,21 +425,29 @@ expected. ## crypto.getDiffieHellman(group_name) Creates a predefined Diffie-Hellman key exchange object. The -supported groups are: `'modp1'`, `'modp2'`, `'modp5'` (defined in [RFC -2412][]) and `'modp14'`, `'modp15'`, `'modp16'`, `'modp17'`, -`'modp18'` (defined in [RFC 3526][]). The returned object mimics the -interface of objects created by [crypto.createDiffieHellman()][] -above, but will not allow to change the keys (with -[diffieHellman.setPublicKey()][] for example). The advantage of using -this routine is that the parties don't have to generate nor exchange -group modulus beforehand, saving both processor and communication -time. +supported groups are: `'modp1'`, `'modp2'`, `'modp5'` (defined in +[RFC 2412][]) and `'modp14'`, `'modp15'`, `'modp16'`, `'modp17'`, +`'modp18'` (defined in [RFC 3526][]). + +The returned object mimics the interface of objects created by +[crypto.createDiffieHellman()][] above, but will not allow to change +the keys (with [diffieHellman.setPublicKey()][] for example). The +advantage of using this routine is that the parties do not have to +generate nor exchange group modulus beforehand, saving both processor +and communication time. + +The groups `'modp1'`, `'modp2'` and `'modp5'` (i.e., the groups with +size smaller than 2048 bits) are considered **deprecated** and should +not be used in new code. Moreover, the use of the `'modp1'` group must +be explicitly enabled: either via `'--enable-small-dh-groups'` switch to +node, or by setting the `'ENABLE_SMALL_DH_GROUPS'` environment variable +to any value. Example (obtaining a shared secret): var crypto = require('crypto'); - var alice = crypto.getDiffieHellman('modp5'); - var bob = crypto.getDiffieHellman('modp5'); + var alice = crypto.getDiffieHellman('modp14'); + var bob = crypto.getDiffieHellman('modp14'); alice.generateKeys(); bob.generateKeys(); diff --git a/src/node.cc b/src/node.cc index e80c1a573f50..34747443e8aa 100644 --- a/src/node.cc +++ b/src/node.cc @@ -2566,6 +2566,7 @@ static void PrintHelp() { " --max-stack-size=val set max v8 stack size (bytes)\n" " --enable-ssl2 enable ssl2\n" " --enable-ssl3 enable ssl3\n" + " --enable-small-dh-groups enable small dh groups (not recommended)\n" "\n" "Environment variables:\n" #ifdef _WIN32 @@ -2577,6 +2578,7 @@ static void PrintHelp() { "NODE_MODULE_CONTEXTS Set to 1 to load modules in their own\n" " global contexts.\n" "NODE_DISABLE_COLORS Set to 1 to disable colors in the REPL\n" + "ENABLE_SMALL_DH_GROUPS enable small dh groups (not recommended)\n" "\n" "Documentation can be found at http://nodejs.org/\n"); } @@ -2605,6 +2607,9 @@ static void ParseArgs(int argc, char **argv) { } else if (strcmp(arg, "--enable-ssl3") == 0) { SSL3_ENABLE = true; argv[i] = const_cast(""); + } else if (strcmp(arg, "--enable-small-dh-groups") == 0) { + SMALL_DH_GROUPS_ENABLE = true; + argv[i] = const_cast(""); } else if (strcmp(arg, "--help") == 0 || strcmp(arg, "-h") == 0) { PrintHelp(); exit(0); @@ -2930,6 +2935,12 @@ char** Init(int argc, char *argv[]) { // Parse a few arguments which are specific to Node. node::ParseArgs(argc, argv); + + const char *enableSmallDHGroups = getenv("ENABLE_SMALL_DH_GROUPS"); + if (enableSmallDHGroups != NULL) { + SMALL_DH_GROUPS_ENABLE = true; + } + // Parse the rest of the args (up to the 'option_end_index' (where '--' was // in the command line)) int v8argc = option_end_index; diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 7a3922a797f8..d625051e0ba0 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -71,6 +71,7 @@ const char* root_certs[] = { bool SSL2_ENABLE = false; bool SSL3_ENABLE = false; +bool SMALL_DH_GROUPS_ENABLE = false; namespace crypto { @@ -802,7 +803,7 @@ size_t ClientHelloParser::Write(const uint8_t* data, size_t len) { HandleScope scope; assert(state_ != kEnded); - + // Just accumulate data, everything will be pushed to BIO later if (state_ == kPaused) return 0; @@ -3538,6 +3539,9 @@ class DiffieHellman : public ObjectWrap { } if (it->name != NULL) { + if (it->bits < 1024 && !SMALL_DH_GROUPS_ENABLE) + return ThrowException(Exception::Error( + String::New("Small DH groups disabled (see documentation)"))); diffieHellman->Init(it->prime, it->prime_size, it->gen, it->gen_size); } else { @@ -4264,6 +4268,7 @@ void InitCrypto(Handle target) { NODE_DEFINE_CONSTANT(target, SSL3_ENABLE); NODE_DEFINE_CONSTANT(target, SSL2_ENABLE); + NODE_DEFINE_CONSTANT(target, SMALL_DH_GROUPS_ENABLE); } } // namespace crypto diff --git a/src/node_crypto.h b/src/node_crypto.h index 54b9b88e437a..7f365bf10f3d 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -47,6 +47,7 @@ namespace node { extern bool SSL2_ENABLE; extern bool SSL3_ENABLE; +extern bool SMALL_DH_GROUPS_ENABLE; namespace crypto { diff --git a/src/node_crypto_groups.h b/src/node_crypto_groups.h index 4a521f45a5bc..05e23c50c09f 100644 --- a/src/node_crypto_groups.h +++ b/src/node_crypto_groups.h @@ -1,8 +1,15 @@ - /* These modular groups were literally taken from: - * RFC 2412 (groups 1 and 2) - * RFC 3526 (groups 5, 14, 15, 16, 17 and 18) + * RFC 2412: + - group 1 (768 bits) + - group 2 (1024 bits) + - group 5 (1536 bits) + * RFC 3526: + - group 14 (2048 bits) + - group 15 (3072 bits) + - group 16 (4096 bits) + - group 17 (6144 bits) + - group 18 (8192 bits) They all use 2 as a generator. */ @@ -367,6 +374,7 @@ unsigned char group_modp18[] = { typedef struct { const char* name; + unsigned int bits; unsigned char* prime; int prime_size; unsigned char* gen; @@ -374,13 +382,13 @@ typedef struct { } modp_group; modp_group modp_groups[] = { - { "modp1", group_modp1, sizeof(group_modp1) / sizeof(group_modp1[0]), two_generator, 1 }, - { "modp2", group_modp2, sizeof(group_modp2) / sizeof(group_modp2[0]), two_generator, 1 }, - { "modp5", group_modp5, sizeof(group_modp5) / sizeof(group_modp5[0]), two_generator, 1 }, - { "modp14", group_modp14, sizeof(group_modp14) / sizeof(group_modp14[0]), two_generator, 1 }, - { "modp15", group_modp15, sizeof(group_modp15) / sizeof(group_modp15[0]), two_generator, 1 }, - { "modp16", group_modp16, sizeof(group_modp16) / sizeof(group_modp16[0]), two_generator, 1 }, - { "modp17", group_modp17, sizeof(group_modp17) / sizeof(group_modp17[0]), two_generator, 1 }, - { "modp18", group_modp18, sizeof(group_modp18) / sizeof(group_modp18[0]), two_generator, 1 }, - { NULL, NULL, 0, NULL, 0 } -}; \ No newline at end of file + { "modp1", 768, group_modp1, sizeof(group_modp1) / sizeof(group_modp1[0]), two_generator, 1 }, + { "modp2", 1024, group_modp2, sizeof(group_modp2) / sizeof(group_modp2[0]), two_generator, 1 }, + { "modp5", 1536, group_modp5, sizeof(group_modp5) / sizeof(group_modp5[0]), two_generator, 1 }, + { "modp14", 2048, group_modp14, sizeof(group_modp14) / sizeof(group_modp14[0]), two_generator, 1 }, + { "modp15", 3072, group_modp15, sizeof(group_modp15) / sizeof(group_modp15[0]), two_generator, 1 }, + { "modp16", 4096, group_modp16, sizeof(group_modp16) / sizeof(group_modp16[0]), two_generator, 1 }, + { "modp17", 6144, group_modp17, sizeof(group_modp17) / sizeof(group_modp17[0]), two_generator, 1 }, + { "modp18", 8192, group_modp18, sizeof(group_modp18) / sizeof(group_modp18[0]), two_generator, 1 }, + { NULL, 0, NULL, 0, NULL, 0 } +}; diff --git a/test/simple/test-crypto.js b/test/simple/test-crypto.js index 25bb359190e3..87c8e09afe11 100644 --- a/test/simple/test-crypto.js +++ b/test/simple/test-crypto.js @@ -24,6 +24,7 @@ var common = require('../common'); var assert = require('assert'); +var spawn = require('child_process').spawn; try { var crypto = require('crypto'); @@ -722,6 +723,38 @@ var aSecret = alice.computeSecret(bob.getPublicKey()).toString('hex'); var bSecret = bob.computeSecret(alice.getPublicKey()).toString('hex'); assert.equal(aSecret, bSecret); +function node_output(code, args, env, cb) { + var out = '', err = ''; + var p = spawn(process.execPath, [ '-e', code ].concat(args), { env: env }); + p.stdout.on('data', function(data) { out += data; }); + p.stderr.on('data', function(data) { err += data; }); + p.on('close', function(code, signal) { cb(out, err, code); }); +} + +function no_output(out, err, code) { + assert.equal(out + err, ''); + assert.equal(code, 0); +} + +// test if fails on deprecated group +node_output("require('crypto').getDiffieHellman('modp1')", + [], {}, function(out, err, code) { + assert.equal(out, ''); + assert.ok(err.indexOf('Small DH groups disabled') > -1); + assert.notEqual(code, 1); + }); + +// test if the environment variable makes it work +node_output("require('crypto').getDiffieHellman('modp1')", + [], { 'ENABLE_SMALL_DH_GROUPS': '' }, no_output); + +// test if the cmdline switch makes it work +node_output("require('crypto').getDiffieHellman('modp1')", + [ '--enable-small-dh-groups' ], {}, no_output); + +// test if does not fail on the next group +node_output("require('crypto').getDiffieHellman('modp2')", + [], {}, no_output); // https://github.com/joyent/node/issues/2338 assert.throws(function() {