Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Critical vulnerability of NPM package macaddress

nodesecurity.io

Post reply on HN
Print view View original

61–70 of 99 posts

Re: Critical vulnerability of NPM package macaddress

#61

Post by xorcist »

megous wrote: post #54
rms_returns wrote: post #53

Earlier quoted context omitted.

There is no dearth of amateurish shenanigans in node infrastructure, there is a reason that people are still reluctant to switch from php/python despite so many selling points of node.

These kinds of bugs are language independent, the source is between the screen and the keyboard.

The "amateurish shenanigans" alluded to probably has nothing to do with the language but the project itself.

If anyone can commit anything and nobody ever reads what is to be commited, the repository must be regarded as attacker controlled. Some people will likely find that problematic.

Re: Critical vulnerability of NPM package macaddress

#62

Post by 00oo00 »

royjacobs wrote: post #52

That yet another NPM package proves to be pretty broken seems to be par for the course, but what especially shocks me is that on Hackerone the people working on this vulnerability can't reach the original maintainer and then say: "Doesn't look epic to me, also last publish was 3 years ago." It doesn't look epic, yet it's a package that has over 1.2 million weekly downloads. Also, fun fact, one of the dependent packag…

Checkout react-input-select's package.json... It's just full of unrelated random dependencies. Might be some way to advertise his node package?

Re: Critical vulnerability of NPM package macaddress

#63

Post by furgooswft13 »

koolba wrote: post #2

Here's the offending line of code[1]: exec("cat /sys/class/net/" + iface + "/address", function (err, out) { That's some serious amateur hour and may even be a new contender for the Useless Use of Cat (and "exec(...)") Award. Would be simpler and less error prone to just read the file via fs.readFile(...) after verifying that the iface parameter does not contain a directory separator. [1]: https://github.com/scravy/n…

Yikes. Even if fixed I would be highly suspect of the rest of the code written by someone who felt they needed an exec call to read a file.

Perhaps I should make a new npm module named cat

    module.exports = require('fs').readFile;
and saved the day.

Re: Critical vulnerability of NPM package macaddress

#64

Post by f055 »

Boulth wrote: post #51
koolba wrote: post #2

Here's the offending line of code[1]: exec("cat /sys/class/net/" + iface + "/address", function (err, out) { That's some serious amateur hour and may even be a new contender for the Useless Use of Cat (and "exec(...)") Award. Would be simpler and less error prone to just read the file via fs.readFile(...) after verifying that the iface parameter does not contain a directory separator. [1]: https://github.com/scravy/n…

This is kind of embarrassing but the advisories page is full of gems like that: https://nodesecurity.io/advisories

I'm shocked. There are almost 30 advisories in May alone that are 7+.

To put this into perspective Perl, which I like, with its modules had 3 this year. And 3 last year. Three per year. Three. https://www.cvedetails.com/vulnerability-list/vendor_id-1885...

Re: Critical vulnerability of NPM package macaddress

#65

Post by grumdan »

megous wrote: post #54
rms_returns wrote: post #53

Earlier quoted context omitted.

There is no dearth of amateurish shenanigans in node infrastructure, there is a reason that people are still reluctant to switch from php/python despite so many selling points of node.

These kinds of bugs are language independent, the source is between the screen and the keyboard.

They are language-independent but may still have a higher incidence in one community than another. Anecdotally, these basic mistakes seem to happen more in Node packages or JS-based projects, but I have no concrete evidence for this speculation. The only study I know of looking at this in a serious way is this one:

http://web.cs.ucdavis.edu/~filkov/papers/lang_github.pdf (https://news.ycombinator.com/item?id=8558740)

However, JavaScript ends up a being less prone to defective commits than C++ and C, as well as PHP and Python, but there are a number of issues that don't allow us to conclude all that much from these results (imo).

Re: Critical vulnerability of NPM package macaddress

#66

Post by caymanjim »

arghwhat wrote: post #7
koolba wrote: post #2

Here's the offending line of code[1]: exec("cat /sys/class/net/" + iface + "/address", function (err, out) { That's some serious amateur hour and may even be a new contender for the Useless Use of Cat (and "exec(...)") Award. Would be simpler and less error prone to just read the file via fs.readFile(...) after verifying that the iface parameter does not contain a directory separator. [1]: https://github.com/scravy/n…

This is so awful that its existence is borderline suspicious. It's also conveniently just part of a massive "v1.0" (initial commit).

Never attribute to malice that which is adequately explained by stupidity.

Re: Critical vulnerability of NPM package macaddress

#67

Post by cyphar »

pi-squared wrote: post #35
georgyo wrote: post #27

