Skip to content

http2: refactor to avoid unsafe array iteration #36700

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

Merged
merged 1 commit into from
Jan 15, 2021
Merged

http2: refactor to avoid unsafe array iteration #36700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion lib/internal/http2/compat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const {
ReflectApply,
ReflectGetPrototypeOf,
StringPrototypeIncludes,
SafeArrayIterator,
StringPrototypeToLowerCase,
StringPrototypeTrim,
Symbol,
Expand Down Expand Up @@ -148,7 +149,8 @@ function onStreamTrailers(trailers, flags, rawTrailers) {
const request = this[kRequest];
if (request !== undefined) {
ObjectAssign(request[kTrailers], trailers);
ArrayPrototypePush(request[kRawTrailers], ...rawTrailers);
ArrayPrototypePush(request[kRawTrailers],
...new SafeArrayIterator(rawTrailers));
}
}

Expand Down
28 changes: 17 additions & 11 deletions lib/internal/http2/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const {
ArrayIsArray,
ArrayPrototypeForEach,
ArrayPrototypePush,
ArrayPrototypeUnshift,
FunctionPrototypeBind,
FunctionPrototypeCall,
MathMin,
Expand All @@ -20,6 +21,7 @@ const {
ReflectApply,
ReflectGetPrototypeOf,
RegExpPrototypeTest,
SafeArrayIterator,
SafeMap,
SafeSet,
StringPrototypeSlice,
Expand Down Expand Up @@ -187,21 +189,22 @@ let debug = require('internal/util/debuglog').debuglog('http2', (fn) => {
// this seems pretty fast, though.
function debugStream(id, sessionType, message, ...args) {
debug('Http2Stream %s [Http2Session %s]: ' + message,
id, sessionName(sessionType), ...args);
id, sessionName(sessionType), ...new SafeArrayIterator(args));
}

function debugStreamObj(stream, message, ...args) {
const session = stream[kSession];
const type = session ? session[kType] : undefined;
debugStream(stream[kID], type, message, ...args);
debugStream(stream[kID], type, message, ...new SafeArrayIterator(args));
}

function debugSession(sessionType, message, ...args) {
debug('Http2Session %s: ' + message, sessionName(sessionType), ...args);
debug('Http2Session %s: ' + message, sessionName(sessionType),
...new SafeArrayIterator(args));
}

function debugSessionObj(session, message, ...args) {
debugSession(session[kType], message, ...args);
debugSession(session[kType], message, ...new SafeArrayIterator(args));
}

const kMaxFrameSize = (2 ** 24) - 1;
Expand Down Expand Up @@ -317,7 +320,7 @@ const SESSION_FLAGS_DESTROYED = 0x4;

// Top level to avoid creating a closure
function emit(self, ...args) {
self.emit(...args);
ReflectApply(self.emit, self, args);
}

// Called when a new block of headers has been received for a given
Expand Down Expand Up @@ -1020,7 +1023,7 @@ function setupHandle(socket, type, options) {

if (type === NGHTTP2_SESSION_SERVER &&
ArrayIsArray(options.origins)) {
this.origin(...options.origins);
ReflectApply(this.origin, this, options.origins);
}

process.nextTick(emit, this, 'connect', this, socket);
Expand Down Expand Up @@ -1495,7 +1498,7 @@ class Http2Session extends EventEmitter {
[EventEmitter.captureRejectionSymbol](err, event, ...args) {
switch (event) {
case 'stream':
const [stream] = args;
const stream = args[0];
stream.destroy(err);
break;
default:
Expand Down Expand Up @@ -1663,7 +1666,9 @@ class ClientHttp2Session extends Http2Session {
this[kUpdateTimer]();

if (headers !== null && headers !== undefined) {
for (const header of ObjectKeys(headers)) {
const keys = ObjectKeys(headers);
for (let i = 0; i < keys.length; i++) {
const header = keys[i];
if (header[0] === ':') {
assertValidPseudoHeader(header);
} else if (header && !checkIsHttpToken(header))
Expand Down Expand Up @@ -3095,7 +3100,7 @@ Http2Server.prototype[EventEmitter.captureRejectionSymbol] = function(
case 'stream':
// TODO(mcollina): we might want to match this with what we do on
// the compat side.
const [stream] = args;
const { 0: stream } = args;
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this also "vulnerable" to prototype pollution "attacks," if we really want to consider that "unsafe?" I mean, one could define a getter on the Array.prototype that returns something "malicious."

I'm really not sure if all this complexity and illegibility are worth the minimal benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Defining a 0 property on the Array.prototype would have effect only on empty arrays, right? args here is created by the spread operator, so I think this code would actually be safe even in case of Array.prototype pollution.

Object.defineProperty(Array.prototype, '0', {get(){return 9}, set(v){/* no op */}})
((t, ...args)=>args[0])(1, 2, 3) === 2 // true

I don't think we are concerned about code being "malicious" anyway, what we are trying to achieve with the move to primordials is to allow users to monkey-patch the built-in objects (for debugging purposes, for tinkering, etc.) without having errors thrown by Node.js internals.

Copy link
Member

Choose a reason for hiding this comment

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

so I think this code would actually be safe even in case of Array.prototype pollution.

Might be, I am not sure.

without having errors thrown by Node.js internals.

I understand the motivation, but I am not sure it's worth the cost to maintainability, readability etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To address the maintainability concern, would you prefer if we added a linter rule to forbid array destructuring assignment?

if (stream.sentHeaders) {
stream.destroy(err);
} else {
Expand All @@ -3104,7 +3109,7 @@ Http2Server.prototype[EventEmitter.captureRejectionSymbol] = function(
}
break;
case 'request':
const [, res] = args;
const { 1: res } = args;
if (!res.headersSent && !res.finished) {
// Don't leak headers.
for (const name of res.getHeaderNames()) {
Expand All @@ -3117,8 +3122,9 @@ Http2Server.prototype[EventEmitter.captureRejectionSymbol] = function(
}
break;
default:
ArrayPrototypeUnshift(args, err, event);
ReflectApply(net.Server.prototype[EventEmitter.captureRejectionSymbol],
this, [err, event, ...args]);
this, args);
}
};

Expand Down