Skip to content
Open
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
34 changes: 24 additions & 10 deletions lib/https.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ let debug = require('internal/util/debuglog').debuglog('https', (fn) => {
debug = fn;
});
const net = require('net');
const { Buffer } = require('buffer');
const { URL, urlToHttpOptions, isURL } = require('internal/url');
const { validateObject } = require('internal/validators');
const { isIP } = require('internal/net');
Expand Down Expand Up @@ -212,16 +213,18 @@ function getTunnelConfigForProxiedHttps(agent, reqOptions) {

function establishTunnel(agent, socket, options, tunnelConfig, afterSocket) {
const { proxyTunnelPayload } = tunnelConfig;
const maxHeaderSize = tunnelConfig.requestOptions.maxHeaderSize ||

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can be configured in getTunnelConfigForProxiedHttps instead.

getOptionValue('--max-http-header-size');
// By default, the socket is in paused mode. Read to look for the 200
// connection established response.
function read() {
let chunk;
while ((chunk = socket.read()) !== null) {
if (onProxyData(chunk) !== -1) {
break;
if (onProxyData(chunk)) {
return;
}
}
socket.on('readable', read);
socket.once('readable', read);
}

function cleanup() {
Expand All @@ -239,14 +242,25 @@ function establishTunnel(agent, socket, options, tunnelConfig, afterSocket) {

// Read the headers from the chunks and check for the status code. If it fails we
// clean up the socket and return an error. Otherwise we establish the tunnel.
let buffer = '';
let buffer;
function onProxyData(chunk) {
const str = chunk.toString();
debug('onProxyData', str);
buffer += str;
debug('onProxyData', chunk.toString());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think if we want to defend against the case where the server does not conform to RFC and somehow sends a malformed CONNECT response, then there's no need to log the chunk at all, otherwise we end up transcoding it twice.

Suggested change
debug('onProxyData', chunk.toString());
debug('onProxyData', chunk.length);

buffer = buffer === undefined ? chunk :
Buffer.concat([buffer, chunk], buffer.length + chunk.length);
const headerEndIndex = buffer.indexOf('\r\n\r\n');
if (headerEndIndex === -1) return headerEndIndex;
const statusLine = buffer.substring(0, buffer.indexOf('\r\n'));
const headerLength = headerEndIndex === -1 ?
buffer.length : headerEndIndex + 4;
if (headerLength > maxHeaderSize) {
debug(`Proxy response headers exceed ${maxHeaderSize} bytes, cleaning up`);
cleanup();
const err = new ERR_PROXY_TUNNEL(
`Proxy response headers exceeded ${maxHeaderSize} bytes`);
afterSocket(err, socket);
return true;
}
if (headerEndIndex === -1) return false;
const statusLineEndIndex = buffer.indexOf('\r\n');
const statusLine = buffer.subarray(0, statusLineEndIndex).toString();
const statusCode = statusLine.split(' ')[1];
if (statusCode !== '200') {
debug(`onProxyData receives ${statusCode}, cleaning up`);
Expand Down Expand Up @@ -286,7 +300,7 @@ function establishTunnel(agent, socket, options, tunnelConfig, afterSocket) {
});
tunneldSocket.on('error', onTLSHandshakeError);
}
return headerEndIndex;
return true;
}

function onProxyEnd() {
Expand Down
45 changes: 45 additions & 0 deletions test/client-proxy/test-https-proxy-request-header-overflow.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// This tests that the proxy server cannot send unbounded incomplete headers
// while establishing a CONNECT tunnel.

import * as common from '../common/index.mjs';
import assert from 'node:assert';
import { once } from 'events';
import http from 'node:http';
import { runProxiedRequest } from '../common/proxy-server.js';

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

const proxy = http.createServer();
proxy.on('connect', common.mustCall((req, socket) => {
socket.write('HTTP/1.1 200 Connection Established\r\n');

const interval = setInterval(() => {
if (socket.destroyed) {
clearInterval(interval);
return;
}
socket.write('x'.repeat(100));
}, 10);
socket.on('close', () => clearInterval(interval));
socket.on('error', () => clearInterval(interval));
}, 1));
proxy.listen(0);
await once(proxy, 'listening');

const { code, signal, stderr, stdout } = await runProxiedRequest({
NODE_USE_ENV_PROXY: 1,
REQUEST_URL: 'https://localhost:1/test',
HTTPS_PROXY: `http://localhost:${proxy.address().port}`,
}, ['--max-http-header-size=1024']);

assert.match(
stderr,
/ERR_PROXY_TUNNEL.*Proxy response headers exceeded 1024 bytes/,
);
assert.doesNotMatch(stderr, /MaxListenersExceededWarning/);
assert.strictEqual(stdout.trim(), '');
assert.strictEqual(code, 0);
assert.strictEqual(signal, null);

proxy.close();
Loading