Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CVE-2023-28155 by patching request module in nodejs #5562

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
211 changes: 211 additions & 0 deletions SPECS/nodejs/CVE-2023-28155.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
Fixes CVE-2023-28155: https://nvd.nist.gov/vuln/detail/CVE-2023-28155, which is a vulnerability
in the the request module that is used by this package.

Note that request is deprecated (see https://github.com/request/request for details), so this
has not and will never be fixed in the request module itself. However, there is a pull request
that fixes it.

Adapted by tobiasb@microsoft.com from a pull request for a patch to request:
https://github.com/request/request/pull/3444/files

From d42332182512e56ba68446f49c3e3711e04301a2 Mon Sep 17 00:00:00 2001
From: <redacted>
Date: Sun, 12 Mar 2023 19:47:24 +0100
Subject: [PATCH 1/5] Added option "allowInsecureRedirect"

---
PATCH NOTE -- ORIGINAL:
lib/redirect.js | 6 +++++-
PATCH NOTE -- UPDATE:
deps/npm/node_modules/request/lib/redirect.js | 6 +++++-

PATCH NOTE: These tests are not included in the module we use, so they are not included in this patch.
tests/test-httpModule.js | 3 ++-
tests/test-redirect.js | 15 ++++++++++++++-

PATCH NOTE -- ORIGINAL:
3 files changed, 21 insertions(+), 3 deletions(-)
PATCH NOTE -- UPDATED:
1 file changed, 5 insertions(+), 1 deletion(-)

# PATCH NOTE -- ORIGINAL:
#diff --git a/lib/redirect.js b/lib/redirect.js
# PATCH NOTE -- UPDATED with path used within the source tarball:
diff --git a/deps/npm/node_modules/request/lib/redirect.js b/deps/npm/node_modules/request/lib/redirect.js

index b9150e77c..770c7f41b 100644
# PATCH NOTE -- ORIGINAL:
# --- a/lib/redirect.js
# +++ b/lib/redirect.js
# PATCH NOTE -- UPDATED with path used within the source tarball:
--- a/deps/npm/node_modules/request/lib/redirect.js
+++ b/deps/npm/node_modules/request/lib/redirect.js

@@ -14,6 +14,7 @@ function Redirect (request) {
this.redirects = []
this.redirectsFollowed = 0
this.removeRefererHeader = false
+ this.allowInsecureRedirect = false
}

Redirect.prototype.onRequest = function (options) {
@@ -40,6 +41,9 @@ Redirect.prototype.onRequest = function (options) {
if (options.followOriginalHttpMethod !== undefined) {
self.followOriginalHttpMethod = options.followOriginalHttpMethod
}
+ if (options.allowInsecureRedirect !== undefined) {
+ self.allowInsecureRedirect = options.allowInsecureRedirect;
+ }
}

Redirect.prototype.redirectTo = function (response) {
@@ -108,7 +112,7 @@ Redirect.prototype.onResponse = function (response) {
request.uri = url.parse(redirectTo)

// handle the case where we change protocol from https to http or vice versa
- if (request.uri.protocol !== uriPrev.protocol) {
+ if (request.uri.protocol !== uriPrev.protocol && self.allowInsecureRedirect) {
delete request.agent
}

# PATCH NOTE: The rest of the diffs are not applied because they are tests and not
# included in the source tarball.
# diff --git a/tests/test-httpModule.js b/tests/test-httpModule.js
# index 4d4e236bf..a59c427b1 100644
# --- a/tests/test-httpModule.js
# +++ b/tests/test-httpModule.js
# @@ -70,7 +70,8 @@ function runTests (name, httpModules) {
# tape(name, function (t) {
# var toHttps = 'http://localhost:' + plainServer.port + '/to_https'
# var toPlain = 'https://localhost:' + httpsServer.port + '/to_plain'
# - var options = { httpModules: httpModules, strictSSL: false }
# + var options = { httpModules: httpModules, strictSSL: false, allowInsecureRedirect: true }
# + var optionsSecure = { httpModules: httpModules, strictSSL: false }
# var modulesTest = httpModules || {}

# clearFauxRequests()
# diff --git a/tests/test-redirect.js b/tests/test-redirect.js
# index b7b5ca676..48b4982e4 100644
# --- a/tests/test-redirect.js
# +++ b/tests/test-redirect.js
# @@ -345,7 +345,8 @@ tape('http to https redirect', function (t) {
# hits = {}
# request.get({
# uri: require('url').parse(s.url + '/ssl'),
# - rejectUnauthorized: false
# + rejectUnauthorized: false,
# + allowInsecureRedirect: true
# }, function (err, res, body) {
# t.equal(err, null)
# t.equal(res.statusCode, 200)
# @@ -354,6 +355,18 @@ tape('http to https redirect', function (t) {
# })
# })

# +tape('http to https redirect should fail without the explicit "allowInsecureRedirect" option', function (t) {
# + hits = {}
# + request.get({
# + uri: require('url').parse(s.url + '/ssl'),
# + rejectUnauthorized: false
# + }, function (err, res, body) {
# + t.notEqual(err, null)
# + t.equal(err.code, "ERR_INVALID_PROTOCOL","Failed to cross-protocol redirect")
# + t.end()
# + })
# +})
# +
# tape('should have referer header by default when following redirect', function (t) {
# request.post({
# uri: s.url + '/temp',

# From 9d69d750f39cc5ab6f3b011e17472bc28b14dc22 Mon Sep 17 00:00:00 2001
# From: Szymon Drosdzol <szymon@doyensec.com>
# Date: Sun, 12 Mar 2023 19:50:09 +0100
# Subject: [PATCH 2/5] Documented allowInsecureRedirect in Readme

# ---
# README.md | 1 +
# 1 file changed, 1 insertion(+)

# diff --git a/README.md b/README.md
# index 42290d5ce..dd432a768 100644
# --- a/README.md
# +++ b/README.md
# @@ -809,6 +809,7 @@ The first argument can be either a `url` or an `options` object. The only requir
# - `followOriginalHttpMethod` - by default we redirect to HTTP method GET. you can enable this property to redirect to the original HTTP method (default: `false`)
# - `maxRedirects` - the maximum number of redirects to follow (default: `10`)
# - `removeRefererHeader` - removes the referer header when a redirect happens (default: `false`). **Note:** if true, referer header set in the initial request is preserved during redirect chain.
# +- `allowInsecureRedirect` - allows cross-protocol redirects (HTTP to HTTPS and vice versa). **Warning:** may lead to bypassing anti SSRF filters

# ---


# From 8a15249d182e54a261b1539846f76d913a6904f4 Mon Sep 17 00:00:00 2001
# From: SzymonDrosdzol <84710686+SzymonDrosdzol@users.noreply.github.com>
# Date: Fri, 17 Mar 2023 10:09:46 +0100
# Subject: [PATCH 3/5] Removed semicolon

# Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>
# ---
# lib/redirect.js | 2 +-
# 1 file changed, 1 insertion(+), 1 deletion(-)

# diff --git a/lib/redirect.js b/lib/redirect.js
# index 770c7f41b..2864f9f2a 100644
# --- a/lib/redirect.js
# +++ b/lib/redirect.js
# @@ -42,7 +42,7 @@ Redirect.prototype.onRequest = function (options) {
# self.followOriginalHttpMethod = options.followOriginalHttpMethod
# }
# if (options.allowInsecureRedirect !== undefined) {
# - self.allowInsecureRedirect = options.allowInsecureRedirect;
# + self.allowInsecureRedirect = options.allowInsecureRedirect
# }
# }


# From 8535868fc88f24ed652d3f290bfd553a2cdbb811 Mon Sep 17 00:00:00 2001
# From: SzymonDrosdzol <84710686+SzymonDrosdzol@users.noreply.github.com>
# Date: Fri, 17 Mar 2023 10:12:09 +0100
# Subject: [PATCH 4/5] Code style fix

# Co-authored-by: Kevin van Rijn <6368561+kevinvanrijn@users.noreply.github.com>
# ---
# tests/test-redirect.js | 2 +-
# 1 file changed, 1 insertion(+), 1 deletion(-)

# diff --git a/tests/test-redirect.js b/tests/test-redirect.js
# index 48b4982e4..3e1957604 100644
# --- a/tests/test-redirect.js
# +++ b/tests/test-redirect.js
# @@ -362,7 +362,7 @@ tape('http to https redirect should fail without the explicit "allowInsecureRedi
# rejectUnauthorized: false
# }, function (err, res, body) {
# t.notEqual(err, null)
# - t.equal(err.code, "ERR_INVALID_PROTOCOL","Failed to cross-protocol redirect")
# + t.equal(err.code, 'ERR_INVALID_PROTOCOL', 'Failed to cross-protocol redirect')
# t.end()
# })
# })

# From 43647c4bd6e451f350267d5236463b4248dbc8df Mon Sep 17 00:00:00 2001
# From: SzymonDrosdzol <84710686+SzymonDrosdzol@users.noreply.github.com>
# Date: Fri, 17 Mar 2023 10:22:33 +0100
# Subject: [PATCH 5/5] Removed leftover declaration

# ---
# tests/test-httpModule.js | 1 -
# 1 file changed, 1 deletion(-)

# diff --git a/tests/test-httpModule.js b/tests/test-httpModule.js
# index a59c427b1..f12382fe6 100644
# --- a/tests/test-httpModule.js
# +++ b/tests/test-httpModule.js
# @@ -71,7 +71,6 @@ function runTests (name, httpModules) {
# var toHttps = 'http://localhost:' + plainServer.port + '/to_https'
# var toPlain = 'https://localhost:' + httpsServer.port + '/to_plain'
# var options = { httpModules: httpModules, strictSSL: false, allowInsecureRedirect: true }
# - var optionsSecure = { httpModules: httpModules, strictSSL: false }
# var modulesTest = httpModules || {}

# clearFauxRequests()
8 changes: 6 additions & 2 deletions SPECS/nodejs/nodejs.spec
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
Summary: A JavaScript runtime built on Chrome's V8 JavaScript engine.
Name: nodejs
Version: 14.21.1
Release: 1%{?dist}
Release: 2%{?dist}
License: BSD and MIT and Public Domain and naist-2003
Vendor: Microsoft Corporation
Distribution: Mariner
Group: Applications/System
URL: https://github.com/nodejs/node
# !!!! Nodejs code has a vendored version of OpenSSL code that must be removed from source tarball
# !!!! Nodejs code has a vendored version of OpenSSL code that must be removed from source tarball
# !!!! because it contains patented algorithms.
# !!! => use clean-source-tarball.sh script to create a clean and reproducible source tarball.
Source0: https://nodejs.org/download/release/v%{version}/node-v%{version}.tar.xz
Patch0: patch_tls_nodejs14.patch
Patch1: remove_unsupported_tlsv13_ciphers.patch
Patch2: CVE-2023-28155.patch
BuildRequires: coreutils >= 8.22
BuildRequires: openssl-devel >= 1.1.1
BuildRequires: python3
Expand Down Expand Up @@ -79,6 +80,9 @@ make cctest
%{_datadir}/systemtap/tapset/node.stp

%changelog
* Thu May 25 2023 Tobias Brick <tobiasb@microsoft.com> - 14.21.1-2
- Add patch to fix CVE-2023-28155

* Sun Dec 11 2022 CBL-Mariner Servicing Account <cblmargh@microsoft.com> - 14.21.1-1
- Auto-upgrade to 14.21.1 - CVE-2022-3602_CVE-2022-3786_CVE-2022-43548

Expand Down