From 7b9f4844f67a92a3af1b4c5977f16549ca6e848d Mon Sep 17 00:00:00 2001 From: Colin Jones Date: Wed, 26 Feb 2020 20:27:37 -0600 Subject: [PATCH 01/11] Get tests back to passing with ava/xo releases --- package.json | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index ff67270..bea02ae 100644 --- a/package.json +++ b/package.json @@ -10,7 +10,7 @@ "url": "github.com/kevva" }, "engines": { - "node": ">=4" + "node": ">=7.6.0" }, "scripts": { "test": "xo && ava" @@ -41,9 +41,20 @@ }, "devDependencies": { "ava": "*", + "esm": "^3.2.25", "is-jpg": "^1.0.0", "path-exists": "^3.0.0", "pify": "^2.3.0", "xo": "*" + }, + "ava": { + "require": [ + "esm" + ] + }, + "xo": { + "rules": { + "promise/prefer-await-to-then": "off" + } } } From 3788e0366bf2811a13e371a8cb04ebc6c3c356ef Mon Sep 17 00:00:00 2001 From: Colin Jones Date: Wed, 26 Feb 2020 20:29:08 -0600 Subject: [PATCH 02/11] Throw rather than allowing directory traversal --- fixtures/slipping.tar.gz | Bin 0 -> 188 bytes index.js | 4 ++++ test.js | 6 ++++++ 3 files changed, 10 insertions(+) create mode 100644 fixtures/slipping.tar.gz diff --git a/fixtures/slipping.tar.gz b/fixtures/slipping.tar.gz new file mode 100644 index 0000000000000000000000000000000000000000..e03782a03a7c2a21b9fc7ec3cc6582e3a2088db6 GIT binary patch literal 188 zcmb2|=HO^k3Xfy@U!0R!P>`9Iu2+&+#PIg~Uamt1B5e=P#JT1jn03W$uB$ailR)=_ zh%ASNQJ#;_KaJ*Un#|Xvv4@}i-cy};k={lBwyn$JQ-6P}OroZ;aG&mLjXO<0=DeDr z@^bdfE&0y_mp*cNv9U8)ID2lY(@USFD<^o>9Nfvfbb3&T?_T?TKX$*Hv-*Bdjg;!Y kBRP4WGdFILj=XWwYWwPwNeT=o!2Uhoz8eu27&I6d08nF9SpWb4 literal 0 HcmV?d00001 diff --git a/index.js b/index.js index 9193ebd..ba3ec6a 100644 --- a/index.js +++ b/index.js @@ -43,6 +43,10 @@ const extractFile = (input, output, opts) => runPlugins(input, opts).then(files return Promise.all(files.map(x => { const dest = path.join(output, x.path); + if (dest.match(/\.\./)) { + throw (new Error('File path contains "..":', dest)); + } + const mode = x.mode & ~process.umask(); const now = new Date(); diff --git a/test.js b/test.js index d145ba0..04bcc28 100644 --- a/test.js +++ b/test.js @@ -103,3 +103,9 @@ test('return emptpy array if no plugins are set', async t => { const files = await m(path.join(__dirname, 'fixtures', 'file.tar'), {plugins: []}); t.is(files.length, 0); }); + +test('throw when a location outside the root is given', async t => { + await t.throwsAsync(async () => { + await m(path.join(__dirname, 'fixtures', 'slipping.tar.gz'), 'dist'); + }, {message: /File path contains "\.\."/}); +}); From 1a05f019ad7d0e6cee1a6f2576a63deb8e5fd26b Mon Sep 17 00:00:00 2001 From: Colin Jones Date: Thu, 27 Feb 2020 06:59:46 -0600 Subject: [PATCH 03/11] Handle dots within filenames --- fixtures/edge_case_dots.tar.gz | Bin 0 -> 211 bytes index.js | 2 +- test.js | 6 ++++++ 3 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 fixtures/edge_case_dots.tar.gz diff --git a/fixtures/edge_case_dots.tar.gz b/fixtures/edge_case_dots.tar.gz new file mode 100644 index 0000000000000000000000000000000000000000..c51458079a678a8f9e25e0b365def16efd1f07bb GIT binary patch literal 211 zcmV;^04)C>iwFS9xmR8Q1MQPR3WG2ZMP2t4If0EanVg`Do43PR~rDk&LHqyQB$6=a(Q z_ul56Tcv25+N|7r+z^|U^ZBEg=PiSQbN{um*qOW;x;VB^>tTlb4{%z4`%hruUuy{w z;~r*ja&Z6GNjQ58KmB_fVs94tM^o0On$quU#&-VYH~&Jo`~#$%^1la{K{%zKAPC}U Nya1>-pL+lf001c9Vv7I( literal 0 HcmV?d00001 diff --git a/index.js b/index.js index ba3ec6a..19bc34f 100644 --- a/index.js +++ b/index.js @@ -43,7 +43,7 @@ const extractFile = (input, output, opts) => runPlugins(input, opts).then(files return Promise.all(files.map(x => { const dest = path.join(output, x.path); - if (dest.match(/\.\./)) { + if (dest.match(/\.\.\//)) { throw (new Error('File path contains "..":', dest)); } diff --git a/test.js b/test.js index 04bcc28..19f5665 100644 --- a/test.js +++ b/test.js @@ -109,3 +109,9 @@ test('throw when a location outside the root is given', async t => { await m(path.join(__dirname, 'fixtures', 'slipping.tar.gz'), 'dist'); }, {message: /File path contains "\.\."/}); }); + +test('allows filenames to be written with dots in them', async t => { + const files = await m(path.join(__dirname, 'fixtures', 'edge_case_dots.tar.gz'), __dirname); + t.is(files.length, 3); + await fsP.rmdir(path.join(__dirname, 'edge_case_dots'), {recursive: true}); +}); From 3a60aaacb747442d6ccafd63bd4042e7d78335e8 Mon Sep 17 00:00:00 2001 From: Colin Jones Date: Thu, 27 Feb 2020 10:59:52 -0600 Subject: [PATCH 04/11] Require files to be in given output directory --- fixtures/edge_case_dots.tar.gz | Bin 211 -> 318 bytes fixtures/slip.zip | Bin 0 -> 1953 bytes index.js | 15 ++++++++++----- test.js | 20 +++++++++++++++++--- 4 files changed, 27 insertions(+), 8 deletions(-) create mode 100644 fixtures/slip.zip diff --git a/fixtures/edge_case_dots.tar.gz b/fixtures/edge_case_dots.tar.gz index c51458079a678a8f9e25e0b365def16efd1f07bb..935f979ddf2c3443430b331aaac7e1db35042600 100644 GIT binary patch literal 318 zcmV-E0m1$siwFQ_zE@rV1MQbhYQr!Pg}v@6bOKlZx9#;-Zo5Vu8c5@X+oLg zS`18&-r088m1Q`E3^n5W&{9t5 zA2lEKmnt5l%owQ)LNnB!W#jt4je?V_;7@#)S2u1yrhl#)^Z74-@}Kjlzt)0c{;$F7AULM~R!RJSTB61S^Pe_SUFUzP8Tx+> z-W`zYV0&A|IS><{9 literal 211 zcmV;^04)C>iwFS9xmR8Q1MQPR3WG2ZMP2t4If0EanVg`Do43PR~rDk&LHqyQB$6=a(Q z_ul56Tcv25+N|7r+z^|U^ZBEg=PiSQbN{um*qOW;x;VB^>tTlb4{%z4`%hruUuy{w z;~r*ja&Z6GNjQ58KmB_fVs94tM^o0On$quU#&-VYH~&Jo`~#$%^1la{K{%zKAPC}U Nya1>-pL+lf001c9Vv7I( diff --git a/fixtures/slip.zip b/fixtures/slip.zip new file mode 100644 index 0000000000000000000000000000000000000000..d94b89d549138c8174bf254f6b67c9fe1e719362 GIT binary patch literal 1953 zcmWIWW@h1H00A|BAO9|)F*&xi%Aj42xnVXZDmmOb{A77N8UlJO^$-o@mzAzSq zODnh;7+GF0GcbS&{Qwk`dfQxbfl>@0tOztIJvA@2C^I=eC9_B$$<%_xqSU++kSIR0 z_4Lq8_xR`?B?vSbgikXOVY)s+`$tuPVi^%1qv1h=@IXrWp!}p?l3So(oRe8lkeU)- zm06%yQh_&*MP(K!lw>59C_wda1$Z+u$uZ-~zY@UQ0RjIPf@n0`Lqk|0xfv}dqZ^8v zm5~kg0vbv%V-af&YMzEz11g8MG_D}Vn$h%1gY-%;az{fQE!5HS81@{FE8nrQ0aH4# Q%4B9Z3#>Qyf$B~M0Ci`!G5`Po literal 0 HcmV?d00001 diff --git a/index.js b/index.js index 19bc34f..f2e11fb 100644 --- a/index.js +++ b/index.js @@ -43,10 +43,6 @@ const extractFile = (input, output, opts) => runPlugins(input, opts).then(files return Promise.all(files.map(x => { const dest = path.join(output, x.path); - if (dest.match(/\.\.\//)) { - throw (new Error('File path contains "..":', dest)); - } - const mode = x.mode & ~process.umask(); const now = new Date(); @@ -56,7 +52,16 @@ const extractFile = (input, output, opts) => runPlugins(input, opts).then(files .then(() => x); } - return makeDir(path.dirname(dest)) + return makeDir(output) + .then(() => makeDir(path.dirname(dest))) + .then(() => { + return Promise.all([fsP.realpath(path.dirname(dest)), fsP.realpath(output)]) + .then(([realDestinationDir, realOutputDir]) => { + if (realDestinationDir.indexOf(realOutputDir) !== 0) { + throw (new Error('Refusing to write outside output directory: ' + realDestinationDir)); + } + }); + }) .then(() => { if (x.type === 'link') { return fsP.link(x.linkname, dest); diff --git a/test.js b/test.js index 19f5665..527b160 100644 --- a/test.js +++ b/test.js @@ -107,11 +107,25 @@ test('return emptpy array if no plugins are set', async t => { test('throw when a location outside the root is given', async t => { await t.throwsAsync(async () => { await m(path.join(__dirname, 'fixtures', 'slipping.tar.gz'), 'dist'); - }, {message: /File path contains "\.\."/}); + }, {message: /Refusing to write/}); }); -test('allows filenames to be written with dots in them', async t => { +test('throw when a location outside the root including symlinks is given', async t => { + await t.throwsAsync(async () => { + await m(path.join(__dirname, 'fixtures', 'slip.zip'), 'dist'); + }, {message: /Refusing to write/}); +}); + +test('allows filenames and directories to be written with dots in their names', async t => { const files = await m(path.join(__dirname, 'fixtures', 'edge_case_dots.tar.gz'), __dirname); - t.is(files.length, 3); + t.is(files.length, 6); + t.deepEqual(files.map(f => f.path).sort(), [ + 'edge_case_dots/', + 'edge_case_dots/internal_dots..txt', + 'edge_case_dots/sample../', + 'edge_case_dots/ending_dots..', + 'edge_case_dots/x', + 'edge_case_dots/sample../test.txt' + ].sort()); await fsP.rmdir(path.join(__dirname, 'edge_case_dots'), {recursive: true}); }); From 432db78eb47dcf268163bda4006f4b2f4e539ef5 Mon Sep 17 00:00:00 2001 From: Colin Jones Date: Thu, 27 Feb 2020 13:30:20 -0600 Subject: [PATCH 05/11] Update tested node versions --- .travis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 57505cf..f09222f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,6 +1,6 @@ sudo: false language: node_js node_js: + - '12' + - '10' - '8' - - '6' - - '4' From 8850e7ec79bb0cbbdb9955c9edef7ad36114ee51 Mon Sep 17 00:00:00 2001 From: Colin Jones Date: Thu, 27 Feb 2020 13:52:52 -0600 Subject: [PATCH 06/11] Prevent directories creation outside output path --- fixtures/slipping_directory.tar.gz | Bin 0 -> 161 bytes index.js | 22 ++++++++++++++++++++-- test.js | 10 ++++++++-- 3 files changed, 28 insertions(+), 4 deletions(-) create mode 100644 fixtures/slipping_directory.tar.gz diff --git a/fixtures/slipping_directory.tar.gz b/fixtures/slipping_directory.tar.gz new file mode 100644 index 0000000000000000000000000000000000000000..0ff297c3b9fba971ad38165b74a23cbd8fedafaf GIT binary patch literal 161 zcmb2|=HPG>iHKwRU!0R!P>`9I9-op~l$u_Pirkvw=YCLrX6|0pZg} z9(UYuXpQjRGUeVrN5NfL=37_)PkwOrQSQ90Q)Bbr9gFUITX|w*{IWl5o}|ZIT#+Za zP3K!oeLYuc!9G3L!nmu}hFbLn^QY@cEB~F+_PYJvW1-|)*=Qjaib HFfafB0aZnQ literal 0 HcmV?d00001 diff --git a/index.js b/index.js index f2e11fb..c4ff7dc 100644 --- a/index.js +++ b/index.js @@ -19,6 +19,21 @@ const runPlugins = (input, opts) => { return Promise.all(opts.plugins.map(x => x(input, opts))).then(files => files.reduce((a, b) => a.concat(b))); }; +const safeMakeDir = (dir, realOutputPath) => { + return fsP.realpath(dir) + .catch(_ => { + const parent = path.dirname(dir); + return safeMakeDir(parent, realOutputPath); + }) + .then(realParentPath => { + if (realParentPath.indexOf(realOutputPath) !== 0) { + throw (new Error('Refusing to create a directory outside the output path.')); + } + + return makeDir(dir); + }); +}; + const extractFile = (input, output, opts) => runPlugins(input, opts).then(files => { if (opts.strip > 0) { files = files @@ -47,13 +62,16 @@ const extractFile = (input, output, opts) => runPlugins(input, opts).then(files const now = new Date(); if (x.type === 'directory') { - return makeDir(dest) + return makeDir(output) + .then(outputPath => fsP.realpath(outputPath)) + .then(realOutputPath => safeMakeDir(dest, realOutputPath)) .then(() => fsP.utimes(dest, now, x.mtime)) .then(() => x); } return makeDir(output) - .then(() => makeDir(path.dirname(dest))) + .then(outputPath => fsP.realpath(outputPath)) + .then(realOutputPath => safeMakeDir(path.dirname(dest), realOutputPath)) .then(() => { return Promise.all([fsP.realpath(path.dirname(dest)), fsP.realpath(output)]) .then(([realDestinationDir, realOutputDir]) => { diff --git a/test.js b/test.js index 527b160..5484c27 100644 --- a/test.js +++ b/test.js @@ -107,13 +107,19 @@ test('return emptpy array if no plugins are set', async t => { test('throw when a location outside the root is given', async t => { await t.throwsAsync(async () => { await m(path.join(__dirname, 'fixtures', 'slipping.tar.gz'), 'dist'); - }, {message: /Refusing to write/}); + }, {message: /Refusing/}); }); test('throw when a location outside the root including symlinks is given', async t => { await t.throwsAsync(async () => { await m(path.join(__dirname, 'fixtures', 'slip.zip'), 'dist'); - }, {message: /Refusing to write/}); + }, {message: /Refusing/}); +}); + +test('throw when a directory outside the root including symlinks is given', async t => { + await t.throwsAsync(async () => { + await m(path.join(__dirname, 'fixtures', 'slipping_directory.tar.gz'), 'dist'); + }, {message: /Refusing/}); }); test('allows filenames and directories to be written with dots in their names', async t => { From 949027c40e5d44d7b69d3b260e1ce1e81a5ece12 Mon Sep 17 00:00:00 2001 From: Colin Jones Date: Thu, 27 Feb 2020 16:52:35 -0600 Subject: [PATCH 07/11] Accommodate Node 10's fs.rmdir API with rimraf Also ensures cleanup of decompressed files/directories. Removes Node 8 from the build matrix. The current version of ava is not compatible with Node 8, and Node 8 is in general now at end-of-life. --- .travis.yml | 1 - package.json | 1 + test.js | 14 +++++++++----- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/.travis.yml b/.travis.yml index f09222f..2348d90 100644 --- a/.travis.yml +++ b/.travis.yml @@ -3,4 +3,3 @@ language: node_js node_js: - '12' - '10' - - '8' diff --git a/package.json b/package.json index bea02ae..1ee81e2 100644 --- a/package.json +++ b/package.json @@ -45,6 +45,7 @@ "is-jpg": "^1.0.0", "path-exists": "^3.0.0", "pify": "^2.3.0", + "rimraf": "^3.0.2", "xo": "*" }, "ava": { diff --git a/test.js b/test.js index 5484c27..33dd3a3 100644 --- a/test.js +++ b/test.js @@ -3,10 +3,19 @@ import path from 'path'; import isJpg from 'is-jpg'; import pathExists from 'path-exists'; import pify from 'pify'; +import rimraf from 'rimraf'; import test from 'ava'; import m from '.'; const fsP = pify(fs); +const rimrafP = pify(rimraf); + +test.after('ensure decompressed files and directories are cleaned up', async () => { + await rimrafP(path.join(__dirname, 'directory')); + await rimrafP(path.join(__dirname, 'dist')); + await rimrafP(path.join(__dirname, 'edge_case_dots')); + await rimrafP(path.join(__dirname, 'test.jpg')); +}); test('extract file', async t => { const tarFiles = await m(path.join(__dirname, 'fixtures', 'file.tar')); @@ -46,8 +55,6 @@ test.serial('extract file to directory', async t => { t.is(files[0].path, 'test.jpg'); t.true(isJpg(files[0].data)); t.true(await pathExists(path.join(__dirname, 'test.jpg'))); - - await fsP.unlink(path.join(__dirname, 'test.jpg')); }); test('extract symlink', async t => { @@ -60,7 +67,6 @@ test('extract symlink', async t => { test('extract directory', async t => { await m(path.join(__dirname, 'fixtures', 'directory.tar'), __dirname); t.true(await pathExists(path.join(__dirname, 'directory'))); - await fsP.rmdir(path.join(__dirname, 'directory')); }); test('strip option', async t => { @@ -96,7 +102,6 @@ test.serial('set mtime', async t => { const files = await m(path.join(__dirname, 'fixtures', 'file.tar'), __dirname); const stat = await fsP.stat(path.join(__dirname, 'test.jpg')); t.deepEqual(files[0].mtime, stat.mtime); - await fsP.unlink(path.join(__dirname, 'test.jpg')); }); test('return emptpy array if no plugins are set', async t => { @@ -133,5 +138,4 @@ test('allows filenames and directories to be written with dots in their names', 'edge_case_dots/x', 'edge_case_dots/sample../test.txt' ].sort()); - await fsP.rmdir(path.join(__dirname, 'edge_case_dots'), {recursive: true}); }); From 2d797ce64e37f711c1d42be1fc64084789abda24 Mon Sep 17 00:00:00 2001 From: Colin Jones Date: Thu, 27 Feb 2020 16:59:31 -0600 Subject: [PATCH 08/11] Add Node 13 to the build matrix --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index 2348d90..96c6315 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,5 +1,6 @@ sudo: false language: node_js node_js: + - '13' - '12' - '10' From 325a071b8bc1e8817a78a5ffc3ea44639f2a4a2d Mon Sep 17 00:00:00 2001 From: Colin Jones Date: Fri, 28 Feb 2020 06:46:16 -0600 Subject: [PATCH 09/11] Prevent file writes through a symlink When applied to a path containing a symlink, `fs.writeFile` will write to the place that symlink points. --- fixtures/slip2.zip | Bin 0 -> 1948 bytes fixtures/top_level_example.tar.gz | Bin 0 -> 113 bytes index.js | 28 +++++++++++++++++++++++----- test.js | 29 +++++++++++++++++++++-------- 4 files changed, 44 insertions(+), 13 deletions(-) create mode 100644 fixtures/slip2.zip create mode 100644 fixtures/top_level_example.tar.gz diff --git a/fixtures/slip2.zip b/fixtures/slip2.zip new file mode 100644 index 0000000000000000000000000000000000000000..bd6180b099f75cf131f29702d4ac288743235ce9 GIT binary patch literal 1948 zcmWIWW@h1H00A}Qm;n1Qg;`=iHV6wc$S@S=WEK>pro>le7Q`Frl~k03hHx@4TP%r) z1L4vNZU#n{7t9O{U_!qnw?H35M*xZiy=^YJK=lkDtO&FqJvA@2C^I=eC9_DsxH2~< zGcP;7BtO0&u_!gK1SE>jLOnfwRMThwdK@MWG#P|dJTNj1 zzEmHTS)fpokyxSt)uoV_o|u`(72wUtB*%;^D@y=#1O)tF2%@pr$O_5WXqg(_Skw%R zY-}RXSc2IpG=w;7P;)ZG8b+W6TN?KhW6fwPr9mns7`daNjuz@9 runPlugins(input, opts).then(files return makeDir(output) .then(outputPath => fsP.realpath(outputPath)) - .then(realOutputPath => safeMakeDir(path.dirname(dest), realOutputPath)) - .then(() => { - return Promise.all([fsP.realpath(path.dirname(dest)), fsP.realpath(output)]) - .then(([realDestinationDir, realOutputDir]) => { - if (realDestinationDir.indexOf(realOutputDir) !== 0) { + .then(realOutputPath => { + return safeMakeDir(path.dirname(dest), realOutputPath) + .then(() => realOutputPath); + }) + .then(realOutputPath => { + // Check for symlinks pointing outside output directory + return fsP.readlink(dest) + .catch(_ => { + // File doesn't exist yet + return null; + }) + .then(symlinkPointsTo => { + if (symlinkPointsTo && symlinkPointsTo.indexOf(realOutputPath) !== 0) { + throw (new Error('Refusing to write into a file outside output directory, through a symlink: ' + symlinkPointsTo)); + } + + return realOutputPath; + }); + }) + .then(realOutputPath => { + return fsP.realpath(path.dirname(dest)) + .then(realDestinationDir => { + if (realDestinationDir.indexOf(realOutputPath) !== 0) { throw (new Error('Refusing to write outside output directory: ' + realDestinationDir)); } }); diff --git a/test.js b/test.js index 33dd3a3..52557c0 100644 --- a/test.js +++ b/test.js @@ -10,10 +10,13 @@ import m from '.'; const fsP = pify(fs); const rimrafP = pify(rimraf); -test.after('ensure decompressed files and directories are cleaned up', async () => { +test.serial.afterEach('ensure decompressed files and directories are cleaned up', async () => { await rimrafP(path.join(__dirname, 'directory')); await rimrafP(path.join(__dirname, 'dist')); + await rimrafP(path.join(__dirname, 'example.txt')); + await rimrafP(path.join(__dirname, 'file.txt')); await rimrafP(path.join(__dirname, 'edge_case_dots')); + await rimrafP(path.join(__dirname, 'symlink')); await rimrafP(path.join(__dirname, 'test.jpg')); }); @@ -57,14 +60,12 @@ test.serial('extract file to directory', async t => { t.true(await pathExists(path.join(__dirname, 'test.jpg'))); }); -test('extract symlink', async t => { +test.serial('extract symlink', async t => { await m(path.join(__dirname, 'fixtures', 'symlink.tar'), __dirname, {strip: 1}); t.is(await fsP.realpath(path.join(__dirname, 'symlink')), path.join(__dirname, 'file.txt')); - await fsP.unlink(path.join(__dirname, 'symlink')); - await fsP.unlink(path.join(__dirname, 'file.txt')); }); -test('extract directory', async t => { +test.serial('extract directory', async t => { await m(path.join(__dirname, 'fixtures', 'directory.tar'), __dirname); t.true(await pathExists(path.join(__dirname, 'directory'))); }); @@ -115,19 +116,25 @@ test('throw when a location outside the root is given', async t => { }, {message: /Refusing/}); }); -test('throw when a location outside the root including symlinks is given', async t => { +test.serial('throw when a location outside the root including symlinks is given', async t => { await t.throwsAsync(async () => { await m(path.join(__dirname, 'fixtures', 'slip.zip'), 'dist'); }, {message: /Refusing/}); }); -test('throw when a directory outside the root including symlinks is given', async t => { +test.serial('throw when a top-level symlink outside the root is given', async t => { + await t.throwsAsync(async () => { + await m(path.join(__dirname, 'fixtures', 'slip2.zip'), 'dist'); + }, {message: /Refusing/}); +}); + +test.serial('throw when a directory outside the root including symlinks is given', async t => { await t.throwsAsync(async () => { await m(path.join(__dirname, 'fixtures', 'slipping_directory.tar.gz'), 'dist'); }, {message: /Refusing/}); }); -test('allows filenames and directories to be written with dots in their names', async t => { +test.serial('allows filenames and directories to be written with dots in their names', async t => { const files = await m(path.join(__dirname, 'fixtures', 'edge_case_dots.tar.gz'), __dirname); t.is(files.length, 6); t.deepEqual(files.map(f => f.path).sort(), [ @@ -139,3 +146,9 @@ test('allows filenames and directories to be written with dots in their names', 'edge_case_dots/sample../test.txt' ].sort()); }); + +test.serial('allows top-level file', async t => { + const files = await m(path.join(__dirname, 'fixtures', 'top_level_example.tar.gz'), 'dist'); + t.is(files.length, 1); + t.is(files[0].path, 'example.txt'); +}); From a043c4bfcd8d4d6c29fcea484f0b225bb14b3cf4 Mon Sep 17 00:00:00 2001 From: Colin Jones Date: Sat, 29 Feb 2020 09:41:25 -0600 Subject: [PATCH 10/11] Prevent writing a file through a symlink Because `fs.realpath` requires an actual file to exist, we can't rely on it to detect if a symlink might point outside the output directory. Traversing chains of symlinks, where the targets might be relative or absolute, missing or not, gets complicated very quickly. And because the behavior is generally surprising, of file creation writing through to a pre-existing symlink's target, it seems preferable to disallow that behavior. --- fixtures/slip3.zip | Bin 0 -> 2441 bytes fixtures/slip3a.zip | Bin 0 -> 2450 bytes index.js | 36 +++++++++++++++++++++++------------- test.js | 14 +++++++++++++- 4 files changed, 36 insertions(+), 14 deletions(-) create mode 100644 fixtures/slip3.zip create mode 100644 fixtures/slip3a.zip diff --git a/fixtures/slip3.zip b/fixtures/slip3.zip new file mode 100644 index 0000000000000000000000000000000000000000..434320d3e8a30660fd57bf92dea718ba68a98024 GIT binary patch literal 2441 zcmWIWW@h1H0D)TX*Z`;Ybq_>=Y!K#RkYPy6EGjOE&&kZo4h`XCVE)?_6$iql72FJr zEMFNJ7+6GrD)mcp3-nVmi%ay2Q zaZYAIL262TRc1lFv0h0<399kEZ7#XYK%+re5ommRYF=tlW^#N=W|4kzWiH4A@g@23 z1&Kwec_koGe3t6z>4QuM`HTY`Dkl?DzsZ74W?+b6Cc<=mg7%N9K#h;l@EFnIL0mc0 zeM%=lr`AuUzJ%9Z>(2RQG#lGZ<|XlGtg)dRs0hl$ji#l3Ao* zT$u~DtDp2EN zG(1Lhcwi}T5-|&$?o&De8Yew;0yWN^)Y0(p4fOEwKXWG7<4p6W9Si~9j7)OOxXLsM zVD^Q8w+lfuvW2V+;1Uk4bVD}}wFE;pF9~QKN_j?{fv9B`vVjMI1`;emLPPLbgRAg@ zSOYA<8J0B411W;mjAqOcoiRt#2THSnh}=L-$->G8%zz9mK)8y9f#DJmGcW)Ef?--U literal 0 HcmV?d00001 diff --git a/index.js b/index.js index 3c322eb..6aa67ca 100644 --- a/index.js +++ b/index.js @@ -30,7 +30,24 @@ const safeMakeDir = (dir, realOutputPath) => { throw (new Error('Refusing to create a directory outside the output path.')); } - return makeDir(dir); + return makeDir(dir).then(fsP.realpath); + }); +}; + +const preventWritingThroughSymlink = (destination, realOutputPath) => { + return fsP.readlink(destination) + .catch(_ => { + // Either no file exists, or it's not a symlink. In either case, this is + // not an escape we need to worry about in this phase. + return null; + }) + .then(symlinkPointsTo => { + if (symlinkPointsTo) { + throw new Error('Refusing to write into a symlink'); + } + + // No symlink exists at `destination`, so we can continue + return realOutputPath; }); }; @@ -72,23 +89,16 @@ const extractFile = (input, output, opts) => runPlugins(input, opts).then(files return makeDir(output) .then(outputPath => fsP.realpath(outputPath)) .then(realOutputPath => { + // Attempt to ensure parent directory exists (failing if it's outside the output dir) return safeMakeDir(path.dirname(dest), realOutputPath) .then(() => realOutputPath); }) .then(realOutputPath => { - // Check for symlinks pointing outside output directory - return fsP.readlink(dest) - .catch(_ => { - // File doesn't exist yet - return null; - }) - .then(symlinkPointsTo => { - if (symlinkPointsTo && symlinkPointsTo.indexOf(realOutputPath) !== 0) { - throw (new Error('Refusing to write into a file outside output directory, through a symlink: ' + symlinkPointsTo)); - } + if (x.type === 'file') { + return preventWritingThroughSymlink(dest, realOutputPath); + } - return realOutputPath; - }); + return realOutputPath; }) .then(realOutputPath => { return fsP.realpath(path.dirname(dest)) diff --git a/test.js b/test.js index 52557c0..9f3738d 100644 --- a/test.js +++ b/test.js @@ -110,7 +110,7 @@ test('return emptpy array if no plugins are set', async t => { t.is(files.length, 0); }); -test('throw when a location outside the root is given', async t => { +test.serial('throw when a location outside the root is given', async t => { await t.throwsAsync(async () => { await m(path.join(__dirname, 'fixtures', 'slipping.tar.gz'), 'dist'); }, {message: /Refusing/}); @@ -152,3 +152,15 @@ test.serial('allows top-level file', async t => { t.is(files.length, 1); t.is(files[0].path, 'example.txt'); }); + +test.serial('throw when chained symlinks to /tmp/dist allow escape outside root directory', async t => { + await t.throwsAsync(async () => { + await m(path.join(__dirname, 'fixtures', 'slip3.zip'), '/tmp/dist'); + }, {message: /Refusing/}); +}); + +test.serial('throw when chained symlinks to /private/tmp/dist2 allow escape outside root directory', async t => { + await t.throwsAsync(async () => { + await m(path.join(__dirname, 'fixtures', 'slip3a.zip'), '/private/tmp/dist2'); + }, {message: /Refusing/}); +}); From 49d83eb536830457abb2baa860b580e4a9c1f464 Mon Sep 17 00:00:00 2001 From: Colin Jones Date: Sat, 29 Feb 2020 09:50:40 -0600 Subject: [PATCH 11/11] Remove environment-specific test This was useful on MacOS to expose a bug, where `/tmp` was symlinked to `/private/tmp`, but because the build runs on a Linux host where `/private` is inaccessible and the preceding test does the trick, we can lose this in CI. --- fixtures/slip3a.zip | Bin 2450 -> 0 bytes test.js | 6 ------ 2 files changed, 6 deletions(-) delete mode 100644 fixtures/slip3a.zip diff --git a/fixtures/slip3a.zip b/fixtures/slip3a.zip deleted file mode 100644 index 4c3d0034bb5ea007d35c51743178c46e0f1ec3d8..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 2450 zcmWIWW@h1H0D&Vuu>r`AuUzJ%9Z>(2RQG#lGZ<|XlGtg)dRs0hl$ji#l3Ao* zT$u~DtDp2EN zG(1Lhcwi}T5-|&$?o&De8Yew;0yWN^)Y0(p4fOEwKXWG7<4p6W9Si~9j7)OOxXLsM zVD^Q8w+lfuvW2V+;1Uk4bVD}}wFE;pF9~QKN_j?{fv9B`vVjMI1`;emLPPLbgRAg@ zSOYA<8J0B411W;mjAqOcoiRt#2THSnh}=L-$->G8%zz9mK)8y9f#DJmGcW)Ef?--U diff --git a/test.js b/test.js index 9f3738d..ba99d68 100644 --- a/test.js +++ b/test.js @@ -158,9 +158,3 @@ test.serial('throw when chained symlinks to /tmp/dist allow escape outside root await m(path.join(__dirname, 'fixtures', 'slip3.zip'), '/tmp/dist'); }, {message: /Refusing/}); }); - -test.serial('throw when chained symlinks to /private/tmp/dist2 allow escape outside root directory', async t => { - await t.throwsAsync(async () => { - await m(path.join(__dirname, 'fixtures', 'slip3a.zip'), '/private/tmp/dist2'); - }, {message: /Refusing/}); -});