Skip to content

Commit

Permalink
[security] Fix DoS vulnerability
Browse files Browse the repository at this point in the history
Ignore extension and parameter names that are property names of
`Object.prototype` when parsing the `Sec-WebSocket-Extensions` header.
  • Loading branch information
lpinca committed Nov 8, 2017
1 parent 56f8062 commit c4fe466
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 10 deletions.
17 changes: 14 additions & 3 deletions lib/Extensions.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,13 @@ const parse = (value) => {
value.split(',').forEach((v) => {
const params = v.split(';');
const token = params.shift().trim();
const paramsList = extensions[token] = extensions[token] || [];

if (extensions[token] === undefined) {
extensions[token] = [];
} else if (!extensions.hasOwnProperty(token)) {
return;
}

const parsedParams = {};

params.forEach((param) => {
Expand All @@ -34,10 +40,15 @@ const parse = (value) => {
value = value.slice(0, value.length - 1);
}
}
(parsedParams[key] = parsedParams[key] || []).push(value);

if (parsedParams[key] === undefined) {
parsedParams[key] = [value];
} else if (parsedParams.hasOwnProperty(key)) {
parsedParams[key].push(value);
}
});

paramsList.push(parsedParams);
extensions[token].push(parsedParams);

This comment has been minimized.

Copy link
@1Bennell

1Bennell Jul 21, 2018

Push

This comment has been minimized.

Copy link
@burtonloretta7

burtonloretta7 Aug 1, 2020

Push

});

return extensions;
Expand Down
21 changes: 14 additions & 7 deletions test/Extensions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,21 @@ const Extensions = require('../lib/Extensions');

describe('Extensions', function () {
describe('parse', function () {
it('should parse', function () {
it('parses a single extension', function () {
const extensions = Extensions.parse('foo');

assert.deepStrictEqual(extensions, { foo: [{}] });
});

it('should parse params', function () {
it('parses params', function () {
const extensions = Extensions.parse('foo; bar; baz=1; bar=2');

assert.deepStrictEqual(extensions, {
foo: [{ bar: [true, '2'], baz: ['1'] }]
});
});

it('should parse multiple extensions', function () {
it('parse multiple extensions', function () {

This comment has been minimized.

Copy link
@burtonloretta7

burtonloretta7 Aug 1, 2020

Push

const extensions = Extensions.parse('foo, bar; baz, foo; baz');

assert.deepStrictEqual(extensions, {
Expand All @@ -29,29 +29,36 @@ describe('Extensions', function () {
});
});

it('should parse quoted params', function () {
it('parses quoted params', function () {
const extensions = Extensions.parse('foo; bar="hi"');

assert.deepStrictEqual(extensions, {
foo: [{ bar: ['hi'] }]
});
});

it('ignores names that match Object.prototype properties', function () {
const parse = Extensions.parse;

assert.deepStrictEqual(parse('hasOwnProperty, toString'), {});
assert.deepStrictEqual(parse('foo; constructor'), { foo: [{}] });
});
});

describe('format', function () {
it('should format', function () {
it('formats a single extension', function () {
const extensions = Extensions.format({ foo: {} });

assert.strictEqual(extensions, 'foo');
});

it('should format params', function () {
it('formats params', function () {
const extensions = Extensions.format({ foo: { bar: [true, 2], baz: 1 } });

assert.strictEqual(extensions, 'foo; bar; bar=2; baz=1');
});

it('should format multiple extensions', function () {
it('formats multiple extensions', function () {
const extensions = Extensions.format({
foo: [{}, { baz: true }],
bar: { baz: true }
Expand Down

9 comments on commit c4fe466

@ekristen
Copy link

Choose a reason for hiding this comment

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

Can this be fixed in the 2.x branch of the codebase, there are still many projects out there that rely on 2.x.

@xlcrr
Copy link

@xlcrr xlcrr commented on c4fe466 May 8, 2018

Choose a reason for hiding this comment

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

I got this and 3 other vulnerabilities today

NewOldMax/react-native-validator-form#3

Since running the commands to fix the lower risks, 2 more appeared

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ low           │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ lodash                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ af08040fa91c155023d3074c1e68edf3f8966db827039473fb05fb92ab8… │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ af08040fa91c155023d3074c1e68edf3f8966db827039473fb05fb92ab8… │
│               │ > plist > xmlbuilder > lodash                                │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/577                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ low           │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ deep-extend                                                  │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ af08040fa91c155023d3074c1e68edf3f8966db827039473fb05fb92ab8… │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ af08040fa91c155023d3074c1e68edf3f8966db827039473fb05fb92ab8… │
│               │ > metro > jest-haste-map > sane > fsevents > node-pre-gyp >  │
│               │ rc > deep-extend                                             │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/612   
└───────────────┴──────────────────────────────────────────────────────────────┘

New here, so not sure what to do.

@rasmus-storjohann-PG
Copy link

@rasmus-storjohann-PG rasmus-storjohann-PG commented on c4fe466 May 12, 2018

Choose a reason for hiding this comment

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

I keep getting red flag from npm audit after update, in package.json I have "ws": "^5.1.1", but I get this from the audit:

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Denial of Service                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ ws                                                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >= 1.1.5 <2.0.0 || >=3.3.1                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ react-native                                                 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ react-native > react-devtools-core > ws                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/550                       │
└───────────────┴──────────────────────────────────────────────────────────────┘

Looks like a typo, Patched in references "1.1.5" but it should be "5.1.1".

@lpinca
Copy link
Member Author

@lpinca lpinca commented on c4fe466 May 12, 2018

Choose a reason for hiding this comment

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

@rasmus-storjohann-PG no that is correct. ws@1.1.5 and ws@>=3.3.1 are not vulnerable, all other versions are.

@muditasisodia
Copy link

Choose a reason for hiding this comment

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

I'm using version 5.1.1 and still finding it to be vulnerable the same as @rasmus-storjohann-PG .

@arttarawork
Copy link

Choose a reason for hiding this comment

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

After upgrading to ws@5.2.0, I'm still getting the red flag that @rasmus-storjohann-PG reported. Is this a genuine red flag, or is there an issue with how the version number is recognized?

@lpinca
Copy link
Member Author

@lpinca lpinca commented on c4fe466 Jun 21, 2018

Choose a reason for hiding this comment

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

All versions above 3.3.0 are not vulnerable.

@with-a-k
Copy link

Choose a reason for hiding this comment

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

You're seeing that red flag because the react-devtools-core is using a version with this vulnerability.

@B1gsp4c3
Copy link

Choose a reason for hiding this comment

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

Just wanted to check, if i use a version above 3.3.0 but react-devtools-core is still using a version with this vulnerability, do i need to worry about the vulnerability? Or is it used in a way that doesn't make it a real issue?

Please sign in to comment.