From d3d5a4e4560f18131e64fc62f5a281b238ef2ecf Mon Sep 17 00:00:00 2001
From: isaacs <i@izs.me>
Date: Tue, 3 Aug 2021 14:26:08 -0700
Subject: [PATCH] Avoid an unlikely but theoretically possible redos

When stripping the trailing slash from `files` arguments, we were using
`f.replace(/\/+$/, '')`, which can get exponentially slow when `f`
contains many `/` characters.

Tested a few variants and found one that's faster than the regexp
replacement for short strings, long strings, and long strings containing
many instances of repeated `/` characters.

This is "unlikely but theoretically possible" because it requires that
the user is passing untrusted input into the `tar.extract()` or
`tar.list()` array of entries to parse/extract, which would be quite
unusual.
---
 lib/extract.js                 |  7 ++++---
 lib/list.js                    |  7 ++++---
 lib/strip-trailing-slashes.js  | 24 ++++++++++++++++++++++++
 test/strip-trailing-slashes.js |  7 +++++++
 4 files changed, 39 insertions(+), 6 deletions(-)
 create mode 100644 lib/strip-trailing-slashes.js
 create mode 100644 test/strip-trailing-slashes.js

diff --git a/lib/extract.js b/lib/extract.js
index cbb458a0..6d032016 100644
--- a/lib/extract.js
+++ b/lib/extract.js
@@ -6,6 +6,7 @@ const Unpack = require('./unpack.js')
 const fs = require('fs')
 const fsm = require('fs-minipass')
 const path = require('path')
+const stripSlash = require('./strip-trailing-slashes.js')
 
 const x = module.exports = (opt_, files, cb) => {
   if (typeof opt_ === 'function')
@@ -41,7 +42,7 @@ const x = module.exports = (opt_, files, cb) => {
 // construct a filter that limits the file entries listed
 // include child entries if a dir is included
 const filesFilter = (opt, files) => {
-  const map = new Map(files.map(f => [f.replace(/\/+$/, ''), true]))
+  const map = new Map(files.map(f => [stripSlash(f), true]))
   const filter = opt.filter
 
   const mapHas = (file, r) => {
@@ -55,8 +56,8 @@ const filesFilter = (opt, files) => {
   }
 
   opt.filter = filter
-    ? (file, entry) => filter(file, entry) && mapHas(file.replace(/\/+$/, ''))
-    : file => mapHas(file.replace(/\/+$/, ''))
+    ? (file, entry) => filter(file, entry) && mapHas(stripSlash(file))
+    : file => mapHas(stripSlash(file))
 }
 
 const extractFileSync = opt => {
diff --git a/lib/list.js b/lib/list.js
index 9da3f812..6b25d971 100644
--- a/lib/list.js
+++ b/lib/list.js
@@ -9,6 +9,7 @@ const Parser = require('./parse.js')
 const fs = require('fs')
 const fsm = require('fs-minipass')
 const path = require('path')
+const stripSlash = require('./strip-trailing-slashes.js')
 
 const t = module.exports = (opt_, files, cb) => {
   if (typeof opt_ === 'function')
@@ -54,7 +55,7 @@ const onentryFunction = opt => {
 // construct a filter that limits the file entries listed
 // include child entries if a dir is included
 const filesFilter = (opt, files) => {
-  const map = new Map(files.map(f => [f.replace(/\/+$/, ''), true]))
+  const map = new Map(files.map(f => [stripSlash(f), true]))
   const filter = opt.filter
 
   const mapHas = (file, r) => {
@@ -68,8 +69,8 @@ const filesFilter = (opt, files) => {
   }
 
   opt.filter = filter
-    ? (file, entry) => filter(file, entry) && mapHas(file.replace(/\/+$/, ''))
-    : file => mapHas(file.replace(/\/+$/, ''))
+    ? (file, entry) => filter(file, entry) && mapHas(stripSlash(file))
+    : file => mapHas(stripSlash(file))
 }
 
 const listFileSync = opt => {
diff --git a/lib/strip-trailing-slashes.js b/lib/strip-trailing-slashes.js
new file mode 100644
index 00000000..f702ed5a
--- /dev/null
+++ b/lib/strip-trailing-slashes.js
@@ -0,0 +1,24 @@
+// this is the only approach that was significantly faster than using
+// str.replace(/\/+$/, '') for strings ending with a lot of / chars and
+// containing multiple / chars.
+const batchStrings = [
+  '/'.repeat(1024),
+  '/'.repeat(512),
+  '/'.repeat(256),
+  '/'.repeat(128),
+  '/'.repeat(64),
+  '/'.repeat(32),
+  '/'.repeat(16),
+  '/'.repeat(8),
+  '/'.repeat(4),
+  '/'.repeat(2),
+  '/',
+]
+
+module.exports = str => {
+  for (const s of batchStrings) {
+    while (str.length >= s.length && str.slice(-1 * s.length) === s)
+      str = str.slice(0, -1 * s.length)
+  }
+  return str
+}
diff --git a/test/strip-trailing-slashes.js b/test/strip-trailing-slashes.js
new file mode 100644
index 00000000..198797bf
--- /dev/null
+++ b/test/strip-trailing-slashes.js
@@ -0,0 +1,7 @@
+const t = require('tap')
+const stripSlash = require('../lib/strip-trailing-slashes.js')
+const short = '///a///b///c///'
+const long = short.repeat(10) + '/'.repeat(1000000)
+
+t.equal(stripSlash(short), '///a///b///c')
+t.equal(stripSlash(long), short.repeat(9) + '///a///b///c')