Many people here are saying that the problem had been in the code base for years, was super obvious, and that just no one looked at the code. I think that it is much more relevant that the node community does do many micro packages, that it makes so much nose to look at the quality of pulled in packages. I recently pulled down a web app (Zulip) the packages.json expands to almost 1400 npm packages. How is any human g…

I am not a fan of the whole javascript craze but for comparison on my desktop machine with I assume average setup of ElementaryOS: $ apt list --installed | wc -l 2394 And on my server the number is 502. Couldn't the same argument be applied here as well? I understand that (probably) users of Ubuntu are more involved than supporting npm packages (or... are they) but it's still a lot of packages to eyeball.

That is for everything required to run an entire operating system. You're actually putting Node's micro-package madness into even more of a negative light from this comparison -- if it takes only several hundred packages to run an entire distribution, why is it reasonable to require that many packages to run a single web application (not to mention that most distribution packages don't depend on hundreds of others)?

The difference is that the only people looking into NPM packages are the Node community. The entire Linux community -- which includes a large portion of the Node community mind you -- looks into the set of packages you are looking at (yes, we've had a bad rap in recent years -- but in our defence we really are trying).

I'm a little biased with regards to distributions (I work for SUSE), but I like to think that working on a distribution has given me a much better understanding of how much work goes into those packages. I don't think you could adequately replicate most of that engineering in the time that most micro-package ecosystems have been around.

Re: Critical vulnerability of NPM package macaddress

#68

Post by okdana »

amluto wrote: post #59
koolba wrote: post #2

Here's the offending line of code[1]: exec("cat /sys/class/net/" + iface + "/address", function (err, out) { That's some serious amateur hour and may even be a new contender for the Useless Use of Cat (and "exec(...)") Award. Would be simpler and less error prone to just read the file via fs.readFile(...) after verifying that the iface parameter does not contain a directory separator. [1]: https://github.com/scravy/n…

> after verifying that the iface parameter does not contain a directory separator. No, this is terrible practice. The correct solution is to verify that the interface name is well formed. Use a whitelist, not a blacklist. And, for good measure, use a real path manipulation library if available.

How are you going to white-list network-interface names? On Linux at least you can name them almost anything you want:

  % sudo ip link set eth3 down
  % sudo ip link set eth3 name "'deal-with-it'"
  % sudo ip link show "'deal-with-it'"
  5: 'deal-with-it':  mtu 1500 qdisc mq state DOWN mode DEFAULT group default qlen 10000
      link/ether xx:xx:xx:xx:xx:xx brd ff:ff:ff:ff:ff:ff
The only interface-name restrictions iproute2 sets is that they can't contain NUL, slash, or white space, and they can't be longer than 15 characters.

Re: Critical vulnerability of NPM package macaddress

#69

Post by staticassertion »

f055 wrote: post #64
Boulth wrote: post #51

Earlier quoted context omitted.

This is kind of embarrassing but the advisories page is full of gems like that: https://nodesecurity.io/advisories

I'm shocked. There are almost 30 advisories in May alone that are 7+. To put this into perspective Perl, which I like, with its modules had 3 this year. And 3 last year. Three per year. Three. https://www.cvedetails.com/vulnerability-list/vendor_id-1885...

Counting vulnerabilities is entirely useless. You can easily attribute more vulns to better security, as it may indicate a larger/ better group of people are looking for vulnerabilities.

I actually find that is generally the case.

Re: Critical vulnerability of NPM package macaddress

#70

Post by staticassertion »

grumdan wrote: post #65
megous wrote: post #54

Earlier quoted context omitted.

These kinds of bugs are language independent, the source is between the screen and the keyboard.

They are language-independent but may still have a higher incidence in one community than another. Anecdotally, these basic mistakes seem to happen more in Node packages or JS-based projects, but I have no concrete evidence for this speculation. The only study I know of looking at this in a serious way is this one: http://web.cs.ucdavis.edu/~filkov/papers/lang_github.pdf ( https://news.ycombinator.com/item?id=8558740…

> Anecdotally, these basic mistakes seem to happen more in Node packages or JS-based projects

It's just bias. Python code is riddled with vulns - especially since it's all C under the hood.

https://hackernoon.com/python-sandbox-escape-via-a-memory-co...

Here's a great post that covers some issues in Python modules and why they're extra exploitable because they execute under CPython.

This is a particularly relevant quote:

> Perhaps less recognized is the fact that memory corruption bugs are reported in popular Python modules all the time without so much as a CVE, a security advisory, or even a mention of security fixes in release notes.

Post reply on HN