less / less.js

Less. The dynamic stylesheet language.
http://lesscss.org
Apache License 2.0
17k stars 3.41k forks source link

Vulnerabilities introduced by package hoek #3638

Closed paimon0715 closed 3 years ago

paimon0715 commented 3 years ago

Hi ,@matthew-dean @lukeapage , there are 2 vulnerabilities in your package:

Issue Description

2 vulnerabilities CVE-2018-3728 and npmjs-advisories-566 detected in package hoek@2.16.3 is transitively referenced by less@2.7.3. We noticed that such vulnerabilities have been removed since less@3.0.2.

However, less's popular previous version less@2.7.3 (217,198 downloads per week) is still transitively referenced by a large amount of latest versions of active and popular downstream projects (about 12,232 downstream projects, e.g., safecheck-client 3.0.8-aode-11, telephone-clients 3.0.57-8, apply-clients 3.0.7-125, weaver-mobile 1.17.8, css-inliner 2.0.0, @felixrieseberg/electron-prebuilt-compile@9.4.4, etc.). As such, issues CVE-2018-3728 and npmjs-advisories-566 can be propagated into these downstream projects and expose security threats to them.

These projects cannot easily upgrade less from version 2.7.3 to 3.*.* . For instance, less@2.7.3 is introduced into the above projects via the following package dependency path: (1)@felixrieseberg/electron-prebuilt-compile@9.4.4 ➔ electron-compilers@5.9.0 ➔ less@2.7.3 ➔ request@2.81.0 ➔ hawk@3.1.3 ➔ boom@2.10.1 ➔ hoek@2.16.3 ........

The projects such as electron-compilers which introduced less@2.7.3 are not maintained anymore. These unmaintained packages can neither upgrade less nor be easily migrated by the large amount of affected downstream projects.
On behalf the downstream users, could you help us remove the vulnerabilities from package less@2.7.3?

Suggested Solution

Since these unactive projects set a version constaint ~2.7.* for less on the above vulnerable dependency paths, if less removes the vulnerabilities from 2.7.3 and releases a new patched version less@2.7.4, such a vulnerability patch can be automatically propagated into the 12,232 affected downstream projects.

In less@2.7.4, you can kindly try to perform the following upgrade: request 2.81.0 ➔ 2.82.0;
Note: request@2.82.0 (>=2.82.0) transitively depends on hoek@4.2.1 (a vulnerabilities CVE-2018-3728 and npmjs-advisories-566 patched version)

Thanks again for your contributions.

Best regards, Paimon

iChenLei commented 3 years ago

Thanks for security report, I think release a new version is very easy.

paimon0715 commented 3 years ago

@iChenLei Thanks for your help and understanding.

iChenLei commented 3 years ago

@paimon0715 I send message to matthew-dean on gitter private channel. Wait matthew's response. I am not less npm package collaborator so I can't publish less npm package by myself. 2021-07-20 19 17 24

Who can publish v2.7.4 ? lukeapage | calvinjuarez | cloudhead | meri | seven-phases-max | matthew-dean

paimon0715 commented 3 years ago

@iChenLei Thanks again. Thanks all the collaborators.

matthew-dean commented 3 years ago

So, a few things:

  1. Security vulnerability reports from NPM are largely white noise. They are useless 99% of the time. A vulnerability in a package doesn't necessarily translate to a Less vulnerability; or another way to say it -- patching it doesn't mean that, say, using Less in the browser is more secure.
  2. We don't necessarily have the bandwidth to maintain 3 separate release versions, however...
  3. That said, if someone does want to bump a new 2.x release, I could expand publishing permissions to @iChenLei. I just don't have time personally.

So, I would personally consider this a non-issue considering that it doesn't necessarily mean an additional Less vulnerability, and considering it's patched in a major release.

matthew-dean commented 3 years ago

For additional context, whenever someone opens a security vulnerability report for the react library, they have a policy of immediately closing them because their position is that those security vulnerability reports are mostly useless.

