Skip to content

Loading…

fs: no nullbytes in paths #1493

Closed
wants to merge 3 commits into from

3 participants

@thejh

I added a new function safeWrap to fs.js and used it to wrap all functions that use the native binding and use filenames. When a nullbyte is detected in a path and the function is synchronous, the wrapper throws. When the function is async, it calls the callback with the error if there is a callback.

@bnoordhuis
Node.js Foundation member

I appreciate the effort but I don't want this in core, user land can handle it just fine.

@thejh

@bnoordhuis: Only if you're aware of it. I think that a normal developer will expect an fs function to either open the file he specified or to throw an error and not to open another file. That's a bit as if we'd make the dns module say "huh, 'foo bar' is invalid? Well, let's just pretend we were asked to lookup 'foo'"...

@isaacs

This feature should be in core.

I don't think there's ever a case where you'd want fs.open("foo\0/etc/passwd") to open /etc/passwd, and it's unreasonable to expect that users would know to test for poison nulls before passing paths to the fs module.

I think that the implementation can be a little simpler, though, and pushed down into the binding layer. There was another issue with process.chdir thread safety that @piscisaureus was looking into. It would probably solve both issues to call path.resolve() on all paths before passing them to the thread pool.

@thejh

@isaacs, there are cases where just path.resolve() isn't enough. This, for example, could be used to trick a webserver into delivering a file with the wrong MIME-type and path.resolve() doesn't change anything about that: /var/www/text.txt%00.js
And because ..\0 isn't .., this can be used to get a directory listing of the parent directory in a webserver that doesn't look for nullbytes: http://example.org/..%00

Both are real examples, I didn't just make them up.

I really think that accessing files/directories with a nullbyte in the name should just throw something like an error or a "file not found" or so.

Oh, and by the way, it's /etc/passwd\0foo, not foo\0/etc/passwd.

@isaacs

Oh, right, sorry. Hitting my max mental threads today :)

My main point was that we ought to do this in core, as low as possible before going to the thread pool. In my opinion, it should be in src/node_file.cc, not in lib/fs.js.

@bnoordhuis
Node.js Foundation member

I'm slowly coming around to this. Chalk me down for +0 for now.

@thejh

Sorry, I know that this is near to trolling. But:

https://bugs.php.net/bug.php?id=39863

file_exists() silently truncates anything after a null byte in a string.[...]

Fixed in PHP_5_3, will be part of 5.3.4.

Come on! Even PHP has a fix for this now!

@thejh

bump

@bnoordhuis
Node.js Foundation member

@thejh: Low prio. It doesn't merge cleanly right now and even if it did, it would be too big a change to land before 0.6.

@thejh

Ok, put my monkeypatching code into a module. https://github.com/thejh/node-protect-fs

@bnoordhuis
Node.js Foundation member

Closing, continues in #4294.

@bnoordhuis bnoordhuis closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 11, 2011
  1. @thejh
  2. @thejh

    added safeWrap() for fs functions that use the binding

    thejh committed
    Also, change the order in fs.watchFile to prevent putting non-working watchers
    in statWatchers.
  3. @thejh
Showing with 128 additions and 60 deletions.
  1. +91 −60 lib/fs.js
  2. +37 −0 test/simple/test-fs-nullbyte.js
