From ecef87177ac44d0840ec001aad0921824a2a13ec Mon Sep 17 00:00:00 2001 From: cjihrig Date: Wed, 18 Feb 2015 23:10:15 -0500 Subject: [PATCH] fs: ensure nullCheck() callback is a function Currently, nullCheck() will attempt to invoke any truthy value as a function if the path argument contains a null character. This commit validates that the callback is actually a function before trying to invoke it. fs.access() was vulnerable to this bug, as nullCheck() was called prior to type checking its callback. PR-URL: https://github.com/iojs/io.js/pull/887 Reviewed-By: Ben Noordhuis --- lib/fs.js | 8 ++++---- test/parallel/test-fs-access.js | 4 ++++ test/parallel/test-fs-null-bytes.js | 3 ++- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 18332c7840c8b2..7a89362985f863 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -84,7 +84,7 @@ function nullCheck(path, callback) { if (('' + path).indexOf('\u0000') !== -1) { var er = new Error('Path must be a string without null bytes.'); er.code = 'ENOENT'; - if (!callback) + if (typeof callback !== 'function') throw er; process.nextTick(function() { callback(er); @@ -169,9 +169,6 @@ fs.Stats.prototype.isSocket = function() { }); fs.access = function(path, mode, callback) { - if (!nullCheck(path, callback)) - return; - if (typeof mode === 'function') { callback = mode; mode = fs.F_OK; @@ -179,6 +176,9 @@ fs.access = function(path, mode, callback) { throw new TypeError('callback must be a function'); } + if (!nullCheck(path, callback)) + return; + mode = mode | 0; var req = new FSReqWrap(); req.oncomplete = makeCallback(callback); diff --git a/test/parallel/test-fs-access.js b/test/parallel/test-fs-access.js index 3846b7b7b79caa..cf8dd942cb0c75 100644 --- a/test/parallel/test-fs-access.js +++ b/test/parallel/test-fs-access.js @@ -57,6 +57,10 @@ assert.throws(function() { fs.access(__filename, fs.F_OK); }, /callback must be a function/); +assert.throws(function() { + fs.access(__filename, fs.F_OK, {}); +}, /callback must be a function/); + assert.doesNotThrow(function() { fs.accessSync(__filename); }); diff --git a/test/parallel/test-fs-null-bytes.js b/test/parallel/test-fs-null-bytes.js index 8499c03cfe8977..4d1dcb3972a7ba 100644 --- a/test/parallel/test-fs-null-bytes.js +++ b/test/parallel/test-fs-null-bytes.js @@ -20,6 +20,8 @@ function check(async, sync) { async.apply(null, argsAsync); } +check(fs.access, fs.accessSync, 'foo\u0000bar'); +check(fs.access, fs.accessSync, 'foo\u0000bar', fs.F_OK); check(fs.appendFile, fs.appendFileSync, 'foo\u0000bar'); check(fs.chmod, fs.chmodSync, 'foo\u0000bar', '0644'); check(fs.chown, fs.chownSync, 'foo\u0000bar', 12, 34); @@ -52,4 +54,3 @@ fs.exists('foo\u0000bar', function(exists) { assert(!exists); }); assert(!fs.existsSync('foo\u0000bar')); -