Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit 0a4ad85

Browse filesBrowse files
mcollinaaduh95
authored andcommitted
http: validate ClientRequest path on set
The `path` property on `ClientRequest` was only validated at construction time. Add a getter/setter so that the same `INVALID_PATH_REGEX` check runs whenever `req.path` is reassigned, preventing invalid characters from reaching `_implicitHeader()`. PR-URL: #62030 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com>
1 parent 46cfad4 commit 0a4ad85
Copy full SHA for 0a4ad85

2 files changed

+87-1Lines changed: 87 additions & 1 deletion

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎lib/_http_client.js‎

Copy file name to clipboardExpand all lines: lib/_http_client.js
+19-1Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ const {
2727
Error,
2828
NumberIsFinite,
2929
ObjectAssign,
30+
ObjectDefineProperty,
3031
ObjectKeys,
3132
ObjectSetPrototypeOf,
3233
ReflectApply,
@@ -116,6 +117,7 @@ let debug = require('internal/util/debuglog').debuglog('http', (fn) => {
116117

117118
const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/;
118119
const kError = Symbol('kError');
120+
const kPath = Symbol('kPath');
119121

120122
const kLenientAll = HTTPParser.kLenientAll | 0;
121123
const kLenientNone = HTTPParser.kLenientNone | 0;
@@ -303,7 +305,7 @@ function ClientRequest(input, options, cb) {
303305

304306
this.joinDuplicateHeaders = options.joinDuplicateHeaders;
305307

306-
this.path = options.path || '/';
308+
this[kPath] = options.path || '/';
307309
if (cb) {
308310
this.once('response', cb);
309311
}
@@ -446,6 +448,22 @@ function ClientRequest(input, options, cb) {
446448
ObjectSetPrototypeOf(ClientRequest.prototype, OutgoingMessage.prototype);
447449
ObjectSetPrototypeOf(ClientRequest, OutgoingMessage);
448450

451+
ObjectDefineProperty(ClientRequest.prototype, 'path', {
452+
__proto__: null,
453+
get() {
454+
return this[kPath];
455+
},
456+
set(value) {
457+
const path = String(value);
458+
if (INVALID_PATH_REGEX.test(path)) {
459+
throw new ERR_UNESCAPED_CHARACTERS('Request path');
460+
}
461+
this[kPath] = path;
462+
},
463+
configurable: true,
464+
enumerable: true,
465+
});
466+
449467
ClientRequest.prototype._finish = function _finish() {
450468
OutgoingMessage.prototype._finish.call(this);
451469
if (hasObserver('http')) {
Collapse file
+68Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
'use strict';
2+
require('../common');
3+
const assert = require('assert');
4+
const http = require('http');
5+
6+
// Test that mutating req.path after construction to include
7+
// invalid characters (e.g. CRLF) throws ERR_UNESCAPED_CHARACTERS.
8+
// Regression test for a TOCTOU vulnerability where path was only
9+
// validated at construction time but could be mutated before
10+
// _implicitHeader() flushed it to the socket.
11+
12+
// Use a createConnection that returns nothing to avoid actual connection.
13+
const req = new http.ClientRequest({
14+
host: '127.0.0.1',
15+
port: 1,
16+
path: '/valid',
17+
method: 'GET',
18+
createConnection: () => {},
19+
});
20+
21+
// Attempting to set path with CRLF must throw
22+
assert.throws(
23+
() => { req.path = '/evil\r\nX-Injected: true\r\n\r\n'; },
24+
{
25+
code: 'ERR_UNESCAPED_CHARACTERS',
26+
name: 'TypeError',
27+
message: 'Request path contains unescaped characters',
28+
}
29+
);
30+
31+
// Path must be unchanged after failed mutation
32+
assert.strictEqual(req.path, '/valid');
33+
34+
// Attempting to set path with lone CR must throw
35+
assert.throws(
36+
() => { req.path = '/evil\rpath'; },
37+
{
38+
code: 'ERR_UNESCAPED_CHARACTERS',
39+
name: 'TypeError',
40+
}
41+
);
42+
43+
// Attempting to set path with lone LF must throw
44+
assert.throws(
45+
() => { req.path = '/evil\npath'; },
46+
{
47+
code: 'ERR_UNESCAPED_CHARACTERS',
48+
name: 'TypeError',
49+
}
50+
);
51+
52+
// Attempting to set path with null byte must throw
53+
assert.throws(
54+
() => { req.path = '/evil\0path'; },
55+
{
56+
code: 'ERR_UNESCAPED_CHARACTERS',
57+
name: 'TypeError',
58+
}
59+
);
60+
61+
// Valid path mutation should succeed
62+
req.path = '/also-valid';
63+
assert.strictEqual(req.path, '/also-valid');
64+
65+
req.path = '/path?query=1&other=2';
66+
assert.strictEqual(req.path, '/path?query=1&other=2');
67+
68+
req.destroy();

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.