Security issue in dependencies

[noraj]

Update

Resolved in node-sass@4.9.3. Upgrade to quiet npm.

---

hoek@2.16.3 is vulnerable to CVE-2018-3728

for node-sass the problem comes from requiring request@2.79.0 in the package.json

dependency tree is as follow for 4.8.3

node-sass@4.8.3
|-request@2.79.0
 |-hawk@3.1.3
  |-hoek@2.16.3

and is the same for 4.9.0:

node-sass@4.9.0
|-request@2.79.0
 |-hawk@3.1.3
  |-hoek@2.16.3

Fix

To fix this request@2.82.0 or superior is required.

Context

• NPM version (npm -v): 5.8.0
• Node version (node -v): v9.11.1
• Node Process (node -p process.versions):

{ http_parser: '2.8.0',
  node: '9.11.1',
  v8: '6.2.414.46-node.23',
  uv: '1.19.2',
  zlib: '1.2.11',
  ares: '1.13.0',
  modules: '59',
  nghttp2: '1.29.0',
  napi: '3',
  openssl: '1.0.2o',
  icu: '61.1',
  unicode: '10.0',
  cldr: '33.0',
  tz: '2018c' }

• Node Platform (node -p process.platform): linux
• Node architecture (node -p process.arch): x64
• node-sass version (node -p "require('node-sass').info"):

node-sass	4.9.0	(Wrapper)	[JavaScript]
libsass  	3.5.4	(Sass Compiler)	[C/C++]

• npm node-sass versions (npm ls node-sass):

├─┬ gulp-sass@4.0.1
│ └── node-sass@4.9.0  deduped
└── node-sass@4.9.0 

Related issues

request:

request/request#2926
request/request#2874

node-sass:

#2352
#2288
#2262
#2252
#2170
#2256

Problem

xzyfer in #2352

It cannot be fixed without break node < 4 support

I also see in #2288 that the problem is solved in node-sass v5.

So this ticket need to stay opened until v5 is released. Please don't close it.

[samrispaud]

i see the fix is on master :) is there a timeline on when the next major or minor release will be with this change?

[xzyfer]

Subscribe to the v5 pr. I'm in holidays now so not for a couple weeks.

…

On Mon., 7 May 2018, 10:11 pm Sam Rispaud, ***@***.***> wrote: i see the fix is on master :) is there a timeline on when the next major or minor release will be with this change? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2355 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWHnkAOwGZ6HRxWYeb6rXfVHkiwRkks5twKpTgaJpZM4Tq1z4> .

[noraj]

@samrispaud

That's what I said ;-)

I also see in #2288 that the problem is solved in node-sass v5.

So this ticket need to stay opened until v5 is released. Please don't close it.

[cloakedninjas]

What about creating a 4.10.0 release that just bumps the request dependency ?

[xzyfer]

Bumping the dependency would break support for Node < 4

…

On Wed., 9 May 2018, 12:41 pm Daniel Jackson, ***@***.***> wrote: What about creating a 4.10.0 release that just bumps the request dependency ? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2355 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWPrx7wQEUkTqYNHbDloFb4PBwGhAks5twsfagaJpZM4Tq1z4> .

[noraj]

@cloakedninjas It's a wait and see for v5 release.

[chriseppstein]

As I understand it, you can change the node engine version requirement and do a minor release. It doesn't violate semver for the people on the old node versions because the new versions of the package are excluded from that code's dependency version resolution.

[xzyfer]

My understanding is that the engine property has no actual effect beyond npm producing a warning. The install will still happen. The way to resolve this error without breaking BC is to replace request all together with something that's supports Node 0.10+

…

On Wed., 9 May 2018, 9:22 pm Chris Eppstein, ***@***.***> wrote: As I understand it, you can change the node engine version requirement and do a minor release. It doesn't violate semver for the people on the old node versions because the new versions of the package are excluded from that code's dependency version resolution. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2355 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWJUyGWTrunvxfUG5PIvAVtwZ4xNBks5tw0IQgaJpZM4Tq1z4> .

[ataylorme]

Node 4 became end of life on May 1st (source). Can you provide some background on the decision to continue supporting it after end of life rather than pushing out a security fix? I'm not disagreeing with any decision but rather seek to understand it. For example, are there a large number of Node 4 users?

[florianbrinkmann]

