Skip to content

Commit

Permalink
Avoid an unlikely but theoretically possible redos
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
isaacs committed Aug 3, 2021
1 parent 0b78386 commit 06cbde5
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 6 deletions.
7 changes: 4 additions & 3 deletions lib/extract.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')

module.exports = (opt_, files, cb) => {
if (typeof opt_ === 'function')
Expand Down Expand Up @@ -41,7 +42,7 @@ 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) => {
Expand All @@ -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 => {
Expand Down
7 changes: 4 additions & 3 deletions lib/list.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')

module.exports = (opt_, files, cb) => {
if (typeof opt_ === 'function')
Expand Down Expand Up @@ -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) => {
Expand All @@ -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 => {
Expand Down
24 changes: 24 additions & 0 deletions lib/strip-trailing-slashes.js
Original file line number Diff line number Diff line change
@@ -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
}
7 changes: 7 additions & 0 deletions test/strip-trailing-slashes.js
Original file line number Diff line number Diff line change
@@ -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')

0 comments on commit 06cbde5

Please sign in to comment.