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.

Sign up for GitHub

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

http: replace destroySoon with socket.end #36205

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

http: replace destroySoon with socket.end #36205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dd2e3ed
net: require two events to destroy a socket
mmomtchev Nov 20, 2020
e6699c6
net: replace destroySoon with socket.end()
mmomtchev Nov 21, 2020
8f0e26c
net: close http connections with stream.end()
mmomtchev Nov 22, 2020
f780542
http: lint the PR
mmomtchev Nov 22, 2020
3e1ba08
net: add a unit test for res.end
mmomtchev Nov 23, 2020
1559f5c
http: replace destroySoon with socket.end
mmomtchev Nov 23, 2020
a1b606c
tls: support active closing for TLS sockets
mmomtchev Dec 9, 2020
6038c95
remove useless change
mmomtchev Dec 9, 2020
0dfd1f2
tls: use Writable.end (@ronag)
mmomtchev Dec 9, 2020
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
9 changes: 9 additions & 0 deletions lib/_tls_wrap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ const tls = require('tls');
const common = require('_tls_common');
const JSStreamSocket = require('internal/js_stream_socket');
const { Buffer } = require('buffer');
const Writable = require('internal/streams/writable');
let debug = require('internal/util/debuglog').debuglog('tls', (fn) => {
debug = fn;
});
Expand Down Expand Up @@ -571,6 +572,14 @@ tls_wrap.TLSWrap.prototype.close = function close(cb) {
return this._parent.close(done);
};

// TLS needs special handling when the server initiates the closing
// of the connection because a TLS-compliant client will send more data
// after receiving the server FIN
// If the server immediately destroys its socket, this data will trigger a
// RST packet in response
// https://github.com/nodejs/node/issues/36180
TLSSocket.prototype.destroySoon = Writable.prototype.end;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a timeout for 'close' here? @lpinca

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you want a timeout here. The underlying socket will already have a timeout (from the time the last packet was received). And if it's slowly trickling in data, any hard timeout could break the connection prematurely.

Copy link
Member

@lpinca lpinca Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Wouldn't this allow a Slowloris like attack? To clarify, I think it would make sense to have a timeout on tlsSocket.destroySoon() if it is changed to be an alias for tlsSocket.end(), not on tlsSocket.end() itself that should work like netSocket.end() for feature parity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that seems quite possible. As I mentioned elsewhere, I'm opposed to the general implementation in this PR. At a minimum, it needs to respect the allowHalfOpen / autoDestroy flags. But it also needs to be implemented at the point in the TLS code that is generating this event, and not by disregarding the explicit call to destroy or close it.

Additionally, I expect that, in principle, as soon as this function returns, a sufficiently smart garbage collector could observe that there are no remaining useful references to this TLS socket (we were just requested to destroy the socket for reading and writing, so there's nothing that should generate a new callback event), and thus the underlying socket should be expected to be finalized and then immediately destroyed as soon as the call to 'end' returns. And thus still causing the RST packet this PR was supposedly going to prevent. It's probably not hard to fool the garbage collector into keeping the memory reference live (though who is ever going to clean up this socket in that case?), but I worry that this PR may thus just create new problems, without actually addressing existing ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally, all three major OSes have a built-in default timeout for sockets in FIN_WAIT_2 state (first FIN sent and then ACK from remote) - it is usually about 60-120 seconds.
For sockets in FIN_WAIT_1 state (first FIN sent, waiting for first ACK), the much longer default TCP timeout applies - that would be 300s by default on Linux.
As for the Slowloris attack, I don't see how this change could help an attacker? A Slowloris attack exploits the timeouts by always sending just enough data to keep them from triggering. The most advantageous timeout for this would still be the default TCP ACK timeout - be it by withholding the ACK on a data packet or a FIN packet. In fact, FIN would be a little bit less practical, because you can delay it only once - the connection will be closed afterwards.

As for the event that triggers (or not) the RST, it is not the destruction of the socket object by the garbage collector, it is the explicit calling of the kernel close() syscall by libuv uv_tcp_close(). If any remote data is received after this call, a RST is sent back.


TLSSocket.prototype.disableRenegotiation = function disableRenegotiation() {
this[kDisableRenegotiation] = true;
};
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-http-should-keep-alive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ const countdown = new Countdown(SHOULD_KEEP_ALIVE.length, () => server.close());
const getCountdownIndex = () => SERVER_RESPONSES.length - countdown.remaining;

const server = net.createServer(function(socket) {
socket.write(SERVER_RESPONSES[getCountdownIndex()]);
socket.end(SERVER_RESPONSES[getCountdownIndex()]);
}).listen(0, function() {
function makeRequest() {
const req = http.get({ port: server.address().port }, function(res) {
Expand Down
40 changes: 40 additions & 0 deletions test/parallel/test-https-end-rst.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
'use strict';
const common = require('../common');

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

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

const opt = {
key: fixtures.readKey('agent1-key.pem'),
cert: fixtures.readKey('agent1-cert.pem')
};

const data = 'hello';
const server = https.createServer(opt, common.mustCallAtLeast((req, res) => {
res.setHeader('content-length', data.length);
res.end(data);
}, 2));

server.listen(0, function() {
const options = {
host: '127.0.0.1',
port: server.address().port,
rejectUnauthorized: false,
ALPNProtocols: ['http/1.1'],
allowHalfOpen: true
};
const socket = tls.connect(options, common.mustCall(() => {
socket.write('GET /\n\n');
socket.once('data', common.mustCall(() => {
socket.write('GET /\n\n');
setTimeout(common.mustCall(() => {
socket.destroy();
server.close();
}), common.platformTimeout(10));
}));
}));
});