Skip to content

Commit

Permalink
tls: fix negative sessionTimeout handling
Browse files Browse the repository at this point in the history
For historical reasons, the second argument of SSL_CTX_set_timeout is a
signed integer, and Node.js has so far passed arbitrary (signed) int32_t
values. However, new versions of OpenSSL have changed the handling of
negative values inside SSL_CTX_set_timeout, and we should shield users
of Node.js from both the old and the new behavior. Hence, reject any
negative values by throwing an error from within createSecureContext.

Refs: openssl/openssl#19082
PR-URL: #53002
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
  • Loading branch information
tniessen authored and marco-ippolito committed Jul 19, 2024
1 parent 1dc406b commit 68fcbb6
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 9 deletions.
2 changes: 1 addition & 1 deletion lib/internal/tls/secure-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ function configSecureContext(context, options = kEmptyObject, name = 'options')
}

if (sessionTimeout !== undefined && sessionTimeout !== null) {
validateInt32(sessionTimeout, `${name}.sessionTimeout`);
validateInt32(sessionTimeout, `${name}.sessionTimeout`, 0);
context.setSessionTimeout(sessionTimeout);
}
}
Expand Down
1 change: 1 addition & 0 deletions src/crypto/crypto_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -998,6 +998,7 @@ void SecureContext::SetSessionTimeout(const FunctionCallbackInfo<Value>& args) {
CHECK(args[0]->IsInt32());

int32_t sessionTimeout = args[0].As<Int32>()->Value();
CHECK_GE(sessionTimeout, 0);
SSL_CTX_set_timeout(sc->ctx_.get(), sessionTimeout);
}

Expand Down
39 changes: 31 additions & 8 deletions test/sequential/test-tls-session-timeout.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,43 @@
'use strict';
const common = require('../common');

if (!common.opensslCli)
common.skip('node compiled without OpenSSL CLI.');

if (!common.hasCrypto)
common.skip('missing crypto');

const tmpdir = require('../common/tmpdir');
tmpdir.refresh();

const assert = require('assert');
const tls = require('tls');
const fixtures = require('../common/fixtures');

const key = fixtures.readKey('rsa_private.pem');
const cert = fixtures.readKey('rsa_cert.crt');

{
// Node.js should not allow setting negative timeouts since new versions of
// OpenSSL do not handle those as users might expect

for (const sessionTimeout of [-1, -100, -(2 ** 31)]) {
assert.throws(() => {
tls.createServer({
key: key,
cert: cert,
ca: [cert],
sessionTimeout,
maxVersion: 'TLSv1.2',
});
}, {
code: 'ERR_OUT_OF_RANGE',
message: 'The value of "options.sessionTimeout" is out of range. It ' +
`must be >= 0 && <= ${2 ** 31 - 1}. Received ${sessionTimeout}`,
});
}
}

if (!common.opensslCli)
common.skip('node compiled without OpenSSL CLI.');

doTest();

// This test consists of three TLS requests --
Expand All @@ -42,16 +70,11 @@ doTest();
// that we used has expired by now.

function doTest() {
const assert = require('assert');
const tls = require('tls');
const fs = require('fs');
const fixtures = require('../common/fixtures');
const spawn = require('child_process').spawn;

const SESSION_TIMEOUT = 1;

const key = fixtures.readKey('rsa_private.pem');
const cert = fixtures.readKey('rsa_cert.crt');
const options = {
key: key,
cert: cert,
Expand Down

0 comments on commit 68fcbb6

Please sign in to comment.