From 576038fb611b7da94877360892653471d0f4eaab Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 20 Sep 2018 15:02:26 +0200 Subject: [PATCH] http: add --security-revert for CVE-2018-12116 PR-URL: https://github.com/nodejs-private/node-private/pull/146 Reviewed-By: Michael Dawson Reviewed-By: James M Snell Reviewed-By: Sakthipriyan Vairamani Reviewed-By: Anna Henningsen Reviewed-By: Timothy Gu Reviewed-By: Ruben Bridgewater --- lib/_http_client.js | 43 ++++++++++++++++++++++++++++++++++++++++++- src/node_revert.h | 4 ++-- 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index 20d05efb5ecb64..7b5798fe021a87 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -41,6 +41,36 @@ const { outHeadersKey, ondrain } = require('internal/http'); const { nextTick } = require('internal/process/next_tick'); const is_reused_symbol = require('internal/freelist').symbols.is_reused_symbol; +const REVERT_CVE_2018_12116 = process.REVERT_CVE_2018_12116; + +// DO NOT USE: this is insecure. See CVE-2018-12116. +// The actual list of disallowed characters in regexp form 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. So instead we restrict the +// filter to just control characters and spaces. +// +// This function is used in the case of small paths, where manual character code +// checks can greatly outperform the equivalent regexp (tested in V8 5.4). +function isInvalidPath(s) { + var i = 0; + if (s.charCodeAt(0) <= 32) return true; + if (++i >= s.length) return false; + if (s.charCodeAt(1) <= 32) return true; + if (++i >= s.length) return false; + if (s.charCodeAt(2) <= 32) return true; + if (++i >= s.length) return false; + if (s.charCodeAt(3) <= 32) return true; + if (++i >= s.length) return false; + if (s.charCodeAt(4) <= 32) return true; + if (++i >= s.length) return false; + if (s.charCodeAt(5) <= 32) return true; + ++i; + for (; i < s.length; ++i) + if (s.charCodeAt(i) <= 32) return true; + return false; +} const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/; function validateHost(host, name) { @@ -92,7 +122,18 @@ function ClientRequest(options, cb) { var path; if (options.path) { path = String(options.path); - if (INVALID_PATH_REGEX.test(path)) + var invalidPath; + if (REVERT_CVE_2018_12116) { + if (path.length <= 39) { // Determined experimentally in V8 5.4 + invalidPath = isInvalidPath(path); + } else { + invalidPath = /[\u0000-\u0020]/.test(path); + } + } else { + invalidPath = INVALID_PATH_REGEX.test(path); + } + + if (invalidPath) throw new TypeError('Request path contains unescaped characters'); } diff --git a/src/node_revert.h b/src/node_revert.h index c5963afeafd027..9b4f1a9bbf6a20 100644 --- a/src/node_revert.h +++ b/src/node_revert.h @@ -15,8 +15,8 @@ **/ namespace node { -#define SECURITY_REVERSIONS(XX) -// XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title") +#define SECURITY_REVERSIONS(XX) \ + XX(CVE_2018_12116, "CVE-2018-12116", "HTTP request splitting") enum reversion { #define V(code, ...) SECURITY_REVERT_##code,