iChenLei commented 3 years ago

Agree with @matthew-dean , recent Dan Abramov's blog npm-audit-broken-by-design

paimon0715 commented 3 years ago

@matthew-dean Thanks for your answers. I understand your explanations. The vulnerability reports "may" be a false positive. But 12,232 downstream projects recieved such reports every build. As a popular library version (217,198 downloads per week), if less.js can release a patch versoin less@2.7.4, it can definitively remove the security threats from ten thousands of dependency paths.

paimon0715 commented 3 years ago

Whatever it takes. Thanks.

sigveio commented 3 years ago

bogus

What's up with the bogus user account for posting all these reports anyway, @paimon0715?

I mean... kudos for taking the time to chase up these vulnerability-reports across 80+ repositories; whether they present real threats or not, I agree that it's a good thing to get it cleaned up where possible. So I can't see anything inherently malignant with your behaviour so far.

But I don't agree with seemingly posing as a Microsoft engineer to get extra gravitas in your pursuit.

(21 days old account with profile picture of Frank Darabont)

matthew-dean commented 3 years ago

@sigveio wait, what in the world? 😮

sigveio commented 3 years ago

I thought it was a bit strange that a Microsoft engineer would have a fresh account with no history and no org memberships. So I did a reverse image search for the profile picture, and it turns out it's a photo of the French-American director Frank Darabont. While there could exist plausible explanations for a fresh account, I'm fairly certain no MS employee would impersonate someone else on here. The fact that they swiftly changed their picture to a stock photo and removed the MS title after my post, and closed the thread, would suggest I was right.

For a project I'm contributing to, they suggested bumping the major version of a dependency in a patch version. For a vulnerability that was as good as non-existent (ReDoS in a transient dependency of a build tool). Hypothetically if someone were to follow such advice on the basis of thinking "well, it's coming from a Microsoft engineer... it must be the right thing to do" it could potentially introduce unexpected breaking changes into the NPM ecosystem. So even if the intention of it is not to do harm, it could.

Decided to post the above as an FYI to others... since this thread happened to be highlighted on their profile.

paimon0715 commented 3 years ago

@sigveio

What's up with the bogus user account for posting all these reports anyway, @paimon0715?

I mean... kudos for taking the time to chase up these vulnerability-reports across 80+ repositories; whether they present real threats or not, I agree that it's a good thing to get it cleaned up where possible. So I can't see anything inherently malignant with your behaviour so far.

But I don't agree with seemingly posing as a Microsoft engineer to get extra gravitas in your pursuit.

(21 days old account with profile picture of Frank Darabont)

I think I should give some explanations. Preventing the propagation of vulnerabilities in the npm ecosystem is our common task. I just want to do something to maintain the longterm health of JS projects. I indeed spend much efforts to analyze a large amount dependency relationships among npm packages to identify the issues. I analyzed a lot of projects and find the ones that fix the vulnerability issue with less efforts, but the fix can benefit more downstream users. It is a good starting point and a warm story.

That is why.

After reading your comment, I changed my profile. Not because of this issue report (the report is not malignant at all). I think it is my mistake to use others' picture, which may cause a misunderstanding.

Frank Darabont is my idol,simple reasons.

This is a report representing my own attitude and position, not related to other organizations.

Sorry for the inconvenience caused.

sigveio commented 3 years ago

Thanks for the follow up to explain, @paimon0715.

I believe you when you say that you have noble intentions. 😌

But you can also see that some of maintainers respond almost in a "Yessir!" kind of fashion when they see a very formal/serious sounding report from someone titling themselves a Microsoft engineer. And I hope you can understand that this could be unfortunate if the advice you give them in some cases could have unintended side-effects. People might let their guard down on the basis of your presumed authority/experience and act without properly thinking it through.

Do continue your efforts; just keep it honest ✌️

paimon0715 commented 3 years ago

@sigveio Thanks for your advise. Keep it in my mind. Learnt a lesson.