Skip to content

Commit 06cbde5

Browse files
committedAug 3, 2021
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.
1 parent 0b78386 commit 06cbde5

File tree

4 files changed

+39
-6
lines changed

4 files changed

+39
-6
lines changed
 

‎lib/extract.js

+4-3
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ const Unpack = require('./unpack.js')
66
const fs = require('fs')
77
const fsm = require('fs-minipass')
88
const path = require('path')
9+
const stripSlash = require('./strip-trailing-slashes.js')
910

1011
module.exports = (opt_, files, cb) => {
1112
if (typeof opt_ === 'function')
@@ -41,7 +42,7 @@ module.exports = (opt_, files, cb) => {
4142
// construct a filter that limits the file entries listed
4243
// include child entries if a dir is included
4344
const filesFilter = (opt, files) => {
44-
const map = new Map(files.map(f => [f.replace(/\/+$/, ''), true]))
45+
const map = new Map(files.map(f => [stripSlash(f), true]))
4546
const filter = opt.filter
4647

4748
const mapHas = (file, r) => {
@@ -55,8 +56,8 @@ const filesFilter = (opt, files) => {
5556
}
5657

5758
opt.filter = filter
58-
? (file, entry) => filter(file, entry) && mapHas(file.replace(/\/+$/, ''))
59-
: file => mapHas(file.replace(/\/+$/, ''))
59+
? (file, entry) => filter(file, entry) && mapHas(stripSlash(file))
60+
: file => mapHas(stripSlash(file))
6061
}
6162

6263
const extractFileSync = opt => {

‎lib/list.js

+4-3
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ const Parser = require('./parse.js')
99
const fs = require('fs')
1010
const fsm = require('fs-minipass')
1111
const path = require('path')
12+
const stripSlash = require('./strip-trailing-slashes.js')
1213

1314
module.exports = (opt_, files, cb) => {
1415
if (typeof opt_ === 'function')
@@ -54,7 +55,7 @@ const onentryFunction = opt => {
5455
// construct a filter that limits the file entries listed
5556
// include child entries if a dir is included
5657
const filesFilter = (opt, files) => {
57-
const map = new Map(files.map(f => [f.replace(/\/+$/, ''), true]))
58+
const map = new Map(files.map(f => [stripSlash(f), true]))
5859
const filter = opt.filter
5960

6061
const mapHas = (file, r) => {
@@ -68,8 +69,8 @@ const filesFilter = (opt, files) => {
6869
}
6970

7071
opt.filter = filter
71-
? (file, entry) => filter(file, entry) && mapHas(file.replace(/\/+$/, ''))
72-
: file => mapHas(file.replace(/\/+$/, ''))
72+
? (file, entry) => filter(file, entry) && mapHas(stripSlash(file))
73+
: file => mapHas(stripSlash(file))
7374
}
7475

7576
const listFileSync = opt => {

‎lib/strip-trailing-slashes.js

+24
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// this is the only approach that was significantly faster than using
2+
// str.replace(/\/+$/, '') for strings ending with a lot of / chars and
3+
// containing multiple / chars.
4+
const batchStrings = [
5+
'/'.repeat(1024),
6+
'/'.repeat(512),
7+
'/'.repeat(256),
8+
'/'.repeat(128),
9+
'/'.repeat(64),
10+
'/'.repeat(32),
11+
'/'.repeat(16),
12+
'/'.repeat(8),
13+
'/'.repeat(4),
14+
'/'.repeat(2),
15+
'/',
16+
]
17+
18+
module.exports = str => {
19+
for (const s of batchStrings) {
20+
while (str.length >= s.length && str.slice(-1 * s.length) === s)
21+
str = str.slice(0, -1 * s.length)
22+
}
23+
return str
24+
}

‎test/strip-trailing-slashes.js

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
const t = require('tap')
2+
const stripSlash = require('../lib/strip-trailing-slashes.js')
3+
const short = '///a///b///c///'
4+
const long = short.repeat(10) + '/'.repeat(1000000)
5+
6+
t.equal(stripSlash(short), '///a///b///c')
7+
t.equal(stripSlash(long), short.repeat(9) + '///a///b///c')

0 commit comments

Comments
 (0)
Please sign in to comment.