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

tls: prevent server from using dhe keys < 768 #3890

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion doc/api/tls.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,8 @@ automatically set as a listener for the [secureConnection][] event. The

- `dhparam`: DH parameter file to use for DHE key agreement. Use
`openssl dhparam` command to create it. If the file is invalid to
load, it is silently discarded.
load, it is silently discarded. Its key length should be greater
than or equal to 768 bits, otherwise an error will be thrown.

- `handshakeTimeout`: Abort the connection if the SSL/TLS handshake does not
finish in this many milliseconds. The default is 120 seconds.
Expand Down
2 changes: 2 additions & 0 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3041,6 +3041,8 @@ static void ParseArgs(int* argc,
#if HAVE_OPENSSL
SSL3_ENABLE = true;
#endif
} else if (strcmp(arg, "--allow-insecure-server-dhparam") == 0) {
ALLOW_INSECURE_SERVER_DHPARAM = true;
} else if (strcmp(arg, "--help") == 0 || strcmp(arg, "-h") == 0) {
PrintHelp();
exit(0);
Expand Down
8 changes: 8 additions & 0 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ namespace node {

bool SSL2_ENABLE = false;
bool SSL3_ENABLE = false;
bool ALLOW_INSECURE_SERVER_DHPARAM = false;

namespace crypto {

Expand Down Expand Up @@ -785,6 +786,13 @@ void SecureContext::SetDHParam(const FunctionCallbackInfo<Value>& args) {
if (dh == NULL)
return;

if (!ALLOW_INSECURE_SERVER_DHPARAM) {
const int keylen = BN_num_bits(dh->p);
if (keylen < 768) {
return env->ThrowError("DH parameter is less than 768 bits");
}
}

SSL_CTX_set_options(sc->ctx_, SSL_OP_SINGLE_DH_USE);
int r = SSL_CTX_set_tmp_dh(sc->ctx_, dh);
DH_free(dh);
Expand Down
1 change: 1 addition & 0 deletions src/node_crypto.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ namespace node {

extern bool SSL2_ENABLE;
extern bool SSL3_ENABLE;
extern bool ALLOW_INSECURE_SERVER_DHPARAM;

namespace crypto {

Expand Down
5 changes: 4 additions & 1 deletion test/fixtures/keys/Makefile
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
all: agent1-cert.pem agent2-cert.pem agent3-cert.pem agent4-cert.pem agent5-cert.pem ca2-crl.pem ec-cert.pem dh1024.pem dh2048.pem
all: agent1-cert.pem agent2-cert.pem agent3-cert.pem agent4-cert.pem agent5-cert.pem ca2-crl.pem ec-cert.pem dh512.pem dh1024.pem dh2048.pem


#
Expand Down Expand Up @@ -170,6 +170,9 @@ ec-cert.pem: ec-csr.pem ec-key.pem
-signkey ec-key.pem \
-out ec-cert.pem

dh512.pem:
openssl dhparam -out dh512.pem 512

dh1024.pem:
openssl dhparam -out dh1024.pem 1024

Expand Down
4 changes: 4 additions & 0 deletions test/fixtures/keys/dh512.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
-----BEGIN DH PARAMETERS-----
MEYCQQDpl3okBAjG92NSOaQEsIyqzvJRN06yHuGXunxYVIqxg7TnU8DBZW0ZYyiJ
rJLRA/9b9dCk5DXpq1pFGoAkYLoDAgEC
-----END DH PARAMETERS-----
52 changes: 52 additions & 0 deletions test/simple/test-tls-dhe-allow-insecure-server-dhparam.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// Copyright Joyent, Inc. and other Node contributors.
//
// Permission is hereby granted, free of charge, to any person obtaining a
// copy of this software and associated documentation files (the
// "Software"), to deal in the Software without restriction, including
// without limitation the rights to use, copy, modify, merge, publish,
// distribute, sublicense, and/or sell copies of the Software, and to permit
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// in all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.
//
// Flags: --allow-insecure-server-dhparam

Copy link
Member

Choose a reason for hiding this comment

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

The copyright/license header should be removed. nm... oy, forgot these are v0.12

var common = require('../common');

var assert = require('assert');
var tls = require('tls');
var fs = require('fs');
var key = fs.readFileSync(common.fixturesDir + '/keys/agent2-key.pem');
var cert = fs.readFileSync(common.fixturesDir + '/keys/agent2-cert.pem');

Copy link
Member

Choose a reason for hiding this comment

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

should make these const now nm... oy, forgot these are v0.12

function loadDHParam(n) {
var path = common.fixturesDir;
if (n !== 'error') path += '/keys';
return fs.readFileSync(path + '/dh' + n + '.pem');
}

// validate that the server will accept a key smaller than
// 768 provided the required command line option is specified
// the flags statement above ensures that the test harnesss
// runs the test with the required command line option
function test512() {
var options = {
key: key,
cert: cert,
dhparam: loadDHParam(512)
};

var server = tls.createServer(options)
}

test512();
17 changes: 17 additions & 0 deletions test/simple/test-tls-dhe.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,25 @@ function testError() {
ntests++;
}

// validate that the server will not allow keys less than
// 768
function test512() {
var options = {
key: key,
cert: cert,
dhparam: loadDHParam(512)
};

assert.throws(function() {var server = tls.createServer(options)},
'Should not be able to create server with key smaller than 768');
}

// test client/server communication with different key sizes
test1024();

// test server key length enforcement
test512();

process.on('exit', function() {
assert.equal(ntests, nsuccess);
});