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

51–60 of 99 posts

Re: Critical vulnerability of NPM package macaddress

#51

Post by Boulth »

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

Re: Critical vulnerability of NPM package macaddress

#52

Post by royjacobs »

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 packages is react-input-select...WHY?

Re: Critical vulnerability of NPM package macaddress

#53

Post by rms_returns »

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…

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.

Re: Critical vulnerability of NPM package macaddress

#54

Post by megous »

rms_returns wrote: post #53
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…

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.

Re: Critical vulnerability of NPM package macaddress

#55

Post by xorcist »

tomxor wrote: post #36
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…

Nodejs API docs for exec say: > Never pass unsanitized user input to this function. Any input containing shell metacharacters may be used to trigger arbitrary command execution. I've always wondered what exactly the best way to do this is... searching about sanitising user input into shells always results in suggestions for something quite extreme like stripping non-alphanumerics or something similar. But much of the…

Quoted exactly right, it will not be exploitable. However, this amounts to declaring which input is disallowed, generally not a good idea. It is preferrable to declare which input is allowed.

In this case, matters are worse because without knowing which shell we are running under and which cat we are executing it is impossible to know the complete list of dangerous characters. (In fact, if some sloppy sysadmin have a dot in the search path, the cat binary might even be attacker uploaded.)

(The fact that cat is even in there is of course doubly insane. They start an extra process just to make reading the data slightly more complicated.)

Re: Critical vulnerability of NPM package macaddress

#56

Post by bunderbunder »

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'm starting to wonder if this is just a fundamental weakness of open source software.

Compare with a commercial platform, such as .NET or Cocoa - there you've got more "batteries included" libraries, with relatively large, standardized packages that have short dependency trees, being maintained by people who are paid to pay attention to these sorts of details.

This isn't to say that commercial software is bug-free - far from it. If something passes code review, it's not going to be looked at again until someone's being paid to look at it again. Which gets you to a similar place as with open source: Software is buggy.

Mostly, though, it leaves me thinking that the whole ideal of, "It's open source, so you can assume many eyes are looking at it," is a pipe dream. Probably the bystander effect is closer to the truth: If it's a small project, I'm relatively likely to at least cursorily look over the code before taking a dependency on it. If it's a big project, though, I just assume someone else is doing that. Just like everyone else.

If it's a big project taking dependencies on small projects, though, that seems to be the worst situation. Nobody can make sense of that.

Re: Critical vulnerability of NPM package macaddress

#57

Post by megous »

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.

The trust model of linux distro is different from NPM. Dependning on the distro, there may be many maintainers who are separate from upstream developers. These maintainers may manage 5-10 pcakages on average and usually keep tabs on the upstream.

There's just no equivalent of maintainers in the NPM land.

Re: Critical vulnerability of NPM package macaddress

#58

Post by DisMeUrBro »

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…

> That's some serious amateur hour

Can confirm, "NPM" is in the post's title.

Re: Critical vulnerability of NPM package macaddress

#59

Post by amluto »

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.

Re: Critical vulnerability of NPM package macaddress

#60

Post by roblabla »

bluetech wrote: post #28
roblabla wrote: post #18

Earlier quoted context omitted.

I maintain a package that has an indirect dep to macaddress because of UUID generation. UUID v2 is generated from Mac address + timestamp. My package only generates uuid v4 (fully random) and even if it was, the uuid dep doesn't expose the vulnerable argument. As such it isn't vulnerable. But the dep is still there. This isn't surprising at all tbh.

A dependency for uuid4 is hardly needed: crypto = require('crypto'); function uuid4() { const bytes = crypto.randomBytes(16); bytes[6] = (bytes[6] & 0x0f) | 0x40; bytes[8] = (bytes[8] & 0x3f) | 0x80; return bytes.toString('hex').match(/(.{8})(.{4})(.{4})(.{4})(.{12})/).slice(1).join('-'); }

I mean sure. But I also need to parse UUID. And do other things with it. Even if I didn't need to, I'd still likely use the uuid-1345 crate for it.

Here's the thing: If I import this code in my codebase directly instead of using a library, it suddenly becomes my "responsibility" if it breaks. And when we're talking about a FOSS side-project, I don't have that kind of time. Third-party libraries means I get any improvements and bug-fixes for free. And if it breaks, I get to talk with the original maintainer to figure it out, and we're suddenly two people, with one hopefully knowledgable enough on the topic, working on the problem.

Importing this kind of thing in make codebase makes it an ugly unwieldy mess that will inevitably break, and I suddenly will be alone trying to figure out what I did wrong. This is what's good about npm and libraries: the community that's built around it.

Post reply on HN