From d777da27bc6ca0e1946943ef342e940804ded5e6 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 4 Oct 2016 11:44:54 +0200 Subject: [PATCH] http: reject control characters in http.request() Unsanitized paths containing line feed characters can be used for header injection and request splitting so reject them with an exception. There seems to be no reasonable use case for allowing control characters (characters <= 31) while there are several scenarios where they can be used to exploit software bugs so reject control characters altogether. PR-URL: https://github.com/nodejs/node/pull/8923 Reviewed-By: Anna Henningsen Reviewed-By: Evan Lucas Reviewed-By: Fedor Indutny Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: not-an-aardvark --- lib/_http_client.js | 7 +++---- .../test-http-client-unescaped-path.js | 19 ++++++++++++------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index 4ca03eae6796ac..a71870f90c940e 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -43,13 +43,12 @@ function ClientRequest(options, cb) { if (self.agent && self.agent.protocol) expectedProtocol = self.agent.protocol; - if (options.path && / /.test(options.path)) { + if (options.path && /[\u0000-\u0020]/.test(options.path)) { // The actual regex is more like /[^A-Za-z0-9\-._~!$&'()*+,;=/:@]/ // with an additional rule for ignoring percentage-escaped characters // but that's a) hard to capture in a regular expression that performs - // well, and b) possibly too restrictive for real-world usage. That's - // why it only scans for spaces because those are guaranteed to create - // an invalid request. + // well, and b) possibly too restrictive for real-world usage. + // Restrict the filter to control characters and spaces. throw new TypeError('Request path contains unescaped characters'); } else if (protocol !== expectedProtocol) { throw new Error('Protocol "' + protocol + '" not supported. ' + diff --git a/test/parallel/test-http-client-unescaped-path.js b/test/parallel/test-http-client-unescaped-path.js index e01df255a8042c..2411d0e6be31e2 100644 --- a/test/parallel/test-http-client-unescaped-path.js +++ b/test/parallel/test-http-client-unescaped-path.js @@ -1,9 +1,14 @@ 'use strict'; -var common = require('../common'); -var assert = require('assert'); -var http = require('http'); +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); -assert.throws(function() { - // Path with spaces in it should throw. - http.get({ path: 'bad path' }, common.fail); -}, /contains unescaped characters/); +function* bad() { + for (let i = 0; i <= 32; i += 1) + yield 'bad' + String.fromCharCode(i) + 'path'; +} + +for (const path of bad()) { + assert.throws(() => http.get({ path }, common.fail), + /contains unescaped characters/); +}