From 15231aa6e56d64813994e04293e5adaeba24d0b8 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 | 2 +- .../test-http-client-unescaped-path.js | 19 ++++++++++++------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index c07589e9c5da6f..26590b9aec2e23 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -40,7 +40,7 @@ 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 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/); +}