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

81–90 of 99 posts

Re: Critical vulnerability of NPM package macaddress

#81

Post by tomxor »

furgooswft13 wrote: post #63
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.

You joke but I've come across many one line modules in the npm registry... usually when trying to figure out if I trust the tree of some prospective module i'm considering including in my code - It's a serious problem for module authors because most aren't even aware they are including all of this crap, they see the first level of dependencies looks fairly reasonable, but you have to keep digging and then you find all kinds of crazy ridiculous things just because one otherwise reasonable person in the chain didn't quite look closely enough at something they were depending on which opened up a doorway to hell.

My general rule is to just avoid trees deeper than a few levels unless there is a very good reason (unless i'm creating a public module in which case I aim for zero, usually end up with 1 or 2 one level dependencies) - most node modules are the unix philosophy taken way too far, every pattern is bad when incorrectly used.

Re: Critical vulnerability of NPM package macaddress

#82

Post by wolfgang42 »

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…

react-input-select has 640 (!) dependencies, all of which were added by the author in a commit named 'type fix'[1] which seems to have been ostensibly intended to fix a typo, and none of which seem to be required to actually use the library.

The only thing I can imagine is that the author somehow added their entire node_modules tree as dependencies. Maybe it was an ill-fated attempt at pinning?

[1]: https://github.com/rayescmata/react-input-select/commit/cd7b...

Re: Critical vulnerability of NPM package macaddress

#83

Post by xorcist »

tomxor wrote: post #79
xorcist wrote: post #55

Earlier quoted context omitted.

Quoted exactly right, it will not be exploitable. However, this amounts to declaring which input is dis allowed, 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…

Thanks > matters are worse because without knowing which shell we are running under The shell in exec can be configured, it's /bin/sh by default but of course this is linked to all kinds of different things on different OS, ubuntu links to dash, used to be bash, FreeBSD has a "proper" sh. I suppose if going through a shell was somehow necessary you could try to be more secure by specifying a absolute path to a shell…

> is there really any point in worrying about library security when the $PATH has been effectively hijacked?

Well, there might be legitimate (although somewhat misguided) reasons for someone to do that. People routinely do similar things in dev environments all the time. I can absolutely see how it could reach production. If you don't expect your web app to launch shell scripts you might not even think about it.

It's the small things, which in themselves might not be serious or even noteworthy, which compound together to deliver the nasty surprises.

Re: Critical vulnerability of NPM package macaddress

#84

Post by xorcist »

bunderbunder wrote: post #56
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 -…

Look inside that "commercial" software of yours, chances are that it bundles both five crappy js libs but also a vulnerable version of gzip. Large parts of the code of any non-trivial software package will be open source code.

There are literally people who makes their living from pointing these things out.

Unless your software is among the absolutely most widespread, chances are it's not audited at all.

(Also, can we please leave the perceived distinction between "commercial" and "open source" in the 90s where it belongs?)

Re: Critical vulnerability of NPM package macaddress

#85

Post by drdaeman »

arghwhat wrote: post #17
drdaeman wrote: post #10

Earlier quoted context omitted.

Other platforms do `exec` calls, e.g. `"ifconfig " + iface`, etc. This `cat` is odd, but still within what can be attributed to sloppiness (and copy-paste-driven programming) rather than malice.

exec cat is so odd that I can't seriously attribute it to sloppiness. It's for reading a file, and much more cumbersome. Plus, exec is not something to be taken lightly, to the point where I'd barf if I found out a lib I depended on exec'd internally and immediately remove it, unless there was absolutely no other way to perform the action for some absurd reason.

My point is, it's likely that the first line was something like exec("ifconfig " + name), then it was copied and edited to exec("networksetup -getmacaddress"), then this exec("cat") had happened. Because laziness.

It really wouldn't be a problem (a poor and ineffective code for sure, but no security bugs), if all the arguments would be correctly validated (and escaped, although valid interface names probably don't need it).

Re: Critical vulnerability of NPM package macaddress

#86

Post by f055 »

staticassertion wrote: post #80
f055 wrote: post #77

Earlier quoted context omitted.

Generally, how much vulnerabilities is too much? I think Node is past this point.

There is no limit - as I said, counting is pointless. As an example, lots of projects have active bug bounty programs where they pay people to disclose vulns. This generates more CVEs than a competing product that does not pay for more vulns. Would we then call the project with a bug bounty program less secure? I'd hope not. Alternatively, what about impact? Maybe 100 DOS's is less serious than 5 RCE's, or 10 privesc…

I think comparing is pointless. You are correct that a project with a small number of vulns is no more secure than the one with a high number of vulns. But counting seem sensible - if a project spills major vulns at a continuous rate it means two things: the overall codebase is not improving, and who knows how many more vulns there are. It's not something to be happy about.

