Skip to content

No abort on require() with null bytes #8277

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rvagg wants to merge 2 commits into from
Closed

No abort on require() with null bytes #8277

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de95b66
fs: return undefined on module read uv fs error
rvagg Jul 23, 2016
39db98c
module: apply null bytes check to module read path
rvagg Jul 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
module: apply null bytes check to module read path 
PR-URL: #8277
  • Loading branch information
@rvagg
rvagg committed Aug 27, 2016
commit 39db98ca5e20b1c28b5058c7b049747324d955bd
3 changes: 3 additions & 0 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,9 @@ function assertEncoding(encoding) {
}
}

// This is duplicated in module.js and needs to be moved to internal/fs.js
// once it is OK again to include internal/ resources in fs.js.
// See: https://github.com/nodejs/node/pull/6413
Copy link
Copy Markdown
Member

@ChALkeR ChALkeR Aug 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#6413 should land on Monday.

Copy link
Copy Markdown
Member

@ChALkeR ChALkeR Aug 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see a follow-up #8292, thanks.

function nullCheck(path, callback) {
if (('' + path).indexOf('\u0000') !== -1) {
var er = new Error('Path must be a string without null bytes');
Expand Down
18 changes: 18 additions & 0 deletions lib/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,22 @@ function stat(filename) {
stat.cache = null;


// This is duplicated from fs.js and needs to be moved to internal/fs.js
// once it is OK again to include internal/ resources in fs.js.
// See: https://github.com/nodejs/node/pull/6413
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 (typeof callback !== 'function')
throw er;
process.nextTick(callback, er);
return false;
}
return true;
}


function Module(id, parent) {
this.id = id;
this.exports = {};
Expand Down Expand Up @@ -135,6 +151,8 @@ function tryExtensions(p, exts, isMain) {

var warned = false;
Module._findPath = function(request, paths, isMain) {
nullCheck(request);

if (path.isAbsolute(request)) {
paths = [''];
} else if (!paths || paths.length === 0) {
Expand Down
86 changes: 52 additions & 34 deletions test/parallel/test-fs-null-bytes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,56 +2,74 @@
var common = require('../common');
var assert = require('assert');
var fs = require('fs');
var expectedError = /Path must be a string without null bytes/;

function check(async, sync) {
var expected = /Path must be a string without null bytes/;
var argsSync = Array.prototype.slice.call(arguments, 2);
var argsAsync = argsSync.concat(function(er) {
assert(er && er.message.match(expected));
assert(er && er.message.match(expectedError));
assert.equal(er.code, 'ENOENT');
});

if (sync)
assert.throws(function() {
console.error(sync.name, argsSync);
sync.apply(null, argsSync);
}, expected);
console.error(`fs.${sync}()`, argsSync);
fs[sync].apply(null, argsSync);
}, expectedError);

if (async)
async.apply(null, argsAsync);
if (async) {
console.error(`fs.${async}()`, argsAsync);
fs[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);
check(fs.link, fs.linkSync, 'foo\u0000bar', 'foobar');
check(fs.link, fs.linkSync, 'foobar', 'foo\u0000bar');
check(fs.lstat, fs.lstatSync, 'foo\u0000bar');
check(fs.mkdir, fs.mkdirSync, 'foo\u0000bar', '0755');
check(fs.open, fs.openSync, 'foo\u0000bar', 'r');
check(fs.readFile, fs.readFileSync, 'foo\u0000bar');
check(fs.readdir, fs.readdirSync, 'foo\u0000bar');
check(fs.readlink, fs.readlinkSync, 'foo\u0000bar');
check(fs.realpath, fs.realpathSync, 'foo\u0000bar');
check(fs.rename, fs.renameSync, 'foo\u0000bar', 'foobar');
check(fs.rename, fs.renameSync, 'foobar', 'foo\u0000bar');
check(fs.rmdir, fs.rmdirSync, 'foo\u0000bar');
check(fs.stat, fs.statSync, 'foo\u0000bar');
check(fs.symlink, fs.symlinkSync, 'foo\u0000bar', 'foobar');
check(fs.symlink, fs.symlinkSync, 'foobar', 'foo\u0000bar');
check(fs.truncate, fs.truncateSync, 'foo\u0000bar');
check(fs.unlink, fs.unlinkSync, 'foo\u0000bar');
check(null, fs.unwatchFile, 'foo\u0000bar', common.fail);
check(fs.utimes, fs.utimesSync, 'foo\u0000bar', 0, 0);
check(null, fs.watch, 'foo\u0000bar', common.fail);
check(null, fs.watchFile, 'foo\u0000bar', common.fail);
check(fs.writeFile, fs.writeFileSync, 'foo\u0000bar');
check('access', 'accessSync', 'foo\u0000bar');
check('access', 'accessSync', 'foo\u0000bar', 'F_OK');
check('appendFile', 'appendFileSync', 'foo\u0000bar');
check('chmod', 'chmodSync', 'foo\u0000bar', '0644');
check('chown', 'chownSync', 'foo\u0000bar', 12, 34);
check('link', 'linkSync', 'foo\u0000bar', 'foobar');
check('link', 'linkSync', 'foobar', 'foo\u0000bar');
check('lstat', 'lstatSync', 'foo\u0000bar');
check('mkdir', 'mkdirSync', 'foo\u0000bar', '0755');
check('open', 'openSync', 'foo\u0000bar', 'r');
check('readFile', 'readFileSync', 'foo\u0000bar');
check('readdir', 'readdirSync', 'foo\u0000bar');
check('readlink', 'readlinkSync', 'foo\u0000bar');
check('realpath', 'realpathSync', 'foo\u0000bar');
check('rename', 'renameSync', 'foo\u0000bar', 'foobar');
check('rename', 'renameSync', 'foobar', 'foo\u0000bar');
check('rmdir', 'rmdirSync', 'foo\u0000bar');
check('stat', 'statSync', 'foo\u0000bar');
check('symlink', 'symlinkSync', 'foo\u0000bar', 'foobar');
check('symlink', 'symlinkSync', 'foobar', 'foo\u0000bar');
check('truncate', 'truncateSync', 'foo\u0000bar');
check('unlink', 'unlinkSync', 'foo\u0000bar');
check(null, 'unwatchFile', 'foo\u0000bar', common.fail);
check('utimes', 'utimesSync', 'foo\u0000bar', 0, 0);
check(null, 'watch', 'foo\u0000bar', common.fail);
check(null, 'watchFile', 'foo\u0000bar', common.fail);
check('writeFile', 'writeFileSync', 'foo\u0000bar');

// an 'error' for exists means that it doesn't exist.
// one of many reasons why this file is the absolute worst.
fs.exists('foo\u0000bar', function(exists) {
assert(!exists);
});
assert(!fs.existsSync('foo\u0000bar'));

function checkRequire(arg) {
assert.throws(function() {
console.error(`require(${JSON.stringify(arg)})`);
require(arg);
}, expectedError);
}

checkRequire('\u0000');
checkRequire('foo\u0000bar');
checkRequire('foo\u0000');
checkRequire('foo/\u0000');
checkRequire('foo/\u0000.js');
checkRequire('\u0000/foo');
checkRequire('./foo/\u0000');
checkRequire('./\u0000/foo');