Fixes arbitrary command injection by using execFile instead of exec

[Logikgate]

execFile takes the call arguments as variables and they are not executed under the shell environment

Reference
https://blog.liftsecurity.io/2014/08/19/Avoid-Command-Injection-Node.js/

[jourdanrodrigues]

@Logikgate Do you intend to fix the CI errors? It's failing for Node 0.8, 0.9 and 0.10.

[Logikgate]

@jourdanrodrigues sorry, yes just pushed up the fix. I also modified the .travis.yml to test against both macos and linux

[jourdanrodrigues]

@Logikgate Thanks! And sorry for not expressing myself well. Now that I'm reading my comment, it sounds rude.

[Logikgate]

@jourdanrodrigues no problem! I hadn't noticed the failed build so I appreciate the nudge :)

@scravy Could you take a look at this and if everything looks good release a new version? A lot of packages depend on macaddress and they are all failing snyk and nsp checks. This would also close out issue #19 as well as #9 & #7 since those are just requesting a new release.

[eli-schwartz]

(Pretend I've written some witty "get off my lawn" "I hate nodejs on principle" rant, even though I'm much too young for that.)

It's incredibly wasteful to invoke a subprocess just to cat a file, this module deserves a Useless Use of Cat award as well as the vulnerability.

"but it's so easy to code" ==> but invoking subprocesses to cat a file causes people to get confused about whether they are using a shell or not, and maybe end up introducing the glaring security hole mentioned here.

> const fs = require('fs');
> var iface = 'enp0s25';
> fs.readFileSync('/sys/class/net/' + iface + '/address').toString('utf8').trim();
'3c:97:0e:cc:7c:b3'

Why, whenever people write code in powerful programming languages (unlike shell), do they always obsessively somehow end up running vastly weaker, more dangerously misused/abused/vulnerable shell commands (or even direct execution with argv in a list, which is only maybe a little bit barely safer), in order to get inferior results???

...

Sorry, this is not your fault, you're just trying to fix it and I get that, but I really think it's useful to just rethink the whole paradigm here. The language should be used the way it was intended, not abused for cheap shell hacks. Don't use subprocesses unless there's no other choice.

[Logikgate]

@eli-schwartz I don't disagree at all with what you said and yes I was only trying to fix the issue at hand not rethink how this project is doing things.

Your example of how to get the same information that cat does make sense, but I can't, of the top of my head, think of how to get around the execution of ifconfig and ipconfig that the unix and windows implementations use. Can you?

[eli-schwartz]

You even get informative error messages like:

Error: ENOENT: no such file or directory, open '/sys/class/net/../../../etc/passwd; touch /tmp/poof; echo /address'

On the topic of Windows/Unix compat, I don't know how you'd go about collecting the information in general. Python has a simple ctypes builtin functionfor this (from uuid import getnode; hex(getnode());) or the thirdparty netifaces module.

In-process is "nicer", and comes with benefits like not relying on regular expression parsing of tools without a guaranteed scriptable output format (!!!), but if you absolutely have to run some external command via subprocesses, at least do it with full awareness it is being done as a last resort.

[JeffreyATW]

@eli-schwartz Please open up a separate PR to remove the use of cat.

[gluxon]

Your example of how to get the same information that cat does make sense, but I can't, of the top of my head, think of how to get around the execution of ifconfig and ipconfig that the unix and windows implementations use. Can you?

The best way to do this for Windows is to probably use a native module (C++ addon) and the GetAdaptersInfo MFC function. But I think fixing the vulnerability is the much more important issue at hand; which this pull request does.

The lib/unix.js and lib/linux.js files can be modified to remove cat spawn since that's probably a quick change. I think the Windows implementation is fine for now despite its inefficiencies.

Thanks to everyone that worked on this. I don't use this package directly, but its existence enables my stuff to work. 👍

[udisun]

how do i 'npm install' this pull request over the node-macaddress dependency?

[Logikgate]

I am closing this PR since I accidentally opened it on the master branch of our fork. Please reference the newly opened PR #20