diff --git a/lib/_http_client.js b/lib/_http_client.js index 1358a441c15f90..c14e899dabbf04 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -27,6 +27,7 @@ const { Error, NumberIsFinite, ObjectAssign, + ObjectDefineProperty, ObjectKeys, ObjectSetPrototypeOf, ReflectApply, @@ -116,6 +117,7 @@ let debug = require('internal/util/debuglog').debuglog('http', (fn) => { const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/; const kError = Symbol('kError'); +const kPath = Symbol('kPath'); const kLenientAll = HTTPParser.kLenientAll | 0; const kLenientNone = HTTPParser.kLenientNone | 0; @@ -303,7 +305,7 @@ function ClientRequest(input, options, cb) { this.joinDuplicateHeaders = options.joinDuplicateHeaders; - this.path = options.path || '/'; + this[kPath] = options.path || '/'; if (cb) { this.once('response', cb); } @@ -446,6 +448,22 @@ function ClientRequest(input, options, cb) { ObjectSetPrototypeOf(ClientRequest.prototype, OutgoingMessage.prototype); ObjectSetPrototypeOf(ClientRequest, OutgoingMessage); +ObjectDefineProperty(ClientRequest.prototype, 'path', { + __proto__: null, + get() { + return this[kPath]; + }, + set(value) { + const path = String(value); + if (INVALID_PATH_REGEX.test(path)) { + throw new ERR_UNESCAPED_CHARACTERS('Request path'); + } + this[kPath] = path; + }, + configurable: true, + enumerable: true, +}); + ClientRequest.prototype._finish = function _finish() { OutgoingMessage.prototype._finish.call(this); if (hasObserver('http')) { diff --git a/test/parallel/test-http-client-path-toctou.js b/test/parallel/test-http-client-path-toctou.js new file mode 100644 index 00000000000000..2975ba363d2614 --- /dev/null +++ b/test/parallel/test-http-client-path-toctou.js @@ -0,0 +1,68 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const http = require('http'); + +// Test that mutating req.path after construction to include +// invalid characters (e.g. CRLF) throws ERR_UNESCAPED_CHARACTERS. +// Regression test for a TOCTOU vulnerability where path was only +// validated at construction time but could be mutated before +// _implicitHeader() flushed it to the socket. + +// Use a createConnection that returns nothing to avoid actual connection. +const req = new http.ClientRequest({ + host: '127.0.0.1', + port: 1, + path: '/valid', + method: 'GET', + createConnection: () => {}, +}); + +// Attempting to set path with CRLF must throw +assert.throws( + () => { req.path = '/evil\r\nX-Injected: true\r\n\r\n'; }, + { + code: 'ERR_UNESCAPED_CHARACTERS', + name: 'TypeError', + message: 'Request path contains unescaped characters', + } +); + +// Path must be unchanged after failed mutation +assert.strictEqual(req.path, '/valid'); + +// Attempting to set path with lone CR must throw +assert.throws( + () => { req.path = '/evil\rpath'; }, + { + code: 'ERR_UNESCAPED_CHARACTERS', + name: 'TypeError', + } +); + +// Attempting to set path with lone LF must throw +assert.throws( + () => { req.path = '/evil\npath'; }, + { + code: 'ERR_UNESCAPED_CHARACTERS', + name: 'TypeError', + } +); + +// Attempting to set path with null byte must throw +assert.throws( + () => { req.path = '/evil\0path'; }, + { + code: 'ERR_UNESCAPED_CHARACTERS', + name: 'TypeError', + } +); + +// Valid path mutation should succeed +req.path = '/also-valid'; +assert.strictEqual(req.path, '/also-valid'); + +req.path = '/path?query=1&other=2'; +assert.strictEqual(req.path, '/path?query=1&other=2'); + +req.destroy();