diff --git a/lib/tmp.js b/lib/tmp.js index e6b9bcb..3a3aa1b 100644 --- a/lib/tmp.js +++ b/lib/tmp.js @@ -525,6 +525,29 @@ function _generateTmpName(opts) { return path.join(tmpDir, opts.dir, name); } +/** + * Check the prefix, postfix, and template options. + * + * Rejects non-string inputs so that a non-string `.includes('..')` cannot evade + * the substring check (e.g. an Array whose `.includes('..')` is element-wise, + * or a duck-typed object with a custom `.includes`), and so that the value is + * not later coerced to a string with traversal sequences via `Array.prototype.join` + * or `path.join`. + * + * @private + */ +function _assertPath(option, value) { + if (typeof value !== 'string') { + throw new Error(`${option} option must be a string, got "${typeof value}".`); + } + + if (value.includes("..")) { + throw new Error("Relative value not allowed"); + } + + return value; +} + /** * Asserts and sanitizes the basic options. * @@ -539,13 +562,19 @@ function _assertOptionsBase(options) { // must not fail on valid . or .. or similar such constructs const basename = path.basename(name); - if (basename === '..' || basename === '.' || basename !== name) + if (basename === '..' || basename === '.' || basename !== name) { throw new Error(`name option must not contain a path, found "${name}".`); + } } /* istanbul ignore else */ - if (!_isUndefined(options.template) && !options.template.match(TEMPLATE_PATTERN)) { - throw new Error(`Invalid template, found "${options.template}".`); + if (!_isUndefined(options.template)) { + if (typeof options.template !== 'string') { + throw new Error(`template option must be a string, got "${typeof options.template}".`); + } + if (!options.template.match(TEMPLATE_PATTERN)) { + throw new Error(`Invalid template, found "${options.template}".`); + } } /* istanbul ignore else */ @@ -561,8 +590,9 @@ function _assertOptionsBase(options) { options.unsafeCleanup = !!options.unsafeCleanup; // for completeness' sake only, also keep (multiple) blanks if the user, purportedly sane, requests us to - options.prefix = _isUndefined(options.prefix) ? '' : options.prefix; - options.postfix = _isUndefined(options.postfix) ? '' : options.postfix; + options.prefix = _isUndefined(options.prefix) ? '' : _assertPath('prefix', options.prefix); + options.postfix = _isUndefined(options.postfix) ? '' : _assertPath('postfix', options.postfix); + options.template = _isUndefined(options.template) ? undefined : _assertPath('template', options.template); } /** @@ -578,7 +608,7 @@ function _getRelativePath(option, name, tmpDir, cb) { const relativePath = path.relative(tmpDir, resolvedPath); - if (!resolvedPath.startsWith(tmpDir)) { + if (relativePath.startsWith('..') || path.isAbsolute(relativePath)) { return cb(new Error(`${option} option must be relative to "${tmpDir}", found "${relativePath}".`)); } @@ -597,7 +627,7 @@ function _getRelativePathSync(option, name, tmpDir) { const resolvedPath = _resolvePathSync(name, tmpDir); const relativePath = path.relative(tmpDir, resolvedPath); - if (!resolvedPath.startsWith(tmpDir)) { + if (relativePath.startsWith('..') || path.isAbsolute(relativePath)) { throw new Error(`${option} option must be relative to "${tmpDir}", found "${relativePath}".`); } diff --git a/package-lock.json b/package-lock.json index 2bc18f9..cf833a3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "tmp", - "version": "0.2.5", + "version": "0.2.7", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index fd6aad0..5ede98d 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "tmp", - "version": "0.2.5", + "version": "0.2.7", "description": "Temporary file and directory creator", "author": "KARASZI István ", "contributors": ["Carsten Klein (https://github.com/silkentrance)"], diff --git a/test/GHSA-7c78-jf6q-g5cm-test.js b/test/GHSA-7c78-jf6q-g5cm-test.js new file mode 100644 index 0000000..98adefa --- /dev/null +++ b/test/GHSA-7c78-jf6q-g5cm-test.js @@ -0,0 +1,81 @@ +const assert = require('assert'); +const tmp = require('../lib/tmp'); + +describe('GHSA-7c78-jf6q-g5cm', function () { + describe('#fileSync with non-string `prefix`', function () { + it('should reject an array prefix even when its element is "../foo"', function (done) { + assert.throws(function () { + tmp.fileSync({ prefix: ['../foo'] }); + }, new RegExp('^Error: prefix option must be a string')); + + done(); + }); + + it('should reject a duck-typed object whose includes() returns false', function (done) { + assert.throws(function () { + tmp.fileSync({ + prefix: { toString: function () { return '../foo'; }, includes: function () { return false; } } + }); + }, new RegExp('^Error: prefix option must be a string')); + + done(); + }); + + it('should reject a number prefix', function (done) { + assert.throws(function () { + tmp.fileSync({ prefix: 42 }); + }, new RegExp('^Error: prefix option must be a string')); + + done(); + }); + }); + + describe('#fileSync with non-string `postfix`', function () { + it('should reject an array postfix', function (done) { + assert.throws(function () { + tmp.fileSync({ postfix: ['/../foo'] }); + }, new RegExp('^Error: postfix option must be a string')); + + done(); + }); + }); + + describe('#fileSync with non-string `template`', function () { + it('should reject an array template', function (done) { + assert.throws(function () { + tmp.fileSync({ template: ['XXXXXX/../foo'] }); + }, new RegExp('^Error: template option must be a string')); + + done(); + }); + }); + + describe('#dirSync with non-string `prefix`', function () { + it('should reject an array prefix', function (done) { + assert.throws(function () { + tmp.dirSync({ prefix: ['../escape'] }); + }, new RegExp('^Error: prefix option must be a string')); + + done(); + }); + }); + + describe('#tmpNameSync with non-string `prefix`', function () { + it('should reject an array prefix', function (done) { + assert.throws(function () { + tmp.tmpNameSync({ prefix: ['../escape'] }); + }, new RegExp('^Error: prefix option must be a string')); + + done(); + }); + }); + + describe('valid string prefixes still work', function () { + it('should accept a normal string prefix', function (done) { + const r = tmp.fileSync({ prefix: 'safe-prefix' }); + assert.ok(r.name.indexOf('safe-prefix') !== -1); + r.removeCallback(); + done(); + }); + }); +}); diff --git a/test/GHSA-ph9p-34f9-6g65-test.js b/test/GHSA-ph9p-34f9-6g65-test.js new file mode 100644 index 0000000..f3bd35e --- /dev/null +++ b/test/GHSA-ph9p-34f9-6g65-test.js @@ -0,0 +1,24 @@ +const assert = require('assert'); +const tmp = require('../lib/tmp'); + +describe('GHSA-ph9p-34f9-6g65', function () { + describe('#fileSync with attacker `prefix`', function () { + it('should not allow such prefixes', function (done) { + assert.throws(function () { + tmp.fileSync({ prefix: "../foo"}); + }, new RegExp('^Error: Relative value not allowed')); + + done(); + }); + }); + + describe('#fileSync with attacker `postfix`', function () { + it('should not allow such prefixes', function (done) { + assert.throws(function () { + tmp.fileSync({ postfix: "../foo"}); + }, new RegExp('^Error: Relative value not allowed')); + + done(); + }); + }); +});