View
151 lib/fs.js
@@ -206,7 +206,7 @@ function modeNum(m, def) {
}
}
-fs.open = function(path, flags, mode, callback) {
+fs.open = safeWrap(true, 0, function(path, flags, mode, callback) {
callback = arguments[arguments.length - 1];
if (typeof(callback) !== 'function') {
callback = noop;
@@ -215,12 +215,12 @@ fs.open = function(path, flags, mode, callback) {
mode = modeNum(mode, '0666');
binding.open(path, stringToFlags(flags), mode, callback);
-};
+});
-fs.openSync = function(path, flags, mode) {
+fs.openSync = safeWrap(false, 0, function(path, flags, mode) {
mode = modeNum(mode, '0666');
return binding.open(path, stringToFlags(flags), mode);
-};
+});
fs.read = function(fd, buffer, offset, length, position, callback) {
if (!Buffer.isBuffer(buffer)) {
@@ -313,13 +313,13 @@ fs.writeSync = function(fd, buffer, offset, length, position) {
return binding.write(fd, buffer, offset, length, position);
};
-fs.rename = function(oldPath, newPath, callback) {
+fs.rename = safeWrap(true, 0, 1, function(oldPath, newPath, callback) {
binding.rename(oldPath, newPath, callback || noop);
-};
+});
-fs.renameSync = function(oldPath, newPath) {
+fs.renameSync = safeWrap(false, 0, 1, function(oldPath, newPath) {
return binding.rename(oldPath, newPath);
-};
+});
fs.truncate = function(fd, len, callback) {
binding.truncate(fd, len, callback || noop);
@@ -329,13 +329,13 @@ fs.truncateSync = function(fd, len) {
return binding.truncate(fd, len);
};
-fs.rmdir = function(path, callback) {
+fs.rmdir = safeWrap(true, 0, function(path, callback) {
binding.rmdir(path, callback || noop);
-};
+});
-fs.rmdirSync = function(path) {
+fs.rmdirSync = safeWrap(false, 0, function(path) {
return binding.rmdir(path);
-};
+});
fs.fdatasync = function(fd, callback) {
binding.fdatasync(fd, callback || noop);
@@ -353,13 +353,13 @@ fs.fsyncSync = function(fd) {
return binding.fsync(fd);
};
-fs.mkdir = function(path, mode, callback) {
+fs.mkdir = safeWrap(true, 0, function(path, mode, callback) {
binding.mkdir(path, modeNum(mode), callback || noop);
-};
+});
-fs.mkdirSync = function(path, mode) {
+fs.mkdirSync = safeWrap(false, 0, function(path, mode) {
return binding.mkdir(path, modeNum(mode));
-};
+});
fs.sendfile = function(outFd, inFd, inOffset, length, callback) {
binding.sendfile(outFd, inFd, inOffset, length, callback || noop);
@@ -369,69 +369,69 @@ fs.sendfileSync = function(outFd, inFd, inOffset, length) {
return binding.sendfile(outFd, inFd, inOffset, length);
};
-fs.readdir = function(path, callback) {
+fs.readdir = safeWrap(true, 0, function(path, callback) {
binding.readdir(path, callback || noop);
-};
+});
-fs.readdirSync = function(path) {
+fs.readdirSync = safeWrap(false, 0, function(path) {
return binding.readdir(path);
-};
+});
fs.fstat = function(fd, callback) {
binding.fstat(fd, callback || noop);
};
-fs.lstat = function(path, callback) {
+fs.lstat = safeWrap(true, 0, function(path, callback) {
binding.lstat(path, callback || noop);
-};
+});
-fs.stat = function(path, callback) {
+fs.stat = safeWrap(true, 0, function(path, callback) {
binding.stat(path, callback || noop);
-};
+});
fs.fstatSync = function(fd) {
return binding.fstat(fd);
};
-fs.lstatSync = function(path) {
+fs.lstatSync = safeWrap(false, 0, function(path) {
return binding.lstat(path);
-};
+});
-fs.statSync = function(path) {
+fs.statSync = safeWrap(false, 0, function(path) {
return binding.stat(path);
-};
+});
-fs.readlink = function(path, callback) {
+fs.readlink = safeWrap(true, 0, function(path, callback) {
binding.readlink(path, callback || noop);
-};
+});
-fs.readlinkSync = function(path) {
+fs.readlinkSync = safeWrap(false, 0, function(path) {
return binding.readlink(path);
-};
+});
-fs.symlink = function(destination, path, callback) {
+fs.symlink = safeWrap(true, 0, 1, function(destination, path, callback) {
binding.symlink(destination, path, callback || noop);
-};
+});
-fs.symlinkSync = function(destination, path) {
+fs.symlinkSync = safeWrap(false, 0, 1, function(destination, path) {
return binding.symlink(destination, path);
-};
+});
-fs.link = function(srcpath, dstpath, callback) {
+fs.link = safeWrap(true, 0, 1, function(srcpath, dstpath, callback) {
binding.link(srcpath, dstpath, callback || noop);
-};
+});
-fs.linkSync = function(srcpath, dstpath) {
+fs.linkSync = safeWrap(false, 0, 1, function(srcpath, dstpath) {
return binding.link(srcpath, dstpath);
-};
+});
-fs.unlink = function(path, callback) {
+fs.unlink = safeWrap(true, 0, function(path, callback) {
binding.unlink(path, callback || noop);
-};
+});
-fs.unlinkSync = function(path) {
+fs.unlinkSync = safeWrap(false, 0, function(path) {
return binding.unlink(path);
-};
+});
fs.fchmod = function(fd, mode, callback) {
binding.fchmod(fd, modeNum(mode), callback || noop);
@@ -460,13 +460,13 @@ if (constants.hasOwnProperty('O_SYMLINK')) {
}
-fs.chmod = function(path, mode, callback) {
+fs.chmod = safeWrap(true, 0, function(path, mode, callback) {
binding.chmod(path, modeNum(mode), callback || noop);
-};
+});
-fs.chmodSync = function(path, mode) {
+fs.chmodSync = safeWrap(false, 0, function(path, mode) {
return binding.chmod(path, modeNum(mode));
-};
+});
if (constants.hasOwnProperty('O_SYMLINK')) {
fs.lchown = function(path, uid, gid, callback) {
@@ -494,13 +494,13 @@ fs.fchownSync = function(fd, uid, gid) {
return binding.fchown(fd, uid, gid);
};
-fs.chown = function(path, uid, gid, callback) {
+fs.chown = safeWrap(true, 0, function(path, uid, gid, callback) {
binding.chown(path, uid, gid, callback || noop);
-};
+});
-fs.chownSync = function(path, uid, gid) {
+fs.chownSync = safeWrap(false, 0, function(path, uid, gid) {
return binding.chown(path, uid, gid);
-};
+});
// converts Date or number to a fractional UNIX timestamp
function toUnixTimestamp(time) {
@@ -517,17 +517,17 @@ function toUnixTimestamp(time) {
// exported for unit tests, not for public consumption
fs._toUnixTimestamp = toUnixTimestamp;
-fs.utimes = function(path, atime, mtime, callback) {
+fs.utimes = safeWrap(true, 0, function(path, atime, mtime, callback) {
atime = toUnixTimestamp(atime);
mtime = toUnixTimestamp(mtime);
binding.utimes(path, atime, mtime, callback || noop);
-};
+});
-fs.utimesSync = function(path, atime, mtime) {
+fs.utimesSync = safeWrap(false, 0, function(path, atime, mtime) {
atime = toUnixTimestamp(atime);
mtime = toUnixTimestamp(mtime);
binding.utimes(path, atime, mtime);
-};
+});
fs.futimes = function(fd, atime, mtime, callback) {
atime = toUnixTimestamp(atime);
@@ -603,9 +603,9 @@ function StatWatcher() {
util.inherits(StatWatcher, EventEmitter);
-StatWatcher.prototype.start = function(filename, persistent, interval) {
+StatWatcher.prototype.start = safeWrap(false, 0, function(filename, persistent, interval) {
this._handle.start(filename, persistent, interval);
-};
+});
StatWatcher.prototype.stop = function() {
@@ -639,9 +639,9 @@ fs.watchFile = function(filename) {
if (statWatchers[filename]) {
stat = statWatchers[filename];
} else {
- statWatchers[filename] = new StatWatcher();
- stat = statWatchers[filename];
+ stat = new StatWatcher();
stat.start(filename, options.persistent, options.interval);
+ statWatchers[filename] = stat;
}
stat.addListener('change', listener);
return stat;
@@ -1253,6 +1253,37 @@ WriteStream.prototype.destroy = function(cb) {
}
};
+function safeWrap(isAsync) {
+ var unsafeArgs = [].slice.call(arguments, 1, arguments.length-1);
+ var f = arguments[arguments.length-1];
+ function fail(n) {
+ var err = new Error("fs function was called with a nullbyte in argument #"+n);
+ if (typeof arguments[arguments.length-1] === 'function') {
+ arguments[arguments.length-1](err);
+ } else if (!isAsync) {
+ throw err;
+ }
+ }
+ if (unsafeArgs.length === 1) {
+ var unsafeArg = unsafeArgs[0];
+ return function() {
+ if (arguments[unsafeArg].indexOf('\0') !== -1) {
+ return fail(unsafeArg);
+ }
+ return f.apply(this, arguments);
+ };
+ } else {
+ return function() {
+ for (var i=0; i<unsafeArgs.length; i++) {
+ if (arguments[unsafeArgs[i]].indexOf('\0') !== -1) {
+ return fail(unsafeArgs[i]);
+ }
+ }
+ return f.apply(this, arguments);
+ };
+ }
+}
+
// There is no shutdown() for files.
WriteStream.prototype.destroySoon = WriteStream.prototype.end;
View
37 test/simple/test-fs-nullbyte.js
@@ -0,0 +1,37 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common');
+var assert = require('assert');
+var fs = require('fs');
+var path = require('path');
+
+var file = path.join(common.fixturesDir, 'a.js\0.txt');
+
+var caughtException = false;
+try {
+ fs.openSync(file, 'r');
+}
+catch (e) {
+ caughtException = true;
+}
+assert.ok(caughtException);
+
Something went wrong with that request. Please try again.