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

41–50 of 99 posts

Re: Critical vulnerability of NPM package macaddress

#41

Post by JoshTriplett »

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.

Not the same thing. Your desktop install might include a few thousand packages total, but that doesn't mean any significant number of packages pull in a thousand dependencies by themselves. Node packages might consist of a single function.

Re: Critical vulnerability of NPM package macaddress

#42

Post by georgyo »

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.

Sure, but the major difference is that those packages are for everything on your system. Dependencies are often added very carefully.

In the node case it is not uncommon for a small app to include 50 small packages, and then those packages pulled in 30 unique smaller packages each (on average).

Imagine you did an apt upgrade on your system, or decided to install a new package and it said it was going to install 1500 new packages. Would you continue? It would definitely make me pause for a while. But this is the norm in the node world, and people seem to think it is a good idea.

Re: Critical vulnerability of NPM package macaddress

#43

Post by alxlaz »

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…

In this particular case, you know the result has to be a subpath under /sys/class/net/... and, specifically, an interface name. I'm 95% sure that, if it's not alphanumeric, you can safely throw it out (and the other 5% is in the kernel and udev docs).

Re: Critical vulnerability of NPM package macaddress

#44

Post by throwaway5752 »

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.

I think that's the point. Outside of very few operating systems and languages with the organization and history supporting them, there's not much that one should trust. Since you can't validate every package yourself, you have to ask yourself if you trust your counterparty. Do you trust ElementaryOS to do have dozens of people validating upstreams before pulling into stable and correctly doing proper package source integrity steps like Red Hat, Canonical, SUSE, et al do? I know they are Ubuntu LTS based but what are the additional packages and how carefully are they curated, built, and stored?

May the answer is to not use Node or ElementaryOS depending on how important security is to you. May I'm the weirdo for critically inspecting 3rd party libraries before including them and looking at the transitive dependencies they pull along.

Re: Critical vulnerability of NPM package macaddress

#45

Post by koolba »

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…

Escaping and quoting is the wrong way to go. Even if it works you're technically not totally correct as you can't reference my network interface named "bad'name"[1].

If you can avoid fork/exec you're always better off. In this case reading the file directly is fine. I can't think of a valid reason to fork and invoke cat.

If you have a legit reason to fork and run then use execFile(...) as you can pass the arguments as an actual array. No escaping needed as it gets passed in total without shell parsing of args. Your example would be something like:

    execFile('/bin/cat', ['--', `/sys/class/net/${iface}/address`,], (error, stdout, stderr) => { ... })
Though that has the issue of being able to pass in ../../../ and read a file named "address" anywhere on the filesystem. Should also add in a regex validation that no directory separators are in the text of iface.

[1]: Yes this sounds stupid but it's a valid file name. A more realistic one may be something like "Alice's Wi-Fi".

[2]: https://nodejs.org/api/child_process.html#child_process_chil...

Re: Critical vulnerability of NPM package macaddress

#46

Post by tomxor »

alxlaz wrote: post #43
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…

In this particular case, you know the result has to be a subpath under /sys/class/net/... and, specifically, an interface name. I'm 95% sure that, if it's not alphanumeric, you can safely throw it out (and the other 5% is in the kernel and udev docs).

> In this particular case, you know the result has to be a subpath under /sys/class/net

I get that in _this_ case, but I'm talking more generally, sometimes parameters are allowed to or even need to have various other non alphanumeric chars, and then you start going down the fallible road of differentiation... trying to sanitise not quite everything without letting exploits pass through which is dangerous.

But in the more general case of "it's a parameter" can't we just quote it, sanitise for single quotes and then know it's safe?

Re: Critical vulnerability of NPM package macaddress

#47

Post by bluetech »

_wmd wrote: post #38
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('-'); }

v1 and v2 UUIDs are derived from a MAC address by specification, the uniqueness of the v1 scheme in particular depends on it. Whether that's a good idea or not is another topic entirely, but if you want to conform to spec, an implementation needs to be able to read MAC addresses

Please see the comment I replied to.

Re: Critical vulnerability of NPM package macaddress

#48

Post by [deleted] »

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

Earlier quoted context omitted.

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.

Sure, but the major difference is that those packages are for everything on your system. Dependencies are often added very carefully. In the node case it is not uncommon for a small app to include 50 small packages, and then those packages pulled in 30 unique smaller packages each (on average). Imagine you did an apt upgrade on your system, or decided to install a new package and it said it was going to install 1500…

[deleted]

Re: Critical vulnerability of NPM package macaddress

#49

Post by ashelmire »

This one has been out for about a month:

https://snyk.io/vuln/npm:macaddress:20180511

Unfortunately, it's introduced into any Rails app that uses webpacker or any app using node-sass through a long dependency chain.

And once again demonstrates how the JS community is far too dependent on extremely trivial, unsupported, and unaudited libraries.

Re: Critical vulnerability of NPM package macaddress

#50

Post by tomxor »

koolba wrote: post #45
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…

Escaping and quoting is the wrong way to go. Even if it works you're technically not totally correct as you can't reference my network interface named "bad'name"[1]. If you can avoid fork/exec you're always better off. In this case reading the file directly is fine. I can't think of a valid reason to fork and invoke cat. If you have a legit reason to fork and run then use execFile(...) as you can pass the arguments a…

> Escaping and quoting is the wrong way to go. Even if it works you're technically not totally correct as you can't reference my network interface named "bad'name"[1]. If you can avoid fork/exec you're always better off. In this case reading the file directly is fine. I can't think of a valid reason to fork and invoke cat.

Yes I get that, I should have clarified: in the general use of exec (i.e a valid and necessary use).

> If you have a legit reason to fork and run then use execFile(...)

You are absolutely right, execFile is a much better way, I've used it before and I guess you are right - in _most_ cases this should be the #1 choice.

[edited] - irrelevant details of obscure case where execFile isn't enough.

Anyway, in a case where exec through a shell is necessary for some reason, i'm wondering if this method of single quoting is actually safe or not, as far as I can see it is, but i'm not a security expert.

Post reply on HN