Skip to content

Loading…

Unsecure access to undefined value removed #1240

Closed
wants to merge 1 commit into from

6 participants

@ddrcode

Direct access to undefined value is considered as bad practice in JavaScript, because undefined can be overridden. It is a typical hack which can break an application. On server-side it should be considered as an security issue, because such code can be theoretically injected from client side and break the entire server.

After overriding undefined value, Node.JS - due to insecure comparisons with undefined in http module (and others) can break with errors like "Cannot call method 'slice' of null" (or similar) - which can be difficult to debug (because the real nature of such error lies somewhere else).

I have applied following changes to most of JavaScript files from lib folder:
1. x === undefined replaced with typeof x === 'undefined'
2. x = undefined replaced with x = void 0
3. x == undefined replaced with x == null (works same way, but null can't be overridden)
4. typeof x == 'undefined' replaced with typeof x === 'undefined'
5. in http.js file isUndefinedOrNull function have been introduced and used for comparisons
6. in child_process.js file the local variable undefined have been introduced in ChildProcess.prototype.spawn method to make all internal comparisons to undefined securely working

More about protecting globals like undefined can be found in the description of my node-secure module

@isaacs

Is this really a concern? Where are you overwriting the global undefined?

If you're running untrusted code in the same context, you're already boned many ways anyhow.

@ddrcode

Is not easy to override undefined by mistake, however I've seen the code like this: if(undefined=x) {...}. :-)

The entire thing is probably more about some coding convention. However - with minimal effort there is possible to protect Node from crashing, just because someone by mistake or intentionally wrote something like undefined=666. Using typeof operator in comparisons to undefined is considered as a standard JS practice - would be great if Node could follow the common rules.

@isaacs

It seems like if you overwrite the value of "undefined", then a lot of things will exhibit strange behavior. Maybe the best choice is to crash as soon as possible then.

@tj
tj commented

-1

@ddrcode

With the changes I've suggested Node won't crash at all when you override undefined (I did tests). And I didn't add any extra functionality for it - which means no additional effort is needed to secure the code - just coding convention. If you would have to add some special conditions for it I would agree with you to not bother about it too much. In my opinion the main problem with overridden undefined is that it produces errors which may suggest completely different issues (like mentioned above "Cannot call method 'slice' of null"). Is not easy to debug such problems.

@ddrcode

I just took a look to util.js file of Node and found (starting from line 279) three util functions for type recognition - isRegExp, isDate and isArray. These functions do several quirks just to ensure that passed argument matches expected type. Theoretically it should be enough to just use the instanceof operator, but the code wants to avoid special situations when someone play with the global context or with prototypes or constructor property. My suggestion to recognize safely the undefined value - even in tricky situations - has exactly the same meaning and intention.

@isaacs

The difference is that the "undefined" global is a primitive, so "=== undefined" will mean the same thing even when running with NODE_MODULE_CONTEXTS=1 in the environment, which is considered "normal" from node's point of view (in that everything should work). Not so with Date, Regexp, or Array. Also, the isArray in util.js is designed to also match the arguments free var inside a function.

@bnoordhuis
Node.js Foundation member

@isaacs that's not right, consider this:

// redefine.js
undefined = "BAM!";

// undefined.js
require('./redefine.js');
console.error(undefined);
console.error(process.bam === undefined);
console.error(process.bam === void 0);

node undefined.js:

BAM!
false
true

I like the idea behind this patch, one way less to shoot yourself in the foot. Review and consider it.

@tj
tj commented

there is no end defensive programming, IMO just dont do stupid things

@wavded

i wonder if undefined is not allowed to be overwritten in strict mode, I found fringe sources that said it was but nothing concrete. If so, you could just 'use strict' in these files. May not be bad idea anyway to avoid other unexpected behavior.

@bnoordhuis
Node.js Foundation member

@wavded V8 supports most of strict mode but assigning to undefined still works.

node doesn't work well in strict mode anyway. Running node's test suite with node --strict fails 100% :-(

@wavded

I suppose if you are concerned you could this at the top of affected files:

const undefined;

although V8 seems to error silently when overriding on that one.

@ddrcode

You always can protect the globals from overriding by using property descriptors of ECMAScript 5 - which - in contract to strict mode - works fine in most of modern environments including V8/Node. I'm suggesting solution like this in my node-secure module:

Object.defineProperty && (function(){
   // take global object
   var global = (function(){return this;})();

   // when "strict mode' is on we can leave - globals are protected by default
   if(!global) return;

   // redefine proper values in case they got overridden
   undefined = void 0;
   NaN = 0/0;
   Infinity = 1/0;

   // make globals read only and non-configurable
   ["undefined", "NaN", "Infinity"].forEach(function(key){
      Object.defineProperty(global, key, {writable: false, configurable: false});
   });
})();

However, the example protects the application with external code. I would still vote for secure operations on undefined in Node files because it does not require any additional logic.

@trevnorris

As of node v0.8.14, here's the current standing of assigning undefined.

(function() {
    "use strict";
    undefined = true;
    console.log(undefined);
}());

Throws the following:

TypeError: Cannot assign to read only property 'undefined' of #<Object>
    at /[...]/bug-1240.js:3:12
    at Object.<anonymous> (/[...]/bug-1240.js:5:2)
    at Module._compile (module.js:449:26)
    at Object.Module._extensions..js (module.js:467:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.runMain (module.js:492:10)
    at process.startup.processNextTick.process._tickCallback (node.js:244:9)

And

(function() {
    undefined = true;
    console.log(undefined);
}());

Displays undefined. But there is one interesting case:

(function() {
    "use strict";  // with or w/o this, displays the same
    var undefined = true;
    console.log(undefined);
}());

Displays true.

So I'd say this pull request has already been resolved.

@bnoordhuis bnoordhuis closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 27, 2011
  1. @ddrcode
Showing with 50 additions and 46 deletions.
  1. +1 −1 lib/_debugger.js
  2. +1 −1 lib/assert.js
  3. +5 −5 lib/buffer.js
  4. +2 −2 lib/child_process.js
  5. +1 −1 lib/dgram.js
  6. +1 −1 lib/dns.js
  7. +1 −1 lib/events.js
  8. +12 −12 lib/fs.js
  9. +8 −4 lib/http.js
  10. +1 −1 lib/https.js
  11. +1 −1 lib/module.js
  12. +4 −4 lib/net_legacy.js
  13. +1 −1 lib/querystring.js
  14. +1 −1 lib/readline.js
  15. +2 −2 lib/repl.js
  16. +4 −4 lib/tty_posix.js
  17. +4 −4 lib/url.js
View
2 lib/_debugger.js
@@ -186,7 +186,7 @@ Client.prototype._addScript = function(desc) {
Client.prototype._removeScript = function(desc) {
- this.scripts[desc.id] = undefined;
+ this.scripts[desc.id] = void 0;
};
View
2 lib/assert.js
@@ -163,7 +163,7 @@ function _deepEqual(actual, expected) {
}
function isUndefinedOrNull(value) {
- return value === null || value === undefined;
+ return value === null || typeof value === 'undefined';
}
function isArguments(object) {
View
10 lib/buffer.js
@@ -58,7 +58,7 @@ SlowBuffer.prototype.hexSlice = function(start, end) {
SlowBuffer.prototype.toString = function(encoding, start, end) {
encoding = String(encoding || 'utf8').toLowerCase();
start = +start || 0;
- if (typeof end == 'undefined') end = this.length;
+ if (typeof end === 'undefined') end = this.length;
// Fastpath empty strings
if (+end == start) {
@@ -150,7 +150,7 @@ SlowBuffer.prototype.write = function(string, offset, encoding) {
// slice(start, end)
SlowBuffer.prototype.slice = function(start, end) {
- if (end === undefined) end = this.length;
+ if (typeof end === 'undefined') end = this.length;
if (end > this.length) {
throw new Error('oob');
@@ -328,13 +328,13 @@ Buffer.prototype.write = function(string, offset, encoding) {
Buffer.prototype.toString = function(encoding, start, end) {
encoding = String(encoding || 'utf8').toLowerCase();
- if (typeof start == 'undefined' || start < 0) {
+ if (typeof start === 'undefined' || start < 0) {
start = 0;
} else if (start > this.length) {
start = this.length;
}
- if (typeof end == 'undefined' || end > this.length) {
+ if (typeof end === 'undefined' || end > this.length) {
end = this.length;
} else if (end < 0) {
end = 0;
@@ -450,7 +450,7 @@ Buffer.prototype.copy = function(target, target_start, start, end) {
// slice(start, end)
Buffer.prototype.slice = function(start, end) {
- if (end === undefined) end = this.length;
+ if (typeof end === 'undefined') end = this.length;
if (end > this.length) throw new Error('oob');
if (start > end) throw new Error('oob');
View
4 lib/child_process.js
@@ -124,7 +124,7 @@ exports.execFile = function(file /* args, options, callback */) {
var keys = Object.keys(options);
for (var i = 0, len = keys.length; i < len; i++) {
var k = keys[i];
- if (optionArg[k] !== undefined) options[k] = optionArg[k];
+ if (typeof optionArg[k] !== 'undefined') options[k] = optionArg[k];
}
}
@@ -265,7 +265,7 @@ ChildProcess.prototype.kill = function(sig) {
ChildProcess.prototype.spawn = function(path, args, options, customFds) {
args = args || [];
- var cwd, env, setsid, uid, gid;
+ var cwd, env, setsid, uid, gid, undefined;
if (!options || options.cwd === undefined &&
options.env === undefined &&
options.customFds === undefined &&
View
2 lib/dgram.js
@@ -134,7 +134,7 @@ Socket.prototype.bind = function() {
});
} else if (this.type === 'udp4' || this.type === 'udp6') {
// bind(port, [address])
- if (arguments[1] === undefined) {
+ if (typeof arguments[1] === 'undefined') {
// Not bind()ing a specific address. Use INADDR_ANY and OS will pick one.
// The address can be found with server.address()
binding.bind(self.fd, arguments[0]);
View
2 lib/dns.js
@@ -137,7 +137,7 @@ exports.lookup = function(domain, family, callback) {
// parse arguments
if (arguments.length === 2) {
callback = family;
- family = undefined;
+ family = void 0;
} else if (family && family !== 4 && family !== 6) {
family = parseInt(family, 10);
if (family === dns.AF_INET) {
View
2 lib/events.js
@@ -111,7 +111,7 @@ EventEmitter.prototype.addListener = function(type, listener) {
// Check for listener leak
if (!this._events[type].warned) {
var m;
- if (this._events.maxListeners !== undefined) {
+ if (typeof this._events.maxListeners !== 'undefined') {
m = this._events.maxListeners;
} else {
m = defaultMaxListeners;
View
24 lib/fs.js
@@ -200,7 +200,7 @@ function modeNum(m, def) {
if (def) {
return modeNum(def);
} else {
- return undefined;
+ return void 0;
}
}
}
@@ -284,7 +284,7 @@ fs.write = function(fd, buffer, offset, length, position, callback) {
if (!length) {
if (typeof callback == 'function') {
process.nextTick(function() {
- callback(undefined, 0);
+ callback(void 0, 0);
});
}
return;
@@ -602,8 +602,8 @@ fs.watchFile = function(filename) {
listener = arguments[1];
}
- if (options.persistent === undefined) options.persistent = true;
- if (options.interval === undefined) options.interval = 0;
+ if (typeof options.persistent === 'undefined') options.persistent = true;
+ if (typeof options.interval === 'undefined') options.interval = 0;
if (statWatchers[filename]) {
stat = statWatchers[filename];
@@ -621,7 +621,7 @@ fs.unwatchFile = function(filename) {
if (statWatchers[filename]) {
stat = statWatchers[filename];
stat.stop();
- statWatchers[filename] = undefined;
+ statWatchers[filename] = void 0;
}
};
@@ -891,8 +891,8 @@ var ReadStream = fs.ReadStream = function(path, options) {
if (this.encoding) this.setEncoding(this.encoding);
- if (this.start !== undefined) {
- if (this.end === undefined) {
+ if (typeof this.start !== 'undefined') {
+ if (typeof this.end === 'undefined') {
this.end = Infinity;
}
@@ -942,7 +942,7 @@ ReadStream.prototype._read = function() {
allocNewPool();
}
- if (self.start !== undefined && self._firstRead) {
+ if (typeof self.start !== 'undefined' && self._firstRead) {
self.pos = self.start;
self._firstRead = false;
}
@@ -954,7 +954,7 @@ ReadStream.prototype._read = function() {
var toRead = Math.min(pool.length - pool.used, this.bufferSize);
var start = pool.used;
- if (this.pos !== undefined) {
+ if (typeof this.pos !== 'undefined') {
toRead = Math.min(this.end - this.pos + 1, toRead);
}
@@ -993,7 +993,7 @@ ReadStream.prototype._read = function() {
fs.read(self.fd, pool, pool.used, toRead, self.pos, afterRead);
- if (self.pos !== undefined) {
+ if (typeof self.pos !== 'undefined') {
self.pos += toRead;
}
pool.used += toRead;
@@ -1087,7 +1087,7 @@ var WriteStream = fs.WriteStream = function(path, options) {
this._queue = [];
if (this.fd === null) {
- this._queue.push([fs.open, this.path, this.flags, this.mode, undefined]);
+ this._queue.push([fs.open, this.path, this.flags, this.mode, void 0]);
this.flush();
}
};
@@ -1175,7 +1175,7 @@ WriteStream.prototype.write = function(data) {
} else {
var encoding = 'utf8';
if (typeof(arguments[1]) == 'string') encoding = arguments[1];
- this._queue.push([fs.write, data, undefined, encoding, cb]);
+ this._queue.push([fs.write, data, void 0, encoding, cb]);
}
View
12 lib/http.js
@@ -35,6 +35,10 @@ if (process.env.NODE_DEBUG && /http/.test(process.env.NODE_DEBUG)) {
debug = function() { };
}
+function isUndefinedOrNull(value) {
+ return value === null || typeof value === 'undefined';
+}
+
var parsers = new FreeList('parsers', 1000, function() {
var parser = new HTTPParser('request');
@@ -58,7 +62,7 @@ var parsers = new FreeList('parsers', 1000, function() {
parser.onHeaderField = function(b, start, len) {
var slice = b.toString('ascii', start, start + len).toLowerCase();
- if (parser.value != undefined) {
+ if (!isUndefinedOrNull(parser.value)) {
parser.incoming._addHeaderLine(parser.field, parser.value);
parser.field = null;
parser.value = null;
@@ -80,7 +84,7 @@ var parsers = new FreeList('parsers', 1000, function() {
};
parser.onHeadersComplete = function(info) {
- if (parser.field && (parser.value != undefined)) {
+ if (parser.field && !isUndefinedOrNull(parser.value)) {
parser.incoming._addHeaderLine(parser.field, parser.value);
parser.field = null;
parser.value = null;
@@ -124,7 +128,7 @@ var parsers = new FreeList('parsers', 1000, function() {
parser.onMessageComplete = function() {
this.incoming.complete = true;
- if (parser.field && (parser.value != undefined)) {
+ if (parser.field && !isUndefinedOrNull(parser.value)) {
parser.incoming._addHeaderLine(parser.field, parser.value);
}
if (!parser.incoming.upgrade) {
@@ -1479,7 +1483,7 @@ exports._requestFromAgent = function(options, cb) {
exports.request = function(options, cb) {
- if (options.agent === undefined) {
+ if (typeof options.agent === 'undefined') {
options.agent = getAgent(options);
} else if (options.agent === false) {
options.agent = new Agent(options);
View
2 lib/https.js
@@ -93,7 +93,7 @@ exports.getAgent = getAgent;
exports.Agent = Agent;
exports.request = function(options, cb) {
- if (options.agent === undefined) {
+ if (typeof options.agent === 'undefined') {
options.agent = getAgent(options);
} else if (options.agent === false) {
options.agent = new Agent(options);
View
2 lib/module.js
@@ -432,7 +432,7 @@ function makeDefine(require, module) {
var ret = cb(require, module.exports, module);
- if (ret !== undefined) {
+ if (typeof ret !== 'undefined') {
// set exports with return statement
// define(function () { return { foo: "bar" } })
module.exports = ret;
View
8 lib/net_legacy.js
@@ -113,7 +113,7 @@ function allocEmptyBuffer() {
function setImplmentationMethods(self) {
function noData(buf, off, len) {
return !buf ||
- (off != undefined && off >= buf.length) ||
+ (off != null && off >= buf.length) ||
(len == 0);
};
@@ -221,7 +221,7 @@ function Socket(options) {
this.allowHalfOpen = false;
if (typeof options == 'object') {
- this.fd = options.fd !== undefined ? parseInt(options.fd, 10) : null;
+ this.fd = typeof options.fd !== 'undefined' ? parseInt(options.fd, 10) : null;
this.type = options.type || null;
this.allowHalfOpen = options.allowHalfOpen || false;
} else if (typeof options == 'number') {
@@ -363,7 +363,7 @@ Socket.prototype.write = function(data /* [encoding], [fd], [cb] */) {
this._writeQueueCallbacks.push(cb);
}
- if (fd != undefined) {
+ if (fd != null) {
this._writeQueueFD.push(fd);
}
@@ -895,7 +895,7 @@ function Server(/* [ options, ] listener */) {
if (typeof arguments[l] == 'function') {
self.addListener('connection', arguments[l]);
}
- if (arguments[l] !== undefined) break;
+ if (typeof arguments[l] !== 'undefined') break;
}
self.connections = 0;
View
2 lib/querystring.js
@@ -126,7 +126,7 @@ var stringifyPrimitive = function(v) {
QueryString.stringify = QueryString.encode = function(obj, sep, eq, name) {
sep = sep || '&';
eq = eq || '=';
- obj = (obj === null) ? undefined : obj;
+ obj = (obj === null) ? void 0 : obj;
switch (typeof obj) {
case 'object':
View
2 lib/readline.js
@@ -223,7 +223,7 @@ Interface.prototype.write = function(d, key) {
Interface.prototype._normalWrite = function(b) {
// Very simple implementation right now. Should try to break on
// new lines.
- if (b !== undefined)
+ if (typeof b !== 'undefined')
this._onLine(b.toString());
};
View
4 lib/repl.js
@@ -80,7 +80,7 @@ function REPLServer(prompt, stream) {
process.stdin.resume();
}
- self.prompt = (prompt != undefined ? prompt : '> ');
+ self.prompt = (prompt != null ? prompt : '> ');
function complete(text) {
return self.complete(text);
@@ -168,7 +168,7 @@ function REPLServer(prompt, stream) {
ret = vm.runInContext(self.bufferedCommand, self.context, 'repl');
}
- if (ret !== undefined) {
+ if (typeof ret !== 'undefined') {
self.context._ = ret;
self.outputStream.write(exports.writer(ret) + '\n');
}
View
8 lib/tty_posix.js
@@ -125,7 +125,7 @@ var functionKeyCodeRe =
ReadStream.prototype._emitKey = function(s) {
var char,
key = {
- name: undefined,
+ name: void 0,
ctrl: false,
meta: false,
shift: false
@@ -133,7 +133,7 @@ ReadStream.prototype._emitKey = function(s) {
parts;
if (Buffer.isBuffer(s)) {
- if (s[0] > 127 && s[1] === undefined) {
+ if (s[0] > 127 && typeof s[1] === 'undefined') {
s[0] -= 128;
s = '\x1b' + s.toString(this.encoding || 'utf-8');
} else {
@@ -295,8 +295,8 @@ ReadStream.prototype._emitKey = function(s) {
}
// Don't emit a key if no name was found
- if (key.name === undefined) {
- key = undefined;
+ if (typeof key.name === 'undefined') {
+ key = void 0;
}
if (s.length === 1) {
View
8 lib/url.js
@@ -285,8 +285,8 @@ function urlFormat(obj) {
}
var protocol = obj.protocol || '',
- host = (obj.host !== undefined) ? obj.host :
- obj.hostname !== undefined ? (
+ host = (typeof obj.host !== 'undefined') ? obj.host :
+ typeof obj.hostname !== 'undefined' ? (
(auth ? auth + '@' : '') +
obj.hostname +
(obj.port ? ':' + obj.port : '')
@@ -373,7 +373,7 @@ function urlResolveObject(source, relative) {
var isSourceAbs = (source.pathname && source.pathname.charAt(0) === '/'),
isRelAbs = (
- relative.host !== undefined ||
+ typeof relative.host !== 'undefined' ||
relative.pathname && relative.pathname.charAt(0) === '/'
),
mustEndAbs = (isRelAbs || isSourceAbs ||
@@ -383,7 +383,7 @@ function urlResolveObject(source, relative) {
relPath = relative.pathname && relative.pathname.split('/') || [],
psychotic = source.protocol &&
!slashedProtocol[source.protocol] &&
- source.host !== undefined;
+ typeof source.host !== 'undefined';
// if the url is a non-slashed url, then relative
// links like ../.. should be able
Something went wrong with that request. Please try again.