diff --git a/README.md b/README.md index ffbd70c..183fd7c 100644 --- a/README.md +++ b/README.md @@ -124,10 +124,6 @@ server.ext('onPostHandler', function (request, reply) { }); ``` -Note that paths for files served using the `reply.file()` handler are **NOT** guarded against -access outside the `files.relativeTo` directory, so be careful to guard against malevolent user -input. - ## Usage After registration, this plugin adds a new method to the `reply` object and exposes the `'file'` @@ -150,6 +146,10 @@ type based on filename extension.: - `path` - the file path. - `options` - optional settings: + - `confine` - serve file relative to this directory and returns `403 Forbidden` if the + `path` resolves outside the `confine` directory. + Defaults to `true` which uses the `relativeTo` route option as the `confine`. + Set to `false` to disable this security feature. - `filename` - an optional filename to specify if sending a 'Content-Disposition' header, defaults to the basename of `path` - `mode` - specifies whether to include the 'Content-Disposition' header with the response. @@ -182,6 +182,10 @@ Generates a static file endpoint for serving a single file. `file` can be set to file path. - an object with one or more of the following options: - `path` - a path string or function as described above (required). + - `confine` - serve file relative to this directory and returns `403 Forbidden` if the + `path` resolves outside the `confine` directory. + Defaults to `true` which uses the `relativeTo` route option as the `confine`. + Set to `false` to disable this security feature. - `filename` - an optional filename to specify if sending a 'Content-Disposition' header, defaults to the basename of `path` - `mode` - specifies whether to include the 'Content-Disposition' header with the diff --git a/lib/directory.js b/lib/directory.js index f3426be..8e567b0 100755 --- a/lib/directory.js +++ b/lib/directory.js @@ -77,16 +77,7 @@ exports.handler = function (route, options) { // Append parameter - let selection = null; - const lastParam = request.paramsArray[request.paramsArray.length - 1]; - if (lastParam) { - if (lastParam.indexOf('..') !== -1) { - return reply(Boom.forbidden()); - } - - selection = lastParam; - } - + const selection = request.paramsArray[request.paramsArray.length - 1]; if (selection && !settings.showHidden && internals.isFileHidden(selection)) { @@ -98,11 +89,17 @@ exports.handler = function (route, options) { const resource = request.path; const hasTrailingSlash = resource.endsWith('/'); - const fileOptions = { lookupCompressed: settings.lookupCompressed, etagMethod: settings.etagMethod }; + const fileOptions = { + confine: null, + lookupCompressed: settings.lookupCompressed, + etagMethod: settings.etagMethod + }; + + Items.serial(paths, (baseDir, nextPath) => { - Items.serial(paths, (path, nextPath) => { + fileOptions.confine = baseDir; - path = Path.join(path, selection || ''); + let path = selection || ''; File.load(path, request, fileOptions, (response) => { @@ -186,7 +183,7 @@ exports.handler = function (route, options) { return reply(Boom.forbidden()); } - return internals.generateListing(path, resource, selection, hasTrailingSlash, settings, request, reply); + return internals.generateListing(Path.join(baseDir, path), resource, selection, hasTrailingSlash, settings, request, reply); }); }); }, @@ -234,7 +231,7 @@ internals.generateListing = function (path, resource, selection, hasTrailingSlas internals.isFileHidden = function (path) { - return /(^|[\\\/])\.([^\\\/]|[\\\/]?$)/.test(path); // Starts with a '.' or contains '/.' or '\.', and not followed by a '/' or '\' or end + return /(^|[\\\/])\.([^.\\\/]|\.[^\\\/])/.test(path); // Starts with a '.' or contains '/.' or '\.', which is not followed by a '/' or '\' or '.' }; diff --git a/lib/file.js b/lib/file.js index 62bdf36..9d3116b 100755 --- a/lib/file.js +++ b/lib/file.js @@ -21,6 +21,7 @@ internals.schema = Joi.alternatives([ Joi.func(), Joi.object({ path: Joi.alternatives(Joi.string(), Joi.func()).required(), + confine: Joi.alternatives(Joi.string(), Joi.boolean()).default(true), filename: Joi.string(), mode: Joi.string().valid('attachment', 'inline').allow(false), lookupCompressed: Joi.boolean(), @@ -33,7 +34,8 @@ internals.schema = Joi.alternatives([ exports.handler = function (route, options) { let settings = Joi.attempt(options, internals.schema, 'Invalid file handler options (' + route.path + ')'); - settings = (typeof options !== 'object' ? { path: options } : settings); + settings = (typeof options !== 'object' ? { path: options, confine: '.' } : settings); + settings.confine = settings.confine === true ? '.' : settings.confine; Hoek.assert(typeof settings.path !== 'string' || settings.path[settings.path.length - 1] !== '/', 'File path cannot end with a \'/\':', route.path); const handler = (request, reply) => { @@ -55,11 +57,23 @@ exports.load = function (path, request, options, callback) { exports.response = function (path, options, request, _preloaded) { - options = options || {}; Hoek.assert(!options.mode || ['attachment', 'inline'].indexOf(options.mode) !== -1, 'options.mode must be either false, attachment, or inline'); + if (options.confine) { + const confineDir = Path.resolve(request.route.settings.files.relativeTo, options.confine); + path = Path.isAbsolute(path) ? Path.normalize(path) : Path.join(confineDir, path); + + // Verify that resolved path is within confineDir + if (path.lastIndexOf(confineDir, 0) !== 0) { + path = null; + } + } + else { + path = Path.isAbsolute(path) ? Path.normalize(path) : Path.join(request.route.settings.files.relativeTo, path); + } + const source = { - path: Path.normalize(Path.isAbsolute(path) ? path : Path.join(request.route.settings.files.relativeTo, path)), + path: path, settings: options, stat: null, fd: null @@ -74,6 +88,14 @@ exports.response = function (path, options, request, _preloaded) { internals.prepare = function (response, callback) { const path = response.source.path; + + if (path === null) { + return process.nextTick(() => { + + return callback(Boom.forbidden(null, 'EACCES')); + }); + } + internals.openStat(path, 'r', (err, fd, stat) => { if (err) { diff --git a/lib/index.js b/lib/index.js index 7fbde58..a3ade0f 100755 --- a/lib/index.js +++ b/lib/index.js @@ -29,6 +29,13 @@ exports.register = function (server, options, next) { server.decorate('reply', 'file', function (path, responseOptions) { + // Set correct confine value + + responseOptions = responseOptions || {}; + if (typeof responseOptions.confine === 'undefined' || responseOptions.confine === true) { + responseOptions.confine = '.'; + } + return this.response(File.response(path, responseOptions, this.request)); }); diff --git a/test/file.js b/test/file.js index 89c0027..7fe32af 100755 --- a/test/file.js +++ b/test/file.js @@ -45,7 +45,7 @@ describe('file', () => { const server = provisionServer({ routes: { files: { relativeTo: __dirname } } }); const handler = (request, reply) => { - reply.file('../package.json').code(499); + reply.file('package.json', { confine: '../' }).code(499); }; server.route({ method: 'GET', path: '/file', handler: handler }); @@ -66,7 +66,7 @@ describe('file', () => { const server = provisionServer(); const handler = (request, reply) => { - reply.file('../package.json'); + reply.file('../package.json', { confine: false }); }; server.route({ method: 'GET', path: '/file', handler: handler, config: { files: { relativeTo: __dirname } } }); @@ -180,7 +180,7 @@ describe('file', () => { const server = provisionServer({ routes: { files: { relativeTo: __dirname } } }); const handler = (request, reply) => { - reply.file(Path.join(__dirname, '..', 'package.json'), { mode: 'attachment' }); + reply.file(Path.join(__dirname, '..', 'package.json'), { confine: '..', mode: 'attachment' }); }; server.route({ method: 'GET', path: '/file', handler: handler }); @@ -201,7 +201,7 @@ describe('file', () => { const server = provisionServer({ routes: { files: { relativeTo: __dirname } } }); const handler = (request, reply) => { - reply.file(Path.join(__dirname, '..', 'package.json'), { mode: 'attachment', filename: 'attachment.json' }); + reply.file(Path.join(__dirname, '..', 'package.json'), { confine: '..', mode: 'attachment', filename: 'attachment.json' }); }; server.route({ method: 'GET', path: '/file', handler: handler }); @@ -222,7 +222,7 @@ describe('file', () => { const server = provisionServer({ routes: { files: { relativeTo: __dirname } } }); const handler = (request, reply) => { - reply.file(Path.join(__dirname, '..', 'package.json'), { mode: 'inline' }); + reply.file(Path.join(__dirname, '..', 'package.json'), { confine: '..', mode: 'inline' }); }; server.route({ method: 'GET', path: '/file', handler: handler }); @@ -243,7 +243,7 @@ describe('file', () => { const server = provisionServer({ routes: { files: { relativeTo: __dirname } } }); const handler = (request, reply) => { - reply.file(Path.join(__dirname, '..', 'package.json'), { mode: 'inline', filename: 'attachment.json' }); + reply.file(Path.join(__dirname, '..', 'package.json'), { confine: '..', mode: 'inline', filename: 'attachment.json' }); }; server.route({ method: 'GET', path: '/file', handler: handler }); @@ -287,7 +287,7 @@ describe('file', () => { it('returns a file using the built-in handler config', (done) => { - const server = provisionServer({ routes: { files: { relativeTo: __dirname } } }); + const server = provisionServer({ routes: { files: { relativeTo: Path.join(__dirname, '..') } } }); server.route({ method: 'GET', path: '/staticfile', handler: { file: Path.join(__dirname, '..', 'package.json') } }); server.inject('/staticfile', (res) => { @@ -304,10 +304,10 @@ describe('file', () => { const filenameFn = (request) => { - return '../lib/' + request.params.file; + return './lib/' + request.params.file; }; - const server = provisionServer({ routes: { files: { relativeTo: __dirname } } }); + const server = provisionServer({ routes: { files: { relativeTo: Path.join(__dirname, '..') } } }); server.route({ method: 'GET', path: '/filefn/{file}', handler: { file: filenameFn } }); server.inject('/filefn/index.js', (res) => { @@ -325,7 +325,7 @@ describe('file', () => { const server = provisionServer({ routes: { files: { relativeTo: '.' } } }); const relativeHandler = (request, reply) => { - reply.file('./package.json'); + reply.file('./package.json', { confine: true }); }; server.route({ method: 'GET', path: '/relativefile', handler: relativeHandler }); @@ -342,8 +342,8 @@ describe('file', () => { it('returns a file using the built-in handler config (relative path)', (done) => { - const server = provisionServer({ routes: { files: { relativeTo: __dirname } } }); - server.route({ method: 'GET', path: '/relativestaticfile', handler: { file: '../package.json' } }); + const server = provisionServer({ routes: { files: { relativeTo: Path.join(__dirname, '..') } } }); + server.route({ method: 'GET', path: '/relativestaticfile', handler: { file: './package.json' } }); server.inject('/relativestaticfile', (res) => { @@ -373,7 +373,7 @@ describe('file', () => { const server = provisionServer({ routes: { files: { relativeTo: __dirname } } }); const handler = (request, reply) => { - reply.file('../LICENSE').type('application/example'); + reply.file('../LICENSE', { confine: false }).type('application/example'); }; server.route({ method: 'GET', path: '/file', handler: handler }); @@ -870,7 +870,7 @@ describe('file', () => { const server = provisionServer({ routes: { files: { relativeTo: __dirname } } }); const handler = (request, reply) => { - reply.file(Path.join(__dirname, '..', 'package.json')); + reply.file(Path.join(__dirname, '..', 'package.json'), { confine: '..' }); }; server.route({ method: 'GET', path: '/file', handler: handler }); @@ -912,7 +912,7 @@ describe('file', () => { const server = provisionServer({ routes: { files: { relativeTo: __dirname } } }); const handler = (request, reply) => { - reply.file(Path.join(__dirname, '..', 'package.json')); + reply.file(Path.join(__dirname, '..', 'package.json'), { confine: '..' }); }; server.route({ method: 'GET', path: '/file', handler: handler }); @@ -1025,7 +1025,7 @@ describe('file', () => { Fs.writeFileSync(filename, 'data'); const server = provisionServer(); - server.route({ method: 'GET', path: '/', handler: { file: filename } }); + server.route({ method: 'GET', path: '/', handler: { file: { path: filename, confine: false } } }); server.ext('onPreResponse', (request, reply) => { Fs.unlinkSync(filename); @@ -1045,7 +1045,7 @@ describe('file', () => { Fs.writeFileSync(filename, 'data'); const server = provisionServer(); - server.route({ method: 'GET', path: '/', handler: { file: filename } }); + server.route({ method: 'GET', path: '/', handler: { file: { path: filename, confine: false } } }); server.ext('onPreResponse', (request, reply) => { const tempfile = filename + '~'; @@ -1133,7 +1133,7 @@ describe('file', () => { const server = provisionServer(); - server.route({ method: 'GET', path: '/', handler: { file: filename } }); + server.route({ method: 'GET', path: '/', handler: { file: { path: filename, confine: false } } }); server.inject('/', (res) => { @@ -1156,7 +1156,7 @@ describe('file', () => { }; const server = provisionServer(); - server.route({ method: 'GET', path: '/', handler: { file: filename } }); + server.route({ method: 'GET', path: '/', handler: { file: { path: filename, confine: false } } }); server.inject('/', (res) => { diff --git a/test/security.js b/test/security.js index 0248626..9978ee2 100755 --- a/test/security.js +++ b/test/security.js @@ -7,6 +7,7 @@ const Hapi = require('hapi'); const Hoek = require('hoek'); const Inert = require('..'); const Lab = require('lab'); +const Path = require('path'); // Declare internals @@ -39,7 +40,7 @@ describe('security', () => { server.inject('/%00/../security.js', (res) => { - expect(res.statusCode).to.equal(403); + expect(res.statusCode).to.equal(404); done(); }); }); @@ -75,7 +76,7 @@ describe('security', () => { server.inject('/..%252Fsecurity.js', (res) => { - expect(res.statusCode).to.equal(403); + expect(res.statusCode).to.equal(404); done(); }); }); @@ -87,7 +88,7 @@ describe('security', () => { server.inject('/..\u2216security.js', (res) => { - expect(res.statusCode).to.equal(403); + expect(res.statusCode).to.equal(404); done(); }); }); @@ -103,4 +104,69 @@ describe('security', () => { done(); }); }); + + it('blocks access to files outside of base directory for file handler', (done) => { + + const server = provisionServer(); + + const secureHandler = { file: { confine: './directory', path: Path.join(__dirname, 'security.js') } }; + server.route({ method: 'GET', path: '/secure', handler: secureHandler }); + server.route({ method: 'GET', path: '/open', handler: Hoek.applyToDefaults(secureHandler, { file: { confine: false } }) }); + + server.inject('/secure', (res1) => { + + expect(res1.statusCode).to.equal(403); + server.inject('/open', (res2) => { + + expect(res2.statusCode).to.equal(200); + done(); + }); + }); + }); + + it('blocks path traversal to files outside of base directory for file handler', (done) => { + + const server = provisionServer(); + server.route({ method: 'GET', path: '/file', handler: { file: { confine: './directory', path: '../security.js' } } }); + + server.inject('/file', (res) => { + + expect(res.statusCode).to.equal(403); + done(); + }); + }); + + it('blocks access to files outside of base directory for reply.file()', (done) => { + + const server = provisionServer(); + const fileHandler = (request, reply) => { + + reply.file(Path.join(__dirname, 'security.js'), { confine: Path.join(__dirname, 'directory') }); + }; + + server.route({ method: 'GET', path: '/file', handler: fileHandler }); + + server.inject('/file', (res) => { + + expect(res.statusCode).to.equal(403); + done(); + }); + }); + + it('blocks path traversal to files outside of base directory for reply.file()', (done) => { + + const server = provisionServer(); + const fileHandler = (request, reply) => { + + reply.file('../security.js', { confine: Path.join(__dirname, 'directory') }); + }; + + server.route({ method: 'GET', path: '/file', handler: fileHandler }); + + server.inject('/file', (res) => { + + expect(res.statusCode).to.equal(403); + done(); + }); + }); });