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: #887 Reviewed-By: Ben Noordhuis <[email protected]>
nodejs · Feb 19, 2015 · ecef871 · ecef871
1 parent ed240f4
commit ecef871
Show file tree

Hide file tree

Showing 3 changed files with 10 additions and 5 deletions.
diff --git 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,16 +169,16 @@ 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;
   } else if (typeof callback !== 'function') {
     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
@@ -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
@@ -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'));
-