Live data from Hacker News

Critical vulnerability of NPM package macaddress

nodesecurity.io

91–99 of 99 posts

Re: Critical vulnerability of NPM package macaddress

#91
post #37

Earlier quoted context omitted.

If some user input is going into anything this library uses you can take over the box pretty much, execute anything you want (at the user level of node). Probably really hard to figure out what user input triggers that to be honest unless you have the source code of the app. This package has nearly 100M downloads a year which is pretty astonishing.

I get that part, but in what scenario will user input actually be provided to this package? It feels like this is highly unlikely and that there probably won't be any cases where this will actually be an issue. I may be wrong though, I just can't think of anything.

An example off the top of my head would be some sort of IoT device which has a drop-down to communicate via 4G, WiFi, ethernet etc. I'd be they'd put the value of the select to be the interface name and wouldn't check input value, so you could craft a special request with a bad value. But I do agree; not super easy to exploit.

The more worrying fact is something like this which could easily be drawn in to 100m installs.

Re: Critical vulnerability of NPM package macaddress

#92
According to the HackerOne report[1] there was no response from the maintainer and the last package update was 3 years ago. And yet the package is downloaded several million times a month (meaning it's definitely used in something -- I imagine looking at the reverse dependency tree would cause some concern).

This is something that bothers me a lot with most of these home-grown package ecosystems. In distributions we remove packages from their main repositories if old maintainers are no longer available and nobody is interested in becoming the new maintainer (we do this in openSUSE all the time). Does the Node (and Rust, Python, Ruby, ...) community have this sort of process for deciding that a project is no longer adequately maintained?

One of the benefits of having macro-packages is that it's always clear whether a package is being maintained -- if there isn't a release for a long time then it's probably no longer maintained. But with micro-packages there are thousands which are so trivial they require no updates (see left-pad, ansi-{green,blue,red,yellow,cyan,...}, and so on) that a very large number of projects depend on. Not to mention that distributions don't generally have projects with thousands of dependencies each -- so removing a dependency and everything that depends on it will not destroy the entire package ecosystem.

[1]: https://hackerone.com/reports/319467

Re: Critical vulnerability of NPM package macaddress

#93
post #36
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…

I'm not a JS developer, so this as much of a question as it is a statement.

`exec` is a system call and doesn't call bash or any other shell. At least, it shouldn't.

If it does, then it would be wrong to assume that bash is even available.

Either way. The single quote sytax is specific to bash (and maybe sh, dash, etc. I'm not certain.) so, don't rely upon it.

Re: Critical vulnerability of NPM package macaddress

#94
post #7

Earlier quoted context omitted.

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.

Well said/quoted. However, the boundary is very vague.

Re: Critical vulnerability of NPM package macaddress

#95

Earlier quoted context omitted.

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

Hmm, an adequate explanation, but why in the world is someone making a public library out of that, and furthermore, why in the world are people using this library? The library has been downloaded 2300 times in the past year!

I have nothing against bad developers—we all started there—but there is something seriously wrong with the JavaScript ecosystem and the barrier of entry for pushing your code into other people's projects through libraries.

Re: Critical vulnerability of NPM package macaddress

#96
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…

I'm not a JS developer, so this as much of a question as it is a statement. `exec` is a system call and doesn't call bash or any other shell. At least, it shouldn't. If it does, then it would be wrong to assume that bash is even available. Either way. The single quote sytax is specific to bash (and maybe sh, dash, etc. I'm not certain.) so, don't rely upon it.

You are describing execFile in nodejs which directly spawns a binary into a child process and explicitly passes each argument from an array (Other's here have mentioned it's the prefered way as it obviously avoids any arbitrary execution potential of going through a shell)...

exec is not a system call in nodejs, it's just a convenience function that sets up a child process and spawns a shell (of your choosing) and then passes the first argument as the single command.

Re: Critical vulnerability of NPM package macaddress

#97
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…

Also this interface assumes that the kernel is using sysfs.

Re: Critical vulnerability of NPM package macaddress

#98
post #68
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 does Linux allow .. as an interface name? If so, that’s nuts.

Re: Critical vulnerability of NPM package macaddress

#99

Earlier quoted context omitted.

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

Well, everything I've said here are grounded in personal experience working on FOSS projects. They're also grounded in trust of the maintainers. In 5 years of full-time programming as a job and a hobby, I have found that to work to my advantage. I can understand that this might not be everyone's experience. I also understand that different projects have different needs - some might simply be too sensitive to depend on external code if they can help it.

To answer your rhetorical questions: I would certainly depend on leftpad, as it allows me to free my mind to something more useful. I probably wouldn't depend about is-odd because I intuitively know how to make it work. That's pretty personal. The line is basically drawn at "do I need to think to figure this out." Yes, leftpad is trivial. But this is death by the thousand papercuts: When I program something, I'm solving a problem. Having to solve a million other problems at the same time - no matter how trivial - is going to take me away from what I'm supposed to actually doing. It breaks my flow. I don't like that.

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

I have found this to not be the case. In the JS land, depending on a library is beyond trivial. Especially for those "simple" libraries that simply export a function. Yes, I do need to read the docs, but there isn't a million ways to do leftpadding, nor is there a million ways to do UUID generation. Copy pasting code might require playing with requires, figuring out transitive dependencies, and just some manual work that take time away from actually thinking about the code I'm working on - again, breaking my flow, forcing me to context switch.

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

Semver takes care of that for you. And yes, I trust maintainers to follow semver. Again, that generally works. I only had a single case of semver-breaking release in my life, and that was in the Rust land - docopt.rs 0.8.2. https://github.com/docopt/docopt.rs/issues/235. Debugging the issue took me at most an hour, reporting it to upstream took seconds, and the fixed version was released literally 10 minutes later. In the meantime, my install still worked due to the lockfile pinning the previous docopt version. I could easily continue working normally.

Breakage happen. I've found that breakage also happen internally. Sometimes I change my code and break my own invariants. Refactor and forget to fix an instance, which breaks the code in subtle ways. Bugs happen. I accept that.

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

This is where I totally disagree. Depending on a third-party package does not deprive me of the ability to fix code when it breaks. And in fact: it empowers me. Again, the major advantage of dependencies, in my view, comes from the community. And if it needs adjustment that could be beneficial to others, I can PR it. I can fork, and npm has tools to make it easy to inject a fork into the dependency tree, replacing all instances with my own. This lets me experiment with potential fixes with ease, as if it was my own code.

Sure, sometimes, the library might not do exactly what I want it to, exactly how I want it to. Maybe what I want to do is incompatible with said libraries. In those cases, rolling my own might make sense.

In the end, it comes down to this: I trust the community, and believe in taking and giving back. If that's a young point of view, then so be it. Maybe I'll learn the hard way that reality is not so forgiving, that my trust was misplaced.

Post reply on HN