Re: Critical vulnerability of NPM package macaddress

#87

Post by scrollaway »

tzahola wrote: post #23
always_good wrote: post #22

Earlier quoted context omitted.

NPM doesn't change anything here. the same issue would exist if you could only vendor dependencies. Are you sure you understand the issue?

Javascript was a mistake Edit: Answer to “always_good”: If a toddler shoots himself with a handgun, we can say that his mistake was pulling the trigger. While technically correct, it’s not addressing the real issue, that is, how did the toddler get hold of the gun in the first place?

This isn't a case of JS gun gone wrong. Someone's passing cat inside exec with an arbitrary path instead of using native open&read and path.join. That's a thing possible in almost every modern language.

Stop the nonsense. Seriously. Stop.

Re: Critical vulnerability of NPM package macaddress

#88

Post by staticassertion »

f055 wrote: post #86
staticassertion wrote: post #80

Earlier quoted context omitted.

There is no limit - as I said, counting is pointless. As an example, lots of projects have active bug bounty programs where they pay people to disclose vulns. This generates more CVEs than a competing product that does not pay for more vulns. Would we then call the project with a bug bounty program less secure? I'd hope not. Alternatively, what about impact? Maybe 100 DOS's is less serious than 5 RCE's, or 10 privesc…

I think comparing is pointless. You are correct that a project with a small number of vulns is no more secure than the one with a high number of vulns. But counting seem sensible - if a project spills major vulns at a continuous rate it means two things: the overall codebase is not improving, and who knows how many more vulns there are. It's not something to be happy about.

I disagree for exactly the reasons I've stated - "more vulns" doesn't take impact into account. "More vulns" doesn't take who is reporting them into account (internal? paid pentester?). These things matter a lot.

Again, look at projects that pay for vulns. They have hundreds a year - this is a good thing. They come in at a continuous rate.

Counting is pointless.

Re: Critical vulnerability of NPM package macaddress

#89

Post by nucleardog »

arghwhat wrote: post #17
drdaeman wrote: post #10

Earlier quoted context omitted.

Other platforms do `exec` calls, e.g. `"ifconfig " + iface`, etc. This `cat` is odd, but still within what can be attributed to sloppiness (and copy-paste-driven programming) rather than malice.

exec cat is so odd that I can't seriously attribute it to sloppiness. It's for reading a file, and much more cumbersome. Plus, exec is not something to be taken lightly, to the point where I'd barf if I found out a lib I depended on exec'd internally and immediately remove it, unless there was absolutely no other way to perform the action for some absurd reason.

Somebody Google'd "get mac address from script linux" and scrolled through a couple things involving awk and other nonsense until they found something easily digestible--a StackOverflow answer saying:

    For particular interface like for eth0:
    cat /sys/class/net/eth0/address
Then without thought or understanding, put that into their JavaScript as directly as possible.

This is someone doing work without a full understanding of the problem space. Less of a programmer, more of a programming technologist. I don't find it surprising at all.

Re: Critical vulnerability of NPM package macaddress

#90

Post by bluetech »

roblabla wrote: post #60
bluetech wrote: post #28

Earlier quoted context omitted.

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 improv…

Well, surely you put the line somewhere? Would you depend on `leftpad`? How about `is-odd`?

Your view of dependencies is very idealistic. And if I'd venture to guess - apologies if I'm wrong - young, at least to programming. I'm saying this because you don't tend to hear such phrases once reality hits.

Everything you said can be flipped:

> If I import this code in my codebase directly instead of using a library, it suddenly becomes my "responsibility" if it breaks.

I have full control & understanding of the code; I can adjust & trim so it fits as much as possible. If it breaks, I can fix it directly.

> And when we're talking about a FOSS side-project, I don't have that kind of time.

Copying/hand rolling some trivial code is often faster than deciding among a dozen libraries which do the same thing, reading their docs, their issue pages and open PRs, adding and integrating to my project.

> Third-party libraries means I get any improvements and bug-fixes for free.

I must keep the dependencies updated. I must read changelogs and hope nothing breaks.

> And if it breaks, I get to talk with the original maintainer to figure it out

I must use inefficient communication with a more-often-than-not absent maintainer in order to resolve my problem instead of fixing it directly.

> I suddenly will be alone trying to figure out what I did wrong

I am able to be self-reliant in fixing the problem, because I wrote the code and I understand it.

I do not have to trust the maintainer of the package or the maintainers of any of its transitive dependencies to be competent or non-evil.

---

For some things this is worth it. IMO for generating 16 random bytes it is not worth it.

Post reply on HN