node-gyp, another dependency of node-sass, also uses the old version of the request library. I asked if they would update it, but the answer was no. Does that mean that the security issue will remain because both request dependencies will be loaded (the updated one from node-sass and the old one from node-gyp)?

[xzyfer]

Why do you keep deleting comments from people?

"mee too" and "+1" comments add no practical value. They only add noise.

Is there guidelines on how to comment an issue that we're breaking

Comments should add some new information to the issue at hand. Otherwise they're just adding more work for people trying to the useful information in the issue.

In most cases the (now deleted) comments showed no indication the OP had made any effort to read the existing discussion.

The point of commenting on an open source project is to voice our [..]

You can use GitHubs to emoji reactions to add support for an issue, or subscribe for updates.

to support a minority of people using node < 4

Please don't make baseless generalisations. We still get a lot of install for Node < 4. Even a minority of users at our install volume is a lot of installs.

For v5 we made the hard to call to drop support for Node < 6.

---

We've explained multiple times in this issue why this is blocked, however hidden by GitHub due to the high volume of "me too" comments.

As a summary:

• we are unable to upgrade without breaking things Node < 4 users
• v5 updates our dependency on request
  • but node-gyp is still locked the same version for the same reason

After some investigation from @Rohithzr in #2417 there isn't a path for us to drop our dependency on node-gyp at this time. So npm audit will continue to complain (as far as we've been informed).

We will be moving forward with v5 in the coming weeks, but at this point in time it's unclear whether the request update will quiet the warning.

[xzyfer]

Update

request@2.87.0 removed the dependency on hoek (request/request#2943)
recently. This restores support for Node < 4. This means we bump
to the latest request which hopefully resolves npm audit warnings.

Should we need to address node-gyp warning we'll track that in a
different issue.

[Rohithzr]

@xzyfer thats a great news i suppose, I shall run some tests and make a PR

[xzyfer]

@Rohithzr see #2435

I've tested locally and it looks positive. We'll see what CI thinks.

[xzyfer]

@vishnugupta20 if #2435 passes I'll release a v4.9.1 with request updated in the next 24hrs.

[guidobouman]

One thing to note is that this security issue is mainly irrelevant to node-sass. GitHub is flagging this as a security vulnerability, but node-sass never sees any exposure to your live code. The attack vector is when a request from the outside world can trigger a node-sass build with custom data. CSS is (usually) precompiled during a build stage, whether on a CI or a local machine. It runs on machines you protect from the outside world, it's never really exposed on a network.

Anything opening node-sass to the outside world is a highly specific case. The only use case I can currently think of is something like CodePen / JSFiddle. And even then my best guess is that node-sass is not vulnerable because nor they nor request use methods that could lead to remote code execution.

[xzyfer]

You're totally correct. There is no actual security issue to node-sass users. Unfortunately npm@6 makes noise simply if a flagged package is in the dependency tree regardless of context. Lots of people just want to quieten npm regardless of any risk.

…

On Thu., 5 Jul. 2018, 1:08 am Guido Bouman, ***@***.***> wrote: One thing to note is that this security issue is mainly irrelevant to node-sass. GitHub is flagging this as a security vulnerability, but node-sass never sees any exposure to your live code. The attack vector is when a request from the outside world can trigger a node-sass build with custom data. CSS is (usually) precompiled during a build stage, whether on a CI or a local machine. It runs on machines you protect from the outside world, it's never really exposed on a network. Anything opening node-sass to the outside world is a highly specific case. The only use case I can currently think of is something like CodePen / JSFiddle. And even then my best guess is that node-sass is not vulnerable because nor they nor request use methods that could lead to remote code execution. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2355 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWPtyJFDjslJ1cJe5lf5Ju8p21_PWks5uDNprgaJpZM4Tq1z4> .

[xzyfer]

Update

Resolved in node-sass@4.9.1. Upgrade to quiet npm.

[nschonni]

Re-opening till nodejs/node-gyp#1471 lands, but keeping locked

[nschonni]

node-gyp 3.8.0 was released with a bump in their request version https://github.com/nodejs/node-gyp/blob/master/CHANGELOG.md#v380-2018-08-09.
Clearing your lockfile and re-installing may resolve your issues now. PR to update the minimum version in this repo has been submited.

[xzyfer]

Update

Resolved in node-sass@4.9.3. Upgrade to quiet npm.