From cde2ca96e40593fde75b20d3348c2a7b7a71cb0f 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 423445d6a6f091..58fbf0f30a86de 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/); +}